Guard `pr merge --delete-branch` against worktree conflicts by tidy-dev · Pull Request #14007 · cli/cli

tidy-dev · 2026-07-29T14:38:39Z

Closes https://github.com/github/gh-cli-and-desktop/issues/271
Closes #3442

Makes gh pr merge --delete-branch behave safely under git worktrees. Today the local-cleanup half of --delete-branch unconditionally runs git 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.

The two questions

Deleting a local branch is only safe if git can answer two questions. This PR makes --delete-branch check both up front instead of blindly running git checkout <base> + git branch -d:

Is the branch being deleted checked out in a worktree? git refuses to delete a branch that any worktree currently has checked out.
If we're standing on that branch, can we step off it onto the base branch? Deleting the branch you're on first requires git checkout <base>, which fails if the base branch is itself checked out in another worktree.

Every case this PR handles is just a distinct answer to those two questions:

#	Situation	Blocked by	Result
1	cwd is the linked worktree holding the PR head	Q1 - head is checked out here, and a worktree can't remove itself	Warn + skip local delete
2	PR head is checked out in the main worktree (run from elsewhere)	Q1 - head is checked out in main, which can't be removed	Warn + skip local delete
3	PR head is in a sibling linked worktree	Q1 - but the sibling worktree can be removed	Remove worktree, then delete branch
4	On the PR head, but base is checked out in another worktree	Q2 - the required `git checkout <base>` would fail	Warn + skip local delete
5	Sibling worktree holding the head is dirty	Q1 - worktree can't be removed without discarding uncommitted work	Warn + skip, worktree preserved
-	No worktree involvement	Neither	Unchanged: `checkout <base>` + pull + delete

Cases 1, 2, 3, and 5 are all variations of Q1 (the head branch is checked out somewhere) - they differ only in where it's checked out and whether that worktree can be safely removed. Case 4 is the odd one out: you're free to delete the head branch, but you can't get off it because Q2 fails.

In all cases gh pr merge never exits non-zero solely because local worktree cleanup could not complete. The merge and remote-branch deletion always go through; only the local tidy-up is skipped, with a warning and manual instructions.

Demo

The recording runs gh pr merge --delete-branch first in a normal (non-worktree) checkout to show nothing regresses, then walks through all five worktree cases. (Branch names come from the setup script; PR numbers and /private/tmp paths vary per run.)

Text transcript of the demo

Baseline: regular (non-worktree) merge - on the PR head branch in a plain checkout. Existing behavior: switch to base, then delete.

$ cd /tmp/gh-wt-demo/s0-regular
$ git worktree list
/private/tmp/gh-wt-demo/s0-regular  1a2b3c4 [wtdemo-regular]

$ gh pr merge --squash --delete-branch
✓ Squashed and merged pull request tidy-dev/playground#41 (Demo: regular (non-worktree) merge)
✓ Deleted local branch wtdemo-regular and switched to branch main
✓ Deleted remote branch wtdemo-regular

Case 1 (Q1): head branch checked out in the CURRENT worktree - a worktree can't remove itself, so skip local delete.

$ cd /tmp/gh-wt-demo/s1/wt
$ git worktree list
/private/tmp/gh-wt-demo/s1/repo  1a2b3c4 [main]
/private/tmp/gh-wt-demo/s1/wt    5d6e7f8 [wtdemo-cwd]

$ gh pr merge --squash --delete-branch
✓ Squashed and merged pull request tidy-dev/playground#42 (Demo: head branch in the current worktree)
! Branch wtdemo-cwd is checked out in the current worktree (/private/tmp/gh-wt-demo/s1/wt); skipping local delete
  To finish cleanup, first navigate out of the worktree, then run:
  git worktree remove /private/tmp/gh-wt-demo/s1/wt && git branch -D wtdemo-cwd
✓ Deleted remote branch wtdemo-cwd

Case 2 (Q1): head branch checked out in the MAIN worktree - run from a sibling; the main worktree can't be removed, so skip local delete.

$ cd /tmp/gh-wt-demo/s2/side
$ git worktree list
/private/tmp/gh-wt-demo/s2/repo  1a2b3c4 [wtdemo-head-in-main]
/private/tmp/gh-wt-demo/s2/side  5d6e7f8 [wtdemo-s2-side]

