feat: add interactive checkout file from commit selector#499
feat: add interactive checkout file from commit selector#499
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 command Changes
Sequence DiagramsequenceDiagram
participant User
participant GitLog as GitLog/_forgit_git_log
participant FZF_Commit as FZF (commit selector)
participant GitShow as GitShow/_forgit_git_show
participant FZF_File as FZF (file selector)
participant GitCheckout as Git (checkout)
User->>GitLog: request commit list
GitLog-->>FZF_Commit: emit commits + preview
FZF_Commit->>User: show commit list
User->>FZF_Commit: select commit
FZF_Commit->>GitShow: request file list for commit
GitShow-->>FZF_File: emit files + previews
FZF_File->>User: show file list
User->>FZF_File: select file
FZF_File->>GitCheckout: checkout <file> from <commit>
GitCheckout-->>User: file restored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 908-912: The function _forgit_checkout_file_from_commit sets local
variables "opts commit file branch" and unconditionally calls "shift", which
errors when the function is invoked with no positional arguments; change the
logic so "shift" is only executed when at least one argument was actually
provided (e.g., test the positional count or $1 before calling shift) so the
default branch expansion ("branch=${1:-$(git rev-parse --abbrev-ref HEAD)}") can
be used without causing a shift error.
- Around line 935-937: The file extraction currently uses awk '{print $2}' in
the pipeline that assigns file=$(_forgit_git_show "$commit" |
FZF_DEFAULT_OPTS="$opts" fzf | awk '{print $2}'), which breaks on filenames
containing spaces and returns the old name on rename lines; replace that awk
usage with the existing parser function _forgit_get_single_file_from_diff_line
so the selected diff line is passed into _forgit_get_single_file_from_diff_line
and its output assigned to file, keeping the rest of the pipeline (the call to
_forgit_git_show, FZF_DEFAULT_OPTS="$opts" fzf, and the subsequent conditional
that calls _forgit_git_checkout_file "$commit" -- "$file") unchanged.
In `@README.md`:
- Around line 271-272: The README table has a duplicated `gcf` row causing
ambiguity; remove the duplicate and ensure the table contains exactly one row
for `gcf` listing `FORGIT_CHECKOUT_FILE_GIT_OPTS` and
`FORGIT_CHECKOUT_FILE_FZF_OPTS`, and one row for `gcff` listing
`FORGIT_SHOW_GIT_OPTS` and
`FORGIT_CHECKOUT_FILE_FROM_COMMIT_LOG_FZF_OPTS`/`FORGIT_CHECKOUT_FILE_FROM_COMMIT_SHOW_FZF_OPTS`;
update the markdown table so the `gcf` and `gcff` rows are unique and correctly
aligned.
In `@tests/log.test.sh`:
- Line 1: The shebang in tests/log.test.sh is non-portable (uses
#!/bin/env/bash); update the file header to the portable form by replacing the
shebang with #!/usr/bin/env bash so the script starts with the
environment-respecting interpreter lookup (change the first line in
tests/log.test.sh to use /usr/bin/env bash).
- Around line 22-23: The test currently strips a leading hex/graph char with sed
causing flakiness; instead run _forgit_git_log with the graph disabled and
remove the sed hack. Update the invocation to set FORGIT_GLO_GRAPH=0 alongside
FORGIT_GLO_FORMAT='%s' when calling _forgit_git_log (the command using
FORGIT_GLO_FORMAT and _forgit_git_log), capture its output directly into the
output variable, and keep the assert_same "Initial commit" "$output" assertion
to compare the exact subject.
In `@tests/show.test.sh`:
- Line 1: Replace the non-portable shebang in the test script (current line
"#!/bin/env bash") with the portable form "#!/usr/bin/env bash" so the script
uses the env lookup for bash; update the top of tests/show.test.sh accordingly
to ensure correct interpreter resolution across systems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de63bb9f-d719-4d7a-a038-618d0c033970
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/log.test.shtests/show.test.sh
sandr01d
force-pushed
the
checkout-file-from-commit
branch
4 times, most recently
from
f576843 to
da47e2f
Compare
|
@coderabbitai All your comments should be addressed, please recheck and close as appropriate. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch all my open review comments on this PR to verify what's been addressed. 🏁 Script executed: Length of output: 23878
[resolve] 🧠 Learnings used |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/git-forgit (1)
905-943: Add a focused test for the new interactive checkout flowThe new
_forgit_checkout_file_from_commitpath adds argument parsing and two-step selection behavior, but current tests only cover_forgit_git_log/_forgit_git_show. Please add a targeted test that stubs selection output and validates the final checkout invocation path (including filenames with spaces/rename rows).As per coding guidelines, "Add or update tests for behavior changes, especially parsing, selection, and cross-shell integration."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-forgit` around lines 905 - 943, Add a focused unit/integration test for the new interactive flow in _forgit_checkout_file_from_commit: stub the interactive selectors (_forgit_git_log and _forgit_git_show or the fzf invocation) to return a specific commit SHA and then a file entry (include cases for filenames with spaces and a git-rename diff row like "old -> new"); invoke _forgit_checkout_file_from_commit both with an explicit branch argument and with no args (to trigger rev-parse branch fallback), and assert that _forgit_git_checkout_file is called exactly once with the selected commit and the resolved filename (use stubs/mocks to capture the arguments and ensure parsing/selection logic handles spaces and rename lines correctly). Ensure the test runs under the same shell harness used by other tests to cover cross-shell behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/git-forgit`:
- Around line 905-943: Add a focused unit/integration test for the new
interactive flow in _forgit_checkout_file_from_commit: stub the interactive
selectors (_forgit_git_log and _forgit_git_show or the fzf invocation) to return
a specific commit SHA and then a file entry (include cases for filenames with
spaces and a git-rename diff row like "old -> new"); invoke
_forgit_checkout_file_from_commit both with an explicit branch argument and with
no args (to trigger rev-parse branch fallback), and assert that
_forgit_git_checkout_file is called exactly once with the selected commit and
the resolved filename (use stubs/mocks to capture the arguments and ensure
parsing/selection logic handles spaces and rename lines correctly). Ensure the
test runs under the same shell harness used by other tests to cover cross-shell
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f375f180-c827-49a8-99ac-9f147ea4ac9c
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/log.test.shtests/show.test.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- completions/_git-forgit
- README.md
- tests/log.test.sh
- conf.d/forgit.plugin.fish
sandr01d
force-pushed
the
checkout-file-from-commit
branch
from
da47e2f to
31ee2f8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
completions/_git-forgit (1)
143-143:⚠️ Potential issue | 🟡 MinorAlign alias completion backend with command dispatcher.
Line 143 uses
_git-branch, while Line 108 routescheckout_file_from_committhrough_git-branches. Please map thecompdefto_git-branchesso alias completion behavior stays consistent.Suggested fix
-compdef _git-branch forgit::checkout::file::from::commit +compdef _git-branches forgit::checkout::file::from::commitAs per coding guidelines: "Keep command aliases and completion names consistent across Zsh, Bash, and Fish shells."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@completions/_git-forgit` at line 143, The compdef for the forgit alias checkout_file_from_commit currently registers against _git-branch; update the completion backend to _git-branches so it matches the dispatcher used for checkout_file_from_commit (change the compdef forgit::checkout::file::from::commit to point at _git-branches instead of _git-branch).
🧹 Nitpick comments (1)
bin/git-forgit (1)
223-225: Scope helper option arrays locally.
_forgit_log_git_optsand_forgit_show_git_optsare currently function-external mutable arrays. Make them local to avoid accidental cross-function state leakage.Suggested refactor
_forgit_git_log() { local graph log_format + local -a _forgit_log_git_opts graph=() [[ $_forgit_log_graph_enable == true ]] && graph=(--graph) log_format=${FORGIT_GLO_FORMAT:-$_forgit_log_format} @@ _forgit_git_show() { local commit=$1 + local -a _forgit_show_git_opts shiftAs per coding guidelines: "Use small helper functions with
localvariables inside functions for shell code."Also applies to: 419-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-forgit` around lines 223 - 225, The helper option arrays _forgit_log_git_opts and _forgit_show_git_opts are declared as global mutable arrays and can leak state across functions; make them function-local by declaring them with local (e.g., use local _forgit_log_git_opts=() and local _forgit_show_git_opts=() where they are initialized and before calling _forgit_parse_array) in the functions that build/use them (the blocks around git log and git show), so the arrays are scoped to the function and their values won't persist between calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@completions/_git-forgit`:
- Line 143: The compdef for the forgit alias checkout_file_from_commit currently
registers against _git-branch; update the completion backend to _git-branches so
it matches the dispatcher used for checkout_file_from_commit (change the compdef
forgit::checkout::file::from::commit to point at _git-branches instead of
_git-branch).
---
Nitpick comments:
In `@bin/git-forgit`:
- Around line 223-225: The helper option arrays _forgit_log_git_opts and
_forgit_show_git_opts are declared as global mutable arrays and can leak state
across functions; make them function-local by declaring them with local (e.g.,
use local _forgit_log_git_opts=() and local _forgit_show_git_opts=() where they
are initialized and before calling _forgit_parse_array) in the functions that
build/use them (the blocks around git log and git show), so the arrays are
scoped to the function and their values won't persist between calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5793b63c-e8e0-4f21-8292-85dba390a5b9
📒 Files selected for processing (7)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zsh
🚧 Files skipped from review as they are similar to previous changes (3)
- completions/git-forgit.bash
- conf.d/forgit.plugin.fish
- README.md
sandr01d
force-pushed
the
checkout-file-from-commit
branch
from
31ee2f8 to
67d6fe2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 945: The current deletion branch appends "~" to the commit (in the
condition using [[ "$file" =~ ^\[D\] ]]) which produces an invalid ref for a
root commit; replace that manual "~" logic with the existing helper
_forgit_previous_commit so it returns a safe parent or empty/handled value.
Concretely, update the code path that mutates commit (the one matching ^\[D\])
to call _forgit_previous_commit "$commit" (or assign its result back to commit)
instead of doing commit="$commit~", ensuring the root-commit-no-parent case is
handled by the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 252231a1-d9f0-45fe-9fbe-b7dffad1ed75
📒 Files selected for processing (7)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zsh
🚧 Files skipped from review as they are similar to previous changes (2)
- completions/_git-forgit
- README.md
sandr01d
force-pushed
the
checkout-file-from-commit
branch
2 times, most recently
from
c303268 to
bc2797e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/log.test.sh (1)
26-27:⚠️ Potential issue | 🟠 MajorFix helper invocation and remove brittle output stripping.
Line 26 calls
_forgit_git_logwithout its requiredlog_formatargument, which causes empty output in CI, and thesedstripping reintroduces the flakiness previously discussed.Proposed fix
function test_forgit_git_log() { local output - output=$(_forgit_git_log 2>&1 | sed 's/^[0-9a-f] //') + output=$(_forgit_git_log "$FORGIT_GLO_FORMAT" 2>&1) assert_same "Initial commit" "$output" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/log.test.sh` around lines 26 - 27, The test calls _forgit_git_log without the required log_format and then strips output with sed, causing CI flakiness; change the invocation to pass a concrete format (e.g., call _forgit_git_log '%s' to emit the commit subject) and remove the sed 's/^[0-9a-f] //' post-processing so the test compares the raw formatted output directly (update the line that sets output and keep the assert_same "Initial commit" "$output").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/log.test.sh`:
- Around line 26-27: The test calls _forgit_git_log without the required
log_format and then strips output with sed, causing CI flakiness; change the
invocation to pass a concrete format (e.g., call _forgit_git_log '%s' to emit
the commit subject) and remove the sed 's/^[0-9a-f] //' post-processing so the
test compares the raw formatted output directly (update the line that sets
output and keep the assert_same "Initial commit" "$output").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e17d9cb-24bd-479f-aa23-2a95b73b7c52
📒 Files selected for processing (9)
README.mdbin/git-forgitcompletions/_git-forgitcompletions/git-forgit.bashcompletions/git-forgit.fishconf.d/forgit.plugin.fishforgit.plugin.zshtests/log.test.shtests/show.test.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- conf.d/forgit.plugin.fish
- forgit.plugin.zsh
- completions/_git-forgit
- completions/git-forgit.bash
|
Sorry for the last minute changes @wfxr, but I decided to move the log format parsing outside the |
sandr01d
force-pushed
the
checkout-file-from-commit
branch
from
bc2797e to
d336638
Compare
2 participants
Check list
Description
Adds a new
_forgit_checkout_file_from_commitfunction that allows the user to first select a commit (from any branch, defaults to the current branch) and then a file in the specified commit. The file will then be checked out from the selected commit.Closes #476
Type of change
Test environment
Summary by CodeRabbit
New Features
Completions
Documentation
Tests