From 078445db634cfaa6c2c059595783a289218c346f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 1 Dec 2024 10:28:29 +0100 Subject: [PATCH] Allow deleting a merge commit For non-merge commits we change "pick" to "drop" when we delete them. We do this so that we can use the same code for dropping a commit no matter whether we are in an interactive rebase or not. (If we aren't, we could just as well delete the pick line from the todo list instead of setting it to "drop", but if we are, it is better to keep the line around so that the user can change it back to "pick" if they change their mind.) However, merge commits can't be changed to "drop", so we have to delete them from the todo file. We add a new daemon instruction that does this. We still don't allow deleting a merge commit from within an interactive rebase. The reason 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. --- pkg/app/daemon/daemon.go | 26 +++++++++++ pkg/commands/git_commands/rebase.go | 7 +++ .../controllers/local_commits_controller.go | 12 ++++- pkg/i18n/english.go | 2 + .../interactive_rebase/drop_merge_commit.go | 46 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + pkg/utils/rebase_todo.go | 26 +++++++++++ 7 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 pkg/integration/tests/interactive_rebase/drop_merge_commit.go diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index 0d03b61f5..ce1cc2ae0 100644 --- a/pkg/app/daemon/daemon.go +++ b/pkg/app/daemon/daemon.go @@ -38,6 +38,7 @@ const ( DaemonKindMoveTodosDown DaemonKindInsertBreak DaemonKindChangeTodoActions + DaemonKindDropMergeCommit DaemonKindMoveFixupCommitDown DaemonKindWriteRebaseTodo ) @@ -57,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], @@ -242,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) diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 4e920fb7e..5646b5898 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -564,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 { diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 35533b00c..7a1d147a2 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/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) diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index a822f4215..8c8ce9958 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -350,6 +350,7 @@ type TranslationSet struct { DropCommitTitle string DropCommitPrompt string DropUpdateRefPrompt string + DropMergeCommitPrompt string PullingStatus string PushingStatus string FetchingStatus string @@ -1352,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", diff --git a/pkg/integration/tests/interactive_rebase/drop_merge_commit.go b/pkg/integration/tests/interactive_rebase/drop_merge_commit.go new file mode 100644 index 000000000..fc0c21240 --- /dev/null +++ b/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"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index ce7220873..041f0416f 100644 --- a/pkg/integration/tests/test_list.go +++ b/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, diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index dc06111de..eedb3bab1 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -290,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 +}