Skip to content

feat(installer): add legacy migration and prevent recursive loops #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jbdevprimary merged 2 commits into from
Apr 10, 2026
Merged

feat(installer): add legacy migration and prevent recursive loops #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
761fc17
fix(installer): robust legacy migration and cleanup
jbdevprimary Apr 10, 2026
0732751
fix(installer): restore copy_tree and fix corrupted syntax
jbdevprimary Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file removed assets/.DS_Store
View file
Open in desktop
Binary file not shown.
45 changes: 41 additions & 4 deletions install.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ fi
usage() {
cat <<'USAGE'
Usage: install.sh [--prefix PATH] [--force] [--with-ui]
[--auto] [--yes]
[--auto] [--yes]
[--profiles minimal|dev|ops[,..]]
[--features gnu_over_bsd,build_flags,...]
[--install brew,asdf,doppler,...]
Expand Down Expand Up @@ -71,7 +71,7 @@ GET_BASHED_GIT_SIGNING=0

while [[ $# -gt 0 ]]; do
case "$1" in
--prefix)
--prefix)
if [[ $# -lt 2 ]]; then
echo "Error: --prefix requires a value" >&2
usage
Expand Down Expand Up @@ -515,6 +515,41 @@ if [[ "$DRY_RUN" -eq 1 ]]; then
echo " Installers: ${INSTALLS:-<none>}"
fi

# @internal
migrate_legacy() {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The migrate_legacy function performs destructive operations (like rm -rf) and modifies the filesystem, but it does not respect the $DRY_RUN flag. This violates the expectation that --dry-run should not modify the system.

migrate_legacy() {
  [[ "$DRY_RUN" -eq 1 ]] && return 0

local legacy_rc_d="$HOME/.bashrc.d"
local legacy_secrets_d="$HOME/.secrets.d"

if [[ -d "$legacy_rc_d" && ! -L "$legacy_rc_d" ]]; then
echo "Migrating legacy .bashrc.d to $PREFIX/bashrc.d..."
mkdir -p "$PREFIX/bashrc.d"
# Copy files, avoid error if empty
find "$legacy_rc_d" -type f -exec cp -p {} "$PREFIX/bashrc.d/" \;
rm -rf "$legacy_rc_d"
Comment on lines +527 to +528
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using find -exec cp flattens the directory structure, which can lead to filename collisions and loss of organization if the legacy directory contains subdirectories. Additionally, the legacy directory is removed without explicitly verifying the success of the copy operation. Since rsync is already a dependency for this script, it is a more robust and efficient choice for migration.

    rsync -a "$legacy_rc_d/" "$PREFIX/bashrc.d/" && rm -rf "$legacy_rc_d"

fi

if [[ -d "$legacy_secrets_d" && ! -L "$legacy_secrets_d" ]]; then
echo "Migrating legacy .secrets.d to $PREFIX/secrets.d..."
mkdir -p "$PREFIX/secrets.d"
chmod 700 "$PREFIX/secrets.d"
find "$legacy_secrets_d" -type f -exec cp -p {} "$PREFIX/secrets.d/" \;
rm -rf "$legacy_secrets_d"
Comment on lines +535 to +536
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the .bashrc.d migration, using find -exec cp flattens the directory structure and the subsequent rm -rf is not explicitly guarded by the success of the copy operation.

    rsync -a "$legacy_secrets_d/" "$PREFIX/secrets.d/" && rm -rf "$legacy_secrets_d"

fi
Comment on lines +519 to +537
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

migrate_legacy performs filesystem writes (mkdir/cp/rm) even when --dry-run is set, which contradicts the earlier "Dry run enabled. No changes will be made." message. Add an early return when DRY_RUN=1 (or guard the call site) so legacy directories and dotfiles aren’t modified during dry runs.

Copilot uses AI. Check for mistakes.

# Detect and fix "template as file" loop hazard
for f in "$HOME/.bashrc" "$HOME/.bash_profile"; do
if [[ -f "$f" && ! -L "$f" ]]; then
if grep -q "@file bashrc" "$f" || grep -q "@file bash_profile" "$f"; then
if [[ "$LINK_DOTFILES" -eq 0 ]]; then
echo "Detected recursive loop hazard in $f. Cleaning..."
backup_file "$f"
echo "# get-bashed: recovered from loop" > "$f"
fi
Comment on lines +539 to +547
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recursive-loop recovery logic keys off the presence of @file bashrc / @file bash_profile anywhere in the user’s dotfiles, then backs up and replaces the entire file. This heuristic can trigger on legitimately customized files derived from the templates (or any file containing those strings), unexpectedly wiping active configuration. Consider detecting the actual recursive sourcing pattern (or comparing the file content to the shipped templates) and applying the minimal edit needed rather than clobbering the file.

Copilot uses AI. Check for mistakes.
fi
fi
done
}

mkdir -p "$PREFIX"
export GET_BASHED_HOME="$PREFIX"
export GET_BASHED_VIMRC_MODE="$VIMRC_MODE"
Expand All @@ -538,6 +573,8 @@ cp -f "$REPO_DIR/inputrc" "$PREFIX/inputrc"
cp -f "$REPO_DIR/vimrc" "$PREFIX/vimrc"
cp -f "$REPO_DIR/gitconfig" "$PREFIX/gitconfig"

migrate_legacy

Comment on lines 573 to +577
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions preserving custom user modules during upgrades when --force is used, but FORCE is parsed and never referenced anywhere in the installer (no behavior change occurs when passing --force). Either implement the described --force behavior (and document what it does) or remove the flag from usage/arg parsing to avoid misleading users.

Copilot uses AI. Check for mistakes.
# secrets.d bootstrap (only inside GET_BASHED_HOME)
mkdir -p "$PREFIX/secrets.d"
chmod 700 "$PREFIX/secrets.d"
Expand Down Expand Up @@ -597,10 +634,10 @@ else
# shellcheck disable=SC2016
BASHRC_LINE="# get-bashed: source modular bashrc"
# shellcheck disable=SC2016
BASHRC_SNIP='if [[ -r "$HOME/.get-bashed/bashrc" ]]; then source "$HOME/.get-bashed/bashrc"; fi'
BASHRC_SNIP="if [[ -r \"$PREFIX/bashrc\" ]]; then source \"$PREFIX/bashrc\"; fi"
BASH_PROFILE_LINE="# get-bashed: source login bash_profile"
# shellcheck disable=SC2016
BASH_PROFILE_SNIP='if [[ -r "$HOME/.get-bashed/bash_profile" ]]; then source "$HOME/.get-bashed/bash_profile"; fi'
BASH_PROFILE_SNIP="if [[ -r \"$PREFIX/bash_profile\" ]]; then source \"$PREFIX/bash_profile\"; fi"

ensure_block "$HOME/.bashrc" "$BASHRC_LINE" "$BASHRC_SNIP"
ensure_block "$HOME/.bash_profile" "$BASH_PROFILE_LINE" "$BASH_PROFILE_SNIP"
Comment on lines 634 to 643
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .bashrc/.bash_profile snippet update is gated only on the marker line via ensure_block. If a user already has the marker from a previous install, changing the snippet (e.g., switching from $HOME/.get-bashed/... to an absolute $PREFIX/...) will not update the existing block, so the new deterministic/absolute-path behavior won’t take effect for upgrades or for users changing --prefix. Update ensure_block to replace the existing block when the marker exists but the snippet differs, or version the marker to force an update.

Copilot uses AI. Check for mistakes.
Expand Down
Loading