fixup! Make moving todo commits more robust

jesseduffield · jesseduffield · commit 357064ed3e5e · 2023-04-15T10:15:59.000+10:00
diff --git a/pkg/utils/rebaseTodo.go b/pkg/utils/rebaseTodo.go
@@ -52,42 +52,6 @@ func MoveTodoDown(fileName string, sha string, action todo.TodoCommand) error {
 	return WriteRebaseTodoFile(fileName, rearrangedTodos)
 }
 
-func moveTodoDown(todos []todo.Todo, sha string, action todo.TodoCommand) ([]todo.Todo, error) {
-	for i, t := range todos {
-		// Comparing just the sha is not enough; we need to compare both the
-		// action and the sha, as the sha could appear multiple times (e.g. in a
-		// pick and later in a merge)
-		if t.Command == action && equalShas(t.Commit, sha) {
-			// The todos are ordered backwards compared to our model commits, so
-			// actually move the commit _up_ in the todos slice (i.e. towards 0)
-
-			// Skip over any entries that we don't show in lazygit's view (e.g.
-			// label, reset, or comment lines).
-			j := i
-			for ; j > 0 && todos[j-1].Commit == "" && todos[j-1].Command != todo.UpdateRef; j -= 1 {
-			}
-
-			if j == 0 {
-				// We expect callers to guard against this
-				return []todo.Todo{}, fmt.Errorf("Destination position for moving todo out of range")
-			}
-
-			// Need to allocate a new slice so that we don't overwrite the
-			// original one while we're picking things from it
-			rearrangedTodos := make([]todo.Todo, 0, len(todos))
-			rearrangedTodos = append(rearrangedTodos, todos[0:j-1]...) // everything up to the target position
-			rearrangedTodos = append(rearrangedTodos, todos[i])        // the commit we are moving
-			rearrangedTodos = append(rearrangedTodos, todos[j-1:i]...) // the commits between src and dest that we skipped above
-			rearrangedTodos = append(rearrangedTodos, todos[i+1:]...)  // the remaining commits
-
-			return rearrangedTodos, nil
-		}
-	}
-
-	// Should never get here
-	return []todo.Todo{}, fmt.Errorf("Todo %s not found in git-rebase-todo", sha)
-}
-
 func MoveTodoUp(fileName string, sha string, action todo.TodoCommand) error {
 	todos, err := ReadRebaseTodoFile(fileName)
 	if err != nil {
@@ -100,7 +64,45 @@ func MoveTodoUp(fileName string, sha string, action todo.TodoCommand) error {
 	return WriteRebaseTodoFile(fileName, rearrangedTodos)
 }
 
-func moveTodoUp(todos []todo.Todo, sha string, action todo.TodoCommand) ([]todo.Todo, error) {
-	rearrangedTodos, err := moveTodoDown(lo.Reverse(todos), sha, action)
+func moveTodoDown(todos []todo.Todo, sha string, action todo.TodoCommand) ([]todo.Todo, error) {
+	rearrangedTodos, err := moveTodoUp(lo.Reverse(todos), sha, action)
 	return lo.Reverse(rearrangedTodos), err
 }
+
+func moveTodoUp(todos []todo.Todo, sha string, action todo.TodoCommand) ([]todo.Todo, error) {
+	_, sourceIdx, ok := lo.FindIndexOf(todos, func(t todo.Todo) bool {
+		// Comparing just the sha is not enough; we need to compare both the
+		// action and the sha, as the sha could appear multiple times (e.g. in a
+		// pick and later in a merge)
+		return t.Command == action && equalShas(t.Commit, sha)
+	})
+
+	if !ok {
+		// Should never happen
+		return []todo.Todo{}, fmt.Errorf("Todo %s not found in git-rebase-todo", sha)
+	}
+
+	// The todos are ordered backwards compared to our model commits, so
+	// actually move the commit _down_ in the todos slice (i.e. towards
+	// 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)
+
+	if !ok {
+		// We expect callers to guard against this
+		return []todo.Todo{}, fmt.Errorf("Destination position for moving todo is out of range")
+	}
+
+	destinationIdx := sourceIdx + 1 + skip
+
+	rearrangedTodos := MoveElement(todos, sourceIdx, destinationIdx)
+
+	return rearrangedTodos, nil
+}
+
+// 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
+}
diff --git a/pkg/utils/rebaseTodo_test.go b/pkg/utils/rebaseTodo_test.go
@@ -87,7 +87,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) {
 				{Command: todo.Pick, Commit: "abcd"},
 			},
 			shaToMoveDown: "1234",
-			expectedErr:   "Destination position for moving todo out of range",
+			expectedErr:   "Destination position for moving todo is out of range",
 			expectedTodos: []todo.Todo{},
 		},
 		{
@@ -99,7 +99,7 @@ func TestRebaseCommands_moveTodoDown(t *testing.T) {
 				{Command: todo.Pick, Commit: "5678"},
 			},
 			shaToMoveDown: "1234",
-			expectedErr:   "Destination position for moving todo out of range",
+			expectedErr:   "Destination position for moving todo is out of range",
 			expectedTodos: []todo.Todo{},
 		},
 	}
@@ -198,7 +198,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) {
 				{Command: todo.Pick, Commit: "abcd"},
 			},
 			shaToMoveDown: "abcd",
-			expectedErr:   "Destination position for moving todo out of range",
+			expectedErr:   "Destination position for moving todo is out of range",
 			expectedTodos: []todo.Todo{},
 		},
 		{
@@ -210,7 +210,7 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) {
 				{Command: todo.Reset, Label: "otherlabel"},
 			},
 			shaToMoveDown: "5678",
-			expectedErr:   "Destination position for moving todo out of range",
+			expectedErr:   "Destination position for moving todo is out of range",
 			expectedTodos: []todo.Todo{},
 		},
 	}
diff --git a/pkg/utils/slice.go b/pkg/utils/slice.go
@@ -92,3 +92,24 @@ func MuiltiGroupBy[T any, K comparable](slice []T, f func(T) []K) map[K][]T {
 	}
 	return result
 }
+
+// Returns a new slice with the element at index 'from' moved to index 'to'.
+// Does not mutate original slice.
+func MoveElement[T any](slice []T, from int, to int) []T {
+	newSlice := make([]T, len(slice))
+	copy(newSlice, slice)
+
+	if from == to {
+		return newSlice
+	}
+
+	if from < to {
+		copy(newSlice[from:to+1], newSlice[from+1:to+1])
+	} else {
+		copy(newSlice[to+1:from+1], newSlice[to:from])
+	}
+
+	newSlice[to] = slice[from]
+
+	return newSlice
+}
diff --git a/pkg/utils/slice_test.go b/pkg/utils/slice_test.go
@@ -233,3 +233,85 @@ func TestLimitStr(t *testing.T) {
 		}
 	}
 }
+
+func TestMoveElement(t *testing.T) {
+	type scenario struct {
+		testName string
+		list     []int
+		from     int
+		to       int
+		expected []int
+	}
+
+	scenarios := []scenario{
+		{
+			"no elements",
+			[]int{},
+			0,
+			0,
+			[]int{},
+		},
+		{
+			"one element",
+			[]int{1},
+			0,
+			0,
+			[]int{1},
+		},
+		{
+			"two elements, moving first to second",
+			[]int{1, 2},
+			0,
+			1,
+			[]int{2, 1},
+		},
+		{
+			"two elements, moving second to first",
+			[]int{1, 2},
+			1,
+			0,
+			[]int{2, 1},
+		},
+		{
+			"three elements, moving first to second",
+			[]int{1, 2, 3},
+			0,
+			1,
+			[]int{2, 1, 3},
+		},
+		{
+			"three elements, moving second to first",
+			[]int{1, 2, 3},
+			1,
+			0,
+			[]int{2, 1, 3},
+		},
+		{
+			"three elements, moving second to third",
+			[]int{1, 2, 3},
+			1,
+			2,
+			[]int{1, 3, 2},
+		},
+		{
+			"three elements, moving third to second",
+			[]int{1, 2, 3},
+			2,
+			1,
+			[]int{1, 3, 2},
+		},
+	}
+
+	for _, s := range scenarios {
+		s := s
+		t.Run(s.testName, func(t *testing.T) {
+			assert.EqualValues(t, s.expected, MoveElement(s.list, s.from, s.to))
+		})
+	}
+
+	t.Run("from out of bounds", func(t *testing.T) {
+		assert.Panics(t, func() {
+			MoveElement([]int{1, 2, 3}, 3, 0)
+		})
+	})
+}
