Better local branch delete confirmation

Currently we try to delete a branch normally, and if git returns an error and
its output contains the text "branch -D", then we prompt the user to force
delete, and try again using -D. Besides just being ugly, this has the
disadvantage that git's logic to decide whether a branch is merged is not very
good; it only considers a branch merged if it is either reachable from the
current head, or from its own upstream. In many cases I want to delete a branch
that has been merged to master, but I don't have master checked out, so the
current branch is really irrelevant, and it should rather (or in addition) check
whether the branch is reachable from one of the main branches. The problem is
that git doesn't know what those are.

But lazygit does, so make the check on our side, prompt the user if necessary,
and always use -D. This is both cleaner, and works better.

See this mailing list discussion for more:
https://lore.kernel.org/git/bf6308ce-3914-4b85-a04b-4a9716bac538@haller-berlin.de/

pkg/commands/git_commands/branch.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 	"github.com/jesseduffield/lazygit/pkg/utils"
 	"github.com/mgutz/str"
@@ -260,3 +261,26 @@ func (self *BranchCommands) AllBranchesLogCmdObj() oscommands.ICmdObj {
 
 	return self.cmd.New(str.ToArgv(candidates[i])).DontLog()
 }
+
+func (self *BranchCommands) IsBranchMerged(branch *models.Branch, mainBranches *MainBranches) (bool, error) {
+	branchesToCheckAgainst := []string{"HEAD"}
+	if branch.RemoteBranchStoredLocally() {
+		branchesToCheckAgainst = append(branchesToCheckAgainst, fmt.Sprintf("%s@{upstream}", branch.Name))
+	}
+	branchesToCheckAgainst = append(branchesToCheckAgainst, mainBranches.Get()...)
+
+	cmdArgs := NewGitCmd("rev-list").
+		Arg("--max-count=1").
+		Arg(branch.Name).
+		Arg(lo.Map(branchesToCheckAgainst, func(branch string, _ int) string {
+			return fmt.Sprintf("^%s", branch)
+		})...).
+		ToArgv()
+
+	stdout, _, err := self.cmd.New(cmdArgs).RunWithOutputs()
+	if err != nil {
+		return false, err
+	}
+
+	return stdout == "", nil
+}

pkg/gui/controllers.go

@@ -107,7 +107,7 @@ func (gui *Gui) resetHelpersAndControllers() {
 		Files:           helpers.NewFilesHelper(helperCommon),
 		WorkingTree:     helpers.NewWorkingTreeHelper(helperCommon, refsHelper, commitsHelper, gpgHelper),
 		Tags:            helpers.NewTagsHelper(helperCommon, commitsHelper),
-		BranchesHelper:  helpers.NewBranchesHelper(helperCommon),
+		BranchesHelper:  helpers.NewBranchesHelper(helperCommon, worktreeHelper),
 		GPG:             helpers.NewGpgHelper(helperCommon),
 		MergeAndRebase:  rebaseHelper,
 		MergeConflicts:  mergeConflictsHelper,

pkg/gui/controllers/branches_controller.go

@@ -3,7 +3,6 @@ package controllers
 import (
 	"errors"
 	"fmt"
-	"strings"
 
 	"github.com/jesseduffield/gocui"
 	"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
@@ -521,92 +520,14 @@ func (self *BranchesController) createNewBranchWithName(newBranchName string) er
 	return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC, KeepBranchSelectionIndex: true})
 }
 
