Guard pr merge --delete-branch against worktree conflicts - #13955
Open
tidy-dev wants to merge 1 commit into
Open
Guard pr merge --delete-branch against worktree conflicts#13955tidy-dev wants to merge 1 commit into
pr merge --delete-branch against worktree conflicts#13955tidy-dev wants to merge 1 commit into
Conversation
Rework the local-cleanup half of --delete-branch to handle git worktrees: Scenario 1 - cwd is the PR head worktree: skip local cleanup entirely and print a warning with manual cleanup instructions, since we cannot safely check out another branch or remove the worktree we are standing inside. Scenario 2 - cwd is not the PR head worktree but a sibling worktree has the branch: remove that worktree via git worktree remove, then delete the branch ref. If removal fails (e.g. dirty worktree), warn and skip rather than exiting non-zero after a successful merge. The conventional single-working-directory path (no worktrees) is unchanged. Remote branch deletion proceeds normally in all cases. Also adds git.Client.WorktreeRemove() helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates gh pr merge --delete-branch local cleanup to be safe in repositories using git worktrees, avoiding disruptive git checkout <base> behavior when the PR head branch is checked out in a linked worktree.
Changes:
- Detect whether the PR head branch is checked out in the current linked worktree vs a sibling linked worktree vs no worktree involvement, and adjust local cleanup accordingly.
- Add a
git.Client.WorktreeRemovehelper and wire it into PR merge local cleanup. - Expand/update PR merge tests to stub
git worktree list/--show-topleveland add new worktree-specific scenarios.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/pr/merge/merge.go | Adds worktree-aware branching to local branch deletion behavior and introduces linkedWorktreeForBranch. |
| pkg/cmd/pr/merge/merge_test.go | Adds new worktree scenarios and updates existing stubs for new git calls. |
| git/client.go | Adds a WorktreeRemove helper on the git client. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/cmd/pr/merge/merge_test.go:2149
- If
git worktree removeis invoked with a--delimiter (to prevent option injection via paths starting with-), this stub needs to include--so the test continues to match the executed command.
cs.Register(`git worktree remove /path/to/feature-wt`, 0, "")
pkg/cmd/pr/merge/merge_test.go:2202
- If
git worktree removeis invoked with a--delimiter, this failing stub should also include--so the test matches the executed command.
cs.Register(`git worktree remove /path/to/feature-wt`, 128, "fatal: '/path/to/feature-wt' contains modified or untracked files, use --force to delete it")
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Low
Comment on lines
+420
to
+421
| _ = m.warnf(" git worktree remove %s && git branch -D %s\n", | ||
| currentWorkdir, m.pr.HeadRefName) |
Comment on lines
+412
to
+415
| // worktrees[0] is always the main worktree (the initially cloned repo). | ||
| // If our workdir differs we are inside a linked worktree. | ||
| isInLinkedWorktree := len(worktrees) > 1 && worktrees[0].Path != currentWorkdir | ||
|
|
| // WorktreeRemove removes the worktree at the given path via | ||
| // `git worktree remove <path>`. | ||
| func (c *Client) WorktreeRemove(ctx context.Context, path string) error { | ||
| cmd, err := c.Command(ctx, "worktree", "remove", path) |
| assert.Equal(t, "", output.String()) | ||
| assert.Contains(t, output.Stderr(), "skipping local delete") | ||
| assert.Contains(t, output.Stderr(), "navigate out of the worktree") | ||
| assert.Contains(t, output.Stderr(), "git worktree remove /path/to/feature-wt && git branch -D feature") |
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.
Closes https://github.com/github/gh-cli-and-desktop/issues/271
Closes #3442
Makes
gh pr merge --delete-branchbehave safely under git worktrees. Today the local-cleanup half of--delete-branchunconditionally runsgit checkout <base-branch>then deletes the branch. In a worktree setup this either fails (fatal: '<base>' is already used by worktree at ...) or silently repurposes the worktree, leaving a confusing partial state.Changes
Branches the local-cleanup logic on whether the PR head branch is checked out in a linked worktree:
git worktree removeit, then delete the local branch ref. Nogit checkout <base>needed.In all cases
gh pr mergenever exits non-zero solely because local worktree cleanup could not complete.Demo
Text transcript of the demo
Three worktrees are set up:
main-checkout(main),feature-wt(linked, onwt-demo-scenario1), andsibling-wt(linked, onwt-demo-scenario2).Show starting state
git worktree listshows all three worktrees.Scenario 1: merge from inside the PR's linked worktree
Scenario 2: merge from main worktree (PR branch lives in sibling)
Final state
git worktree listshows onlymain-checkoutandfeature-wtremain -sibling-wtwas removed automatically, whilefeature-wtwas preserved with cleanup instructions.Design decisions
linkedWorktreeForBranch()skipsworktrees[0]- git always lists the main worktree first. By iterating[1:]we only consider linked worktrees, eliminating false positives in normal (non-worktree) repos.len(worktrees) > 1guard - if only the main worktree exists, no linked worktrees are possible so we skip worktree logic entirely.--forceongit worktree remove- a dirty worktree is reported as a warning rather than forced, preserving uncommitted work.git checkout <base>+ pull + delete path only runs when there's no worktree involvement.Test scenarios (4 new + 9 updated)
All 9 existing
deleteLocalBranchtests updated with worktree stubs to prevent panics from the newgit worktree listcall.