Repository navigation
Add basic support for worktree branches (issue #629) - #633
Conversation
09eac74 to
f9583c5
Compare
99c496a to
b0d90ae
Compare
a229648 to
3a4a07e
Compare
3a4a07e to
deba78e
Compare
|
Cool and thank you for the image rich explanation. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved branch-state, duplicate-worktree handling, and main-thread performance issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds support for branches checked out in linked Git worktrees, including safer actions, visual indicators, and worktree-aware ref labels.
Changes:
- Detects and caches worktree branch ownership while preventing unsafe removal.
- Adds worktree icons, tooltips, colors, and “Open Worktree” actions.
- Centralizes ref colors and adds coverage for repository and menu behavior.
File summaries
| File | Summary |
|---|---|
Resources/Images/Images.svg |
Adds worktree branch icon artwork. |
Resources/html/views/history/history.js |
Applies worktree-aware ref styling. |
Resources/html/views/history/history.css |
Adds shared ref color variables and teal styling. |
GitXTests/PBGitRepositoryWorktreeTests.m |
Tests worktree parsing and deletion protection. |
GitXTests/PBGitHistoryWorktreeMenuTests.m |
Tests worktree context-menu behavior. |
GitX.xcodeproj/project.pbxproj |
Registers updated sources, tests, and resources. |
Classes/Views/PBSourceViewItem.m |
Supports named icon loading. |
Classes/Views/PBSourceViewItem.h |
Declares named-icon loading. |
Classes/Views/PBSidebarTableViewCell.m |
Displays worktree tooltips. |
Classes/Views/PBSidebarTableViewCell.h |
Adds worktree path state. |
Classes/Views/PBGitRevisionCell.m |
Applies centralized and worktree label colors. |
Classes/Views/PBGitRefLabelColors.m |
Defines shared ref colors. |
Classes/Views/PBGitRefLabelColors.h |
Declares the color provider API. |
Classes/git/PBGitRepository.m |
Reads worktrees and blocks unsafe deletion. |
Classes/git/PBGitRepository.h |
Exposes worktree lookup APIs. |
Classes/Controllers/PBWebHistoryController.m |
Passes worktree refs and colors to the web pane. |
Classes/Controllers/PBGitWindowController.m |
Opens linked worktrees. |
Classes/Controllers/PBGitWindowController.h |
Declares the open-worktree action. |
Classes/Controllers/PBGitSidebarController.m |
Shows worktree icons and paths. |
Classes/Controllers/PBGitHistoryController.m |
Updates checkout and remove menu actions. |
Review details
Suppressed comments (2)
Classes/Controllers/PBWebHistoryController.m:124
- When the selected branch changes to another branch with the same tip commit, this path only refreshes
worktreeRefs;commit.currentRefremains the previous branch becausereloadnever receives or updates it. The details pane will therefore keep the orange class on the old branch. Pass the current head ref intoreload(or otherwise refreshcurrentRef) beforeshowRefs.
[[self script] callWebScriptMethod:@"reload" withArguments:@[ [repository refNamesHeldByOtherWorktrees] ]];
Classes/Views/PBGitRevisionCell.m:223
colorForRef:is called fromdrawLabelAtIndex:, so the first label drawn after every refs reload synchronously launchesgit worktree listand waits forPBTaskon the main thread. A slow or unavailable git process can freeze the history view, and this new call makes the commit list pay that cost as well as the sidebar; populate or refresh the worktree map outside drawing (or asynchronously) before using it here.
if ([[controller repository] isRefHeldByAnotherWorktree:ref])
return [PBGitRefLabelColors worktreeBranchColor];
- Files reviewed: 19/21 changed files
- Comments generated: 2
- 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 (![PBGitBinary path].length) | ||
| return @{}; | ||
|
|
||
| NSString *output = [self outputOfTaskWithArguments:@[ @"worktree", @"list", @"--porcelain" ] error:NULL]; |
There was a problem hiding this comment.
Not fixed here, tracked as #647. Measured at 9.4ms median, once per refs reload; the drawing paths predate this PR, so moving the snapshot off the main thread is better done on its own.
|
@hannesa2, it is always good for someone else to review your work, even if it is a bot :) Copilot found a design-flaw, a bug and a performance issue I should also fix... - On It! |
…#635) One palette written down twice had drifted apart, and one label never reached its color at all. The history list and the commit details pane each carry their own copy of the ref label colors, which no longer agree, and in that pane the current branch is drawn green rather than orange. Issue gitx#635 has both, and how the second one hid for so long. * Write the colors down once and hand them to the page as CSS variables when it loads, leaving the same values in the stylesheet as fallbacks * Keep the history list's shades, the paler set, for both panes * Call the ref through, so the current branch is told apart again
A branch checked out somewhere else cannot be checked out here. GitX showed such a branch like any other, in the sidebar and in both commit panes alike, and went on to offer actions that git refuses or, worse, that `update-ref -d` carries out and leaves the other worktree pointing at a branch that is gone. * Read `git worktree list --porcelain` and answer which worktree holds a given ref, our own excluded, re-read whenever the refs are reloaded * Give those branches their own sidebar icon and a teal label in both commit panes, and name the worktree holding them in the row's tooltip * Offer that worktree in place of Checkout, and grey out Remove with a tooltip saying which worktree holds it, both settled while the menu is built * Refuse the removal in the repository itself, so a caller that never asked cannot walk into it either
fcf70af to
f9db304
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain in worktree activation, synchronous rendering lookups, and detached-ref state handling; cache lifecycle coverage is also incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Classes/Views/PBGitRevisionCell.m:223
isRefHeldByAnotherWorktree:reachesworktreePathsByRefName, which synchronously launchesgit worktree listwhen the cache is empty. Because this call is made fromdrawLabelAtIndex:, the first commit-list draw after everyreloadRefsnow blocks the main thread on a subprocess; populate or refresh this data off the drawing path and invalidate the cells when it is ready.
if ([[controller repository] isRefHeldByAnotherWorktree:ref])
return [PBGitRefLabelColors worktreeBranchColor];
Classes/git/PBGitRepository.m:316
outputOfTaskWithArguments:uses PBTask's synchronouslaunchTask:(with a 30-second timeout), and the new callers reach this lazy getter from sidebar and commit-label drawing. A slow or unavailable git can freeze the main UI during the first lookup; when the command fails, the nil cache also causes another blocking process launch on each draw. Populate this map before rendering or expose an asynchronous refresh/invalidation path.
NSString *output = [self outputOfTaskWithArguments:@[ @"worktree", @"list", @"--porcelain" ] error:NULL];
Classes/git/PBGitRepository.m:314
- The added tests cover porcelain parsing and an injected dictionary, but not this getter’s cache lifecycle: the no-git/failed-command
{}paths must remain uncached andreloadRefsmust invalidate a previously populated cache. Without coverage here, either regression can silently make worktree branches look ordinary until an unrelated reload.
if (_worktreePathsByRefName)
return _worktreePathsByRefName;
if (![PBGitBinary path].length)
return @{};
- Files reviewed: 19/21 changed files
- Comments generated: 2
- Review effort level: Lite
|
Both are real: the double-click path bypasses the worktree handling entirely and still tries to check out, and the |
The sidebar marked these branches, but nothing else followed the marking: one could still fall out of the map entirely, double-clicking one still attempted the checkout git refuses, and the details pane could go on naming a branch HEAD had already left. * Leave out our own worktree by the path it sits at, so a branch shared with another worktree is still reported as held there * Open the worktree when such a branch is double-clicked, instead of attempting the checkout and showing the refusal * Name the current branch again when the details pane refreshes in place, and let an empty name clear it rather than be ignored
f9db304 to
f747025
Compare
A branch checked out somewhere else cannot be checked out here.
Closes the basic half of #629, independent of #630.
Two commits. The first fixes #635 - the ref label colors that had drifted into
two hard-coded palettes, and the current branch never reaching its orange in
the commit details pane. Both predate this work and can be reviewed on their
own. The second is the worktree support on top.
Worth knowing while reading the second: the #635 fix is also what makes the
worktree labels work, since a ref reaches the page through WebScripting as a
function and the class comparison had to be called through.
GitX showed such a branch like any other and then offered actions on it that
git refuses - or, in one case, that git does not refuse and should.
git worktree list --porcelainand answer which worktree holds a givenref, our own excluded, re-read whenever the refs are reloaded
drawn on the same 16x16 grid as the icons beside it and kept in
Resources/Images/Images.svgwith the others; the row's tooltip names theworktree, since the icon says "held elsewhere" but not where
colorForRef:switches on reftype, and a worktree branch is still type
head, so those labels were plaingreen
path, and grey out Remove with a tooltip saying which worktree holds the
branch - both settled while the menu is built rather than after the action
fails. Remove stays in place rather than disappearing, so the entry does not
go missing from under a reader who knows it should be there; a disabled item
is built without its action, which is what keeps it from firing wherever the
menu is shown.
deleteRef:as well, so a caller that never askedcannot walk into it either
Why Remove matters more than Checkout
Checkout of such a branch is refused by git itself, so the cost of not knowing
was a confusing error (which #630 improves). Remove is different: GitX deletes
refs with
update-ref -d, which has none of the safetygit branch -dhas.The other worktree is left on a branch that no longer exists. That is why the
check is in the repository as well as in the menu.
On the teal: the palette draws black text with no color set, so every existing
label color sits between 10.7:1 and 17.5:1 against it.
#8fe0d5is 13.8:1, andat 36% saturation it sits between the green (42%) and the remote blue (32%)
rather than standing out from both.
Screenshots of Suggested Design
Local Branch - Show the Correct Colors (Bug-Fix 1)
Current Branch - Show the Correct Colors (Bug-Fix 2)
Worktree Branch - Show the New "Teal" Color and the New Icon
Worktree Branch - Show the New Context-Menu
Test plan
make unit-test: 113 tests, 0 failures, 12 of them new, inPBGitRepositoryWorktreeTests(the porcelain reader over a main worktree, alinked one, a detached one, a locked one and a bare repository; our own
branch excluded; the refusal to remove) and
PBGitHistoryWorktreeMenuTests(Open Worktree replacing Checkout, Remove present but disabled and saying
why, an ordinary branch unchanged)
fail
its own. Read back through accessibility, every branch another worktree holds
carries
WorktreeBranchTemplate, whilemaster- held by this checkout -carries
BranchTemplateplus the checkmark, andpr625, held nowhere,carries
BranchTemplatealone. That matchesgit branchexactly, and itpicked up a worktree added after the first run without being told.
those refs and no others
16x16 and magnified, in both tints
the page:
master [refs head currentBranch], a worktree branch[refs head worktree], a remote[refs remote]exposes no
AXHelpfor those rows) and the context menu itself, which doesnot survive an accessibility query - hence the unit test over
menuItemsForRef:Two things worth a look
The reader caches per refs-reload, and an early draw that finds no git binary
must not poison that cache with an empty answer - an earlier build did exactly
that, and no icon appeared until something forced a reload. It now returns empty
without caching, so the next draw retries.
The first lookup after each reload shells out to git inside a draw pass on the
main thread. That is the shape the sidebar already had, one subprocess per
reload, but the commit list reaches for it too now.
Not in scope: creating or removing worktrees, and a WORKTREES section in the
sidebar next to SUBMODULES.