$ gh pr merge wtdemo-head-in-main --squash --delete-branch
✓ Squashed and merged pull request tidy-dev/playground#43 (Demo: head branch in the main worktree)
! Branch wtdemo-head-in-main is checked out in the main worktree (/private/tmp/gh-wt-demo/s2/repo); skipping local delete
  To finish cleanup, switch the main worktree off wtdemo-head-in-main, then run:
  git branch -D wtdemo-head-in-main
✓ Deleted remote branch wtdemo-head-in-main

Case 3 (Q1): head branch in a SIBLING worktree - the sibling is removable, so remove it and delete the branch. No git checkout <base> needed.

$ cd /tmp/gh-wt-demo/s3/repo
$ git worktree list
/private/tmp/gh-wt-demo/s3/repo     1a2b3c4 [main]
/private/tmp/gh-wt-demo/s3/sibling  5d6e7f8 [wtdemo-head-in-sibling]

$ gh pr merge wtdemo-head-in-sibling --squash --delete-branch
✓ Squashed and merged pull request tidy-dev/playground#44 (Demo: head branch in a sibling worktree)
✓ Removed worktree /private/tmp/gh-wt-demo/s3/sibling
✓ Deleted local branch wtdemo-head-in-sibling
✓ Deleted remote branch wtdemo-head-in-sibling

Case 4 (Q2): base branch checked out in ANOTHER worktree - on the head branch, but git checkout <base> would fail, so skip local delete.

$ cd /tmp/gh-wt-demo/s4/repo
$ git worktree list
/private/tmp/gh-wt-demo/s4/repo  1a2b3c4 [wtdemo-base-elsewhere]
/private/tmp/gh-wt-demo/s4/base  5d6e7f8 [main]

$ gh pr merge --squash --delete-branch
✓ Squashed and merged pull request tidy-dev/playground#45 (Demo: base branch checked out elsewhere)
! Base branch main is checked out in another worktree (/private/tmp/gh-wt-demo/s4/base); skipping local delete
  To finish cleanup, switch off wtdemo-base-elsewhere in another worktree, then run:
  git branch -D wtdemo-base-elsewhere
✓ Deleted remote branch wtdemo-base-elsewhere

Case 5 (Q1): DIRTY sibling worktree - the head's worktree has uncommitted work, so it's kept (not force-removed) and local delete is skipped. Note the merge still succeeds and exits 0.

$ cd /tmp/gh-wt-demo/s5/repo
$ git worktree list
/private/tmp/gh-wt-demo/s5/repo     1a2b3c4 [main]
/private/tmp/gh-wt-demo/s5/sibling  5d6e7f8 [wtdemo-dirty-sibling]
$ git -C /tmp/gh-wt-demo/s5/sibling status --short
 M scenario-wtdemo-dirty-sibling.txt

$ gh pr merge wtdemo-dirty-sibling --squash --delete-branch
✓ Squashed and merged pull request tidy-dev/playground#46 (Demo: dirty sibling worktree)
! Could not remove worktree /private/tmp/gh-wt-demo/s5/sibling; skipping local branch delete: failed to run git: fatal: '/private/tmp/gh-wt-demo/s5/sibling' contains modified or untracked files, use --force to delete it
✓ Deleted remote branch wtdemo-dirty-sibling

Design decisions

linkedWorktreeForBranch() skips worktrees[0] - git always lists the main worktree first. By iterating [1:] we only consider linked worktrees, eliminating false positives in normal (non-worktree) repos.
worktreeForBranch() scans all worktrees - a companion helper that includes the main worktree, used to detect when the base branch is checked out elsewhere (Q2) before attempting git checkout <base>.
len(worktrees) > 1 guard - if only the main worktree exists, no linked worktrees are possible so we skip worktree logic entirely.
No --force on git worktree remove - a dirty worktree is reported as a warning rather than forced, preserving uncommitted work (Case 5).
Conventional single-working-directory path unchanged - the git checkout <base> + pull + delete path only runs when there's no worktree involvement.

Tests

pkg/cmd/pr/merge worktree scenarios are covered by a single table-driven test (TestPrMerge_deleteBranch_worktrees) with named subtests:

