perf: speed up modified file lookup in large repositories - #544
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesWorking-tree path handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitDiff
participant ModifiedFiles
participant PathRewriter
participant RestoreCheckout
GitDiff->>ModifiedFiles: Return null-delimited modified paths
ModifiedFiles->>PathRewriter: Rewrite repository paths for cwd
PathRewriter-->>ModifiedFiles: Return cwd-relative display paths
ModifiedFiles->>RestoreCheckout: Provide selectable modified files
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
| } | ||
| } | ||
| ' "$rootdir" "$cwd" "$mode" "$separator" | ||
| } |
There was a problem hiding this comment.
This change was largely just pulled from _forgit_build_status_entries below, with a few ergonomic tweaks to make it work for both situations. The new tests should hopefully make sure this is stable
|
Also, hi! Long time no see. I see from my inbox you all have been super hard at work in this repo. Please let me know if I've run afoul of any new norms or if I've missed anything. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/git-forgit (1)
1131-1142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptional: capture the modified-file list once instead of invoking it twice.
_forgit_list_modified_filesruns a fullgit diff+ Perl pipeline at Line 1131 (count) and again at Line 1141 (fzf input). Since this PR targets responsiveness, capturing the result once avoids the redundant subprocess and closes the small window where the two calls could disagree.♻️ Proposed single-invocation refactor
_forgit_checkout_file() { _forgit_inside_work_tree || return 1 - local files opts + local files opts modified_files _forgit_contains_non_flags "$@" && { _forgit_git_checkout_file "$@" return $? } - [[ $(_forgit_list_modified_files | wc -l) -eq 0 ]] && echo 'Nothing to checkout.' && return 1 + modified_files=$(_forgit_list_modified_files) + [[ -z $modified_files ]] && echo 'Nothing to checkout.' && return 1 opts=" $FORGIT_FZF_DEFAULT_OPTS -m -0 --preview=\"$FORGIT preview checkout_file_preview {}\" $FORGIT_CHECKOUT_FILE_FZF_OPTS " files=() while IFS='' read -r file; do files+=("$file") - done < <(_forgit_list_modified_files | - FZF_DEFAULT_OPTS="$opts" fzf) + done < <(printf '%s\n' "$modified_files" | + FZF_DEFAULT_OPTS="$opts" fzf) [[ ${`#files`[@]} -gt 0 ]] && _forgit_git_checkout_file "$@" "${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` around lines 1131 - 1142, Capture the output of _forgit_list_modified_files once at the start of the checkout flow, use that captured value for the empty-list check, and feed the same value into the fzf input while preserving filename handling and array population. Remove the second invocation so the modified-file list cannot change between checks.
🤖 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`:
- Around line 1131-1142: Capture the output of _forgit_list_modified_files once
at the start of the checkout flow, use that captured value for the empty-list
check, and feed the same value into the fzf input while preserving filename
handling and array population. Remove the second invocation so the modified-file
list cannot change between checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b2ba7f4-5e0d-40af-bca2-fcadba08ddc4
📒 Files selected for processing (2)
bin/git-forgittests/working-tree-changes.test.sh
|
I like the coderabbit nitpick review, I think that's also a good improvement. I can either apply it here or do it in a follow up commit. Let me know what you think (human reviewers) |
I agree. As to whether it should go in a follow up commit or not, I'm fine with both. Maybe leaning a bit more towards having a separate commit because it is a different kind of optimization, but fine either way. |
Cool, I lean to doing a follow on, so I will do that! Thanks for the review, applied the two comment suggestions. |
Use a modified-file-only path for `git-forgit restore` (`grs`) and `git-forgit checkout_file` (`gcf`) so these interactive selectors do not scan large sets of untracked files. This keeps both commands responsive in repositories with many untracked files.
bc01d24 to
2688968
Compare
2 participants
Check list
Description
Use a modified-file-only path for
git-forgit restore(grs) andgit-forgit checkout_file(gcf) so these interactive selectors do not scan large sets of untracked files. This keeps both commands responsive in repositories with many untracked files.On a repo I use for work, comparing the raw
git diffwith the rawgit ls-filesfor listing modified files is ~8x faster, the difference between ~1.5s and ~10s. The quality of life improvement is huge.To get this change working when in sibling directories, I pulled out an existing
perlscript used in one of the other functions.Type of change
Test environment
Summary by CodeRabbit
Bug Fixes
Tests