fix: preserve add paths across special filenames#506
Conversation
|
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
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
from
1b8e2db to
c4cbf7d
Compare
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.
| 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.
There was a problem hiding this comment.
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.
I see, thanks for looking into it.
Regarding the perl script, I think that you have good arguments. I am still not 100% happy with it due to the reasons I've stated above but, as you mentioned, it is a tradeoff. I'm also not aware of a better solution, so I won't stay in the way and approve this PR.
I've done my best to review the perl code, but I have no experience with perl at all, so I'd appreciate if @cjappl or @carlfriedrich could give this a second pair of eyes.
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
3 times, most recently
from
d58c993 to
362b0e4
Compare
|
I think we should squash all the commits from this PR together, as they all relate to the same change with multiple rounds of fixes. I prefer squashing addressed reviewer feedback in general, because I think those commits unnecessarily pollute the history. |
|
This restores relative path. diff --git a/bin/git-forgit b/bin/git-forgit
index cd7b2da..b12e857 100755
--- a/bin/git-forgit
+++ b/bin/git-forgit
@@ -218,16 +218,31 @@ _forgit_list_files() {
# Always includes modified and unmerged files. Includes untracked files when
# status.showUntrackedFiles is true or unset. Never includes staged files.
_forgit_worktree_changes() {
- local changed unmerged untracked show_untracked
+ local changed unmerged untracked show_untracked rootdir cdup pwd cdup_level i
+ local -a cdup_filter
changed=$(git config --get-color color.status.changed red)
unmerged=$(git config --get-color color.status.unmerged red)
untracked=$(git config --get-color color.status.untracked red)
show_untracked=$(git config status.showUntrackedFiles)
+ rootdir=$(git rev-parse --show-toplevel)
+ pwd="$PWD"
+ cdup_level=$(
+ git rev-parse --show-cdup |
+ tr -s '/' '\n' |
+ wc -l
+ )
+
+ cdup_filter+=(-e "s/^(..[^[:space:]]*)([[:space:]]+)(.*)$/\1\2${rootdir//\//\\/}\/\3/")
+ for ((i=0; i <= cdup_level; i++));do
+ cdup_filter+=(-e "s/^(..[^[:space:]]*)([[:space:]]+)${pwd//\//\\/}\\/(.*)$/\1\2$cdup\3/")
+ pwd="${pwd%/*}"
+ cdup+="..\\/"
+ done
- git -c color.status=always -c status.relativePaths=true status --porcelain -zs --untracked="${show_untracked:-all}" |
- tr '\0' '\n' |
- grep -F -e "$changed" -e "$unmerged" -e "$untracked" |
- sed -E 's/^(..[^[:space:]]*)[[:space:]]+(.*)$/[\1] \2/'
+ git -c color.status=always status --porcelain -zs --untracked="${show_untracked:-all}" |
+ grep -z -F -e "$changed" -e "$unmerged" -e "$untracked" |
+ sed -zE "${cdup_filter[@]}" -e 's/^(..[^[:space:]]*)[[:space:]]+(.*)$/[\1] \2/' |
+ xargs -0 printf '%s\n'
}
_forgit_is_submodule() { |
|
Thanks for sharing @ahmubashshir. I think this is a smart solution. @wfxr do you think we could include this in this PR instead of the perl script? |
@ahmubashshir @sandr01d Thanks for sharing this. I tried to map it against the current implementation and I do not think we can adopt it as-is. The main issue is that the current There are also a few regressions / constraints it does not cover yet:
So I think the idea is useful as a sketch for “display-path rewriting inside |
Keep `forgit add` on the porcelain `git status --porcelain -zs` path so filenames containing backslashes continue to work, while restoring the behaviors that regressed when we moved away from Git's cwd-relative output. This change makes the status picker emit a display label and a hidden absolute-path payload separately. That lets the UI keep showing intuitive cwd-relative paths, while preview, edit, and add actions operate on the real path instead of reparsing the rendered status line. As a result, untracked files, subdirectory workflows, and special filenames now share one consistent path flow. It also restores the old-Git fallback for plain `?? path` output before status filtering, raises the required fzf version for `--accept-nth`, and adds regression coverage for backslashes, spaces, tabs, subdirectory entries, sibling directories, and logical symlink paths. We explored a few alternatives before landing here. Keeping a single human-readable line and reparsing it downstream remained too fragile for quoted paths and backslashes. Shell-only display-path rewriting worked for some cases but stayed brittle across logical vs physical paths and still failed in macOS CI. A per-path `realpath` approach would have been easier to read, but GNU-style relative-path support is not portable across the platforms we test and would add one external process per file in a hot path. The final tradeoff keeps the pipeline batch-oriented and portable by doing the path normalization once in a single helper step, even though that is less lightweight than the earlier shell-only versions. That complexity is justified here because it fixes the old-Git untracked regression, preserves correct preview/add behavior from subdirectories, and avoids reintroducing long-standing filename parsing bugs.
wfxr
force-pushed
the
fix/untracked-color-fallback
branch
from
362b0e4 to
77ce82a
Compare
|
Gotcha @wfxr. Let's keep what we have then. |
4 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