Scenario	Behavior
cwd is the head branch's linked worktree	Warn + skip local delete
Head branch is in the main worktree (run elsewhere)	Warn + skip local delete
Head branch is in a sibling linked worktree	Remove worktree + delete branch
Base branch is checked out in another worktree	Warn + skip local delete
Dirty sibling worktree	Warn, skip branch delete

Additional coverage:

TestPrMerge_deleteBranch_noWorktreeConflict - normal single-working-directory repo, existing switch + pull + delete behavior unchanged.
TestParseWorktrees (git package) - table-driven unit tests for git worktree list --porcelain parsing: empty output, single/multiple records, detached HEAD, bare main worktree, and no-trailing-blank-line variants.
The 9 existing deleteLocalBranch tests were updated with worktree stubs to account for the new git worktree list call.

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>

Copilot

🟡 Not ready to approve

There is a confirmed unhandled worktree conflict case that can still cause gh pr merge --delete-branch to exit non-zero due to local cleanup (head branch checked out in the main worktree while running from another linked worktree), and the new worktree parsing logic lacks direct unit tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR makes gh pr merge --delete-branch safe to run in repositories using git worktree, avoiding failures or confusing partial cleanup states by detecting when the PR head branch is checked out in a linked worktree and adjusting local cleanup behavior accordingly.

Changes:

Add git worktree discovery/removal support (git worktree list --porcelain, git worktree remove) to enable worktree-aware cleanup decisions.
Update pr merge local branch deletion logic to (a) warn+skip when run inside the head branch’s linked worktree, (b) remove a sibling linked worktree before deleting the local branch, while preserving existing behavior for normal repos.
Expand and adjust pr merge tests to cover new worktree scenarios and stub new git calls.

File summaries

File	Description
pkg/cmd/pr/merge/merge.go	Implements worktree-aware branching of local cleanup and adds helper to locate linked worktrees for a branch.
pkg/cmd/pr/merge/merge_test.go	Updates existing delete-branch tests for new git calls and adds new worktree-focused scenarios.
git/objects.go	Introduces a `git.Worktree` struct used to represent parsed worktree entries.
git/client.go	Adds `Worktrees` / `WorktreeRemove` APIs and parsing logic for `git worktree list --porcelain`.

Review details

Files reviewed: 4/4 changed files
Comments generated: 3
Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Guard against git branch -D failing when the PR head branch is checked out in the main worktree while running from a different worktree. Warn and skip local delete instead of exiting non-zero on local cleanup. Also add unit tests for parseWorktrees and simplify its record parsing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

When deleting the PR head branch requires checking out the base branch, git fails if the base branch is checked out in another worktree. Detect that case and warn+skip local delete instead of exiting non-zero, and fix an unrealistic worktree fixture in the no-conflict test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Collapse the five per-scenario worktree deleteBranch tests into a single table-driven test with named subtests, matching the AGENTS.md testing guidance and removing repeated setup boilerplate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…rge-worktree-guards

Trunk migrated BranchDeleteRemote to safeurl.JoinPath, which escapes the branch ref's slash (heads%2Ffeature). Update the new worktree test stubs to match the encoded path so httpmock.REST matches after merging trunk. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

babakks

Thanks for the PR, @tidy-dev! 🍻 I think my main comment is this one for now (skip the child ones).

I haven't fully reviewed the PR yet (i.e. tests), but wanted to share thoughts with you. Sorry if is being so verbose.

Just acknowledging the scenarios laid out below are already covered by the current PR. So, this is just about making it easier to follow. This is also why I'm happy to keep it as is if you don't see value in the refactor, or you'd rather see it as a follow-up.

I'm having a hard time following the flow here, and I think I've worked out why and how to improve it. Obviously, the original cleanup logic was written under an assumption that no longer holds: it only ever asked "is cwd currently on the PR head branch?", because in a single working directory a branch can only ever be checked out in that one place. So "I'm on the head branch" and "the head branch is checked out somewhere" were the same statement, and the code could safely do checkout-base then delete.

