From a9ef69b9c7ae1d704630832cda4c6d7e66644275 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 2 Dec 2024 11:35:35 +0100 Subject: [PATCH] Fix moving a commit across a branch boundary in a stack See the previous commit for a detailed explanation. --- pkg/app/daemon/daemon.go | 4 +- pkg/commands/git_commands/rebase.go | 4 +- ...e_across_branch_boundary_outside_rebase.go | 7 --- pkg/utils/rebase_todo.go | 28 +++++------ pkg/utils/rebase_todo_test.go | 46 +++++++++++++++++-- 5 files changed, 60 insertions(+), 29 deletions(-) diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index ce1cc2ae0..782df5f1f 100644 --- a/pkg/app/daemon/daemon.go +++ b/pkg/app/daemon/daemon.go @@ -325,7 +325,7 @@ func (self *MoveTodosUpInstruction) run(common *common.Common) error { }) return handleInteractiveRebase(common, func(path string) error { - return utils.MoveTodosUp(path, todosToMove, getCommentChar()) + return utils.MoveTodosUp(path, todosToMove, false, getCommentChar()) }) } @@ -355,7 +355,7 @@ func (self *MoveTodosDownInstruction) run(common *common.Common) error { }) return handleInteractiveRebase(common, func(path string) error { - return utils.MoveTodosDown(path, todosToMove, getCommentChar()) + return utils.MoveTodosDown(path, todosToMove, false, getCommentChar()) }) } diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 5646b5898..757257750 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -369,7 +369,7 @@ func (self *RebaseCommands) MoveTodosDown(commits []*models.Commit) error { return todoFromCommit(commit) }) - return utils.MoveTodosDown(fileName, todosToMove, self.config.GetCoreCommentChar()) + return utils.MoveTodosDown(fileName, todosToMove, true, self.config.GetCoreCommentChar()) } func (self *RebaseCommands) MoveTodosUp(commits []*models.Commit) error { @@ -378,7 +378,7 @@ func (self *RebaseCommands) MoveTodosUp(commits []*models.Commit) error { return todoFromCommit(commit) }) - return utils.MoveTodosUp(fileName, todosToMove, self.config.GetCoreCommentChar()) + return utils.MoveTodosUp(fileName, todosToMove, true, self.config.GetCoreCommentChar()) } // SquashAllAboveFixupCommits squashes all fixup! commits above the given one diff --git a/pkg/integration/tests/interactive_rebase/move_across_branch_boundary_outside_rebase.go b/pkg/integration/tests/interactive_rebase/move_across_branch_boundary_outside_rebase.go index 50da17f44..d925acffe 100644 --- a/pkg/integration/tests/interactive_rebase/move_across_branch_boundary_outside_rebase.go +++ b/pkg/integration/tests/interactive_rebase/move_across_branch_boundary_outside_rebase.go @@ -37,18 +37,11 @@ var MoveAcrossBranchBoundaryOutsideRebase = NewIntegrationTest(NewIntegrationTes NavigateToLine(Contains("commit 04")). Press(keys.Commits.MoveDownCommit). Lines( - /* EXPECTED: Contains("CI commit 05"), Contains("CI * commit 03"), Contains("CI commit 04").IsSelected(), Contains("CI commit 02"), Contains("CI commit 01"), - ACTUAL: */ - Contains("CI commit 05"), - Contains("CI * commit 04"), - Contains("CI commit 03").IsSelected(), - Contains("CI commit 02"), - Contains("CI commit 01"), ) }, }) diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index 6be9fd6b5..e2c9dc442 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -141,41 +141,41 @@ func deleteTodos(todos []todo.Todo, todosToDelete []Todo) ([]todo.Todo, error) { return todos, nil } -func MoveTodosDown(fileName string, todosToMove []Todo, commentChar byte) error { +func MoveTodosDown(fileName string, todosToMove []Todo, isInRebase bool, commentChar byte) error { todos, err := ReadRebaseTodoFile(fileName, commentChar) if err != nil { return err } - rearrangedTodos, err := moveTodosDown(todos, todosToMove) + rearrangedTodos, err := moveTodosDown(todos, todosToMove, isInRebase) if err != nil { return err } return WriteRebaseTodoFile(fileName, rearrangedTodos, commentChar) } -func MoveTodosUp(fileName string, todosToMove []Todo, commentChar byte) error { +func MoveTodosUp(fileName string, todosToMove []Todo, isInRebase bool, commentChar byte) error { todos, err := ReadRebaseTodoFile(fileName, commentChar) if err != nil { return err } - rearrangedTodos, err := moveTodosUp(todos, todosToMove) + rearrangedTodos, err := moveTodosUp(todos, todosToMove, isInRebase) if err != nil { return err } return WriteRebaseTodoFile(fileName, rearrangedTodos, commentChar) } -func moveTodoDown(todos []todo.Todo, todoToMove Todo) ([]todo.Todo, error) { - rearrangedTodos, err := moveTodoUp(lo.Reverse(todos), todoToMove) +func moveTodoDown(todos []todo.Todo, todoToMove Todo, isInRebase bool) ([]todo.Todo, error) { + rearrangedTodos, err := moveTodoUp(lo.Reverse(todos), todoToMove, isInRebase) return lo.Reverse(rearrangedTodos), err } -func moveTodosDown(todos []todo.Todo, todosToMove []Todo) ([]todo.Todo, error) { - rearrangedTodos, err := moveTodosUp(lo.Reverse(todos), lo.Reverse(todosToMove)) +func moveTodosDown(todos []todo.Todo, todosToMove []Todo, isInRebase bool) ([]todo.Todo, error) { + rearrangedTodos, err := moveTodosUp(lo.Reverse(todos), lo.Reverse(todosToMove), isInRebase) return lo.Reverse(rearrangedTodos), err } -func moveTodoUp(todos []todo.Todo, todoToMove Todo) ([]todo.Todo, error) { +func moveTodoUp(todos []todo.Todo, todoToMove Todo, isInRebase bool) ([]todo.Todo, error) { sourceIdx, ok := findTodo(todos, todoToMove) if !ok { @@ -188,7 +188,7 @@ func moveTodoUp(todos []todo.Todo, todoToMove Todo) ([]todo.Todo, error) { // the end of the slice) // Find the next todo that we show in lazygit's commits view (skipping the rest) - _, skip, ok := lo.FindIndexOf(todos[sourceIdx+1:], isRenderedTodo) + _, skip, ok := lo.FindIndexOf(todos[sourceIdx+1:], func(t todo.Todo) bool { return isRenderedTodo(t, isInRebase) }) if !ok { // We expect callers to guard against this @@ -202,10 +202,10 @@ func moveTodoUp(todos []todo.Todo, todoToMove Todo) ([]todo.Todo, error) { return rearrangedTodos, nil } -func moveTodosUp(todos []todo.Todo, todosToMove []Todo) ([]todo.Todo, error) { +func moveTodosUp(todos []todo.Todo, todosToMove []Todo, isInRebase bool) ([]todo.Todo, error) { for _, todoToMove := range todosToMove { var newTodos []todo.Todo - newTodos, err := moveTodoUp(todos, todoToMove) + newTodos, err := moveTodoUp(todos, todoToMove, isInRebase) if err != nil { return nil, err } @@ -287,8 +287,8 @@ func RemoveUpdateRefsForCopiedBranch(fileName string, commentChar byte) error { // We render a todo in the commits view if it's a commit or if it's an // update-ref or exec. We don't render label, reset, or comment lines. -func isRenderedTodo(t todo.Todo) bool { - return t.Commit != "" || t.Command == todo.UpdateRef || t.Command == todo.Exec +func isRenderedTodo(t todo.Todo, isInRebase bool) bool { + return t.Commit != "" || (isInRebase && (t.Command == todo.UpdateRef || t.Command == todo.Exec)) } func DropMergeCommit(fileName string, hash string, commentChar byte) error { diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go index 180c6371f..9daf7db01 100644 --- a/pkg/utils/rebase_todo_test.go +++ b/pkg/utils/rebase_todo_test.go @@ -14,6 +14,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { testName string todos []todo.Todo todoToMoveDown Todo + isInRebase bool expectedErr string expectedTodos []todo.Todo } @@ -65,13 +66,14 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { }, }, { - testName: "move across update-ref todo", + testName: "move across update-ref todo in rebase", todos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, {Command: todo.Pick, Commit: "5678"}, }, todoToMoveDown: Todo{Hash: "5678"}, + isInRebase: true, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -79,6 +81,22 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, }, }, + { + testName: "move across update-ref todo outside of rebase", + todos: []todo.Todo{ + {Command: todo.Pick, Commit: "1234"}, + {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, + {Command: todo.Pick, Commit: "5678"}, + }, + todoToMoveDown: Todo{Hash: "5678"}, + isInRebase: false, + expectedErr: "", + expectedTodos: []todo.Todo{ + {Command: todo.Pick, Commit: "5678"}, + {Command: todo.Pick, Commit: "1234"}, + {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, + }, + }, { testName: "move across exec todo", todos: []todo.Todo{ @@ -87,6 +105,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, }, todoToMoveDown: Todo{Hash: "5678"}, + isInRebase: true, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, @@ -153,7 +172,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) { for _, s := range scenarios { t.Run(s.testName, func(t *testing.T) { - rearrangedTodos, err := moveTodoDown(s.todos, s.todoToMoveDown) + rearrangedTodos, err := moveTodoDown(s.todos, s.todoToMoveDown, s.isInRebase) if s.expectedErr == "" { assert.NoError(t, err) } else { @@ -170,6 +189,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { testName string todos []todo.Todo todoToMoveUp Todo + isInRebase bool expectedErr string expectedTodos []todo.Todo } @@ -221,13 +241,14 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { }, }, { - testName: "move across update-ref todo", + testName: "move across update-ref todo in rebase", todos: []todo.Todo{ {Command: todo.Pick, Commit: "1234"}, {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, {Command: todo.Pick, Commit: "5678"}, }, todoToMoveUp: Todo{Hash: "1234"}, + isInRebase: true, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, @@ -235,6 +256,22 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, }, }, + { + testName: "move across update-ref todo outside of rebase", + todos: []todo.Todo{ + {Command: todo.Pick, Commit: "1234"}, + {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, + {Command: todo.Pick, Commit: "5678"}, + }, + todoToMoveUp: Todo{Hash: "1234"}, + isInRebase: false, + expectedErr: "", + expectedTodos: []todo.Todo{ + {Command: todo.UpdateRef, Ref: "refs/heads/some_branch"}, + {Command: todo.Pick, Commit: "5678"}, + {Command: todo.Pick, Commit: "1234"}, + }, + }, { testName: "move across exec todo", todos: []todo.Todo{ @@ -243,6 +280,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { {Command: todo.Pick, Commit: "5678"}, }, todoToMoveUp: Todo{Hash: "1234"}, + isInRebase: true, expectedErr: "", expectedTodos: []todo.Todo{ {Command: todo.Exec, ExecCommand: "make test"}, @@ -309,7 +347,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) { for _, s := range scenarios { t.Run(s.testName, func(t *testing.T) { - rearrangedTodos, err := moveTodoUp(s.todos, s.todoToMoveUp) + rearrangedTodos, err := moveTodoUp(s.todos, s.todoToMoveUp, s.isInRebase) if s.expectedErr == "" { assert.NoError(t, err) } else {