-func (self *BranchesController) checkedOutByOtherWorktree(branch *models.Branch) bool {
-	return git_commands.CheckedOutByOtherWorktree(branch, self.c.Model().Worktrees)
-}
-
-func (self *BranchesController) promptWorktreeBranchDelete(selectedBranch *models.Branch) error {
-	worktree, ok := self.worktreeForBranch(selectedBranch)
-	if !ok {
-		self.c.Log.Error("promptWorktreeBranchDelete out of sync with list of worktrees")
-		return nil
-	}
-
-	title := utils.ResolvePlaceholderString(self.c.Tr.BranchCheckedOutByWorktree, map[string]string{
-		"worktreeName": worktree.Name,
-		"branchName":   selectedBranch.Name,
-	})
-	return self.c.Menu(types.CreateMenuOptions{
-		Title: title,
-		Items: []*types.MenuItem{
-			{
-				Label: self.c.Tr.SwitchToWorktree,
-				OnPress: func() error {
-					return self.c.Helpers().Worktree.Switch(worktree, context.LOCAL_BRANCHES_CONTEXT_KEY)
-				},
-			},
-			{
-				Label:   self.c.Tr.DetachWorktree,
-				Tooltip: self.c.Tr.DetachWorktreeTooltip,
-				OnPress: func() error {
-					return self.c.Helpers().Worktree.Detach(worktree)
-				},
-			},
-			{
-				Label: self.c.Tr.RemoveWorktree,
-				OnPress: func() error {
-					return self.c.Helpers().Worktree.Remove(worktree, false)
-				},
-			},
-		},
-	})
-}
-
 func (self *BranchesController) localDelete(branch *models.Branch) error {
-	if self.checkedOutByOtherWorktree(branch) {
+	return self.c.Helpers().BranchesHelper.ConfirmLocalDelete(branch)
-		return self.promptWorktreeBranchDelete(branch)
-	}
-
-	return self.c.WithWaitingStatus(self.c.Tr.DeletingStatus, func(_ gocui.Task) error {
-		self.c.LogAction(self.c.Tr.Actions.DeleteLocalBranch)
-		err := self.c.Git().Branch.LocalDelete(branch.Name, false)
-		if err != nil && strings.Contains(err.Error(), "git branch -D ") {
-			return self.forceDelete(branch)
-		}
-		if err != nil {
-			return err
-		}
-		return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC, Scope: []types.RefreshableView{types.BRANCHES}})
-	})
 }
 
 func (self *BranchesController) remoteDelete(branch *models.Branch) error {
 	return self.c.Helpers().BranchesHelper.ConfirmDeleteRemote(branch.UpstreamRemote, branch.UpstreamBranch)
 }
 
