feat: add interactive git restore selector#537
Open
sandr01d wants to merge 1 commit into
Open
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:
📝 WalkthroughWalkthroughAdds an interactive ChangesGit Restore Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant ForgitRestore as _forgit_restore
participant StagedList as _forgit_list_staged_files
participant FZF
participant Preview as _forgit_restore_preview
participant GitRestore as git restore
User->>ForgitRestore: invoke `forgit restore [flags]`
ForgitRestore->>StagedList: collect staged paths (if --staged)
ForgitRestore->>FZF: assemble candidate list (worktree/staged)
FZF->>Preview: request `git diff` preview for highlighted file
User->>FZF: confirm selection(s)
ForgitRestore->>GitRestore: execute `git restore` on selections
GitRestore-->>User: restored files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![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
🧹 Nitpick comments (2)
tests/list-staged-files.sh (1)
23-26: ⚡ Quick winUse a local variable for test output.
outputis currently global in the test function; make it local to avoid cross-test state leakage.Suggested patch
function test_list_staged_files_only_lists_staged() { - output=$(_forgit_list_staged_files) + local output + output=$(_forgit_list_staged_files) assert_same "$output" "staged.txt" }As per coding guidelines,
**/*.{sh,zsh,fish}: “Use small helper functions withlocalvariables inside functions and descriptive private names such as_forgit_extract_branch_name.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/list-staged-files.sh` around lines 23 - 26, The test function test_list_staged_files_only_lists_staged uses a global variable output; change it to a local variable to avoid cross-test leakage by declaring local output and assigning it from the helper _forgit_list_staged_files (e.g., local output=$(_forgit_list_staged_files)), keeping the assert_same call unchanged to reference the local output.Source: Coding guidelines
tests/restore.test.sh (1)
17-49: ⚡ Quick winScope test variables with
localin each function.
output,staged,worktree, andpreview_argare global right now; declaring them local keeps tests isolated.Suggested patch
function test_restore_shows_message_when_no_modified_files() { + local output output=$(_forgit_restore 2>&1) assert_general_error assert_same "Nothing to restore." "$output" } function test_parse_restore_args() { + local staged worktree preview_arg IFS=$'\t' read -r staged worktree preview_arg < <(_forgit_parse_restore_flags --staged)As per coding guidelines,
**/*.{sh,zsh,fish}: “Use small helper functions withlocalvariables inside functions and descriptive private names such as_forgit_extract_branch_name.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/restore.test.sh` around lines 17 - 49, The tests declare globals; make variables local inside each test function to avoid leaks: in test_restore_shows_message_when_no_modified_files() mark output as local, and in test_parse_restore_args() mark staged, worktree, and preview_arg as local before the IFS/read lines; keep the same variable names and behavior (no other logic changes) so only scope changes for functions _forgit_restore and _forgit_parse_restore_flags are applied.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/git-forgit`:
- Around line 683-685: The _forgit_restore_preview function forwards
user-supplied paths to git diff without an option terminator, allowing filenames
like “--staged” to be parsed as options; update the call in
_forgit_restore_preview to pass an explicit "--" before "$@" (i.e., git diff
--color=always -- "$@") to stop option injection, and make the same change at
the other occurrence noted (the similar git/restore invocation around the other
reported line).
In `@tests/list-staged-files.sh`:
- Line 1: tests/list-staged-files.sh is not using the repository test naming
convention and will be skipped by test discovery; rename the file to follow the
pattern (for example tests/list-staged-files.test.sh), keep the existing shebang
and executable permission, and update any CI/test discovery config or scripts
that reference tests/list-staged-files.sh so they point to the new filename.
---
Nitpick comments:
In `@tests/list-staged-files.sh`:
- Around line 23-26: The test function test_list_staged_files_only_lists_staged
uses a global variable output; change it to a local variable to avoid cross-test
leakage by declaring local output and assigning it from the helper
_forgit_list_staged_files (e.g., local output=$(_forgit_list_staged_files)),
keeping the assert_same call unchanged to reference the local output.
In `@tests/restore.test.sh`:
- Around line 17-49: The tests declare globals; make variables local inside each
test function to avoid leaks: in
test_restore_shows_message_when_no_modified_files() mark output as local, and in
test_parse_restore_args() mark staged, worktree, and preview_arg as local before
the IFS/read lines; keep the same variable names and behavior (no other logic
changes) so only scope changes for functions _forgit_restore and
_forgit_parse_restore_flags are applied.
🪄 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: 79cef172-2c41-4ef0-b729-1212eec3aa34
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/list-staged-files.shtests/restore.test.sh
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/restore.test.sh (3)
18-18: ⚡ Quick winDeclare test variables as
localfor proper isolation.The
outputvariable should be declaredlocalto prevent namespace pollution between test runs. As per coding guidelines, shell functions should uselocalvariables.♻️ Proposed fix
function test_restore_shows_message_when_no_modified_files() { - output=$(_forgit_restore 2>&1) + local output + output=$(_forgit_restore 2>&1) assert_general_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/restore.test.sh` at line 18, The test assigns to the global variable `output`; change it to a function-local variable by declaring `local output` before or at the assignment in the test that calls `_forgit_restore` so the `output` value is isolated between test runs and doesn't pollute the global namespace (i.e., update the test invocation that currently uses `output=$(_forgit_restore 2>&1)` to use a `local output` declaration).Source: Coding guidelines
45-45: 💤 Low valueFix grammatical error in comment.
The comment contains a grammatical error: "to not" should be "do not".
📝 Proposed fix
- # other flags to not change the outcome + # other flags do not change the outcome IFS=$'\t' read -r staged worktree preview_arg < <(_forgit_parse_restore_flags -W -S -U --unknown-flag)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/restore.test.sh` at line 45, Update the inline comment that reads "other flags to not change the outcome" to correct the grammar by changing it to "other flags do not change the outcome" so the comment in tests/restore.test.sh (the line containing the phrase "other flags to not change the outcome") is grammatically correct.
23-50: ⚡ Quick winDeclare test variables as
localfor proper isolation.The
staged,worktree, andpreview_argvariables should be declaredlocalto prevent namespace pollution between test assertions. As per coding guidelines, shell functions should uselocalvariables.♻️ Proposed fix
function test_parse_restore_args() { + local staged worktree preview_arg + IFS=$'\t' read -r staged worktree preview_arg < <(_forgit_parse_restore_flags --staged) assert_same "$staged" true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/restore.test.sh` around lines 23 - 50, In test_parse_restore_args, declare the temporary variables used for assertions as local (staged, worktree, preview_arg) to avoid leaking them into the global shell namespace; update the function to use "local staged worktree preview_arg" before each IFS read (or once at the top of the function) so each test invocation has isolated variables while still calling _forgit_parse_restore_flags as shown.Source: Coding guidelines
tests/list-staged-files.test.sh (1)
24-24: ⚡ Quick winDeclare test variables as
localfor proper isolation.The
outputvariable should be declaredlocalto prevent namespace pollution between test runs. As per coding guidelines, shell functions should uselocalvariables.♻️ Proposed fix
function test_list_staged_files_only_lists_staged() { - output=$(_forgit_list_staged_files) + local output + output=$(_forgit_list_staged_files) assert_same "$output" "staged.txt"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/list-staged-files.test.sh` at line 24, The test assigns output=$(_forgit_list_staged_files) using a global variable; change this to a local variable to avoid leaking state between tests—declare output as local (e.g., local output) inside the test function that calls _forgit_list_staged_files so the variable is scoped to that test and won't pollute other tests or the shell environment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/list-staged-files.test.sh`:
- Line 24: The test assigns output=$(_forgit_list_staged_files) using a global
variable; change this to a local variable to avoid leaking state between
tests—declare output as local (e.g., local output) inside the test function that
calls _forgit_list_staged_files so the variable is scoped to that test and won't
pollute other tests or the shell environment.
In `@tests/restore.test.sh`:
- Line 18: The test assigns to the global variable `output`; change it to a
function-local variable by declaring `local output` before or at the assignment
in the test that calls `_forgit_restore` so the `output` value is isolated
between test runs and doesn't pollute the global namespace (i.e., update the
test invocation that currently uses `output=$(_forgit_restore 2>&1)` to use a
`local output` declaration).
- Line 45: Update the inline comment that reads "other flags to not change the
outcome" to correct the grammar by changing it to "other flags do not change the
outcome" so the comment in tests/restore.test.sh (the line containing the phrase
"other flags to not change the outcome") is grammatically correct.
- Around line 23-50: In test_parse_restore_args, declare the temporary variables
used for assertions as local (staged, worktree, preview_arg) to avoid leaking
them into the global shell namespace; update the function to use "local staged
worktree preview_arg" before each IFS read (or once at the top of the function)
so each test invocation has isolated variables while still calling
_forgit_parse_restore_flags as shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff67bda2-d49a-499a-8258-396c77123f5a
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/list-staged-files.test.shtests/restore.test.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- conf.d/forgit.plugin.fish
- completions/git-forgit.bash
- completions/_git-forgit
- bin/git-forgit
- completions/git-forgit.fish
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bin/git-forgit (1)
748-748:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
--before the selected restore paths.Line 748 still forwards fzf-selected filenames straight into
git restore. A tracked path like--source=HEAD~1or--stagedwill be re-parsed as an option instead of a pathname.Suggested patch
- [[ ${`#files`[@]} -gt 0 ]] && _forgit_git_restore "$@" "${files[@]}" + [[ ${`#files`[@]} -gt 0 ]] && _forgit_git_restore "$@" -- "${files[@]}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/git-forgit` at line 748, The fzf-selected filenames are being passed directly to _forgit_git_restore which lets values like --source=... be parsed as git options; update the invocation that currently reads _forgit_git_restore "$@" "${files[@]}" to insert the argument separator "--" before the file list so git treats selections as paths (i.e., call _forgit_git_restore with "$@", then "--", then the "${files[@]}"); keep the rest of the call and array checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@bin/git-forgit`:
- Line 748: The fzf-selected filenames are being passed directly to
_forgit_git_restore which lets values like --source=... be parsed as git
options; update the invocation that currently reads _forgit_git_restore "$@"
"${files[@]}" to insert the argument separator "--" before the file list so git
treats selections as paths (i.e., call _forgit_git_restore with "$@", then "--",
then the "${files[@]}"); keep the rest of the call and array checks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d5a187d-5597-478e-9239-4746ee1e94f0
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/list-staged-files.test.shtests/restore.test.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- conf.d/forgit.plugin.fish
- completions/git-forgit.bash
- completions/_git-forgit
- tests/list-staged-files.test.sh
- completions/git-forgit.fish
- forgit.plugin.zsh
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/git-forgit (3)
721-721: ⚡ Quick winRemove unused variable
args.The variable
argsis declared but never used in the function.🧹 Suggested cleanup
- local files opts args staged worktree candidates preview_arg + local files opts staged worktree candidates preview_arg🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/git-forgit` at line 721, The local variable declaration includes an unused symbol `args`; remove `args` from the local declaration (the line declaring "local files opts args staged worktree candidates preview_arg") so the function no longer declares the unused variable `args`, keeping the remaining locals intact.
705-709: 💤 Low valueConsider adding a comment to explain the preview argument mapping.
The conditional logic maps flag combinations to preview arguments for
git diff:
--stagedonly →--staged(show staged changes)- Both flags →
HEAD(show both staged and worktree changes)- Neither or
--worktreeonly → `` (empty, show worktree changes)A brief comment would make this mapping more immediately clear to future maintainers.
📝 Suggested comment
+ # Map flags to git diff preview argument: + # --staged only: show staged changes (--staged) + # both flags: show staged + worktree changes (HEAD) + # neither/--worktree only: show worktree changes (empty) if [[ $staged == true && $worktree != true ]]; then preview_arg=--staged elif [[ $staged == true && $worktree == true ]]; then preview_arg=HEAD fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/git-forgit` around lines 705 - 709, Add a brief inline comment above the conditional that explains how the staged and worktree flags map to the preview_arg for git diff: that staged-only sets preview_arg="--staged" to show staged changes, staged+worktree sets preview_arg="HEAD" to show both staged and worktree changes relative to HEAD, and neither or worktree-only leaves preview_arg empty to show only worktree changes; place this comment immediately above the if block that sets preview_arg (variables: staged, worktree, preview_arg) so future maintainers understand the mapping at a glance.
222-226: Refine_forgit_list_staged_filespath handling
git diff --name-only --cached -z "$rootdir"yields the same staged file list as omitting the path argument, so passing$rootdiris redundant here. Keeping$rootdiris safe since it comes fromgit rev-parse --show-toplevel(absolute repo root, not an option-like path), but for consistency/defensiveness you can drop$rootdiror use an explicit separator:... -z -- "$rootdir".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/git-forgit` around lines 222 - 226, The _forgit_list_staged_files function passes $rootdir to git diff unnecessarily; remove the argument so the command becomes `git diff --name-only --cached -z` (pipe to tr as before) or, if you prefer to keep the path for defensiveness, add an explicit separator and use `git diff --name-only --cached -z -- "$rootdir"`; update the function `_forgit_list_staged_files` accordingly to use one of these two forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bin/git-forgit`:
- Line 721: The local variable declaration includes an unused symbol `args`;
remove `args` from the local declaration (the line declaring "local files opts
args staged worktree candidates preview_arg") so the function no longer declares
the unused variable `args`, keeping the remaining locals intact.
- Around line 705-709: Add a brief inline comment above the conditional that
explains how the staged and worktree flags map to the preview_arg for git diff:
that staged-only sets preview_arg="--staged" to show staged changes,
staged+worktree sets preview_arg="HEAD" to show both staged and worktree changes
relative to HEAD, and neither or worktree-only leaves preview_arg empty to show
only worktree changes; place this comment immediately above the if block that
sets preview_arg (variables: staged, worktree, preview_arg) so future
maintainers understand the mapping at a glance.
- Around line 222-226: The _forgit_list_staged_files function passes $rootdir to
git diff unnecessarily; remove the argument so the command becomes `git diff
--name-only --cached -z` (pipe to tr as before) or, if you prefer to keep the
path for defensiveness, add an explicit separator and use `git diff --name-only
--cached -z -- "$rootdir"`; update the function `_forgit_list_staged_files`
accordingly to use one of these two forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87714ac6-b54f-40ba-9f80-6d787fce93ae
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/list-staged-files.test.shtests/restore.test.sh
✅ Files skipped from review due to trivial changes (2)
- README.md
- conf.d/forgit.plugin.fish
🚧 Files skipped from review as they are similar to previous changes (6)
- forgit.plugin.zsh
- completions/git-forgit.fish
- completions/git-forgit.bash
- tests/restore.test.sh
- completions/_git-forgit
- tests/list-staged-files.test.sh
sandr01d
force-pushed
the
restore
branch
2 times, most recently
from
04bbba0 to
7477eb9
Compare
Co-Authored-By: Javier Tia <floss@jetm.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Check list
Description
This PR is built upon #489 and supersedes it, thank you @jetm for providing it!
Add an interactive
git restoreselector that supports restoring files from the worktree (no flag or--worktree/-W), from the index (--staged/-S) or both (--staged --worktree/-S -W). Othergit restorearguments are, of course, supported too so something likegrs --staged --patchalso works.closes #447
Type of change
Test environment
Summary by CodeRabbit
New Features
grsavailable.Shell Integration
Documentation
grs/restore and its per-command options.Tests