The worktree scenarios have been layered on top of that same structure, reusing the old checkout-base-then-delete path and bolting guard branches around it. That reuse is what makes this hard to read: the legacy path and the new worktree guards are interleaved, several of the if arms are implicitly coupled (one only runs because an earlier one didn't), and the base branch handling is tangled up with cases that never actually touch the base branch.

Suggestion

I think it's worth stepping back and reassessing the scenarios from scratch. The key realization is that currentBranch == head is just one special case of a single broader question: "where is the PR head branch checked out, relative to cwd?" Keying off that one dimension gives a flat, mutually exclusive set of cases:

Head branch is checked out...	git constraint in play	Action
nowhere (ref only)	branch not checked out anywhere, `-D` succeeds	delete the ref directly
in the current worktree, which is the main worktree	cannot delete the branch we are on, must move off it first	switch to base, pull, delete (the only row that touches base)
in the current worktree, which is a linked worktree	cannot repurpose the worktree we are standing in	warn and skip
in another linked worktree	must un-check-out before `-D`, and a linked worktree is removable	remove that worktree, then delete the ref
in the main worktree (we are elsewhere)	cannot remove the main worktree, cannot `-D` a branch checked out there	warn and skip

Every row reduces to the same precondition: make sure no worktree has the head branch checked out, then delete the ref. The base branch logic is only needed in a single row, which is what decouples everything else.

Here is a sketch of how the body could read (not real code):

// ---- Locate the head branch: the single master dimension ----
headWt = worktreeForBranch(worktrees, pr.HeadRefName)   // nil if not checked out anywhere
mainWt = worktrees[0]                                   // git always lists main first

// Row 1: head not checked out anywhere -> just delete the ref.
if headWt == nil:
    return deleteRefAndReport(switchedTo = "")

// Row 2 & 3: head is checked out in THIS worktree.
if headWt.Path == currentWorkdir:
    isMain = (currentWorkdir == mainWt.Path)

    // Row 3: linked worktree we are standing in -> cannot clean up safely.
    if not isMain:
        warnSkip_currentLinked(currentWorkdir)
        return nil

    // Row 2: main worktree, sitting on head -> legacy "switch off head" path.
    // This is the ONLY leaf that touches the base branch.
    baseWt = worktreeForBranch(worktrees, pr.BaseRefName)
    if baseWt != nil and baseWt.Path != currentWorkdir:
        // base busy in another worktree -> checkout would fatal.
        warnSkip_baseBusy(baseWt.Path)
        return nil

    if not switchToBase():           // CheckoutBranch or CheckoutNewBranch + Pull
        return nil                   // warn inside; do not fail the merge
    return deleteRefAndReport(switchedTo = pr.BaseRefName)

// Row 4 & 5: head is checked out in ANOTHER worktree.
if headWt.Path == mainWt.Path:
    // Row 5: main worktree elsewhere -> cannot remove it, cannot -D.
    warnSkip_headInMain(mainWt.Path)
    return nil

// Row 4: another linked worktree -> remove it, then delete the ref.
if err = GitClient.WorktreeRemove(headWt.Path); err != nil:
    warn("could not remove worktree %s; skipping local delete: %s", headWt.Path, err)
    return nil
info("Removed worktree %s", headWt.Path)
return deleteRefAndReport(switchedTo = "")

A nice side effect is that currentBranch disappears entirely (locating the head via the worktree list subsumes it and naturally handles detached HEAD), and worktreeForBranch replaces both current helpers. Happy to talk it through if you want to pair on it.

babakks · 2026-08-05T10:08:14Z

+	// Branch is the fully qualified ref checked out in the worktree
+	// (e.g. "refs/heads/main"). It is empty when the worktree has a detached
+	// HEAD or is the bare main worktree.
+	Branch string


Let's call it Ref as per docs.

babakks · 2026-08-05T10:11:50Z

+	worktrees, _ := m.opts.GitClient.Worktrees(ctx)
+	currentWorkdir, _ := m.opts.GitClient.ToplevelDir(ctx)


In a follow-up we can extract git commands used in pr checkout --worktree. For instance, this TopLevelDir method can be used in there.

babakks · 2026-08-05T10:50:23Z

+	if len(worktrees) > 0 &&
+		worktrees[0].Branch == "refs/heads/"+m.pr.HeadRefName &&
+		worktrees[0].Path != currentWorkdir {


Read the other comment first.

nitpick: len(worktrees) is always > 0 even if there are no worktrees other than the main working directory, or even if the main working directory is a bare clone. So, this part of the check can be misleading (the reader may read it as there are cases where worktrees can be empty).

Suggested change

if len(worktrees) > 0 &&

worktrees[0].Branch == "refs/heads/"+m.pr.HeadRefName &&

worktrees[0].Path != currentWorkdir {

if worktrees[0].Branch == "refs/heads/"+m.pr.HeadRefName &&

worktrees[0].Path != currentWorkdir {

babakks · 2026-08-05T10:59:27Z

+	worktrees, _ := m.opts.GitClient.Worktrees(ctx)
+	currentWorkdir, _ := m.opts.GitClient.ToplevelDir(ctx)


Let's now swallow the errors here. This would also secure the length check removal (see the other comment).

Suggested change

worktrees, _ := m.opts.GitClient.Worktrees(ctx)

currentWorkdir, _ := m.opts.GitClient.ToplevelDir(ctx)

worktrees, err := m.opts.GitClient.Worktrees(ctx)

if err != nil {

return err

}

currentWorkdir, err := m.opts.GitClient.ToplevelDir(ctx)

if err != nil {

return err

}

babakks · 2026-08-05T11:35:29Z

+	if len(worktrees) > 0 &&
+		worktrees[0].Branch == "refs/heads/"+m.pr.HeadRefName &&
+		worktrees[0].Path != currentWorkdir {


question: why can't we simplify this if-statement to:

Suggested change

if len(worktrees) > 0 &&

worktrees[0].Branch == "refs/heads/"+m.pr.HeadRefName &&

worktrees[0].Path != currentWorkdir {

if isInLinkedWorktree && worktrees[0].Branch == "refs/heads/"+m.pr.HeadRefName {

The len(worktrees) > 0 guard was always true since Worktrees() always lists at least the main worktree. Reuse the existing isInLinkedWorktree predicate instead, which already implies len > 1 (keeping worktrees[0] access safe) and folds in the path comparison. Behavior is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

tidy-dev marked this pull request as ready for review July 29, 2026 15:20

tidy-dev requested a review from a team as a code owner July 29, 2026 15:20

tidy-dev requested review from Copilot and williammartin July 29, 2026 15:20

Copilot started reviewing on behalf of tidy-dev July 29, 2026 15:21 View session

tidy-dev requested a review from babakks July 29, 2026 15:23

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Comment thread pkg/cmd/pr/merge/merge.go

Comment thread pkg/cmd/pr/merge/merge_test.go Outdated

Comment thread git/client.go

tidy-dev marked this pull request as draft July 30, 2026 13:00

williammartin reviewed Jul 30, 2026

View reviewed changes

Comment thread git/client.go

tidy-dev and others added 5 commits August 4, 2026 12:04

Merge remote-tracking branch 'origin/trunk' into pr/14007/tidy-dev-me…

2a98d3b

…rge-worktree-guards

babakks reviewed Aug 5, 2026

View reviewed changes

Navigation Menu

Uh oh!

Guard `pr merge --delete-branch` against worktree conflicts - #14007

Guard `pr merge --delete-branch` against worktree conflicts#14007
tidy-dev wants to merge 7 commits into
trunkfrom
tidy-dev-merge-worktree-guards

tidy-dev commented Jul 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

babakks left a comment

Uh oh!

babakks Aug 5, 2026

Uh oh!

babakks Aug 5, 2026

Uh oh!

babakks Aug 5, 2026

Uh oh!

babakks Aug 5, 2026

Uh oh!

babakks Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		worktrees, _ := m.opts.GitClient.Worktrees(ctx)
		currentWorkdir, _ := m.opts.GitClient.ToplevelDir(ctx)

-	worktrees, _ := m.opts.GitClient.Worktrees(ctx)
-	currentWorkdir, _ := m.opts.GitClient.ToplevelDir(ctx)
+	worktrees, err := m.opts.GitClient.Worktrees(ctx)
+	if err != nil {
+		return err
+	}
+	currentWorkdir, err := m.opts.GitClient.ToplevelDir(ctx)
+	if err != nil {
+		return err
+	}

Uh oh!

Conversation

tidy-dev commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The two questions

Demo

Design decisions

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

babakks left a comment

Choose a reason for hiding this comment

Suggestion

Uh oh!

babakks Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

babakks Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

babakks Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

babakks Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

babakks Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tidy-dev commented Jul 29, 2026 •

edited

Loading