Git subprocess diet: 161 → 133 spawns, 32 → 12 peak concurrency - #1062
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughGit execution now supports repository-scoped caching, read throttling, mutation invalidation, and common-directory memoization. Git consumers and tests use the new behavior. The chat Git toolbar now counts distinct changed paths. ChangesGit repository caching
Chat Git toolbar
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
bb83103 to
78b6093
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b60938b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runGitRepoCached( | ||
| ["remote", "get-url", "origin"], | ||
| { cwd: lane.worktreePath, timeoutMs: 8_000 }, | ||
| { key: "remote-url:origin", cacheClass: "stable" }, |
There was a problem hiding this comment.
When extensions.worktreeConfig is enabled, git config --worktree can give each lane a different remote.origin.url, user.name, or user.email—Git's git config -h explicitly describes this as a per-worktree config file. Because this result is keyed only by the common Git directory and remote-url:origin, whichever lane loads first supplies every other lane's remote for five minutes, potentially producing incorrect handoff/history links; the identity cache at line 1700 has the same problem. Keep effective-config queries worktree-aware rather than sharing them across the repository.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/desktop/src/main/services/git/git.test.ts (2)
224-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test does not assert the concurrency cap.
The assertions prove that 40 reads finish and that no result is lost. They pass equally well with an unbounded fan-out, so the 12-process ceiling, which is the headline behavior of this change, has no regression guard. Both refs also resolve to the same commit, so the comment about distinct refs is not verified by the assertions.
Export a small test seam from
git.tsand assert the observed peak.♻️ Proposed seam and assertion
// apps/desktop/src/main/services/git/git.ts export function gitProcessConcurrencyStatsForTests(): { running: number; peak: number; max: number } { return { running: runningGitProcesses, peak: peakGitProcesses, max: MAX_CONCURRENT_GIT_PROCESSES }; }expect(results).toHaveLength(40); + const stats = gitProcessConcurrencyStatsForTests(); + expect(stats.peak).toBeGreaterThan(1); + expect(stats.peak).toBeLessThanOrEqual(stats.max); for (const result of results) {🤖 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 `@apps/desktop/src/main/services/git/git.test.ts` around lines 224 - 240, Export a gitProcessConcurrencyStatsForTests seam from git.ts that reports runningGitProcesses, peakGitProcesses, and MAX_CONCURRENT_GIT_PROCESSES. Update the “bounds concurrent read fan-out” test to reset or capture stats before launching the 40 runGit calls, then assert the observed peak never exceeds the configured max of 12 while preserving the existing result assertions.Source: Coding guidelines
176-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cleanup and cache isolation to the fixture.
scratchRepocreates a temporary repository per test and never removes it, so each run leaves three directories behind. The tests also share the module-level caches ingit.tsandgitRepoCache.ts. Isolation holds today only becausemkdtempSyncreturns a fresh path each time. Reset both caches and remove the directories so the suite stays deterministic if a fixture path is ever reused.♻️ Proposed fixture cleanup
describe("repo cache invalidation through runGit", () => { + const scratchRepos: string[] = []; + + beforeEach(() => { + resetGitRepoCacheForTests(); + resetGitCommonDirCacheForTests(); + }); + + afterEach(() => { + for (const dir of scratchRepos.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + function scratchRepo(): string { const repoRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-git-repo-cache-")); + scratchRepos.push(repoRoot);Import
afterEachandbeforeEachfromvitest,resetGitCommonDirCacheForTestsfrom./git, andresetGitRepoCacheForTestsfrom./gitRepoCache.The three
detect-non-literal-fs-filenamewarnings on these lines are false positives.repoRootcomes fromfs.mkdtempSync, not from external input.🤖 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 `@apps/desktop/src/main/services/git/git.test.ts` around lines 176 - 186, Update the repo cache invalidation tests around scratchRepo to track each mkdtempSync-created repository, remove those directories in an afterEach cleanup, and reset both git caches before each test using resetGitCommonDirCacheForTests and resetGitRepoCacheForTests. Import the required Vitest hooks and reset helpers, while preserving the existing fixture setup and treating the filename-lint warnings as safe for generated repoRoot paths.Source: Linters/SAST tools
apps/desktop/src/main/services/git/git.ts (1)
362-371: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUser-initiated index mutations are throttled as reads.
isRefAffectingGitCommandnow decides two separate questions: whether to invalidate the cache, and whether to bypass the queue. The two answers are not the same.add,rm,mv, andcleando not move refs, so they are correctly absent fromREF_AFFECTING_GIT_VERBS, and they therefore take the read path here.stageFile,stageAll, anddiscardFileingitOperationsService.tsrun those verbs, so a click on Stage can now wait behind up to 12 bulk-assessment reads. That is the priority inversion the comment above says the bypass exists to prevent.Use a second predicate for the queue decision instead of reusing the ref predicate.
♻️ Proposed split of the two decisions
- const mutating = isRefAffectingGitCommand(args); - if (!mutating) await acquireGitSlot(); + const mutating = isRefAffectingGitCommand(args); + // Queue bypass is a wider question than cache invalidation: `add`, `rm`, + // `mv`, and `clean` never move a ref, but they are user-initiated and must + // not wait behind a bulk read fan-out. + const bypassQueue = mutating || isUserInitiatedGitWrite(args); + if (!bypassQueue) await acquireGitSlot(); let result: GitRunResult; try { result = await runGitOnce(args, opts); if (await shouldRetryAfterIndexLock(result)) { result = await runGitOnce(args, opts); } } finally { - if (!mutating) releaseGitSlot(); + if (!bypassQueue) releaseGitSlot(); }🤖 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 `@apps/desktop/src/main/services/git/git.ts` around lines 362 - 371, Use a separate predicate for queue bypass in runGit instead of reusing isRefAffectingGitCommand, so user-initiated mutations such as add, rm, mv, and clean skip the read throttle while ref-affecting commands retain cache invalidation behavior. Update the mutating check and its supporting command classification without changing the existing ref-invalidation predicate.
🤖 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 `@apps/desktop/src/main/services/git/git.ts`:
- Around line 507-513: The cache keys for HEAD and shared refs are built
inconsistently from raw paths. In
apps/desktop/src/main/services/git/git.ts:507-513, export a gitRefCacheKey(cwd,
ref) helper that uses pathKey(cwd) for HEAD and omits the path for other refs,
then use it in the runGitRepoCached call; in
apps/desktop/src/main/services/conflicts/conflictService.ts:278-282, replace the
inline key template with gitRefCacheKey(cwd, ref) so both readers share
normalized keys.
- Around line 430-459: Update the successful common-directory memo used by
gitCommonDirFor so it is invalidated whenever a worktree mutation runs through
runGit. Remove the corresponding cwd entry from gitCommonDirByCwd (and keep
related in-flight or retry state consistent as needed) before or after executing
the worktree command, ensuring later lookups do not reuse a repository path from
a previous worktree at the same absolute cwd.
In `@apps/desktop/src/main/services/git/gitOperationsService.ts`:
- Around line 1696-1701: Update the readConfig caching in the git operations
flow around readConfig so cache entries include lane.worktreePath (or an
equivalent unique worktree identifier) in addition to the config key. Preserve
the existing repository bucket and stable caching behavior while ensuring
identities from different lane worktrees cannot share cached results.
In `@apps/desktop/src/main/services/git/gitRepoCache.ts`:
- Around line 215-231: Update the cache write in the load path guarded by
repo.epoch === loadEpoch to preserve the deterministic nowMs anchor while
ensuring expiresAt remains at least one volatile TTL beyond the load completion
time. Add a named regression test in gitRepoCache.test.ts that resolves a load
after the volatile TTL elapses and verifies the following read is served from
cache.
---
Nitpick comments:
In `@apps/desktop/src/main/services/git/git.test.ts`:
- Around line 224-240: Export a gitProcessConcurrencyStatsForTests seam from
git.ts that reports runningGitProcesses, peakGitProcesses, and
MAX_CONCURRENT_GIT_PROCESSES. Update the “bounds concurrent read fan-out” test
to reset or capture stats before launching the 40 runGit calls, then assert the
observed peak never exceeds the configured max of 12 while preserving the
existing result assertions.
- Around line 176-186: Update the repo cache invalidation tests around
scratchRepo to track each mkdtempSync-created repository, remove those
directories in an afterEach cleanup, and reset both git caches before each test
using resetGitCommonDirCacheForTests and resetGitRepoCacheForTests. Import the
required Vitest hooks and reset helpers, while preserving the existing fixture
setup and treating the filename-lint warnings as safe for generated repoRoot
paths.
In `@apps/desktop/src/main/services/git/git.ts`:
- Around line 362-371: Use a separate predicate for queue bypass in runGit
instead of reusing isRefAffectingGitCommand, so user-initiated mutations such as
add, rm, mv, and clean skip the read throttle while ref-affecting commands
retain cache invalidation behavior. Update the mutating check and its supporting
command classification without changing the existing ref-invalidation predicate.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67818252-9a27-4c26-bf5f-ab3895aae4c4
⛔ Files ignored due to path filters (3)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**
📒 Files selected for processing (7)
apps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/git/git.test.tsapps/desktop/src/main/services/git/git.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/git/gitRepoCache.test.tsapps/desktop/src/main/services/git/gitRepoCache.tsapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx
| export async function gitCommonDirFor(cwd: string): Promise<string> { | ||
| const cacheKey = pathKey(cwd); | ||
| const memo = gitCommonDirByCwd.get(cacheKey); | ||
| if (memo !== undefined) return memo; | ||
| const retryAt = gitCommonDirRetryAtByCwd.get(cacheKey); | ||
| if (retryAt !== undefined && retryAt > Date.now()) return ""; | ||
| const existing = gitCommonDirInFlight.get(cacheKey); | ||
| if (existing) return await existing; | ||
|
|
||
| const request = (async () => { | ||
| const res = await runGit(["rev-parse", "--git-common-dir"], { | ||
| cwd, | ||
| timeoutMs: 8_000, | ||
| maxOutputBytes: 8 * 1024, | ||
| }); | ||
| const raw = res.exitCode === 0 ? res.stdout.trim() : ""; | ||
| if (!raw) { | ||
| gitCommonDirRetryAtByCwd.set(cacheKey, Date.now() + GIT_COMMON_DIR_RETRY_MS); | ||
| return ""; | ||
| } | ||
| const resolved = path.resolve(cwd, raw); | ||
| gitCommonDirByCwd.set(cacheKey, resolved); | ||
| gitCommonDirRetryAtByCwd.delete(cacheKey); | ||
| return resolved; | ||
| })().finally(() => { | ||
| if (gitCommonDirInFlight.get(cacheKey) === request) gitCommonDirInFlight.delete(cacheKey); | ||
| }); | ||
| gitCommonDirInFlight.set(cacheKey, request); | ||
| return await request; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The successful common-dir memo is never dropped.
gitCommonDirByCwd keeps a resolved path for the process lifetime, and no worktree mutation clears it. If ADE removes a lane worktree and a later worktree of a different repository is created at the same absolute path, knownGitCommonDir returns the previous repository. Two consequences follow: runGitRepoCached reads land in the wrong repository bucket, and mutation invalidation drops the wrong bucket. The retry map is already cleared for failures, so only the success path is affected.
Drop the memo entry for a cwd when a worktree command runs through runGit, or key the memo on the worktree's git dir instead of the cwd.
🤖 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 `@apps/desktop/src/main/services/git/git.ts` around lines 430 - 459, Update the
successful common-directory memo used by gitCommonDirFor so it is invalidated
whenever a worktree mutation runs through runGit. Remove the corresponding cwd
entry from gitCommonDirByCwd (and keep related in-flight or retry state
consistent as needed) before or after executing the worktree command, ensuring
later lookups do not reuse a repository path from a previous worktree at the
same absolute cwd.
| // Shares the repo cache — and the key shape — with conflictService's own HEAD | ||
| // read, which is how the two spawns every lane used to pay become one. | ||
| const res = await runGitRepoCached( | ||
| ["rev-parse", "HEAD"], | ||
| { cwd: worktreePath, timeoutMs: 8_000 }, | ||
| { key: `rev-parse:${worktreePath}:HEAD`, cacheClass: "volatile" }, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Both HEAD readers build the cache key from a raw path string. The repository bucket is normalized with pathKey, but neither key is. The documented collision between the two services therefore holds only when both callers pass byte-identical spellings of one path, and a repo-wide ref such as the shared base branch cannot share a single entry across lane worktrees of one repository. Add one shared key builder that normalizes the path and omits it for refs that are not worktree-specific.
apps/desktop/src/main/services/git/git.ts#L507-L513: export agitRefCacheKey(cwd, ref)helper that returnsrev-parse:${pathKey(cwd)}:HEADforHEADandrev-parse:${ref}for any other ref, then use it here.apps/desktop/src/main/services/conflicts/conflictService.ts#L278-L282: replace the inline template key with the samegitRefCacheKey(cwd, ref)helper so the two services collide by construction instead of by convention.
📍 Affects 2 files
apps/desktop/src/main/services/git/git.ts#L507-L513(this comment)apps/desktop/src/main/services/conflicts/conflictService.ts#L278-L282
🤖 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 `@apps/desktop/src/main/services/git/git.ts` around lines 507 - 513, The cache
keys for HEAD and shared refs are built inconsistently from raw paths. In
apps/desktop/src/main/services/git/git.ts:507-513, export a gitRefCacheKey(cwd,
ref) helper that uses pathKey(cwd) for HEAD and omits the path for other refs,
then use it in the runGitRepoCached call; in
apps/desktop/src/main/services/conflicts/conflictService.ts:278-282, replace the
inline key template with gitRefCacheKey(cwd, ref) so both readers share
normalized keys.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b089f0a022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const res = await runGitRepoCached( | ||
| ["rev-parse", "HEAD"], | ||
| { cwd: worktreePath, timeoutMs: 8_000 }, | ||
| { key: `rev-parse:${worktreePath}:HEAD`, cacheClass: "volatile" }, |
There was a problem hiding this comment.
When HEAD changes outside runGit—for example, an agent or user commits in an ADE terminal—this cache can return the previous SHA for 1.5 seconds. undoLastHeadChange uses getHeadSha for its "head has changed" guard and then runs reset --hard; during that window the stale value can make the guard pass and reset past the external commit, while runLaneOperation can also record an incorrect preHeadSha. Keep operational snapshots and destructive-action guards on an uncached HEAD read, reserving this cache for status/prediction polling.
Useful? React with 👍 / 👎.
Measured on this checkout (14 lanes), one full lane status refresh spawned **161 git processes with 32 peak concurrency**. 31 of those were byte-identical argv at a byte-identical cwd: `git rev-parse main` ran **14 times at the project root inside a single refresh**, and every lane paid two separate `rev-parse HEAD` because conflictService and rebaseSuggestionService each asked without knowing the other had. After: **133 processes, 12 peak, ~676ms -> ~560ms.** Lane data verified byte-identical against a cache-off control run in the same process — all 14 LaneSummary entries field-for-field, plus listSuggestions, listStatuses, and getBatchAssessment (ignoring its `computedAt` stamp). New `gitRepoCache.ts` is keyed by the repository's **common git dir**, so every lane worktree of one repo shares an entry. That sharing is the whole point: keying per worktree would leave a 14-lane repo holding 14 copies of one answer. Two freshness classes — 1.5s for resolved ref SHAs (long enough to collapse one refresh's duplicates, and less than a sixth of the 10s the lane list itself already caches for), 5 min for config-shaped answers. Concurrent misses share one load, which is what actually collapses a parallel fan-out; a plain TTL check lets all 14 through before the first resolves. `runGit` now bounds concurrent git processes at 12. Mutations bypass the queue deliberately — the cap exists to bound read fan-out, while mutations are user-initiated, few, and some allow 300s, so queueing a Commit behind a bulk conflict assessment would be a priority inversion the unbounded code did not have. Invalidation lives in `runGit` itself rather than at each mutation call site, so nothing has to remember — including code written later. It fires on failure as well as success: a rejected push can still have moved a remote-tracking ref, and an aborted rebase leaves HEAD moved. Two findings from review that were bugs, not polish: - `git -c core.editor=true rebase --continue` — the argv ADE actually uses — read `core.editor=true` as the verb, so a rebase that moved HEAD never invalidated. `runLaneOperation` then recorded a `postHeadSha` equal to the pre-rebase SHA, and Undo refuses to act when the recorded head does not match reality: Undo would have been permanently dead for every rebase/merge continue faster than 1.5s. - `ChatGitToolbar`'s changed-file badge summed `staged.length + unstaged.length`. `git status` reports a file with both staged and unstaged edits as `MM` and the parser puts it in both lists, so one dirty file counted as two. Both have regression tests verified to fail without their fix. Not done, with evidence: t3's staged/unstaged `--numstat` merge does not apply here. `git diff HEAD --numstat` reports the composite (a file staged +2/-1 then edited +1/-0 shows `3 1`, which is neither side), and a file whose worktree edit cancels its staged edit vanishes from the output while `git status` still reports it `MM`. ADE shows the two as separate lists, so the two calls are different questions, not one asked twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`git config` is repo-wide only until a repo enables `extensions.worktreeConfig`, after which each lane can carry its own `user.name`, `user.email`, or `remote.origin.url`. A repo-wide key served whichever lane asked first to every other lane, mis-attributing commits. ADE never enables the extension, but a user's repo can — and these two reads are per-lane anyway, so the repo-wide key was saving nothing measurable. Also treat `git branch -r/-a` listings as read-only. ADE's actual history query (`branch -r --contains <sha> <upstream>`) was already classified correctly via `--contains`; a bare `-r` listing was not. Raised independently by Greptile and Codex. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b089f0a to
43566e5
Compare
1 participant
Item 2 of three hygiene fixes from the 2026-08-09 t3code research (§3.4, git subprocess hygiene). Stacked on #1061 — the two share no files, so this retargets to
maincleanly once #1061 lands.Measured
Instrumented
runGitOnce(the thing that actually spawns) and drove the real service graph against this checkout — 14 active lanes, real data, no mocks. Numbers are deterministic across five runs.git rev-parse main@ project rootrev-parse HEADper lanePeak concurrency was unbounded before — linear in lane count, so 40 lanes would have reached ~90 simultaneous git processes.
Correctness verified, not assumed: a cache-off control run in the same process returns byte-identical lane data — all 14
LaneSummaryentries field-for-field including ahead/behind/dirty/rebaseInProgress, pluslistSuggestions,listStatuses, andgetBatchAssessment(ignoring itscomputedAtwall-clock stamp).What changed
gitRepoCache.ts— repo-scoped cache keyed by the repository's common git dir, so every lane worktree of one repo shares an entry. That sharing is the entire point: keying per worktree would leave a 14-lane repo holding 14 copies of one answer. Two freshness classes — 1.5 s for resolved ref SHAs (long enough to collapse one refresh's duplicates; the lane list itself already caches for 10 s, so this is six times more conservative), 5 min for config-shaped answers. Concurrent misses share a single load, which is what actually collapses a parallel fan-out — a plain TTL check lets all 14 through before the first resolves.Concurrency ceiling —
runGitbounds concurrent git processes at 12. Mutations bypass the queue deliberately: the cap exists to bound read fan-out, while mutations are user-initiated, few, and some allow 300 s (rebase --continue). Queueing a Commit behind a bulk conflict assessment would be a priority inversion the unbounded code did not have — and in a runtime-backed build this is one gate shared by every project on the machine.Invalidation lives in
runGit, not at each mutation call site, so nothing has to remember — including code written later. It fires on failure as well as success: a rejected push can still have moved a remote-tracking ref, and an aborted rebase leaves HEAD moved.Two review findings that were bugs, not polish
Undo was permanently breakable.
git -c core.editor=true rebase --continue— the argv ADE actually ships at two call sites — hadcore.editor=trueread as its verb, so a rebase that moved HEAD never invalidated.runLaneOperationthen recorded apostHeadShaequal to the pre-rebase SHA, andundoLastHeadChangerefuses to act when the recorded head disagrees with reality. Every rebase/merge continue faster than 1.5 s would have had Undo silently dead.The changed-file badge double-counted.
ChatGitToolbar.dirtyFileCountsummedstaged.length + unstaged.length;git statusreports a file with both staged and unstaged edits asMMand the parser puts it in both lists, so one dirty file showed as two. (This is the double-count the research predicted — found by checking, not by taking it on faith.)Both have regression tests verified to fail without their fix.
Not done, with evidence
t3's staged/unstaged
--numstatmerge does not apply to ADE. Empirically, in a scratch repo withf.txtstaged +2/−1 then further edited +1/−0:Worse, a file whose worktree edit cancels its staged edit disappears from
git diff HEADentirely whilegit statusstill reports itMM— it would render as a staged file with 0 additions. ADE shows staged and unstaged as separate lists, so these are two different questions, not one asked twice. The two calls are alreadyPromise.all'd, so they cost one spawn of wall time.Follow-ups (measured, deliberately not in this PR)
git worktree list --porcelainalready emits aHEAD <sha>line per worktree that the parser discards — wiring it through would remove the remaining 14rev-parse HEADspawns.rev-list --count/log -n 20for two lanes sitting at the same HEAD, fromrebaseSuggestionService.Verification
/quality: 1 Blocker, 1 High, 5 Medium, 3 Low — all applied. The Blocker (the-cargv) was a defect in my own first cut.validate-docs.mjs: 228 files.--path-format=absolutewas dropped so the probe no longer needs git ≥ 2.31; the returned path is resolved against cwd. All repo/cwd map keys go through the repo's canonicalpathKey, so two spellings of one directory cannot become two caches with one missed invalidation.🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Bug Fixes
Greptile Summary
The PR introduces a common-Git-directory cache and a global ceiling for concurrent read-only Git subprocesses while invalidating cached data after mutations.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Caller["Lane and Git services"] --> RunGit["runGit"] RunGit --> Classify{"Ref-affecting mutation?"} Classify -->|No| Gate["12-process read ceiling"] Gate --> Spawn["Git subprocess"] Classify -->|Yes| Spawn Spawn -->|Mutation completes| Invalidate["Invalidate common-repo cache"] Caller --> CachedRead["runGitRepoCached"] CachedRead --> CommonDir["Resolve common Git directory"] CommonDir --> Cache["Repository cache"] Cache -->|Hit or shared in-flight load| Caller Cache -->|Miss| RunGitReviews (4): Last reviewed commit: "review: key config-derived cache entries..." | Re-trigger Greptile
Context used: