Make moving todo commits more robust

4 files changed, 407 insertions, 0 deletions

diff --git a/pkg/utils/rebaseTodo.go b/pkg/utils/rebaseTodo.go
index 3e071321b..0b6a6a40c 100644
--- a/pkg/utils/rebaseTodo.go
+++ b/pkg/utils/rebaseTodo.go
@@ -1,11 +1,18 @@
 package utils
 
 import (
+	"fmt"
 	"os"
+	"strings"
 
 	"github.com/fsmiamoto/git-todo-parser/todo"
+	"github.com/samber/lo"
 )
 
+func equalShas(a, b string) bool {
+	return strings.HasPrefix(a, b) || strings.HasPrefix(b, a)
+}
+
 func ReadRebaseTodoFile(fileName string) ([]todo.Todo, error) {
 	f, err := os.Open(fileName)
 	if err != nil {
@@ -32,3 +39,70 @@ func WriteRebaseTodoFile(fileName string, todos []todo.Todo) error {
 	}
 	return err
 }
+
+func MoveTodoDown(fileName string, sha string, action todo.TodoCommand) error {
+	todos, err := ReadRebaseTodoFile(fileName)
+	if err != nil {
+		return err
+	}
+	rearrangedTodos, err := moveTodoDown(todos, sha, action)
+	if err != nil {
+		return err
+	}
+	return WriteRebaseTodoFile(fileName, rearrangedTodos)
+}
+
+func MoveTodoUp(fileName string, sha string, action todo.TodoCommand) error {
+	todos, err := ReadRebaseTodoFile(fileName)
+	if err != nil {
+		return err
+	}
+	rearrangedTodos, err := moveTodoUp(todos, sha, action)
+	if err != nil {
+		return err
+	}
+	return WriteRebaseTodoFile(fileName, rearrangedTodos)
+}
+
+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
new file mode 100644
index 000000000..a4b1a46d0
--- /dev/null
+++ b/pkg/utils/rebaseTodo_test.go
@@ -0,0 +1,230 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/fsmiamoto/git-todo-parser/todo"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRebaseCommands_moveTodoDown(t *testing.T) {
+	type scenario struct {
+		testName      string
+		todos         []todo.Todo
+		shaToMoveDown string
+		expectedErr   string
+		expectedTodos []todo.Todo
+	}
+
+	scenarios := []scenario{
+		{
+			testName: "simple case 1 - move to beginning",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "5678",
+			expectedErr:   "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+		},
+		{
+			testName: "simple case 2 - move from end",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "abcd",
+			expectedErr:   "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "abcd"},
+				{Command: todo.Pick, Commit: "5678"},
+			},
+		},
+		{
+			testName: "skip an invisible todo",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "abcd"},
+				{Command: todo.Label, Label: "myLabel"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "def0"},
+			},
+			shaToMoveDown: "5678",
+			expectedErr:   "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+				{Command: todo.Label, Label: "myLabel"},
+				{Command: todo.Pick, Commit: "def0"},
+			},
+		},
+
+		// Error cases
+		{
+			testName: "commit not found",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "def0",
+			expectedErr:   "Todo def0 not found in git-rebase-todo",
+			expectedTodos: []todo.Todo{},
+		},
+		{
+			testName: "trying to move first commit down",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "1234",
+			expectedErr:   "Destination position for moving todo is out of range",
+			expectedTodos: []todo.Todo{},
+		},
+		{
+			testName: "trying to move commit down when all commits before are invisible",
+			todos: []todo.Todo{
+				{Command: todo.Label, Label: "myLabel"},
+				{Command: todo.Reset, Label: "otherlabel"},
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+			},
+			shaToMoveDown: "1234",
+			expectedErr:   "Destination position for moving todo is out of range",
+			expectedTodos: []todo.Todo{},
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.testName, func(t *testing.T) {
+			rearrangedTodos, err := moveTodoDown(s.todos, s.shaToMoveDown, todo.Pick)
+			if s.expectedErr == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.ErrorContains(t, err, s.expectedErr)
+			}
+			assert.Equal(t, s.expectedTodos, rearrangedTodos)
+		},
+		)
+	}
+}
+
+func TestRebaseCommands_moveTodoUp(t *testing.T) {
+	type scenario struct {
+		testName      string
+		todos         []todo.Todo
+		shaToMoveDown string
+		expectedErr   string
+		expectedTodos []todo.Todo
+	}
+
+	scenarios := []scenario{
+		{
+			testName: "simple case 1 - move to end",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "5678",
+			expectedErr:   "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "abcd"},
+				{Command: todo.Pick, Commit: "5678"},
+			},
+		},
+		{
+			testName: "simple case 2 - move from beginning",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "1234",
+			expectedErr:   "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+		},
+		{
+			testName: "skip an invisible todo",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "abcd"},
+				{Command: todo.Label, Label: "myLabel"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "def0"},
+			},
+			shaToMoveDown: "abcd",
+			expectedErr:   "",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Label, Label: "myLabel"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+				{Command: todo.Pick, Commit: "def0"},
+			},
+		},
+
+		// Error cases
+		{
+			testName: "commit not found",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "def0",
+			expectedErr:   "Todo def0 not found in git-rebase-todo",
+			expectedTodos: []todo.Todo{},
+		},
+		{
+			testName: "trying to move last commit up",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Pick, Commit: "abcd"},
+			},
+			shaToMoveDown: "abcd",
+			expectedErr:   "Destination position for moving todo is out of range",
+			expectedTodos: []todo.Todo{},
+		},
+		{
+			testName: "trying to move commit up when all commits after it are invisible",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "1234"},
+				{Command: todo.Pick, Commit: "5678"},
+				{Command: todo.Label, Label: "myLabel"},
+				{Command: todo.Reset, Label: "otherlabel"},
+			},
+			shaToMoveDown: "5678",
+			expectedErr:   "Destination position for moving todo is out of range",
+			expectedTodos: []todo.Todo{},
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.testName, func(t *testing.T) {
+			rearrangedTodos, err := moveTodoUp(s.todos, s.shaToMoveDown, todo.Pick)
+			if s.expectedErr == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.ErrorContains(t, err, s.expectedErr)
+			}
+			assert.Equal(t, s.expectedTodos, rearrangedTodos)
+		},
+		)
+	}
+}
diff --git a/pkg/utils/slice.go b/pkg/utils/slice.go
index 2281d8a73..aff6ae470 100644
--- 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
index e66edcd61..b3cad8885 100644
--- 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)
+		})
+	})
+}