-func (self *BranchesController) forceDelete(branch *models.Branch) error {
-	title := self.c.Tr.ForceDeleteBranchTitle
-	message := utils.ResolvePlaceholderString(
-		self.c.Tr.ForceDeleteBranchMessage,
-		map[string]string{
-			"selectedBranchName": branch.Name,
-		},
-	)
-
-	self.c.Confirm(types.ConfirmOpts{
-		Title:  title,
-		Prompt: message,
-		HandleConfirm: func() error {
-			if err := self.c.Git().Branch.LocalDelete(branch.Name, true); err != nil {
-				return err
-			}
-			return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC, Scope: []types.RefreshableView{types.BRANCHES}})
-		},
-	})
-
-	return nil
-}
-
 func (self *BranchesController) delete(branch *models.Branch) error {
 	checkedOutBranch := self.c.Helpers().Refs.GetCheckedOutRef()
 

pkg/gui/controllers/helpers/branches_helper.go

@@ -4,20 +4,68 @@ import (
 	"strings"
 
 	"github.com/jesseduffield/gocui"
+	"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
+	"github.com/jesseduffield/lazygit/pkg/gui/context"
 	"github.com/jesseduffield/lazygit/pkg/gui/types"
 	"github.com/jesseduffield/lazygit/pkg/utils"
 )
 
 type BranchesHelper struct {
-	c *HelperCommon
+	c              *HelperCommon
+	worktreeHelper *WorktreeHelper
 }
 
-func NewBranchesHelper(c *HelperCommon) *BranchesHelper {
+func NewBranchesHelper(c *HelperCommon, worktreeHelper *WorktreeHelper) *BranchesHelper {
 	return &BranchesHelper{
-		c: c,
+		c:              c,
+		worktreeHelper: worktreeHelper,
 	}
 }
 
+func (self *BranchesHelper) ConfirmLocalDelete(branch *models.Branch) error {
+	if self.checkedOutByOtherWorktree(branch) {
+		return self.promptWorktreeBranchDelete(branch)
+	}
+
+	isMerged, err := self.c.Git().Branch.IsBranchMerged(branch, self.c.Model().MainBranches)
+	if err != nil {
+		return err
+	}
+
+	doDelete := func() error {
+		return self.c.WithWaitingStatus(self.c.Tr.DeletingStatus, func(_ gocui.Task) error {
+			self.c.LogAction(self.c.Tr.Actions.DeleteLocalBranch)
+			if err := self.c.Git().Branch.LocalDelete(branch.Name, true); err != nil {
+				return err
+			}
+			return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC, Scope: []types.RefreshableView{types.BRANCHES}})
+		})
+	}
+
+	if isMerged {
+		return doDelete()
+	}
+
+	title := self.c.Tr.ForceDeleteBranchTitle
+	message := utils.ResolvePlaceholderString(
+		self.c.Tr.ForceDeleteBranchMessage,
+		map[string]string{
+			"selectedBranchName": branch.Name,
+		},
+	)
+
+	self.c.Confirm(types.ConfirmOpts{
+		Title:  title,
+		Prompt: message,
+		HandleConfirm: func() error {
+			return doDelete()
+		},
+	})
+
+	return nil
+}
+
 func (self *BranchesHelper) ConfirmDeleteRemote(remoteName string, branchName string) error {
 	title := utils.ResolvePlaceholderString(
 		self.c.Tr.DeleteBranchTitle,
@@ -52,3 +100,48 @@ func (self *BranchesHelper) ConfirmDeleteRemote(remoteName string, branchName st
 func ShortBranchName(fullBranchName string) string {
 	return strings.TrimPrefix(strings.TrimPrefix(fullBranchName, "refs/heads/"), "refs/remotes/")
 }
+
+func (self *BranchesHelper) checkedOutByOtherWorktree(branch *models.Branch) bool {
+	return git_commands.CheckedOutByOtherWorktree(branch, self.c.Model().Worktrees)
+}
+
+func (self *BranchesHelper) worktreeForBranch(branch *models.Branch) (*models.Worktree, bool) {
+	return git_commands.WorktreeForBranch(branch, self.c.Model().Worktrees)
+}
+
+func (self *BranchesHelper) promptWorktreeBranchDelete(selectedBranch *models.Branch) error {
+	worktree, ok := self.worktreeForBranch(selectedBranch)
+	if !ok {
+		self.c.Log.Error("promptWorktreeBranchDelete out of sync with list of worktrees")
+		return nil
+	}
+
+	title := utils.ResolvePlaceholderString(self.c.Tr.BranchCheckedOutByWorktree, map[string]string{
+		"worktreeName": worktree.Name,
+		"branchName":   selectedBranch.Name,
+	})
+	return self.c.Menu(types.CreateMenuOptions{
+		Title: title,
+		Items: []*types.MenuItem{
+			{
+				Label: self.c.Tr.SwitchToWorktree,
+				OnPress: func() error {
+					return self.worktreeHelper.Switch(worktree, context.LOCAL_BRANCHES_CONTEXT_KEY)
+				},
+			},
+			{
+				Label:   self.c.Tr.DetachWorktree,
+				Tooltip: self.c.Tr.DetachWorktreeTooltip,
+				OnPress: func() error {
+					return self.worktreeHelper.Detach(worktree)
+				},
+			},
+			{
+				Label: self.c.Tr.RemoveWorktree,
+				OnPress: func() error {
+					return self.worktreeHelper.Remove(worktree, false)
+				},
+			},
+		},
+	})
+}

pkg/integration/tests/branch/delete.go

@@ -15,27 +15,43 @@ var Delete = NewIntegrationTest(NewIntegrationTestArgs{
 			CloneIntoRemote("origin").
 			EmptyCommit("blah").
 			NewBranch("branch-one").
+			EmptyCommit("on branch-one 01").
 			PushBranchAndSetUpstream("origin", "branch-one").
+			EmptyCommit("on branch-one 02").
+			Checkout("master").
+			Merge("branch-one"). // branch-one is contained in master, so no delete confirmation
 			NewBranch("branch-two").
-			PushBranchAndSetUpstream("origin", "branch-two").
+			EmptyCommit("on branch-two 01").
-			EmptyCommit("deletion blocker").
+			PushBranchAndSetUpstream("origin", "branch-two"). // branch-two is contained in its own upstream, so no delete confirmation either
-			NewBranch("branch-three")
+			NewBranchFrom("branch-three", "master").
+			EmptyCommit("on branch-three 01").
+			NewBranch("current-head"). // branch-three is contained in the current head, so no delete confirmation
+			EmptyCommit("on current-head").
+			NewBranchFrom("branch-four", "master").
+			EmptyCommit("on branch-four 01").
+			PushBranchAndSetUpstream("origin", "branch-four").
+			EmptyCommit("on branch-four 02"). // branch-four is not contained in any of these, so we get a delete confirmation
+			Checkout("current-head")
 	},
 	Run: func(t *TestDriver, keys config.KeybindingConfig) {
 		t.Views().Branches().
 			Focus().
 			Lines(
-				MatchesRegexp(`\*.*branch-three`).IsSelected(),
+				Contains("current-head").IsSelected(),
-				MatchesRegexp(`branch-two`),
+				Contains("branch-four ↑1"),
-				MatchesRegexp(`branch-one`),
+				Contains("branch-three"),
-				MatchesRegexp(`master`),
+				Contains("branch-two ✓"),
+				Contains("master"),
+				Contains("branch-one ↑1"),
 			).
+
+			// Deleting the current branch is not possible
 			Press(keys.Universal.Remove).
 			Tap(func() {
 				t.ExpectPopup().
 					Menu().
 					Tooltip(Contains("You cannot delete the checked out branch!")).
-					Title(Equals("Delete branch 'branch-three'?")).
+					Title(Equals("Delete branch 'current-head'?")).
 					Select(Contains("Delete local branch")).
 					Confirm().
 					Tap(func() {
@@ -43,8 +59,51 @@ var Delete = NewIntegrationTest(NewIntegrationTestArgs{
 					}).
 					Cancel()
 			}).
+
+			// Delete branch-four. This is the only branch that is not fully merged, so we get
+			// a confirmation popup.
 			SelectNextItem().
 			Press(keys.Universal.Remove).
+			Tap(func() {
+				t.ExpectPopup().
+					Menu().
+					Title(Equals("Delete branch 'branch-four'?")).
+					Select(Contains("Delete local branch")).
+					Confirm()
+				t.ExpectPopup().
+					Confirmation().
+					Title(Equals("Force delete branch")).
+					Content(Equals("'branch-four' is not fully merged. Are you sure you want to delete it?")).
+					Confirm()
+			}).
+			Lines(
+				Contains("current-head"),
+				Contains("branch-three").IsSelected(),
+				Contains("branch-two ✓"),
+				Contains("master"),
+				Contains("branch-one ↑1"),
+			).
+
+			// Delete branch-three. This branch is contained in the current head, so this just works
+			// without any confirmation.
+			Press(keys.Universal.Remove).
+			Tap(func() {
+				t.ExpectPopup().
+					Menu().
+					Title(Equals("Delete branch 'branch-three'?")).
+					Select(Contains("Delete local branch")).
+					Confirm()
+			}).
+			Lines(
+				Contains("current-head"),
+				Contains("branch-two ✓").IsSelected(),
+				Contains("master"),
+				Contains("branch-one ↑1"),
+			).
+
+			// Delete branch-two. This branch is contained in its own upstream, so this just works
+			// without any confirmation.
+			Press(keys.Universal.Remove).
 			Tap(func() {
 				t.ExpectPopup().
 					Menu().
@@ -52,18 +111,14 @@ var Delete = NewIntegrationTest(NewIntegrationTestArgs{
 					Select(Contains("Delete local branch")).
 					Confirm()
 			}).
-			Tap(func() {
-				t.ExpectPopup().
-					Confirmation().
-					Title(Equals("Force delete branch")).
-					Content(Equals("'branch-two' is not fully merged. Are you sure you want to delete it?")).
-					Confirm()
-			}).
 			Lines(
-				MatchesRegexp(`\*.*branch-three`),
+				Contains("current-head"),
-				MatchesRegexp(`branch-one`).IsSelected(),
+				Contains("master").IsSelected(),
-				MatchesRegexp(`master`),
+				Contains("branch-one ↑1"),
 			).
+
+			// Delete remote branch of branch-one. We only get the normal remote branch confirmation for this one.
+			SelectNextItem().
 			Press(keys.Universal.Remove).
 			Tap(func() {
 				t.ExpectPopup().
@@ -87,7 +142,10 @@ var Delete = NewIntegrationTest(NewIntegrationTestArgs{
 
 				t.Views().
 					RemoteBranches().
-					Lines(Equals("branch-two")).
+					Lines(
+						Equals("branch-four"),
+						Equals("branch-two"),
+					).
 					Press(keys.Universal.Return)
 
 				t.Views().
@@ -95,10 +153,13 @@ var Delete = NewIntegrationTest(NewIntegrationTestArgs{
 					Focus()
 			}).
 			Lines(
-				MatchesRegexp(`\*.*branch-three`),
+				Contains("current-head"),
-				MatchesRegexp(`branch-one \(upstream gone\)`).IsSelected(),
+				Contains("master"),
-				MatchesRegexp(`master`),
+				Contains("branch-one (upstream gone)").IsSelected(),
 			).
+
+			// Delete local branch of branch-one. Even though its upstream is gone, we don't get a confirmation
+			// because it is contained in master.
 			Press(keys.Universal.Remove).
 			Tap(func() {
 				t.ExpectPopup().
@@ -108,8 +169,8 @@ var Delete = NewIntegrationTest(NewIntegrationTestArgs{
 					Confirm()
 			}).
 			Lines(
-				MatchesRegexp(`\*.*branch-three`),
+				Contains("current-head"),
-				MatchesRegexp(`master`).IsSelected(),
+				Contains("master").IsSelected(),
 			)
 	},
 })