modularize bin/gtr into libraries and add BATS test suite#119
modularize bin/gtr into libraries and add BATS test suite#119
Conversation
Consolidate common logic for AI and editor adapters into reusable functions. Introduce _ai_define_standard and _editor_define_standard to streamline adapter setup, reducing redundancy across multiple adapter scripts. This refactor enhances maintainability and clarity in the codebase.
WalkthroughReplaces many per-tool adapter scripts with a registry-driven adapter loader, moves CLI subcommands into modular Changes
sequenceDiagram
participant CLI as "gtr (user CLI)"
participant Dispatcher as "bin/gtr (dispatcher)"
participant Core as "lib/core.sh"
participant Loader as "lib/adapters.sh"
participant Adapter as "adapter (file override / registry / PATH)"
participant Tool as "Editor / AI tool"
rect rgba(200,230,255,0.5)
CLI->>Dispatcher: invoke subcommand (e.g., ai/editor/create)
Dispatcher->>Core: resolve_worktree(identifier)
Core-->>Dispatcher: worktree_path, branch
end
rect rgba(220,255,200,0.5)
Dispatcher->>Loader: load_editor_adapter / load_ai_adapter(name)
Loader->>Adapter: check file override → registry → PATH fallback
Adapter-->>Loader: provide editor_open / ai_start implementations
end
rect rgba(255,230,200,0.5)
Dispatcher->>Adapter: call editor_open(worktree_path) / ai_start(worktree_path, args)
Adapter->>Tool: exec command in worktree (foreground/background)
Tool-->>Dispatcher: exit status
Dispatcher-->>CLI: report result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/config.sh`:
- Line 318: The process-substitution "done < <(git config -f \"$config_file\"
--get-regexp '.' 2>/dev/null || true)" is not compatible with Bash 3.2; replace
it with a compatible pattern such as piping the git output into the loop or
using a here-string. Specifically, change the loop that reads git config entries
(the block that ends with that "done < <(...)" line referencing "$config_file")
to either: run git config ... 2>/dev/null || true | while IFS= read -r line; do
... done, or collect the output into a variable and feed it with a here-string:
while IFS= read -r line; do ... done <<< "$(git config -f \"$config_file\"
--get-regexp '.' 2>/dev/null || true)"; this preserves behavior while remaining
Bash 3.2 compatible.
In `@lib/core.sh`:
- Around line 142-151: get_current_branch uses an unquoted dir_flag ("-C $1")
which breaks on paths with spaces; change it to build and expand the option
safely (e.g., use a local array like dir_args and push "-C" and "$1" when $1 is
set, or conditionally call git with "-C" "$1"), and ensure both git invocations
(branch --show-current and rev-parse --abbrev-ref HEAD) use the safe expansion
(e.g., "${dir_args[@]}") so paths with spaces are handled correctly in
get_current_branch.
🧹 Nitpick comments (1)
lib/config.sh (1)
284-298: Consider declaringkeyandvalueas local inside the helper.The variables
keyandvalueare used in_cfg_list_parse_entriesbut are not declaredlocalwithin the function. While they're declared in the outer scope (line 258), the helper modifies them, which could lead to subtle bugs if the outer code relies on their values after the call.Proposed fix
_cfg_list_parse_entries() { local origin="$1" local entries="$2" + local key value while IFS= read -r line; do
Consolidate the logic for checking and opening editors (Emacs, Neovim, Vim) into a single terminal editor adapter function. This refactor reduces redundancy in the editor scripts and enhances maintainability by utilizing shared global variables for command execution and error messaging.
Refactor the get_current_branch function to improve clarity and maintainability. The updated implementation uses conditional statements to handle directory input more cleanly, eliminating redundancy in the command execution for both specified and current directories.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/config.sh`:
- Around line 204-239: The git config calls in cfg_set, cfg_add, and cfg_unset
use the unquoted variable $flag which violates the quoting guideline; update
each call to quote the flag variable (i.e., change git config $flag ... to git
config "$flag" ...) so that cfg_set, cfg_add, and cfg_unset all pass a quoted
scope flag to git config and eliminate the need for unguarded word-splitting.
🧹 Nitpick comments (1)
adapters/editor/vim.sh (1)
4-6: Use platform‑agnostic install guidance.The current message nudges users toward package managers, which isn’t portable across platforms. Consider pointing to the official Vim site instead.
Proposed fix
-_EDITOR_ERR_MSG="Vim not found. Install via your package manager." +_EDITOR_ERR_MSG="Vim not found. Install from https://www.vim.org"Based on learnings: “When error messages reference missing CLI tools (such as gh for GitHub CLI), use platform-agnostic installation guidance by directing users to the official website (e.g.,
https://cli.github.com/) instead of platform-specific package managers (brew, apt, choco) to ensure the guidance works across all operating systems. Apply this messaging to the bin/gtr script where applicable.”
Introduce a GitHub Actions workflow to automate linting with ShellCheck and testing with BATS. The workflow triggers on pushes and pull requests to the main branch, ensuring code quality and functionality through automated checks. Additionally, refactor the copy function in lib/copy.sh to improve maintainability by extracting the file copying logic into a dedicated helper function.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/copy.sh (1)
301-303:⚠️ Potential issue | 🟠 MajorFix ShellCheck SC2015 by replacing the
A && B || truepattern with an explicitifstatement.This pattern triggers ShellCheck's SC2015 warning because if
log_inforeturns non-zero, the error is silently suppressed by|| true. The explicit conditional is clearer and safer.Proposed fix
- [ "$removed_any" -eq 1 ] && log_info "Excluded subdirectory $exclude_pattern" || true + if [ "$removed_any" -eq 1 ]; then + log_info "Excluded subdirectory $exclude_pattern" + fi
🤖 Fix all issues with AI agents
In @.github/workflows/lint.yml:
- Around line 16-18: The workflow step "Install ShellCheck" currently runs "sudo
apt-get install -y shellcheck" without updating package lists; insert a step or
modify the step to run "sudo apt-get update" immediately before any apt-get
install commands (including the step named "Install ShellCheck" and the similar
install step around lines 29-30) so installs use fresh package lists and avoid
flaky CI failures.
In `@lib/platform.sh`:
- Around line 93-109: The inline command substitution $([ -n "$safe_cmd" ] &&
echo "write text \"$safe_cmd\"") can return non‑zero when safe_cmd is empty and
cause set -e to abort; replace this subshell with a precomputed snippet variable
(e.g., cmd_snippet) before the osascript heredoc: test safe_cmd with an if and
set cmd_snippet to either an empty string or the AppleScript line 'write text
"<escaped cmd>"', then insert "$cmd_snippet" in the heredoc instead of the
command substitution so the heredoc never runs a failing subshell; reference
safe_cmd and the current osascript heredoc block where the $([ -n ... ])
appears.
In `@tests/resolve_base_dir.bats`:
- Around line 7-11: The temporary repo commit fails because git user identity
isn’t set; before running git -C "$TEST_REPO" commit, configure the repo-local
git user.name and user.email for TEST_REPO (e.g., via git -C "$TEST_REPO" config
user.name and git -C "$TEST_REPO" config user.email) so the commit succeeds in
CI environments where global identity isn’t present.
In `@tests/sanitize_branch_name.bats`:
- Around line 43-46: Update the test description to accurately reflect the
expected behavior: change the `@test` title that currently reads "multiple special
chars produce single hyphens" to indicate that consecutive special characters
produce consecutive hyphens (e.g., "multiple special chars produce consecutive
hyphens") so the test for sanitize_branch_name (the case calling
sanitize_branch_name "a//b" expecting "a--b") correctly documents the behavior.
- Add apt-get update before installs in CI workflow - Suppress SC2317 on nested adapter functions in bin/gtr - Replace SC2015 pattern with if-statement in lib/copy.sh - Pre-compute AppleScript cmd line to avoid set -e abort in heredoc - Add git identity config in test setup for CI environments - Fix misleading test description for special char replacement
helizaga
changed the title
Update the copy_directories function in lib/copy.sh to enhance error handling by using a conditional statement for the removal of matched paths. This change ensures that the removal status is only set when the command succeeds, improving code clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bin/gtr`:
- Around line 1058-1061: Replace direct calls to get_current_branch and manual
detached-HEAD normalization with the normalized wrapper current_branch: find
usages like the block that sets branch=$(get_current_branch "$repo_root")
followed by the if check for empty or "HEAD" (the code referencing
get_current_branch, the variable branch, and the manual check) and change them
to call current_branch "$repo_root" instead; update the three locations
mentioned (the block around lines 1058–1061, similar blocks at ~1089–1092, and
~287) so they all use branch=$(current_branch "$repo_root") and remove the
duplicated if-checks.
🧹 Nitpick comments (1)
lib/copy.sh (1)
289-295: Address ShellCheck SC2015 warning for clearer control flow.The
A && B || Cpattern on line 293 triggered a ShellCheck warning becauseCmay run even whenAsucceeds ifBfails. Whileremoved_any=1won't fail in practice, restructuring improves clarity.Proposed fix
for matched_path in $pattern_suffix; do # Check if glob matched anything (avoid literal pattern if no match) if [ -e "$matched_path" ]; then - rm -rf "$matched_path" 2>/dev/null && removed_any=1 || true + if rm -rf "$matched_path" 2>/dev/null; then + removed_any=1 + fi fi done
Update the current_branch function to handle cases where the branch is either empty or set to HEAD, normalizing the output to indicate a detached state. Additionally, refactor cmd_list to utilize the updated current_branch function, improving code clarity and maintainability.
- Introduced a new adapter loading infrastructure in `lib/adapters.sh`, consolidating editor and AI adapter definitions into registries for improved maintainability. - Updated the `bin/gtr` script to source command handlers dynamically, streamlining command execution. - Added new commands for listing available adapters and starting AI tools, enhancing user interaction and functionality. - Removed outdated individual adapter scripts, transitioning to a registry-based approach for standard adapters, which simplifies the addition of new tools. - Improved documentation in various instruction files to clarify when to use registry entries versus custom adapter files.
- Added shellcheck disable directives in various command scripts to suppress specific warnings related to variable usage and sourcing. - Improved clarity and maintainability of command context management by ensuring proper variable handling across multiple command files. - Streamlined the sourcing of command handlers in `bin/gtr` for better organization and execution flow.
- Updated CLAUDE.md to reflect the introduction of BATS tests for core functions and clarified the manual testing process. - Enhanced CONTRIBUTING.md with detailed instructions for running automated tests and manual testing across different platforms. - Added help command functionality to various command scripts, providing users with guidance on command usage. - Introduced a new launch.sh script for managing editor and AI tool interactions, improving the overall command structure and user experience. - Merged copy pattern handling into a single function for better maintainability and clarity in the copy command workflow.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/copy.sh (1)
146-152:⚠️ Potential issue | 🟠 MajorGuard
findin command substitution to preventset -escript abort.Under
set -e(active in bin/gtr and inherited by sourced lib/copy.sh), command substitutions that return non-zero exit codes cause the script to abort. Iffindencounters permission denied on subdirectories, it returns exit code 1 even though stderr is suppressed. This terminates the script unexpectedly.Add
|| trueto ensure the find command always returns success:Proposed fix
-$(find . -path "./$pattern" -type f 2>/dev/null) +$(find . -path "./$pattern" -type f 2>/dev/null || true)
🤖 Fix all issues with AI agents
In `@lib/adapters.sh`:
- Around line 232-261: The resolve_workspace_file function currently uses a
pipeline with find ... | head -1 which spawns an extra subshell; replace that
pipeline by using find with -print -quit to return the first match directly
(preserve the same parameters: search "$worktree_path" with -maxdepth 1 -name
"*.code-workspace" -type f and 2>/dev/null) and assign its output to ws_file
exactly as before so behavior is unchanged but without the extra head process.
- Around line 81-106: The splitting of info_lines_raw in _load_from_ai_registry
uses unquoted set -- which allows globbing; replace that block with a safe read
-a approach: use IFS=';' and read -ra into a local array (e.g. parts) from the
quoted "$info_lines_raw", then assign _AI_INFO_LINES=("${parts[@]}") and restore
IFS—this eliminates pathname expansion while preserving the semicolon split for
_AI_INFO_LINES.
In `@lib/commands/ai.sh`:
- Around line 12-15: The --ai case currently does shift 2 without validating a
following value, which will fail if --ai is the last argument; update the --ai)
handler in the argument parsing to first check that "$2" exists and is a
non-option value (e.g. if [ -z "$2" ] || [[ "$2" == --* ]]; then echo "Error:
--ai requires an argument" >&2; exit 1; fi), then set ai_tool="$2" and only
after that do shift 2; reference the --ai) case, the ai_tool variable, and the
shift 2 statement when making this change.
In `@lib/commands/copy.sh`:
- Around line 5-104: Change targets from a space-delimited string to a bash
array and iterate it safely: declare local targets=() at the top, replace the
appending site (currently targets="$targets $1") with array append (use
targets+=("$1")), and when building for --all use readarray -t targets <
<(list_worktree_branches "$base_dir" "$prefix") (then check ${`#targets`[@]} for
emptiness). Update the validation to test [ ${`#targets`[@]} -eq 0 ] when all_mode
is off, iterate with for target_id in "${targets[@]}"; do and declare local
target_id inside the loop, and ensure all uses of $targets are updated to the
array form.
- Around line 14-17: In the `--from` case where you set source="$2" and then do
`shift 2`, add a validation that `$2` exists and is not another option before
calling `shift 2`; if `$2` is empty or starts with `-` (indicating no value),
print a clear usage/error message and exit non-zero instead of performing `shift
2`. Locate the `--from)` case and implement the check (e.g., test `[ -z "$2" ]
|| [[ "$2" == -* ]]`) and only assign `source="$2"` and `shift 2` when the check
passes.
In `@lib/commands/doctor.sh`:
- Around line 30-33: The current find-based worktree count miscounts when prefix
is empty (because "${prefix}*" becomes "*"); replace the count logic in
doctor.sh (the block using base_dir, prefix and count) with an invocation of git
to accurately enumerate worktrees: call git -C "$base_dir" worktree list
--porcelain and count the "worktree" entries (or grep -c '^worktree ') to set
count; keep a fallback to the safer filtered find approach only if git fails.
This ensures count reflects actual git worktrees rather than arbitrary
directories.
In `@lib/commands/editor.sh`:
- Around line 11-14: The --editor case lacks validation for a missing value:
before assigning editor="$2" and doing shift 2 in the --editor branch, check
that $2 exists and is not another flag (e.g., not empty and not starting with
'-'); if the argument is missing, print a helpful usage/error message and exit
nonzero. Update the case handling for the --editor option (the editor variable
assignment and shift 2) to perform this validation and error handling.
In `@lib/commands/remove.sh`:
- Around line 91-95: The postRemove hook invocation (run_hooks postRemove ...)
is currently allowed to fail and, under set -e, will abort the whole removal
loop; change it to treat failures as warnings by running run_hooks postRemove in
a guarded/ignored-failure fashion: capture its exit status, and if non-zero emit
a warning (including the identifier context using REPO_ROOT, WORKTREE_PATH,
BRANCH or $branch_name) but do not exit or return non-zero so the loop continues
processing remaining identifiers. Keep using the same run_hooks command and the
existing environment variables but ensure the hook failure is handled locally
(check $? or use ||) and logged as a warning.
- Around line 5-45: The loop currently builds identifiers as a string and
iterates with for identifier in $identifiers which allows
word-splitting/globbing and leaves identifier non-local; change to collect
positional args into a local array (e.g. declare -a identifiers and append with
identifiers+=("$1")) and iterate with for identifier in "${identifiers[@]}" and
declare identifier local in the loop; also guard the post-remove hook invocation
run_hooks postRemove by appending || true so a failing postRemove does not abort
processing of remaining identifiers (keep preRemove behavior intact); update
references to the identifiers variable and the loop to use the new array form
and ensure resolve_repo_context usage is unchanged.
In `@lib/commands/run.sh`:
- Around line 58-59: Replace the subshell invocation so the command runs in the
current shell and the original directory is restored afterward: save the current
directory (e.g. old_pwd="$PWD"), cd to "$worktree_path" (fail if cd fails),
execute "${run_args[@]}" capturing its exit status, then cd back to the saved
directory and exit/return with the captured status; refer to the variables
worktree_path and run_args in your changes and ensure the command's exit code is
propagated unchanged.
- Around line 9-23: The argument parser currently treats any token matching -*
as an unknown flag; update the while/case logic so that once identifier is set
the parser stops treating -* as an error and instead assigns remaining args to
run_args (or respects -- as an explicit delimiter). Specifically, in the case
block for "-*" change behavior to: if identifier is empty, keep existing error
path; if identifier is already set, capture remaining args into run_args
(including tokens beginning with -) or handle a `--` token by shifting past it
and then capturing the rest into run_args; adjust uses of identifier, run_args,
shift and break accordingly so commands like `git gtr run myid -n` and `git gtr
run myid -- -n` are passed through to the invoked command.
🧹 Nitpick comments (12)
lib/commands/completion.sh (1)
8-10: Consider guarding against unsetGTR_DIR.If
GTR_DIRis not set when the user runsgit gtr completion bash, thecatcommand will fail with an unclear error. A guard or clearer error message would improve UX.💡 Proposed defensive check
bash) + if [ ! -f "$GTR_DIR/completions/gtr.bash" ]; then + log_error "Completion file not found: $GTR_DIR/completions/gtr.bash" + return 1 + fi cat "$GTR_DIR/completions/gtr.bash" ;;Similar guards could be added for the
fishcase at Line 33..github/instructions/editor.instructions.md (1)
16-18: Add language specifier to fenced code block.Static analysis flagged this code block as missing a language specifier. Adding one improves rendering and consistency.
📝 Proposed fix
-``` +```text yourname|yourcmd|standard|EditorName not found. Install from https://...|flags</details> </blockquote></details> <details> <summary>CONTRIBUTING.md (2)</summary><blockquote> `109-111`: **Add language specifier to fenced code block.** Static analysis flagged this code block as missing a language specifier. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text yourname|yourcmd|standard|YourEditor not found. Install from https://...|flags</details> --- `129-131`: **Add language specifier to fenced code block.** Static analysis flagged this code block as missing a language specifier. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text yourname|yourcmd|YourTool not found. Install with: ...|Extra info line 1;Extra info line 2lib/commands/rename.sh (1)
94-99: Potential Bash 3.2 compatibility issue with empty array expansion.On Bash 3.2 (macOS default), expanding an empty array with
"${move_args[@]}"whenset -uis active can cause an "unbound variable" error. Whileset -umay not be active here, this pattern can be fragile.💡 Proposed alternative using conditional
- local move_args=() - if [ "$force" -eq 1 ]; then - move_args+=(--force) - fi - - if ! git -C "$repo_root" worktree move "$old_path" "$new_path" "${move_args[@]}"; then + local force_flag="" + if [ "$force" -eq 1 ]; then + force_flag="--force" + fi + + # shellcheck disable=SC2086 + if ! git -C "$repo_root" worktree move "$old_path" "$new_path" $force_flag; thenAs per coding guidelines: "Target Bash 3.2+ compatibility (macOS default)."
lib/commands/init.sh (1)
14-55: Bash and Zsh wrappers are nearly identical — consider extracting shared logic.The
gtr()function implementations for bash (lines 14-55) and zsh (lines 69-110) are ~95% identical. While the shell integration output is generated at runtime and must be self-contained, this duplication increases maintenance burden if hook logic changes.This is acceptable for now since the generated code must be standalone, but consider documenting why duplication exists (self-contained shell snippets) to prevent future refactoring attempts that would break the design.
Also applies to: 69-110
.github/instructions/ai.instructions.md (1)
16-18: Add language specifier to fenced code block.The code block showing the registry format is missing a language identifier. Even for plain text formats, specifying a language improves rendering and satisfies markdown linting.
📝 Proposed fix
-``` +```text yourname|yourcmd|ToolName not found. Install with: ...|Extra info;More info</details> </blockquote></details> <details> <summary>lib/commands/clean.sh (2)</summary><blockquote> `86-90`: **Branch deletion fallback may silently fail.** Line 88 attempts safe delete (`-d`), then force delete (`-D`), with `|| true` at the end: ```bash git branch -d "$branch" 2>/dev/null || git branch -D "$branch" 2>/dev/null || trueIf both deletions fail (e.g., branch is checked out elsewhere, or protected), the failure is silently ignored. The worktree was already removed at this point, so this is non-critical, but the user might expect the local branch to be deleted.
Consider logging a warning if branch deletion fails:
📝 Proposed enhancement
# Also delete the local branch - git branch -d "$branch" 2>/dev/null || git branch -D "$branch" 2>/dev/null || true + if ! git branch -d "$branch" 2>/dev/null && ! git branch -D "$branch" 2>/dev/null; then + log_warn "Could not delete local branch: $branch" + fi log_info "Removed: $branch"
134-141: Unknown positional arguments are silently ignored.Lines 138-140 silently discard any positional arguments:
*) shift ;;Users passing unexpected arguments (e.g.,
git gtr clean some-branch) receive no feedback that their input was ignored. Consider warning or erroring on unexpected positional args.📝 Proposed fix
*) - shift + log_error "Unexpected argument: $1" + log_info "Usage: git gtr clean [--merged] [--yes] [--dry-run]" + exit 1 ;;lib/commands/create.sh (3)
83-89: Usecurrent_branch()for consistent detached HEAD handling.Line 83-84 calls
get_current_branch()and manually checks for "HEAD":from_ref=$(get_current_branch) if [ -z "$from_ref" ] || [ "$from_ref" = "HEAD" ]; thenThe
current_branch()wrapper inlib/core.shalready normalizes detached HEAD from "HEAD" to "(detached)". For consistency across the codebase, usecurrent_branchwith the appropriate check:♻️ Proposed fix
if [ -z "$from_ref" ]; then if [ "$from_current" -eq 1 ]; then - from_ref=$(get_current_branch) - if [ -z "$from_ref" ] || [ "$from_ref" = "HEAD" ]; then + from_ref=$(current_branch "$repo_root") + if [ -z "$from_ref" ] || [ "$from_ref" = "(detached)" ]; then log_warn "Currently in detached HEAD state - falling back to default branch" from_ref=$(resolve_default_branch "$repo_root")Note:
current_branch()requires a path argument perlib/core.sh:158-174. If calling from a context where the cwd is already the repo root, you may need to pass"."or"$repo_root".
98-105: Config default helpers are duplicated across command files.
_cfg_editor_default()and_cfg_ai_default()are defined here but also used inlib/commands/doctor.sh. Since all command files are sourced at startup, this works, but the helpers logically belong inlib/config.shor a shared location to:
- Avoid confusion about where they're defined
- Make them available to other consumers (e.g., adapters)
Consider moving these to
lib/config.shin a follow-up.
156-159: Variableopen_editorshadows conceptually related function_open_editor.The local variable
open_editor(line 158) and the function_open_editor(line 110) have similar names, which could cause confusion during maintenance. Consider renaming the flag variable tolaunch_editororshould_open_editorfor clarity.📝 Proposed rename
- local open_editor=0 + local launch_editor=0 local start_ai=0 ... --editor|-e) - open_editor=1 + launch_editor=1 shift ... - [ "$open_editor" -eq 1 ] && { _auto_launch_editor "$worktree_path" || true; } + [ "$launch_editor" -eq 1 ] && { _auto_launch_editor "$worktree_path" || true; } [ "$start_ai" -eq 1 ] && { _auto_launch_ai "$worktree_path" || true; } - if [ "$open_editor" -eq 0 ] && [ "$start_ai" -eq 0 ]; then + if [ "$launch_editor" -eq 0 ] && [ "$start_ai" -eq 0 ]; then
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/commands/ai.sh`:
- Around line 67-69: Replace the two plain echo statements that print Directory
and Branch in ai.sh with calls to the logging helper so they go to stderr;
specifically, after the existing log_step call use log_info to emit "Directory:
$worktree_path" and "Branch: $branch" instead of echo, keeping the log_step and
variable names (worktree_path, branch, ai_tool) intact so stdout remains clean
for the AI tool.
In `@lib/commands/create.sh`:
- Around line 103-142: The flag handlers for --from, --track, --name, and
--folder currently do unguarded shift 2 and assign $2 to from_ref, track_mode,
custom_name, and folder_override; add a validation step before using $2: verify
that $2 is non-empty and does not begin with a hyphen (so it’s not the next
flag), otherwise print a clear error (e.g., "Missing value for --from") and exit
non‑zero. Update the blocks handling --from (from_ref), --track (track_mode),
--name (custom_name), and --folder (folder_override) to perform this check and
only then consume shift 2; leave other flags behavior unchanged.
In `@lib/commands/go.sh`:
- Around line 25-31: The script prints user-facing status directly with echo
(using variables is_main and branch); replace those echo calls with the
standardized logger by calling log_info so messages follow project formatting
and routing: where the code currently does echo "Main repo" >&2 or echo
"Worktree: $branch" >&2, call log_info "Main repo" or log_info "Worktree:
$branch" respectively, and replace echo "Branch: $branch" >&2 with log_info
"Branch: $branch"; remove the ad‑hoc redirected echoes and ensure lib/ui.sh's
log_info is sourced so the function is available.
In `@lib/launch.sh`:
- Around line 32-36: The call to open_in_gui can fail and, under set -e, abort
the script; change the else branch so open_in_gui "$worktree_path" is invoked in
a non-fatal way (e.g., open_in_gui "$worktree_path" || true or wrap it in an if
open_in_gui "$worktree_path"; then :; fi) so that worktree creation is not
aborted—modify the code surrounding the editor check (the branch that calls
open_in_gui and log_info) to guard the open_in_gui invocation while leaving the
log_info behavior unchanged.
🧹 Nitpick comments (3)
lib/commands/rename.sh (1)
97-108: Consider guarding empty array expansion for Bash 3.2 compatibility.In Bash versions prior to 4.4, expanding an empty array with
"${move_args[@]}"whenset -uis active can cause an "unbound variable" error. Whileset -umay not be active here, it's safer to guard against this for defensive coding:♻️ Suggested defensive approach
# Move the worktree local move_args=() if [ "$force" -eq 1 ]; then move_args+=(--force) fi - if ! git -C "$repo_root" worktree move "$old_path" "$new_path" "${move_args[@]}"; then + if ! git -C "$repo_root" worktree move "$old_path" "$new_path" ${move_args[@]+"${move_args[@]}"}; thenlib/commands/list.sh (1)
41-45: Redundant detached HEAD check.The
current_branch()function (perlib/core.sh:158-174) already returns"(detached)"when the branch is empty or equals"HEAD". The check on line 43 is redundant but harmless.♻️ Optional: Remove redundant check
local branch status branch=$(current_branch "$dir") - [ -z "$branch" ] && branch="(detached)" status=$(worktree_status "$dir")lib/commands/adapter.sh (1)
22-26: Quote the function variable for safety.The function variable
$can_funcshould be quoted to prevent word splitting if the variable ever contains spaces (though unlikely in this context).♻️ Suggested fix
- if $can_func 2>/dev/null; then + if "$can_func" 2>/dev/null; thenThe same applies to line 39.
- Introduced a shared argument parser in `lib/args.sh` to streamline command argument handling across various commands. - Updated command scripts to utilize the new parser, improving clarity and maintainability by reducing repetitive code. - Enhanced completion scripts for Bash, Zsh, and Fish to reflect updated command options and improve user experience. - Added integration tests for the argument parser to ensure functionality and reliability in command execution.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/commands/copy.sh`:
- Around line 68-76: The variables used as loop iterators are not declared local
and leak into the function scope; add local declarations for _branch before the
while-read loop (the `while IFS= read -r _branch; do` block) and for target_id
before the for-loop that iterates `for target_id in "${targets[@]}"; do` so both
_branch and target_id are function-local and do not pollute the outer scope.
In `@lib/commands/doctor.sh`:
- Around line 28-37: The variable `prefix` is assigned via cfg_default in the
doctor script but never used, causing a ShellCheck SC2034 warning; either remove
the unused assignment to `prefix=$(cfg_default gtr.worktrees.prefix
GTR_WORKTREES_PREFIX "")` or reintroduce prefix-based filtering into the
worktree count logic (replace the current git worktree list/count pipeline with
one that filters by the `prefix` value), updating references around
resolve_base_dir, the count calculation, and the echo that prints $base_dir and
$count so the script no longer leaves `prefix` unused.
In `@lib/commands/editor.sh`:
- Around line 31-34: The call to open_in_gui in the editor.sh branch should be
guarded so we only call log_info "Opened in file browser" on success; update the
block around open_in_gui "$worktree_path" to check its exit status (e.g., if
open_in_gui "$worktree_path"; then ... else ... fi), call log_error with a
descriptive message when open_in_gui fails, and return or exit with a non-zero
status so failures are propagated instead of logging success; reference the
open_in_gui and log_info/log_error symbols to locate and change the logic.
🧹 Nitpick comments (10)
lib/args.sh (1)
111-117: Verify_pa_match_flagreceives correct arguments.The call
_pa_match_flag "$_pa_spec" "$@"passes all remaining CLI arguments, but_pa_match_flagexpects(spec, flag, next_val). When$@expands,$1becomes the flag and$2becomes next_val, which works correctly. However,$3and beyond are passed but ignored, which is fine.The logic is correct, but consider adding a comment for clarity since this relies on positional expansion behavior.
lib/copy.sh (1)
346-346: Considercaseinstead of[[ ]]for POSIX portability.Line 346 uses
[[ "$pattern" == */* ]]which requires Bash. Per coding guidelines preferring[ ]over[[ ]]for POSIX portability, consider using a case statement:case "$pattern" in */*) ... ;; *) ... ;; esacHowever, since the script already uses Bash-specific features (shopt, arrays) and the shebang is
#!/usr/bin/env bash, this is acceptable.lib/launch.sh (2)
18-25: Consider guardingeditor_openfailure to avoid aborting underset -e.If
editor_openfails (e.g., editor binary not found after adapter load), the command will abort. Since the worktree is already created/resolved at this point, a failed editor launch shouldn't be fatal.🔧 Proposed fix
_open_editor() { local editor="$1" worktree_path="$2" load_editor_adapter "$editor" || return 1 local workspace_file workspace_file=$(resolve_workspace_file "$worktree_path") log_step "Opening in $editor..." - editor_open "$worktree_path" "$workspace_file" + if ! editor_open "$worktree_path" "$workspace_file"; then + log_warn "Failed to open editor" + return 1 + fi }As per coding guidelines, "Keep
set -eactive in shell scripts; ensure non-critical failures are guarded withcommand || true".
50-54: Consider guardingai_startfailure similarly to editor handling.If
ai_startfails after successful adapter load, the script will abort underset -e. Consider consistent error handling:🔧 Proposed fix
else load_ai_adapter "$ai_tool" || return 1 log_step "Starting $ai_tool..." - ai_start "$worktree_path" + if ! ai_start "$worktree_path"; then + log_warn "Failed to start AI tool" + return 1 + fi fiAs per coding guidelines, "Keep
set -eactive in shell scripts; ensure non-critical failures are guarded withcommand || true".scripts/generate-completions.sh (1)
72-79: Minor:xargswith empty input may produce unexpected output.On some systems,
xargswith no input still produces output or runs the default command. Consider using explicit trimming:🔧 Alternative trimming approach
get_config_keys() { local pair keys="" for pair in "${_CFG_KEY_MAP[@]}"; do keys="$keys ${pair%%|*}" done - echo "$keys" | xargs # trim whitespace + # Trim leading/trailing whitespace + keys="${keys# }" + keys="${keys% }" + echo "$keys" }lib/commands/clean.sh (1)
47-57: Verify glob behavior whenprefixis empty.When
prefixis empty,"$base_dir/${prefix}"*becomes"$base_dir/"*, which matches all directories underbase_dir, not just worktrees. This could iterate over unrelated directories ifbase_dircontains non-worktree folders.Consider validating that each
$diris actually a git worktree before processing:🔧 Proposed fix to validate worktree
for dir in "$base_dir/${prefix}"*; do [ -d "$dir" ] || continue + # Skip if not a git worktree + [ -d "$dir/.git" ] || [ -f "$dir/.git" ] || continue local branch branch=$(current_branch "$dir")tests/test_helper.bash (1)
37-52:localdeclaration inside pipeline subshell has no effect.The
whileloop reads from a pipe (| while), which executes in a subshell in Bash. Thelocal wt_pathdeclaration at line 44 is inside this subshell and won't behave as expected—it's scoped to the subshell, not the function. While this doesn't cause a bug here (sincewt_pathis only used within the loop), it's misleading.Consider using process substitution to avoid the subshell, though note this requires Bash 4.0+:
♻️ Bash 4.0+ alternative using process substitution
teardown_integration_repo() { cd / 2>/dev/null || true if [ -d "$TEST_REPO" ]; then # Remove worktrees properly before deleting - git -C "$TEST_REPO" worktree list --porcelain 2>/dev/null | while IFS= read -r line; do + local wt_path + while IFS= read -r line; do case "$line" in "worktree "*) - local wt_path="${line#worktree }" + wt_path="${line#worktree }" [ "$wt_path" = "$TEST_REPO" ] && continue git -C "$TEST_REPO" worktree remove --force "$wt_path" 2>/dev/null || true ;; esac - done + done < <(git -C "$TEST_REPO" worktree list --porcelain 2>/dev/null) rm -rf "$TEST_REPO" "$TEST_WORKTREES_DIR" fi }Given the coding guidelines mention "Bash 3.2+ compatible (macOS default), but 4.0+ features allowed where appropriate," and this is test code, the current implementation is acceptable. Just remove the misleading
localkeyword.lib/commands/create.sh (3)
33-62: Minor: trailing whitespace on line 43.There appears to be trailing whitespace after the
unpack_targetcall. This is a minor formatting issue.♻️ Remove trailing whitespace
unpack_target "$resolve_result" - + if [ "$_ctx_is_main" = "1" ]; then
85-86: Missing blank line before function definition.Per common shell style conventions, there should be a blank line separating the end of
_create_resolve_from_reffrom the start ofcmd_create.♻️ Add blank line
printf "%s" "$from_ref" } + cmd_create() {
169-172: Unhandled failure path leaks partial state.If
create_worktreefails, the script exits but doesn't provide context about what failed. Consider adding a more descriptive error before exiting.♻️ Add error context
# Create the worktree - if ! worktree_path=$(create_worktree "$base_dir" "$prefix" "$branch_name" "$from_ref" "$track_mode" "$skip_fetch" "$force" "$custom_name" "$folder_override"); then + if ! worktree_path=$(create_worktree "$base_dir" "$prefix" "$branch_name" "$from_ref" "$track_mode" "$skip_fetch" "$force" "$custom_name" "$folder_override"); then + log_error "Failed to create worktree for branch: $branch_name" exit 1 fi
- Updated `lib/adapters.sh` to improve documentation and structure for editor and AI tool registries, clarifying the format and loading process. - Introduced new helper functions in `lib/config.sh` for deduplicating and formatting configuration entries, enhancing the `cfg_list` functionality. - Added tests for adapter and configuration functionalities in `tests/adapters.bats` and `tests/config.bats`, ensuring robust validation of registry lookups and configuration mappings. - Improved error handling and output formatting in the configuration listing process, streamlining user interaction and enhancing clarity.
- Enhanced error handling in `lib/launch.sh` to log warnings when failing to open the editor or start the AI tool. - Updated `lib/commands/clean.sh` to ensure the current branch retrieval does not fail the script execution. - Improved user feedback in `lib/commands/editor.sh` by logging warnings when the file browser fails to open. - Added local variables in `lib/commands/copy.sh` for better clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/adapters.sh`:
- Around line 286-297: The _load_adapter function builds adapter_file from the
user-controlled name, which allows path traversal; validate and reject unsafe
names before constructing adapter_file: check the name variable for disallowed
patterns (e.g. contains '/' or '..' or any path-separators) and enforce a
whitelist like only alphanumeric, dash, underscore (e.g. ^[A-Za-z0-9_-]+$); if
validation fails, log an error and return non-zero instead of sourcing; update
references in _load_adapter and any callers to use this validation (or use
basename as a secondary safeguard) so adapter_file cannot escape
$GTR_DIR/adapters.
In `@lib/core.sh`:
- Around line 309-326: The _try_worktree_add function uses an unquoted
$force_flag in the git worktree add call; replace this by building a small local
array (e.g., force_args) that contains the flag only when $force_flag is
non-empty, then pass the array quoted as "${force_args[@]}" alongside "$wt_path"
and "$@" to git so variables are correctly quoted and the shellcheck disable is
unnecessary; update references to force_flag usage in _try_worktree_add
accordingly.
- Removed the unused helper function `__fish_git_gtr_config_is_write` from `completions/git-gtr.fish` and `scripts/generate-completions.sh` to streamline the code. - Deleted the `copy_file` function from `lib/copy.sh` as it was redundant and not utilized in the current codebase. - Eliminated the `list_worktrees` function from `lib/core.sh` to enhance clarity and maintainability of the code.
- Updated the `_try_worktree_add` and `create_worktree` functions to eliminate the use of a separate force flag, replacing it with an array for force arguments. - Improved clarity in the command usage documentation and streamlined the logic for adding and removing worktrees. - Enhanced maintainability by reducing complexity in the argument handling for worktree operations.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@lib/commands/create.sh`:
- Around line 58-62: Replace the user-facing echo calls that print the "Next
steps:" block (the three echo lines referencing next_steps_id and the empty
echo) with the logging helpers from lib/ui.sh: use log_info for the
informational lines (including the header "Next steps:" and each command line
that references the next_steps_id variable) so output follows standard
formatting and stderr routing; if any of these were intended as error messages
use log_error instead. Apply the same replacement to the other echo occurrences
around the later block (the lines noted at 167–170) so all user-facing messages
in create.sh use log_info/log_error consistently.
- Around line 171-174: The variable worktree_path is currently assigned without
scoping and leaks to the caller; declare it as a local function-scoped variable
before you assign it. Specifically, add a local declaration for worktree_path in
the function that calls create_worktree (so that worktree_path is local) and
then perform the command substitution with create_worktree as before (i.e.,
declare local worktree_path; then if ! worktree_path=$(create_worktree ...)
...). This ensures worktree_path is limited to the function scope and prevents
leakage to the caller.
In `@lib/commands/go.sh`:
- Around line 8-10: The if-statement uses an unquoted array length expansion
(_pa_positional array) which violates shell quoting guidelines; update the
conditional that checks the array length (the expression referencing the array
length of _pa_positional) to use a quoted expansion so the test reads the quoted
length value, e.g., replace the unquoted ${`#_pa_positional`[@]} with a quoted
form and keep the rest of the conditional logic the same.
In `@lib/commands/rename.sh`:
- Around line 55-57: Replace direct echo user-facing messages with the UI
logging helpers so output goes to stderr and follows the standard formatter: in
the rename flow replace the echo lines that print branch and folder (referencing
variables old_branch, new_name, old_path, prefix, new_sanitized) with calls to
log_info (and use log_error where appropriate for failures); also apply the same
replacement for the subsequent messages in the same script block (the messages
around the 87-96 region) so all user status lines use lib/ui.sh's
log_info/log_error instead of echo.
- Around line 74-80: The git worktree move invocation places options after
positional args so flags like --force are ignored; update the call that uses
move_args (the git -C "$repo_root" worktree move "$old_path" "$new_path"
"${move_args[@]}") to place "${move_args[@]}" immediately after the subcommand
(e.g., git -C "$repo_root" worktree move "${move_args[@]}" "$old_path"
"$new_path") so --force is passed correctly when move_args contains it.
In `@lib/commands/run.sh`:
- Around line 50-52: Quote the array-length expansion in the run args check to
follow shell quoting rules: change the if condition that currently uses
${`#run_args`[@]} to use "${`#run_args`[@]}" so the test becomes if [
"${`#run_args`[@]}" -eq 0 ]; then; this touches the run_args array length check in
the run.sh file and leaves the existing log_error "Usage: git gtr run
<id|branch> <command...>" and log_error "No command specified" calls unchanged.
- Around line 66-73: The current code prints user-facing messages with echo
(echo "Command: ${run_args[*]}" and echo ""), bypassing the standardized logger;
replace those echoes with the project's logging helper: call log_info to emit
the "Command: ${run_args[*]}" message (preserving the same content) and remove
or replace the empty echo with an appropriate log_info or no-op so output
follows the log_step/log_info formatting; update the block around log_step and
the run_args usage in run.sh so all user messages use log_info (and keep
log_step as-is).
In `@lib/copy.sh`:
- Around line 295-324: The pattern check using [[ "$pattern" == */* ]] in the
command substitution is non-POSIX; change it to a POSIX-compatible test (e.g.,
use a case statement or expr) so the find invocation works on /bin/sh.
Specifically, locate the command substitution that starts with $(if [[
"$pattern" == */* ]]; then find . -type d -path "./$pattern" ...; else find .
-type d -name "$pattern" ...; fi) and replace the [[ ... ]] conditional with a
POSIX-safe alternative (for example: use case "$pattern" in */*) ... ;; *) ...
;; esac) while preserving the existing find branches, quoting, and the here-doc
feeding the while loop that iterates over dir_path.
In `@scripts/generate-completions.sh`:
- Around line 90-92: The generated completion script uses a hardcoded shebang
"#!/bin/bash" in the here-doc template; update the template in
scripts/generate-completions.sh (the cat <<'HEADER' block that emits the header)
to replace "#!/bin/bash" with "#!/usr/bin/env bash" so generated scripts follow
the coding guideline.
- Changed the shebang in `gtr.bash` and `generate-completions.sh` to use `/usr/bin/env bash` for better portability. - Refactored pattern matching logic in `copy.sh` to utilize a case statement, enhancing readability and maintainability. - Added a local variable declaration in `create.sh` for improved clarity in the command execution flow. - Updated conditional checks in `go.sh` and `run.sh` to use quotes around array length checks, preventing potential errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/commands/create.sh`:
- Around line 124-132: Replace the raw echo calls that print the example usage
in the error branch with the UI logging helpers so messages go through
lib/ui.sh; keep the same text and branching on $branch_name but call log_info
(or log_error if you want them treated as errors) instead of echo for the two
example lines, and ensure you still call log_error(...) first for the main
message (this affects the block around the variables force, custom_name,
folder_override and the branch_name conditional).
In `@lib/copy.sh`:
- Around line 322-324: Guard the two find invocations inside the here-doc so
they can't cause a set -e exit: update the case arms that call find (the find .
-type d -path "./$pattern" and find . -type d -name "$pattern") to append a
non-fatal fallback (e.g., "|| true") or otherwise ensure they return zero on
permission failures, so the here-doc substitution won't abort the script when
find returns non-zero.
🧹 Nitpick comments (2)
scripts/generate-completions.sh (2)
36-38: Consider adding a warning if registry data is empty.Sourcing errors are silently suppressed, which could result in empty or incomplete completion lists without any indication. This might complicate debugging if completions are missing.
🔧 Optional: Add post-source validation
# shellcheck disable=SC1091 . "$SCRIPT_DIR/lib/adapters.sh" 2>/dev/null || true # shellcheck disable=SC1091 . "$SCRIPT_DIR/lib/config.sh" 2>/dev/null || true + +# Validate critical data was loaded +if [ -z "${_EDITOR_REGISTRY:-}" ] && [ -z "${_AI_REGISTRY:-}" ]; then + echo "Warning: No adapter registries found - completions may be incomplete" >&2 +fi
593-616: Consider documenting intentional word-splitting.The
for key in $CONFIG_KEYSloop intentionally uses unquoted expansion for word-splitting, which is an exception to the "always quote variables" guideline. A brief comment clarifying this is intentional would help future maintainers.
- Updated CLAUDE.md to clarify testing procedures, including automated BATS tests and manual smoke tests. - Introduced new functions in `lib/args.sh` for validating positional arguments, improving error handling in command scripts. - Refactored `lib/copy.sh` to streamline file copying logic with a new `_expand_and_copy_pattern` function, enhancing maintainability. - Improved `create_worktree` and related functions in `lib/core.sh` for better folder name resolution and branch checking. - Added comprehensive tests for new and existing functionalities, ensuring robust validation of command behaviors and error handling.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@lib/args.sh`:
- Around line 103-105: The -h|--help case in parse_args currently calls
show_command_help but doesn't stop execution; update the case so that after
calling show_command_help it exits with status 0 (e.g., call exit 0) to prevent
further parsing/command execution when help is requested; locate the -h|--help
branch in the parse_args switch/case and add the exit 0 immediately after the
show_command_help invocation.
In `@lib/commands/clean.sh`:
- Around line 161-162: The current pipeline uses grep -v "^${base_dir}$" which
treats ${base_dir} as a regex and can mis-exclude or fail on paths with regex
metacharacters; update the filtering step that uses find and grep (the
empty_dirs assignment) to perform a literal, whole-line exclusion of base_dir by
using fixed-string and exact-match options (e.g., grep -vFx or grep -v -F -x) or
otherwise escape ${base_dir} so it is matched literally; ensure you modify the
empty_dirs line that invokes find "$base_dir" ... | grep ... so the base_dir is
excluded safely for any path containing characters like . + * [ ] etc.
In `@tests/cmd_clean.bats`:
- Around line 59-72: The two tests "_clean_should_skip skips dirty worktree" and
"_clean_should_skip skips worktree with untracked files" call _clean_should_skip
directly but omit the run/status assertion pattern used elsewhere; update each
to invoke the helper via the Bats runner (use run _clean_should_skip
"$TEST_WORKTREES_DIR/dirty-test" "dirty-test" and run _clean_should_skip
"$TEST_WORKTREES_DIR/untracked-test" "untracked-test") and add an assertion that
the test exit status equals 0 (e.g. check "$status" -eq 0) so the tests mirror
the correct pattern used in the other test block.
- Around line 49-57: The two tests call _clean_should_skip but never assert its
exit status, so update each test to use Bats' run to invoke _clean_should_skip
with the same arguments (e.g., run _clean_should_skip "/some/dir" "(detached)"
and run _clean_should_skip "/some/dir" "") and then assert the function returned
success by checking the status (e.g., [ "$status" -eq 0 ] or using an existing
assert_success helper if present); target the test cases that call
_clean_should_skip to ensure they explicitly verify the skip return value.
In `@tests/hooks.bats`:
- Line 21: The hook command added to the git config uses an unquoted variable
which will break if TEST_REPO contains spaces; update the gtr.hook.postCreate
value to quote the path when building the command (reference the config key
gtr.hook.postCreate and the hook command that runs touch $TEST_REPO/hook-ran) so
the shell receives a single proper path token (i.e., ensure the command
constructs touch "$TEST_REPO/hook-ran" when writing the config).
- Around line 38-44: The two git config --add gtr.hook.postCreate lines in the
"run_hooks executes multiple hooks in order" test embed an unquoted $TEST_REPO
into the hook commands; update both hook command strings to quote $TEST_REPO
(e.g., ensure the hook writes to "$TEST_REPO/order" rather than
$TEST_REPO/order) so paths with spaces are handled safely, keeping the rest of
the test (run_hooks postCreate REPO_ROOT="$TEST_REPO" and the assertions)
unchanged.
🧹 Nitpick comments (2)
tests/core_create_worktree.bats (1)
50-63: Consider adding remote branch detection test if feasible.The tests cover local branch detection and the unknown branch case, but don't test remote branch detection. This is understandable given that the test repo doesn't have a remote origin. If the test infrastructure supports it in the future, adding a test for
_wt_remote_existswould improve coverage.tests/test_helper.bash (1)
41-49:localdeclaration inside pipeline subshell has no effect.The
whileloop is in a pipeline (git ... | while ...), which runs in a subshell. Thelocalkeyword on line 44 declares a variable in that subshell, but since the entire loop is discarded after the pipeline completes, this is harmless—but also misleading.Consider removing
localor restructuring to avoid the pipeline subshell (e.g., using process substitution if Bash 4+ is acceptable for tests):Option: Remove misleading `local` keyword
case "$line" in "worktree "*) - local wt_path="${line#worktree }" + wt_path="${line#worktree }" [ "$wt_path" = "$TEST_REPO" ] && continue git -C "$TEST_REPO" worktree remove --force "$wt_path" 2>/dev/null || true ;;
- Updated the `clean.sh` script to enhance the logic for finding empty directories by using `grep -Fxv` for better accuracy. - Modified tests in `cmd_clean.bats` and `hooks.bats` to use `run` for executing commands, improving consistency in test status checks and output handling. - Ensured that hooks in `hooks.bats` reference the correct variable for repository paths, enhancing portability and reliability of hook execution.
helizaga
changed the title
helizaga
changed the title
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests / Chores