Better branch delete confirmation (#3915)

- **PR Description**

When deleting a local branch, put up the "This branch is not fully
merged, do you want to force delete it" confirmation only when the
branch is not merged into any of the main branches.

- **Please check if the PR fulfills these requirements**

* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [x] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [ ] If a new UserConfig entry was added, make sure it can be
hot-reloaded (see
[here](https://github.com/jesseduffield/lazygit/blob/master/docs/dev/Codebase_Guide.md#using-userconfig))
* [ ] Docs have been updated if necessary
* [x] You've read through your own file changes for silly mistakes etc

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,91 +520,12 @@ 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
-	}
-
-	// TODO: i18n
-	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.Name)
+	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 {

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/i18n/english.go

@@ -472,7 +472,6 @@ type TranslationSet struct {
 	RemoveRemoteTooltip                   string
 	RemoveRemotePrompt                    string
 	DeleteRemoteBranch                    string
-	DeleteRemoteBranchMessage             string
 	DeleteRemoteBranchTooltip             string
 	SetAsUpstream                         string
 	SetAsUpstreamTooltip                  string
@@ -849,7 +848,6 @@ type Actions struct {
 	CheckoutBranch                    string
 	ForceCheckoutBranch               string
 	DeleteLocalBranch                 string
-	DeleteBranch                      string
 	Merge                             string
 	SquashMerge                       string
 	RebaseBranch                      string
@@ -1461,9 +1459,8 @@ func EnglishTranslationSet() *TranslationSet {
 		EditRemoteUrl:                        `Enter updated remote url for {{.remoteName}}:`,
 		RemoveRemote:                         `Remove remote`,
 		RemoveRemoteTooltip:                  `Remove the selected remote. Any local branches tracking a remote branch from the remote will be unaffected.`,
-		RemoveRemotePrompt:                   "Are you sure you want to remove remote",
+		RemoveRemotePrompt:                   "Are you sure you want to remove remote?",
 		DeleteRemoteBranch:                   "Delete remote branch",
-		DeleteRemoteBranchMessage:            "Are you sure you want to delete remote branch",
 		DeleteRemoteBranchTooltip:            "Delete the remote branch from the remote.",
 		SetAsUpstream:                        "Set as upstream",
 		SetAsUpstreamTooltip:                 "Set the selected remote branch as the upstream of the checked-out branch.",
@@ -1480,7 +1477,7 @@ func EnglishTranslationSet() *TranslationSet {
 		ViewUpstreamRebaseOptionsTooltip:     "View options for rebasing the checked-out branch onto {{upstream}}. Note: this will not rebase the selected branch onto the upstream, it will rebase the checked-out branch onto the upstream.",
 		UpstreamGenericName:                  "upstream of selected branch",
 		SetUpstreamTitle:                     "Set upstream branch",
-		SetUpstreamMessage:                   "Are you sure you want to set the upstream branch of '{{.checkedOut}}' to '{{.selected}}'",
+		SetUpstreamMessage:                   "Are you sure you want to set the upstream branch of '{{.checkedOut}}' to '{{.selected}}'?",
 		EditRemoteTooltip:                    "Edit the selected remote's name or URL.",
 		TagCommit:                            "Tag commit",
 		TagCommitTooltip:                     "Create a new tag pointing at the selected commit. You'll be prompted to enter a tag name and optional description.",
@@ -1797,7 +1794,6 @@ func EnglishTranslationSet() *TranslationSet {
 			CheckoutBranch:                 "Checkout branch",
 			ForceCheckoutBranch:            "Force checkout branch",
 			DeleteLocalBranch:              "Delete local branch",
-			DeleteBranch:                   "Delete branch",
 			Merge:                          "Merge",
 			SquashMerge:                    "Squash merge",
 			RebaseBranch:                   "Rebase branch",

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(),
 			)
 	},
 })

pkg/integration/tests/branch/delete_remote_branch_with_different_name.go

@@ -0,0 +1,49 @@
+package branch
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var DeleteRemoteBranchWithDifferentName = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Delete a remote branch that has a different name than the local branch",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig: func(config *config.AppConfig) {
+	},
+	SetupRepo: func(shell *Shell) {
+		shell.EmptyCommit("one")
+		shell.CloneIntoRemote("origin")
+		shell.NewBranch("mybranch-local")
+		shell.PushBranchAndSetUpstream("origin", "mybranch-local:mybranch-remote")
+		shell.Checkout("master")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Branches().
+			Focus().
+			Lines(
+				Contains("master").IsSelected(),
+				Contains("mybranch-local ✓"),
+			).
+			SelectNextItem().
+			Press(keys.Universal.Remove).
+			Tap(func() {
+				t.ExpectPopup().
+					Menu().
+					Title(Equals("Delete branch 'mybranch-local'?")).
+					Select(Contains("Delete remote branch")).
+					Confirm()
+			}).
+			Tap(func() {
+				t.ExpectPopup().
+					Confirmation().
+					Title(Equals("Delete branch 'mybranch-remote'?")).
+					Content(Equals("Are you sure you want to delete the remote branch 'mybranch-remote' from 'origin'?")).
+					Confirm()
+			}).
+			Lines(
+				Contains("master"),
+				Contains("mybranch-local (upstream gone)").IsSelected(),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -43,6 +43,7 @@ var tests = []*components.IntegrationTest{
 	branch.CreateTag,
 	branch.Delete,
 	branch.DeleteRemoteBranchWithCredentialPrompt,
+	branch.DeleteRemoteBranchWithDifferentName,
 	branch.DetachedHead,
 	branch.NewBranchAutostash,
 	branch.NewBranchFromRemoteTrackingDifferentName,