From 75d2fb1df213a3e9436dfb1fa810c6e7055a4514 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 30 Nov 2024 15:20:10 +0100 Subject: [PATCH 1/6] Disable moving merge commits Not much of a change in behavior, because moving merge commits was already not possible. However, it failed with a cryptic error message ("Todo fa1afe1 not found in git-rebase-todo"), so disable it properly instead. --- pkg/gui/controllers/local_commits_controller.go | 4 ++++ pkg/i18n/english.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index d5e6265bd..306a7173a 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -1420,6 +1420,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 } diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 675d6fcab..0d2af423f 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -160,6 +160,7 @@ type TranslationSet struct { MoveDownCommit string MoveUpCommit string CannotMoveAnyFurther string + CannotMoveMergeCommit string EditCommit string EditCommitTooltip string AmendCommitTooltip string @@ -1153,6 +1154,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.", From bd0d9ef25911129846012baeb81005ec5f254710 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 30 Nov 2024 15:28:07 +0100 Subject: [PATCH 2/6] Disable fixup/squash for merge commits --- pkg/gui/controllers/local_commits_controller.go | 6 +++++- pkg/i18n/english.go | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 306a7173a..b7744788f 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -1358,11 +1358,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 } diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 0d2af423f..2813a1a5f 100644 --- a/pkg/i18n/english.go +++ b/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 @@ -1135,6 +1136,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?", From d5f2fb6003e9f23dcbc321be04d0b6a1a15ac218 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 30 Nov 2024 15:32:03 +0100 Subject: [PATCH 3/6] Disable dropping merge commits if it's not a single selection --- pkg/gui/controllers/local_commits_controller.go | 4 ++++ pkg/i18n/english.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index b7744788f..35533b00c 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -1448,6 +1448,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 } diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 2813a1a5f..a822f4215 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -322,6 +322,7 @@ type TranslationSet struct { YouDied string RewordNotSupported string ChangingThisActionIsNotAllowed string + DroppingMergeRequiresSingleSelection string CherryPickCopy string CherryPickCopyTooltip string CherryPickCopyRangeTooltip string @@ -1324,6 +1325,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.", From 2823a7cff045efde2ef507387c5ebb6d2b08d014 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 1 Dec 2024 17:29:43 +0100 Subject: [PATCH 4/6] Make equalHash more correct So far it didn't have to handle the case where one hash is empty and the other isn't, but in the next commit we need that, so let's handle that case correctly. There's enough logic in the function now that it's worth covering it with tests. --- pkg/utils/rebase_todo.go | 7 ++++++- pkg/utils/rebase_todo_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index d08bd1bef..993d90005 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -56,7 +56,12 @@ 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) { diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go index fbcc82d20..0c56e3bea 100644 --- a/pkg/utils/rebase_todo_test.go +++ b/pkg/utils/rebase_todo_test.go @@ -2,6 +2,7 @@ package utils import ( "errors" + "fmt" "testing" "github.com/stefanhaller/git-todo-parser/todo" @@ -453,3 +454,26 @@ func TestRebaseCommands_deleteTodos(t *testing.T) { }) } } + +func Test_equalHash(t *testing.T) { + scenarios := []struct { + a string + b string + expected bool + }{ + {"", "", true}, + {"", "123", false}, + {"123", "", false}, + {"123", "123", true}, + {"123", "123abc", true}, + {"123abc", "123", true}, + {"123", "a", false}, + {"1", "abc", false}, + } + + for _, scenario := range scenarios { + t.Run(fmt.Sprintf("'%s' vs. '%s'", scenario.a, scenario.b), func(t *testing.T) { + assert.Equal(t, scenario.expected, equalHash(scenario.a, scenario.b)) + }) + } +} From 64eb3d560b9f3d1cf2791c04d27920268aff3729 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 30 Nov 2024 19:53:42 +0100 Subject: [PATCH 5/6] Simplify finding rebase todos One of the comments we are deleting here said: // 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). I don't remember what I was thinking when I wrote this code, but it's nonsense of course. Maybe I was thinking that the hash that appears in a "merge" todo would be the hash of the commit that is being merged in (which would then actually appear in an earlier pick), but it isn't, it's the hash of the merge commit itself (so that the rebase can reuse its commit message). Which means that hashes are unique, no need to compare the action. --- pkg/app/daemon/daemon.go | 8 ++----- pkg/commands/git_commands/rebase.go | 5 ++--- pkg/utils/rebase_todo.go | 21 +++++------------- pkg/utils/rebase_todo_test.go | 34 ++++++++++++++--------------- 4 files changed, 26 insertions(+), 42 deletions(-) diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index 575ceab88..0d03b61f5 100644 --- a/pkg/app/daemon/daemon.go +++ b/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. @@ -235,7 +234,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, } }) @@ -296,8 +294,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 +324,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, } }) diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 3d1d36635..4e920fb7e 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/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, } }) diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index 993d90005..dc06111de 100644 --- a/pkg/utils/rebase_todo.go +++ b/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 } @@ -66,13 +60,8 @@ func equalHash(a, b string) bool { 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 } diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go index 0c56e3bea..4896c8a1d 100644 --- a/pkg/utils/rebase_todo_test.go +++ b/pkg/utils/rebase_todo_test.go @@ -26,7 +26,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveDown: Todo{Hash: "5678", Action: todo.Pick}, + todoToMoveDown: Todo{Hash: "5678"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "5678"}, @@ -41,7 +41,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveDown: Todo{Hash: "abcd", Action: todo.Pick}, + todoToMoveDown: Todo{Hash: "abcd"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -56,7 +56,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, }, - todoToMoveDown: Todo{Ref: "refs/heads/some_branch", Action: todo.UpdateRef}, + todoToMoveDown: Todo{Ref: "refs/heads/some_branch"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -73,7 +73,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "def0"}, }, - todoToMoveDown: Todo{Hash: "5678", Action: todo.Pick}, + todoToMoveDown: Todo{Hash: "5678"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -92,7 +92,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveDown: Todo{Hash: "def0", Action: todo.Pick}, + todoToMoveDown: Todo{Hash: "def0"}, expectedErr: "Todo def0 not found in git-rebase-todo", expectedTodos: []todo.Todo{}, }, @@ -103,7 +103,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveDown: Todo{Hash: "1234", Action: todo.Pick}, + todoToMoveDown: Todo{Hash: "1234"}, expectedErr: "Destination position for moving todo is out of range", expectedTodos: []todo.Todo{}, }, @@ -115,7 +115,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "1234"}, {Command: todo.Pick, Commit: "5678"}, }, - todoToMoveDown: Todo{Hash: "1234", Action: todo.Pick}, + todoToMoveDown: Todo{Hash: "1234"}, expectedErr: "Destination position for moving todo is out of range", expectedTodos: []todo.Todo{}, }, @@ -152,7 +152,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveUp: Todo{Hash: "5678", Action: todo.Pick}, + todoToMoveUp: Todo{Hash: "5678"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -167,7 +167,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveUp: Todo{Hash: "1234", Action: todo.Pick}, + todoToMoveUp: Todo{Hash: "1234"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "5678"}, @@ -182,7 +182,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, {Command: todo.Pick, Commit: "5678"}, }, - todoToMoveUp: Todo{Ref: "refs/heads/some_branch", Action: todo.UpdateRef}, + todoToMoveUp: Todo{Ref: "refs/heads/some_branch"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -199,7 +199,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "def0"}, }, - todoToMoveUp: Todo{Hash: "abcd", Action: todo.Pick}, + todoToMoveUp: Todo{Hash: "abcd"}, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -218,7 +218,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveUp: Todo{Hash: "def0", Action: todo.Pick}, + todoToMoveUp: Todo{Hash: "def0"}, expectedErr: "Todo def0 not found in git-rebase-todo", expectedTodos: []todo.Todo{}, }, @@ -229,7 +229,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, {Command: todo.Pick, Commit: "abcd"}, }, - todoToMoveUp: Todo{Hash: "abcd", Action: todo.Pick}, + todoToMoveUp: Todo{Hash: "abcd"}, expectedErr: "Destination position for moving todo is out of range", expectedTodos: []todo.Todo{}, }, @@ -241,7 +241,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Label, Label: "myLabel"}, {Command: todo.Reset, Label: "otherlabel"}, }, - todoToMoveUp: Todo{Hash: "5678", Action: todo.Pick}, + todoToMoveUp: Todo{Hash: "5678"}, expectedErr: "Destination position for moving todo is out of range", expectedTodos: []todo.Todo{}, }, @@ -417,8 +417,8 @@ func TestRebaseCommands_deleteTodos(t *testing.T) { {Command: todo.Pick, Commit: "abcd"}, }, todosToDelete: []Todo{ - {Ref: "refs/heads/some_branch", Action: todo.UpdateRef}, - {Hash: "abcd", Action: todo.Pick}, + {Ref: "refs/heads/some_branch"}, + {Hash: "abcd"}, }, expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -433,7 +433,7 @@ func TestRebaseCommands_deleteTodos(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, }, todosToDelete: []Todo{ - {Hash: "abcd", Action: todo.Pick}, + {Hash: "abcd"}, }, expectedTodos: []todo.Todo{}, expectedErr: errors.New("Todo abcd not found in git-rebase-todo"), From 078445db634cfaa6c2c059595783a289218c346f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 1 Dec 2024 10:28:29 +0100 Subject: [PATCH 6/6] 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 +}