From 64eb3d560b9f3d1cf2791c04d27920268aff3729 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 30 Nov 2024 19:53:42 +0100 Subject: [PATCH] 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"),