Repository navigation
Stop the worktree lookup stalling the history while it draws - #656
Merged
Merged
Conversation
goneng
marked this pull request as ready for review
September 15, 2026 19:54
goneng
marked this pull request as draft
September 15, 2026 19:55
goneng
marked this pull request as ready for review
September 15, 2026 21:39
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Address the stale-snapshot deletion risk and refresh failure-handling issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request moves worktree lookup off the main thread and serves UI consumers from asynchronously refreshed snapshots.
Changes:
- Adds cached, asynchronous worktree refreshes.
- Retains snapshots and limits notifications to actual changes.
- Adds tests for snapshot reads and refresh behavior.
File summaries
| File | Review summary |
|---|---|
GitXTests/PBGitRepositoryWorktreeTests.m |
Adds tests for non-nil snapshot reads, change notifications, and refresh coalescing. |
Classes/git/PBGitRepository.m |
Implements snapshot management. Findings: critical (2 votes) stale snapshots can allow unsafe branch deletion; moderate (2 votes) failed lookups clear valid data; moderate (3 votes) obsolete in-flight results can be published; moderate (1 vote) unavailable Git is treated as an empty successful snapshot. |
Review details
Suppressed comments (1)
Classes/git/PBGitRepository.m:351
takeWorktreePaths:@{}treats an unavailable git binary as a successful empty snapshot. That discards the last known worktree map and emitsrefs, so labels and menu safety can be wrong until another reload; it also removes the old retry-on-next-read behavior. Leave the refresh pending and retain the previous snapshot when git is unavailable.
if (![PBGitBinary path].length) {
self.worktreePathsNeedRefresh = NO;
[self takeWorktreePaths:@{}];
return;
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| if (_worktreePathsByRefName) | ||
| return _worktreePathsByRefName; | ||
| return _worktreePathsByRefName ?: @{}; |
| return; | ||
|
|
||
| mainRepository.worktreeLookupInFlight = NO; | ||
| [mainRepository takeWorktreePaths:paths]; |
| return @{}; | ||
|
|
||
| _worktreePathsByRefName = [PBGitRepository worktreePathsFromPorcelain:output excludingWorktreeAtPath:self.workingDirectory]; | ||
| return [PBGitRepository worktreePathsFromPorcelain:output excludingWorktreeAtPath:ourPath]; |
…itx#647) Which branches other worktrees hold was read by the first label drawn after each refresh, so wherever git is slow to start the window waited on it mid-draw. * Read it on a utility queue when the refs reload, and swap the finished snapshot in on the main thread, behind a generation guard so an overlapping reload cannot land out of order. * Keep the previous snapshot readable until the new one arrives, so the labels do not fall back to their plain colours after every refresh. * Announce only a snapshot that differs. The refs observers rearrange the history and reload the sidebar, so announcing every reload would cost two of those in place of the 9ms it saves. * Answer the drawing paths from the snapshot alone, and never with nil, since -refNamesHeldByOtherWorktrees feeds an array literal.
-reloadRefs is called from -haveRefsBeenModified, which the history list polls while it walks, so starting a lookup per call let an unbounded number of git processes run at once. A wiring test run reloads 22 times locally, and far more where the walk is long. * Mark the snapshot stale on reload and start a lookup only when none is in flight, restarting once at the end if more reloads arrived. That holds it to one process running and one queued. * Confine the flags to the main queue, since -reloadRefs arrives on the history list's operation thread and the completion runs on main. * Drop the generation counter, which existed to discard results that landed out of order and cannot now. * Put the git call behind -readWorktreePathsExcluding:, so a test subclass can count the runs and hold one open.
goneng
force-pushed
the
worktree-lookup-off-the-draw-path
branch
from
September 17, 2026 21:10
17e06d9 to
2b3f451
Compare
This was referenced Sep 18, 2026
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
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.
Which branches other worktrees hold was read lazily, and the caller that found
the cache empty was usually a drawing one:
PBGitRevisionCell -colorForRef:from
-drawLabelAtIndex:, or the sidebar's cell. Both run on the main threadand
PBTaskwaits for the process, so the first label drawn after every refsreload waited on a
git worktree list --porcelain.Fixes #647.
Changes
finished result in on the main thread. A generation counter drops a result
whose reload has been superseded, the same guard
PBGitRevListuses for thestale flush in History view shows duplicate commit rows after rapid branch-changing git operations (e.g. git switch -c) #563.
clearing it. Clearing would leave every label drawn in the gap looking like
an ordinary branch, so each refresh would flash.
refsobserversrearrange the history and reload the sidebar, so announcing unconditionally
would spend two of those per refresh to save 9ms. In the steady state
nothing is announced at all.
-refNamesHeldByOtherWorktreesfeeds an@[...]literal inPBWebHistoryController, which is the crash shape Clicking a commit quits GitX when its repository has gone away #648 was about.Test plan
make unit-test: 129 tests green, including the 12 existing worktree tests.non-nil on all three read paths, and that an unchanged snapshot announces
nothing while a changed one announces once.
What these tests do not cover
They exercise the snapshot logic, not the threading. Nothing here proves the
git process left the main thread, and a green run should not be read as saying
so. Measuring that wants either a main-thread assertion inside the lookup or a
timing harness around a repository on a slow filesystem, and I would rather
raise that than imply the tests settle it.
Worth a reviewer's eye on one consequence: menu building and validation
(
-pathOfWorktreeHoldingRef:from the history and sidebar controllers) nowread a snapshot that can be one refresh stale, where before they would block
and get a fresh one. #633 already refuses the removal in
-deleteRef:aswell, so a stale menu cannot actually perform a refused action, but the
greying could lag by a refresh.