Fix moving a commit across a branch boundary in a stack of branches (#4096)

- **PR Description**

When moving the first commit of the second branch in a stack down by one
(across the branch head of the first branch), the current behavior is
broken: we move the commit only past the update-ref todo of branch1,
which means the order of commits stays the same and only the branch head
icon moves up by one. However, we move the selection down by one, so the
wrong commit is selected now. This is especially bad if you type a bunch
of ctrl-j quickly in a row, because now you are moving the wrong commit.

There would be two possible ways to fix this:
1) keep the moving behavior the same, but don't change the selection
2) change the behavior so that we move the commit not only past the
update-ref, but also past the next real commit.

You could argue that 1) is the more desirable fix, as it gives you more
control over where exactly the moved commit goes; however, it is much
trickier to implement, so we go with 2) for now. If users need more
fine-grained control, they can always enter an interactive rebase first.

While we're at it, also fix moving a commit across an exec todo in an
interactive rebase, which is currently broken (it moves the commit one
too far).

Fixes #4040.

- **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)
* [ ] 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

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())
 	})
 }
 

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

pkg/integration/tests/interactive_rebase/move_across_branch_boundary_outside_rebase.go

@@ -0,0 +1,47 @@
+package interactive_rebase
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var MoveAcrossBranchBoundaryOutsideRebase = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Move a commit across a branch boundary in a stack of branches",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	GitVersion:   AtLeast("2.38.0"),
+	SetupConfig: func(config *config.AppConfig) {
+		config.GetUserConfig().Git.MainBranches = []string{"master"}
+		config.GetAppState().GitLogShowGraph = "never"
+	},
+	SetupRepo: func(shell *Shell) {
+		shell.
+			CreateNCommits(1).
+			NewBranch("branch1").
+			CreateNCommitsStartingAt(2, 2).
+			NewBranch("branch2").
+			CreateNCommitsStartingAt(2, 4)
+
+		shell.SetConfig("rebase.updateRefs", "true")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("CI commit 05").IsSelected(),
+				Contains("CI commit 04"),
+				Contains("CI * commit 03"),
+				Contains("CI commit 02"),
+				Contains("CI commit 01"),
+			).
+			NavigateToLine(Contains("commit 04")).
+			Press(keys.Commits.MoveDownCommit).
+			Lines(
+				Contains("CI commit 05"),
+				Contains("CI * commit 03"),
+				Contains("CI commit 04").IsSelected(),
+				Contains("CI commit 02"),
+				Contains("CI commit 01"),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -223,6 +223,7 @@ var tests = []*components.IntegrationTest{
 	interactive_rebase.InteractiveRebaseOfCopiedBranch,
 	interactive_rebase.MidRebaseRangeSelect,
 	interactive_rebase.Move,
+	interactive_rebase.MoveAcrossBranchBoundaryOutsideRebase,
 	interactive_rebase.MoveInRebase,
 	interactive_rebase.MoveUpdateRefTodo,
 	interactive_rebase.MoveWithCustomCommentChar,

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
 		}
@@ -286,9 +286,9 @@ 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. We don't render label, reset, or comment lines.
-func isRenderedTodo(t todo.Todo) bool {
-	return t.Commit != "" || t.Command == todo.UpdateRef
+// update-ref or exec. We don't render label, reset, or comment lines.
+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 {

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
 	}
@@ -64,6 +65,54 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) {
 				{Command: todo.Pick, Commit: "5678"},
 			},
 		},
+		{
+			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"},
+				{Command: todo.Pick, Commit: "5678"},
+				{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{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Exec, ExecCommand: "make test"},
+				{Command: todo.Pick, Commit: "5678"},
+			},
+			todoToMoveDown: Todo{Hash: "5678"},
+			isInRebase:     true,
+			expectedErr:    "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Exec, ExecCommand: "make test"},
+			},
+		},
 		{
 			testName: "skip an invisible todo",
 			todos: []todo.Todo{
@@ -123,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 {
@@ -140,6 +189,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) {
 		testName      string
 		todos         []todo.Todo
 		todoToMoveUp  Todo
+		isInRebase    bool
 		expectedErr   string
 		expectedTodos []todo.Todo
 	}
@@ -190,6 +240,54 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) {
 				{Command: todo.UpdateRef, Ref: "refs/heads/some_branch"},
 			},
 		},
+		{
+			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"},
+				{Command: todo.Pick, Commit: "1234"},
+				{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{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Exec, ExecCommand: "make test"},
+				{Command: todo.Pick, Commit: "5678"},
+			},
+			todoToMoveUp: Todo{Hash: "1234"},
+			isInRebase:   true,
+			expectedErr:  "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Exec, ExecCommand: "make test"},
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+			},
+		},
 		{
 			testName: "skip an invisible todo",
 			todos: []todo.Todo{
@@ -249,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 {