Drop merge commits (#4094)

- **PR Description**

Allow deleting a merge commit. We only allow this when the merge commit
is the only selected item, and only outside of a rebase. The reason for
this is that we don't show the "label" and "reset" todos in lazygit, so
deleting a merge commit would leave the commits from the branch that is
being merged in the list as "pick" commits, with no indication that they
are going to be dropped because they are on a different branch, and the
merge commit that would have brought them in is gone. This could be very
confusing.

Fixes #3164.

- **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

Author: stefanhaller

pkg/app/daemon/daemon.go

@@ -12,7 +12,6 @@ import (
 	"github.com/jesseduffield/lazygit/pkg/common"
 	"github.com/jesseduffield/lazygit/pkg/utils"
 	"github.com/samber/lo"
-	"github.com/stefanhaller/git-todo-parser/todo"
 )
 
 // Sometimes lazygit will be invoked in daemon mode from a parent lazygit process.
@@ -39,6 +38,7 @@ const (
 	DaemonKindMoveTodosDown
 	DaemonKindInsertBreak
 	DaemonKindChangeTodoActions
+	DaemonKindDropMergeCommit
 	DaemonKindMoveFixupCommitDown
 	DaemonKindWriteRebaseTodo
 )
@@ -58,6 +58,7 @@ func getInstruction() Instruction {
 		DaemonKindRemoveUpdateRefsForCopiedBranch: deserializeInstruction[*RemoveUpdateRefsForCopiedBranchInstruction],
 		DaemonKindCherryPick:                      deserializeInstruction[*CherryPickCommitsInstruction],
 		DaemonKindChangeTodoActions:               deserializeInstruction[*ChangeTodoActionsInstruction],
+		DaemonKindDropMergeCommit:                 deserializeInstruction[*DropMergeCommitInstruction],
 		DaemonKindMoveFixupCommitDown:             deserializeInstruction[*MoveFixupCommitDownInstruction],
 		DaemonKindMoveTodosUp:                     deserializeInstruction[*MoveTodosUpInstruction],
 		DaemonKindMoveTodosDown:                   deserializeInstruction[*MoveTodosDownInstruction],
@@ -235,7 +236,6 @@ func (self *ChangeTodoActionsInstruction) run(common *common.Common) error {
 		changes := lo.Map(self.Changes, func(c ChangeTodoAction, _ int) utils.TodoChange {
 			return utils.TodoChange{
 				Hash:      c.Hash,
-				OldAction: todo.Pick,
 				NewAction: c.NewAction,
 			}
 		})
@@ -244,6 +244,30 @@ func (self *ChangeTodoActionsInstruction) run(common *common.Common) error {
 	})
 }
 
+type DropMergeCommitInstruction struct {
+	Hash string
+}
+
+func NewDropMergeCommitInstruction(hash string) Instruction {
+	return &DropMergeCommitInstruction{
+		Hash: hash,
+	}
+}
+
+func (self *DropMergeCommitInstruction) Kind() DaemonKind {
+	return DaemonKindDropMergeCommit
+}
+
+func (self *DropMergeCommitInstruction) SerializedInstructions() string {
+	return serializeInstruction(self)
+}
+
+func (self *DropMergeCommitInstruction) run(common *common.Common) error {
+	return handleInteractiveRebase(common, func(path string) error {
+		return utils.DropMergeCommit(path, self.Hash, getCommentChar())
+	})
+}
+
 // Takes the hash of some commit, and the hash of a fixup commit that was created
 // at the end of the branch, then moves the fixup commit down to right after the
 // original commit, changing its type to "fixup" (only if ChangeToFixup is true)
@@ -296,8 +320,7 @@ func (self *MoveTodosUpInstruction) SerializedInstructions() string {
 func (self *MoveTodosUpInstruction) run(common *common.Common) error {
 	todosToMove := lo.Map(self.Hashes, func(hash string, _ int) utils.Todo {
 		return utils.Todo{
-			Hash:   hash,
-			Action: todo.Pick,
+			Hash: hash,
 		}
 	})
 
@@ -327,8 +350,7 @@ func (self *MoveTodosDownInstruction) SerializedInstructions() string {
 func (self *MoveTodosDownInstruction) run(common *common.Common) error {
 	todosToMove := lo.Map(self.Hashes, func(hash string, _ int) utils.Todo {
 		return utils.Todo{
-			Hash:   hash,
-			Action: todo.Pick,
+			Hash: hash,
 		}
 	})
 

pkg/commands/git_commands/rebase.go

@@ -324,9 +324,9 @@ func (self *RebaseCommands) MoveFixupCommitDown(commits []*models.Commit, target
 
 func todoFromCommit(commit *models.Commit) utils.Todo {
 	if commit.Action == todo.UpdateRef {
-		return utils.Todo{Ref: commit.Name, Action: commit.Action}
+		return utils.Todo{Ref: commit.Name}
 	} else {
-		return utils.Todo{Hash: commit.Hash, Action: commit.Action}
+		return utils.Todo{Hash: commit.Hash}
 	}
 }
 
@@ -335,7 +335,6 @@ func (self *RebaseCommands) EditRebaseTodo(commits []*models.Commit, action todo
 	commitsWithAction := lo.Map(commits, func(commit *models.Commit, _ int) utils.TodoChange {
 		return utils.TodoChange{
 			Hash:      commit.Hash,
-			OldAction: commit.Action,
 			NewAction: action,
 		}
 	})
@@ -565,6 +564,13 @@ func (self *RebaseCommands) CherryPickCommitsDuringRebase(commits []*models.Comm
 	return utils.PrependStrToTodoFile(filePath, []byte(todo))
 }
 
+func (self *RebaseCommands) DropMergeCommit(commits []*models.Commit, commitIndex int) error {
+	return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
+		baseHashOrRoot: getBaseHashOrRoot(commits, commitIndex+1),
+		instruction:    daemon.NewDropMergeCommitInstruction(commits[commitIndex].Hash),
+	}).Run()
+}
+
 // we can't start an interactive rebase from the first commit without passing the
 // '--root' arg
 func getBaseHashOrRoot(commits []*models.Commit, index int) string {

pkg/gui/controllers/local_commits_controller.go

@@ -497,12 +497,17 @@ func (self *LocalCommitsController) drop(selectedCommits []*models.Commit, start
 		return self.updateTodos(todo.Drop, selectedCommits)
 	}
 
+	isMerge := selectedCommits[0].IsMerge()
+
 	self.c.Confirm(types.ConfirmOpts{
 		Title:  self.c.Tr.DropCommitTitle,
-		Prompt: self.c.Tr.DropCommitPrompt,
+		Prompt: lo.Ternary(isMerge, self.c.Tr.DropMergeCommitPrompt, self.c.Tr.DropCommitPrompt),
 		HandleConfirm: func() error {
 			return self.c.WithWaitingStatus(self.c.Tr.DroppingStatus, func(gocui.Task) error {
 				self.c.LogAction(self.c.Tr.Actions.DropCommit)
+				if isMerge {
+					return self.dropMergeCommit(startIdx)
+				}
 				return self.interactiveRebase(todo.Drop, startIdx, endIdx)
 			})
 		},
@@ -511,6 +516,11 @@ func (self *LocalCommitsController) drop(selectedCommits []*models.Commit, start
 	return nil
 }
 
+func (self *LocalCommitsController) dropMergeCommit(commitIdx int) error {
+	err := self.c.Git().Rebase.DropMergeCommit(self.c.Model().Commits, commitIdx)
+	return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err)
+}
+
 func (self *LocalCommitsController) edit(selectedCommits []*models.Commit, startIdx int, endIdx int) error {
 	if self.isRebasing() {
 		return self.updateTodos(todo.Edit, selectedCommits)
@@ -1358,11 +1368,15 @@ func (self *LocalCommitsController) canFindCommitForSquashFixupsInCurrentBranch(
 	return nil
 }
 
-func (self *LocalCommitsController) canSquashOrFixup(_selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
+func (self *LocalCommitsController) canSquashOrFixup(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
 	if endIdx >= len(self.c.Model().Commits)-1 {
 		return &types.DisabledReason{Text: self.c.Tr.CannotSquashOrFixupFirstCommit}
 	}
 
+	if lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) {
+		return &types.DisabledReason{Text: self.c.Tr.CannotSquashOrFixupMergeCommit}
+	}
+
 	return nil
 }
 
@@ -1420,6 +1434,10 @@ func (self *LocalCommitsController) midRebaseCommandEnabled(selectedCommits []*m
 // Ensures that if we are mid-rebase, we're only selecting commits that can be moved
 func (self *LocalCommitsController) midRebaseMoveCommandEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
 	if !self.isRebasing() {
+		if lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) {
+			return &types.DisabledReason{Text: self.c.Tr.CannotMoveMergeCommit}
+		}
+
 		return nil
 	}
 
@@ -1440,6 +1458,10 @@ func (self *LocalCommitsController) midRebaseMoveCommandEnabled(selectedCommits
 
 func (self *LocalCommitsController) canDropCommits(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
 	if !self.isRebasing() {
+		if len(selectedCommits) > 1 && lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) {
+			return &types.DisabledReason{Text: self.c.Tr.DroppingMergeRequiresSingleSelection}
+		}
+
 		return nil
 	}
 

pkg/i18n/english.go

@@ -140,6 +140,7 @@ type TranslationSet struct {
 	Quit                                  string
 	SquashTooltip                         string
 	CannotSquashOrFixupFirstCommit        string
+	CannotSquashOrFixupMergeCommit        string
 	Fixup                                 string
 	FixupTooltip                          string
 	SureFixupThisCommit                   string
@@ -160,6 +161,7 @@ type TranslationSet struct {
 	MoveDownCommit                        string
 	MoveUpCommit                          string
 	CannotMoveAnyFurther                  string
+	CannotMoveMergeCommit                 string
 	EditCommit                            string
 	EditCommitTooltip                     string
 	AmendCommitTooltip                    string
@@ -320,6 +322,7 @@ type TranslationSet struct {
 	YouDied                               string
 	RewordNotSupported                    string
 	ChangingThisActionIsNotAllowed        string
+	DroppingMergeRequiresSingleSelection  string
 	CherryPickCopy                        string
 	CherryPickCopyTooltip                 string
 	CherryPickCopyRangeTooltip            string
@@ -347,6 +350,7 @@ type TranslationSet struct {
 	DropCommitTitle                       string
 	DropCommitPrompt                      string
 	DropUpdateRefPrompt                   string
+	DropMergeCommitPrompt                 string
 	PullingStatus                         string
 	PushingStatus                         string
 	FetchingStatus                        string
@@ -1134,6 +1138,7 @@ func EnglishTranslationSet() *TranslationSet {
 		UpdateRefHere:                        "Update branch '{{.ref}}' here",
 		ExecCommandHere:                      "Execute the following command here:",
 		CannotSquashOrFixupFirstCommit:       "There's no commit below to squash into",
+		CannotSquashOrFixupMergeCommit:       "Cannot squash or fixup a merge commit",
 		Fixup:                                "Fixup",
 		SureFixupThisCommit:                  "Are you sure you want to 'fixup' the selected commit(s) into the commit below?",
 		SureSquashThisCommit:                 "Are you sure you want to squash the selected commit(s) into the commit below?",
@@ -1153,6 +1158,7 @@ func EnglishTranslationSet() *TranslationSet {
 		MoveDownCommit:                       "Move commit down one",
 		MoveUpCommit:                         "Move commit up one",
 		CannotMoveAnyFurther:                 "Cannot move any further",
+		CannotMoveMergeCommit:                "Cannot move a merge commit",
 		EditCommit:                           "Edit (start interactive rebase)",
 		EditCommitTooltip:                    "Edit the selected commit. Use this to start an interactive rebase from the selected commit. When already mid-rebase, this will mark the selected commit for editing, which means that upon continuing the rebase, the rebase will pause at the selected commit to allow you to make changes.",
 		AmendCommitTooltip:                   "Amend commit with staged changes. If the selected commit is the HEAD commit, this will perform `git commit --amend`. Otherwise the commit will be amended via a rebase.",
@@ -1320,6 +1326,7 @@ func EnglishTranslationSet() *TranslationSet {
 		YouDied:                              "YOU DIED!",
 		RewordNotSupported:                   "Rewording commits while interactively rebasing is not currently supported",
 		ChangingThisActionIsNotAllowed:       "Changing this kind of rebase todo entry is not allowed",
+		DroppingMergeRequiresSingleSelection: "Dropping a merge commit requires a single selected item",
 		CherryPickCopy:                       "Copy (cherry-pick)",
 		CherryPickCopyTooltip:                "Mark commit as copied. Then, within the local commits view, you can press `{{.paste}}` to paste (cherry-pick) the copied commit(s) into your checked out branch. At any time you can press `{{.escape}}` to cancel the selection.",
 		CherryPickCopyRangeTooltip:           "Mark commits as copied from the last copied commit to the selected commit.",
@@ -1346,6 +1353,7 @@ func EnglishTranslationSet() *TranslationSet {
 		AmendCommitPrompt:                    "Are you sure you want to amend this commit with your staged files?",
 		DropCommitTitle:                      "Drop commit",
 		DropCommitPrompt:                     "Are you sure you want to drop the selected commit(s)?",
+		DropMergeCommitPrompt:                "Are you sure you want to drop the selected merge commit? Note that it will also drop all the commits that were merged in by it.",
 		DropUpdateRefPrompt:                  "Are you sure you want to delete the selected update-ref todo(s)? This is irreversible except by aborting the rebase.",
 		PullingStatus:                        "Pulling",
 		PushingStatus:                        "Pushing",

pkg/integration/tests/interactive_rebase/drop_merge_commit.go

@@ -0,0 +1,46 @@
+package interactive_rebase
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+	"github.com/jesseduffield/lazygit/pkg/integration/tests/shared"
+)
+
+var DropMergeCommit = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Drops a merge commit outside of an interactive rebase",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	GitVersion:   AtLeast("2.22.0"), // first version that supports the --rebase-merges option
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shared.CreateMergeCommit(shell)
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("CI ⏣─╮ Merge branch 'second-change-branch' into first-change-branch").IsSelected(),
+				Contains("CI │ ◯ * second-change-branch unrelated change"),
+				Contains("CI │ ◯ second change"),
+				Contains("CI ◯ │ first change"),
+				Contains("CI ◯─╯ * original"),
+				Contains("CI ◯ three"),
+				Contains("CI ◯ two"),
+				Contains("CI ◯ one"),
+			).
+			Press(keys.Universal.Remove).
+			Tap(func() {
+				t.ExpectPopup().Confirmation().
+					Title(Equals("Drop commit")).
+					Content(Equals("Are you sure you want to drop the selected merge commit? Note that it will also drop all the commits that were merged in by it.")).
+					Confirm()
+			}).
+			Lines(
+				Contains("CI ◯ first change").IsSelected(),
+				Contains("CI ◯ * original"),
+				Contains("CI ◯ three"),
+				Contains("CI ◯ two"),
+				Contains("CI ◯ one"),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -208,6 +208,7 @@ var tests = []*components.IntegrationTest{
 	interactive_rebase.DeleteUpdateRefTodo,
 	interactive_rebase.DontShowBranchHeadsForTodoItems,
 	interactive_rebase.DropCommitInCopiedBranchWithUpdateRef,
+	interactive_rebase.DropMergeCommit,
 	interactive_rebase.DropTodoCommitWithUpdateRef,
 	interactive_rebase.DropWithCustomCommentChar,
 	interactive_rebase.EditAndAutoAmend,

pkg/utils/rebase_todo.go

@@ -6,24 +6,18 @@ import (
 	"fmt"
 	"os"
 	"slices"
-	"strings"
 
 	"github.com/samber/lo"
 	"github.com/stefanhaller/git-todo-parser/todo"
 )
 
 type Todo struct {
-	Hash   string // for todos that have one, e.g. pick, drop, fixup, etc.
-	Ref    string // for update-ref todos
-	Action todo.TodoCommand
+	Hash string // for todos that have one, e.g. pick, drop, fixup, etc.
+	Ref  string // for update-ref todos
 }
 
-// In order to change a TODO in git-rebase-todo, we need to specify the old action,
-// because sometimes the same hash appears multiple times in the file (e.g. in a pick
-// and later in a merge)
 type TodoChange struct {
 	Hash      string
-	OldAction todo.TodoCommand
 	NewAction todo.TodoCommand
 }
 
@@ -40,7 +34,7 @@ func EditRebaseTodo(filePath string, changes []TodoChange, commentChar byte) err
 		t := &todos[i]
 		// This is a nested loop, but it's ok because the number of todos should be small
 		for _, change := range changes {
-			if t.Command == change.OldAction && equalHash(t.Commit, change.Hash) {
+			if equalHash(t.Commit, change.Hash) {
 				matchCount++
 				t.Command = change.NewAction
 			}
@@ -56,18 +50,18 @@ func EditRebaseTodo(filePath string, changes []TodoChange, commentChar byte) err
 }
 
 func equalHash(a, b string) bool {
-	return strings.HasPrefix(a, b) || strings.HasPrefix(b, a)
+	if len(a) == 0 && len(b) == 0 {
+		return true
+	}
+
+	commonLength := min(len(a), len(b))
+	return commonLength > 0 && a[:commonLength] == b[:commonLength]
 }
 
 func findTodo(todos []todo.Todo, todoToFind Todo) (int, bool) {
 	_, idx, ok := lo.FindIndexOf(todos, func(t todo.Todo) bool {
-		// Comparing just the hash is not enough; we need to compare both the
-		// action and the hash, as the hash could appear multiple times (e.g. in a
-		// pick and later in a merge). For update-ref todos we also must compare
-		// the Ref.
-		return t.Command == todoToFind.Action &&
-			equalHash(t.Commit, todoToFind.Hash) &&
-			t.Ref == todoToFind.Ref
+		// For update-ref todos we also must compare the Ref (they have an empty hash)
+		return equalHash(t.Commit, todoToFind.Hash) && t.Ref == todoToFind.Ref
 	})
 	return idx, ok
 }
@@ -296,3 +290,29 @@ func RemoveUpdateRefsForCopiedBranch(fileName string, commentChar byte) error {
 func isRenderedTodo(t todo.Todo) bool {
 	return t.Commit != "" || t.Command == todo.UpdateRef
 }
+
+func DropMergeCommit(fileName string, hash string, commentChar byte) error {
+	todos, err := ReadRebaseTodoFile(fileName, commentChar)
+	if err != nil {
+		return err
+	}
+
+	newTodos, err := dropMergeCommit(todos, hash)
+	if err != nil {
+		return err
+	}
+
+	return WriteRebaseTodoFile(fileName, newTodos, commentChar)
+}
+
+func dropMergeCommit(todos []todo.Todo, hash string) ([]todo.Todo, error) {
+	isMerge := func(t todo.Todo) bool {
+		return t.Command == todo.Merge && t.Flag == "-C" && equalHash(t.Commit, hash)
+	}
+	if lo.CountBy(todos, isMerge) != 1 {
+		return nil, fmt.Errorf("Expected exactly one merge commit with hash %s", hash)
+	}
+
+	_, idx, _ := lo.FindIndexOf(todos, isMerge)
+	return slices.Delete(todos, idx, idx+1), nil
+}