Conversation
Keep the porcelain -zs path in _forgit_worktree_changes so filenames with backslashes continue to work. Add a small normalization step that colorizes plain '?? ' lines before filtering, plus helper tests for the fallback behavior.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces an internal fzf delimiter ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as git-forgit
participant Git
participant FZF
User->>Script: invoke "forgit add"
Script->>Git: run git status (porcelain + colors)
Git-->>Script: status lines
Script->>Script: normalize colors & build "label{_ffsep}abs_path" rows
Script->>FZF: feed rows with --delimiter=$_ffsep --with-nth=1 --accept-nth=2 (preview uses {2})
FZF-->>User: interactive list + preview
User->>FZF: select entries
FZF-->>Script: return selected hidden fields (abs paths)
Script->>Git: perform git add using returned abs paths
Script-->>User: completion feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This MR fixes the issue, however I unfortunately found another issue introduced by 2c17635: When using |
wfxr
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/git-forgit`:
- Around line 608-612: The REQUIRED_FZF_VERSION variable needs to be raised to
at least 0.60.0 because the script uses the --accept-nth option (see the fzf
invocation with --accept-nth=2); update the REQUIRED_FZF_VERSION constant
(REQUIRED_FZF_VERSION) to "0.60.0" or later and ensure any version-check logic
that validates fzf meets this new minimum so the script won't run with older fzf
that lacks --accept-nth.
In `@tests/preview-context.test.sh`:
- Around line 70-92: The two new tests
(test_custom_whitespace_delimiter_restores_with_nth_behavior and
test_global_forgit_delimiter_breaks_with_nth_without_override) call the real fzf
binary and fail in CI where fzf is absent; modify the tests to either (A)
early-skip when fzf is not installed by checking command -v fzf and calling skip
or return, or (B) mock fzf like the existing tests do by defining a shell
function or temporary script named fzf in the test scope that simulates the
expected --filter behavior for the provided inputs; update the two functions to
use one of these approaches so CI no longer errors with "fzf: command not
found".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 145493d0-d076-45a0-bbf1-baca6bcedc86
📒 Files selected for processing (3)
bin/git-forgittests/preview-context.test.shtests/working-tree-changes.test.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/working-tree-changes.test.sh
@sandr01d Thanks for pointing this out. You were right that keeping I ended up addressing that by changing the add picker so it no longer relies on parsing the visible This also keeps the old-Git untracked fallback in place, so the PR now covers both issues:
I also added regressions for subdirectory paths and special filenames (spaces, tabs, backslashes), plus coverage for the old plain More broadly, I think this is also a better pattern for forgit in general: instead of reparsing a human-readable fzf line back into a path, we can keep the visible label and the real payload separate. That should give us a cleaner way to fix similar path-parsing edge cases elsewhere over time. |
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
2 times, most recently
from
d039094 to
d2a855c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/working-tree-changes.test.sh (2)
137-153: Consider separating stderr from stdout in zsh test.The
2>&1redirect merges stderr into stdout, which could cause assertions to pass/fail based on error messages rather than actual function output. If the intent is to suppress or capture errors, consider handling them separately.♻️ Potential improvement
output=$( zsh -c ' source "'"$FORGIT_REPO_ROOT"'/bin/git-forgit" cd "$(mktemp -d)" || exit 1 git init --quiet touch "space name.txt" "back\\slash.txt" $'"'"'tab\tname.txt'"'"' _forgit_worktree_changes - ' 2>&1 + ' )Or if error capture is needed for debugging:
output=$(zsh -c '...' 2>/dev/null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/working-tree-changes.test.sh` around lines 137 - 153, The test function test_forgit_worktree_changes_works_in_zsh currently merges stderr into stdout via "2>&1", which can let error messages affect assertions; update the zsh invocation in test_forgit_worktree_changes_works_in_zsh that calls _forgit_worktree_changes so stderr is not merged (either remove "2>&1" or redirect stderr to /dev/null if you want to ignore errors), or capture stderr separately if you need to inspect it—ensure the change only affects the command substitution that sets output so assertions (assert_contains) only examine the function's stdout.
87-98: Minor style inconsistency: missingfunctionkeyword.This test function and several others below (lines 117, 129, 137) omit the
functionkeyword prefix that other tests in this file use (e.g., lines 31, 39, 47, 71). Consider using a consistent style throughout the file.♻️ Suggested fix for consistency
-function test_forgit_worktree_changes_emits_absolute_payloads_for_subdir_entries() { +function test_forgit_worktree_changes_emits_absolute_payloads_for_subdir_entries() {Note: Actually looking more closely, both styles appear in the file. The newer tests use
function name()without the leading space. This is a very minor formatting nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/working-tree-changes.test.sh` around lines 87 - 98, Several test functions use an inconsistent definition style; update the functions (e.g., test_forgit_worktree_changes_emits_absolute_payloads_for_subdir_entries and the tests referenced around lines 117, 129, 137) to follow the project's preferred style by declaring them with the `function name()` form (matching other tests like those around lines 31–71), keeping the same spacing/parenthesis convention so all test definitions use the same `function` keyword format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/working-tree-changes.test.sh`:
- Around line 137-153: The test function
test_forgit_worktree_changes_works_in_zsh currently merges stderr into stdout
via "2>&1", which can let error messages affect assertions; update the zsh
invocation in test_forgit_worktree_changes_works_in_zsh that calls
_forgit_worktree_changes so stderr is not merged (either remove "2>&1" or
redirect stderr to /dev/null if you want to ignore errors), or capture stderr
separately if you need to inspect it—ensure the change only affects the command
substitution that sets output so assertions (assert_contains) only examine the
function's stdout.
- Around line 87-98: Several test functions use an inconsistent definition
style; update the functions (e.g.,
test_forgit_worktree_changes_emits_absolute_payloads_for_subdir_entries and the
tests referenced around lines 117, 129, 137) to follow the project's preferred
style by declaring them with the `function name()` form (matching other tests
like those around lines 31–71), keeping the same spacing/parenthesis convention
so all test definitions use the same `function` keyword format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d254086-0086-4977-8a12-d423cd54e79e
📒 Files selected for processing (3)
bin/git-forgittests/preview-context.test.shtests/working-tree-changes.test.sh
✅ Files skipped from review due to trivial changes (1)
- bin/git-forgit
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/preview-context.test.sh
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
2 times, most recently
from
a3d521a to
1b8e2db
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/git-forgit`:
- Around line 298-303: The sed expression currently uses [[:space:]]+ which
swallows any leading spaces/tabs that belong to the filename; update the sed in
the pipeline (the invocation starting with sed -E
$'s/^(..[^[:space:]]*)[[:space:]]+(.*)$/\\1\t\\2/') so it only consumes the
single separator between the two-character status and the path (e.g. replace
[[:space:]]+ with a single separator like [[:space:]] or explicitly match one
space/tab) to preserve leading whitespace in the captured repo_path; keep the
downstream IFS=$'\t' read -r line_status repo_path, and unchanged use of
display_path=$(_forgit_display_path_from_repo_path "$repo_path" "$prefix"
"$cdup") and the printf that prints "$repo_path".
- Line 35: FORGIT_FZF_DEFAULT_OPTS currently sets --delimiter=$_ffsep globally
which breaks fzf field expansion in commands like _forgit_ignore and
_forgit_attributes; remove the --delimiter=$_ffsep from FORGIT_FZF_DEFAULT_OPTS
and instead add that delimiter only where needed (e.g., when constructing the
fzf options inside _forgit_add or its helper that builds the fzf command), so
_forgit_add uses "--delimiter=$_ffsep" locally while
_forgit_ignore/_forgit_attributes continue using the default whitespace
delimiter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f294058-04e1-4b3c-a465-e232d4af1ecc
📒 Files selected for processing (4)
bin/git-forgittests/fzf.test.shtests/preview-context.test.shtests/working-tree-changes.test.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/preview-context.test.sh
- tests/working-tree-changes.test.sh
Keep the porcelain -zs worktree path for forgit add so filenames with backslashes still preview correctly. Restore untracked entries on older Git versions, split the visible add label from its hidden path payload, and cover subdirectory and special-character paths without breaking selectors that still rely on whitespace field splitting.
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
from
1b8e2db to
c4cbf7d
Compare
Write display paths into a caller-provided variable so _forgit_format_add_lines no longer uses command substitution for every worktree entry. This keeps the add-path formatting logic centralized, documents the three display-path branches inline, and updates the tests to exercise each branch through the new helper contract.
There was a problem hiding this comment.
Thank you @wfxr for fixing this! I think we're very close to a solution that finally solves all of these issues.
More broadly, I think this is also a better pattern for forgit in general: instead of reparsing a human-readable fzf line back into a path, we can keep the visible label and the real payload separate. That should give us a cleaner way to fix similar path-parsing edge cases elsewhere over time.
I very much agree! Separating display from logic is always a good idea. The --accept-nth option of fzf looks very useful for us in general too. I think we might also be able to replace some awk commands with it too.
bin/git-forgit
Outdated
| resolved_path=${repo_path#"$prefix"} | ||
| elif [[ -n "$cdup" ]]; then | ||
| # Path is outside the current subdirectory. | ||
| resolved_path=${cdup}${repo_path} |
There was a problem hiding this comment.
This will cause some files to be displayed somewhat incorrectly. Take the following example:
Assume you have a repository with the following structure:
root
└─ dir1
└─ file1
└─ dir2
└─ file2
When your cwd is dir2 and you want to add file1, it will show up as ../../dir1/file1, whereas before it used to be ../file1.
I think we could fix this by passing the toplevel (git rev-parse --show-toplevel) concatenated with $repo_path to realpath --relative-to=.. This would also drastically simplify this function:
_forgit_display_path_from_repo_path() {
local repo_path outvar toplevel resolved_path
repo_path=$1
outvar=$2
toplevel=$(git rev-parse --show-toplevel)
resolved_path=$(realpath --relative-to=. "$toplevel/$repo_path")
printf -v "$outvar" '%s' "$resolved_path"
}There was a problem hiding this comment.
Thanks, good catch.
I agree the current display-path logic is still too manual. The right behavior here is to compute a path relative to the current working directory, not just rebuild it from prefix/cdup.
Considering realpath --relative-to is not available on macOS by default, I’m going to rework this with a small portable shell helper so it behaves consistently on both macOS and Linux.
There was a problem hiding this comment.
Gotta say I'm not happy about the perl script. I agree with you that this is probably fine regarding portability, but IMO this adds a lot of complexity (I consider the complexity of adding a second programming language to the project high). I also think having the perl script inlined as a string hurts readability (no syntax highlighting). Given that the issue we're trying to solve is very minor (no preview for files containing backslashes), I'm starting to question whether this is actually worth adding all this complexity for.
I researched for alternatives to realpath on macOS and found this thread that claims that macOS 13+ does come with realpath preinstalled. Given that macOS 12 is EOL it might be possible to use realpath after all?
There was a problem hiding this comment.
@sandr01d I looked into this again. Unfortunately, the realpath available on macOS is not the GNU version and does not support --relative-to, which is required here to turn Git's repo-root-relative paths into paths relative to the current working directory. So we can't rely on realpath for this use case. Beyond portability, there's also a performance consideration — the current implementation is designed as a streaming pipeline that processes all paths in one pass via stdin/stdout. A realpath-based approach would fork a new process for every path from git status, which will cause noticeable latency in repos with a large number of untracked files.
As for the Perl concern — I agree that inline Perl is less readable than plain shell. The reason I went with it here is that it keeps path normalization and relative-path conversion in a single batch step, instead of pushing that logic back into fragile shell string handling or per-path subprocesses. Perl is already available on the platforms we support in CI, so this does not add a new practical dependency for supported environments. With the logic isolated in one helper and covered by tests, I think the complexity stays manageable.
It's also worth noting that _forgit_build_status_entries is designed as shared infrastructure, not just a one-off fix for backslash filenames in forgit::add. The goal is to use it across all commands that involve path selection, so the investment will pay off as we roll it out more broadly. We've had quite a few path-related issues and bugs over time — if this approach can address them systematically, I think the added complexity is well justified.
Let me know if you still think the tradeoff is not worth it, and I can revisit the approach.
bin/git-forgit
Outdated
| resolved_path=$repo_path | ||
| fi | ||
|
|
||
| printf -v "$outvar" '%s' "$resolved_path" |
There was a problem hiding this comment.
Would be helpful to have a comment why this IMO unusual assignment is necessary instead of just printing to stdout (AFAIK because the stdout would become visible in fzf).
There was a problem hiding this comment.
Good point. I’d prefer to keep the output-variable style here, since this helper runs in the per-entry formatting path and returning through stdout would require a command substitution for each line, which in turn introduces a subshell/fork in a hot path. I’ll add a short comment to make that clearer.
bin/git-forgit
Outdated
| rootdir=$(git rev-parse --show-toplevel) | ||
| prefix=$(git rev-parse --show-prefix) | ||
| cdup=$(git rev-parse --show-cdup) |
There was a problem hiding this comment.
We assign these variables here, but only to pass them to other functions. I think it would make things easier to read if we would run the git rev-parse commands inside the functions where we need the results.
There was a problem hiding this comment.
I’m going to simplify this a bit, but probably not move all of it into the inner helpers. prefix/cdup can go away with the display-path rework, but I’d still prefer to resolve the repo root once up front rather than re- running git rev-parse in the per-entry path.
There was a problem hiding this comment.
I initially tried to keep this in pure Bash in 6326a2d, mainly to avoid adding another runtime dependency here. That worked locally, but the shell-based path conversion still turned out to be too fragile, and there was still a macOS CI failure: https://github.com/wfxr/forgit/actions/runs/23883072484/job/69640106388
There was also a performance concern with continuing down that route. I had already tried to avoid per-entry overhead in d705540, so I did not want to keep pushing the display-path conversion into logic that might require command substitution or one external process per file.
The final version in 362b0e4 moves the path conversion into a single batch normalization step using Perl. I think Perl is the best fit here because it works naturally as one stdin-to-stdout pipeline stage, the path handling comes from core modules (Cwd::realpath and File::Spec->abs2rel), and it avoids the per-entry process cost that a shell-based realpath approach would introduce.
I don’t think this hurts portability in practice, since it stays within the default toolchain available on both macOS and Ubuntu CI and does not rely on GNU-specific realpath behavior.
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
2 times, most recently
from
359852b to
d58c993
Compare
Keep `forgit add` showing intuitive cwd-relative paths even when Git reports repo-root-relative entries, so the picker remains easy to read from nested directories. The previous path conversion depended on shell-level prefix matching. That was fragile across platforms and failed on macOS when the current directory and repository root were represented through different logical and physical path forms. In those cases, the picker showed incorrect long fallback paths instead of the expected relative ones. Build status picker entries in a single batch normalization step and add coverage for subdirectory, sibling-directory, and logical-symlink path scenarios. This preserves the hidden absolute payload used by picker actions while making the displayed paths consistent across platforms.
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
from
d58c993 to
362b0e4
Compare
2 participants
Check list
Description
Fixes #503 without reverting 2c17635.
Keep the
status --porcelain -zspath in_forgit_worktree_changes()sofilenames with backslashes still work, and restore untracked entries on older
Git versions before status filtering.
To avoid reparsing quoted
git statusoutput, this changesforgit addtoseparate the visible selector label from its hidden path payload. The add
picker now uses an internal fzf separator so preview, edit, and add actions
operate on the real path even for subdirectory entries and special filenames.
The PR also adds regressions for:
??lines without colorType of change
Test environment
Summary by CodeRabbit
New Features
Bug Fixes
Tests