Repository navigation
perf(git): normalize tracked discard paths once per operation - #20299
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds shared Git path-spec matching and partitioning utilities. The main and relay bulk-discard flows now use the shared partitioner. The local matcher and re-export are removed. Tests cover path normalization, ordering, duplicates, literal patterns, early exits, and discard command routing. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No merge-blocking risk remains from the reviewed change. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed technical context, behavior guarantees, test results, limitations, and merge-risk analysis. However, it omits required template information, including the mandatory Linked Issue, the Visual Proof section with an N/A explanation, and several checklist and review sections. Resolution Add the required template sections. Provide a valid issue reference after ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor cleanup suggestion inline.
Reviewed changes
- New shared matcher —
src/shared/git-tracked-pathspecs.tsaddscreateTrackedPathSpecMatcher,isTrackedPathSpec, andpartitionTrackedPathSpecs; one matcher per operation normalizes each request once and each visited tracked entry at most once through a lazy index-keyed cache, while keeping the directory early-exit. - Main discard —
bulkDiscardChangesreplaces the two.filter()passes with a singlepartitionTrackedPathSpecscall; the oldnormalizeGitPathForCompareis removed fromgit-pathspec.ts. - Relay discard — the duplicated private matcher in
GitHandlerDiscardOperationsis deleted in favour of the shared module, which is dependency-free and safe for the relay bundle. - Tests — a differential suite against the previous implementation, normalization-count contracts, cross-operation non-reuse, plus exact-argv action-order tests in both the main and relay suites.
I traced the new lazy cache against the old implementation: normalization is deterministic and pure, ??= caches the empty string correctly (non-nullish), short-circuit order is unchanged, and duplicate/order/spelling semantics are preserved. I also ran git-tracked-pathspecs.test.ts, status-discard-and-bulk-staging.test.ts, and git-handler-working-tree-changes.test.ts — all green.
openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
f7a4a8d to
145f717
Compare
|
Rebased onto Both PRs touched the tracked/untracked split in Tests on the rebased tree: |
What
Bulk discard classified requested paths with two
.filter()passes, each callingisTrackedPathSpec, which re-normalized every tracked record inside.some()— worst case ≈2R(T+1)normalizations. Main and relay each carried their own copy of the same matcher.Now a single shared pure module,
src/shared/git-tracked-pathspecs.ts, does one classification pass: each request is normalized once, and each visited tracked entry is normalized lazily at most once within that synchronous partition. Main and relay both call it, so the duplicate relay implementation is gone.The module is in
src/shared(not undersrc/main/git) so relay does not pull main's WSL/command-resolution dependencies. It imports nothing at all — nonode:*, no main-only module.Behavior preserved
partitionTrackedPathSpecscall and dies with it.ELI5
Discarding a big selection of files means sorting them into two buckets: files Git already knows about (rewind them) and files Git has never seen (delete them). To sort them, the old code tidied up the spelling of every known-file name over and over — once for every single file you selected, and then a second time because it walked the list twice. If you selected 64 files and Git knew about 256, it did that tidying about 33,000 times to answer 64 questions. Now it tidies each name once and remembers the tidy version for the rest of that one sort, which is 320 times instead of 33,000. It's like alphabetising a deck of cards once instead of re-alphabetising it before every card you look up — the answers are identical, you just stop redoing the same chore. Honestly: for normal selections you will not notice any difference; this is a cheap-operation cleanup, not a speedup you can feel.
Measurement
Normalization counts only — not CPU time, not wall clock, not latency. Matching comparisons remain worst-case O(R×T); only the per-comparison normalization is removed. The lazy cache temporarily retains O(visited tracked records) normalized strings, a real memory tradeoff rather than a free win. No wall-clock benchmark was run, so no timing claim is made.
Tests
src/shared/git-tracked-pathspecs.test.tsdiffs against the previous implementation across path edges (Windows/UNC paths, trailing slashes, glob-looking literals, Unicode NFC-vs-NFD, empty strings) and asserts the normalization counts above. New action-order tests in the main and relay suites pin the exactls-files/restore/cleanargv, including duplicates and original spelling.Re-verified on the current branch head:
ORCA_BACKGROUND_LAUNCH=1 pnpm run test:perf:contracts→ 11 files / 74 tests passed.ORCA_BACKGROUND_LAUNCH=1 pnpm tc→ passed (thenodeproject typecheckssrc/shared,src/main, andsrc/relaytogether).ORCA_CODE_QUALITY_BASE=main pnpm run check:code-quality:changed→ 0 new findings across 6 changed files (native, type-aware, React Doctor).oxfmt --writeon the changed files → no reformatting needed.Additionally, an out-of-band randomized differential check ran the old and new partition side by side over 200,000 generated request/tracked list pairs built from adversarial atoms (empty string,
/,//,\,\\,.,..,C:, UNC shapes,[a],*,:(glob), spaces, newlines, NUL, NFC/NFDé, ligatures) with mixed separators: 0 divergences, and repeated calls on one matcher instance returned stable results.Limitations
Not validated on native Windows, a real WSL distro, or a live SSH host; relay coverage is the mocked handler path plus real local Git. Windows/UNC/WSL path shapes are covered as strings through the shared module and through the WSL pathspec suites, not by executing Git on those hosts.
Merge risk
Low.
src/main/git/source-control/discard-changes.ts:121andsrc/relay/git-handler-discard-operations.ts:90. Single-file discard, staging, status, and every other Git path are untouched. Nothing persisted, no schema, no wire format, no IPC/RPC payload, no settings — so there is no mixed-version or rollback surface.git restore --worktreeversusgit clean -ffdx. If the lazy normalization cache ever returned a stale or wrong value, a tracked file could be routed toclean(uncommitted and committed content deleted) or an untracked file routed torestore(which would simply fail, harmless). The first direction is the only one that loses data, and it is the reason this is called out rather than filed as a plain refactor.src/shared/git-tracked-pathspecs.ts:15—const normalizedTracked = (normalizedTrackedPaths[index] ??= normalizeGitPathForCompare(trackedPath)). Everything about safety rests on that memo being keyed to the same immutabletrackedPathsarray for the life of one partition, and on??=not mistaking a legitimately empty normalized path ('', which/and//normalize to) for "not yet computed".''is not nullish, so it caches and reuses correctly; both call sites buildtrackedPathSpecslocally and never mutate it during the partition.git statuswould show a deletion rather than a clean tree. The pinned-argv tests in both the main and relay suites would fail first in CI.d72d30bthe refactor,f7a4a8da dead re-export cleanup). Reverting the squashed merge commit restores the two previous private copies of the matcher; there is no data or state to migrate back.User-facing trade-offs
One, and it is invisible in practice: during a single bulk discard, the partition now holds an extra array of up to one entry per visited tracked record — bounded by the
ls-filesoutput for the selection, freed as soon as the partition returns. For a 150,000-descendant directory selection that is ~150k array slots; the normalized strings themselves are mostly not new allocations, becauseString.prototype.replacewith a non-matching pattern returns the original string, andgit ls-filesemits forward slashes. Before, those strings were created and discarded one comparison at a time. This is a peak-memory-for-work trade, not a leak: nothing survives the call.Beyond that, none identified. What was actually checked to justify that:
\\host\share, drive letters, trailing/repeated slashes,./..segments, bracket and glob-looking literals,:(glob)prefixes, spaces, newlines, NFC vs NFD) — so no file changes which ofrestore/cleanit receives.ls-files/restore/cleanargv rather than just the bucket contents.DOCSstill does not matchdocs/readme.md) — this is pre-existing behavior, deliberately pinned by test, not introduced or altered here.git-discard-path-safety), worktree-escape validation, WSL pathspec conversion and login-shell banner fencing, and command-line budget chunking are all untouched and still green.Unproven, disclosed rather than claimed: no execution on native Windows, in a real WSL distro, or against a live SSH/relay host; no wall-clock or CPU benchmark, so the "faster" framing is deliberately limited to operation counts; relay coverage is the mocked handler path.