Repository: jesseduffield/lazygit (language: go)

Issue:
Wrong commit is focused after moving a commit past a branch boundary
**Describe the bug**
Using `rebase.updateRefs` set to `true` in Git config, when moving a commit past a branch boundary the focused commit changes.

**To Reproduce**
Steps to reproduce the behavior:

1. Starting from the Git state in the screenshot below, add a new commit with summary `Part 1, Commit 3`
2. Focus the commit in the Commits panel and use <c-j> to move the commit down twice. The commit is now located between `Part 2, Commit 1` and `Part 1, Commit 2` at the HEAD of the `feature/part-1` branch.

**Expected behavior**
The `Part 1, Commit 3` commit remains focused in the Commits panel.

**Actual behavior**
The `Part 1, Commit 2` commit is now focused in the Commits panel.

Moving a commit up past a branch boundary with <c-k> also results in the focused commit unexpectedly changing.

**Screenshots**
![CleanShot 2024-11-05 at 15 15 02@2x](https://github.com/user-attachments/assets/64adedce-9976-4f59-aab0-3849debadbf9)

**Version info:**
commit=, build date=, build source=homebrew, version=0.44.1, os=darwin, arch=arm64, git version=2.47.0
git version 2.47.0


Discussion / hints:
That's true, I have run into this too. To work around it, I have gotten used to typing `i` to enter an interactive rebase for the whole stack, and then move the commits. This gives you full control over whether your moved commit(s) go before or after an update-ref todo, it doesn't have the selection issue, and it's also faster anyway (in a larger code base, at least).

Still, it would be nice to fix this, but I'm actually not sure if the selection behavior should be fixed or the moving behavior. The issue exists because moving the commit down only moves it past the update-ref todo (which is not an entry in the commits list); we could fix the problem also by moving it past the next real commit, in which case the selection would be correct again. If we keep the behavior the way it is (and only fix the selection), the only thing that would happen is that you see the branch head icon move up by one, which could be rather confusing. On the other hand, it does give you more control over where exactly your commits go, once you understand what happens.

Any opinions?
> To work around it, I have gotten used to typing i to enter an interactive rebase for the whole stack, and then move the commits.

Aha, yes; I'd figured the same thing after raising this issue.

> Any opinions?

I'd lean towards fixing the selection behaviour. While potentially confusing it conceptually aligns with what happens when moving a commit past an update-ref todo when interactively rebasing. If the commit panel was expanded with `+` the change would be more obvious as you'd see the entire branch name move up by one.


Fix patch:
diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go
index ce1cc2ae056..782df5f1fed 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 5646b589892..7572577505b 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/utils/rebase_todo.go b/pkg/utils/rebase_todo.go
index eedb3bab1c3..e2c9dc4421b 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
 		}
@@ -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 {


Test patch:
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
new file mode 100644
index 00000000000..d925acffe30
--- /dev/null
+++ b/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"),
+			)
+	},
+})
diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go
index 041f0416fb1..782bdcb1bd6 100644
--- a/pkg/integration/tests/test_list.go
+++ b/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,
diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go
index 4896c8a1d90..9daf7db01c1 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
 	}
@@ -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 {