Make moving todo commits more robust

pkg/commands/git_commands/rebase.go

@@ -2,7 +2,6 @@ package git_commands
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 	"strings"
 
@@ -226,34 +225,16 @@ func (self *RebaseCommands) EditRebaseTodo(commit *models.Commit, action todo.To
 	return fmt.Errorf("Todo %s not found in git-rebase-todo", commit.Sha)
 }
 
-func (self *RebaseCommands) getTodoCommitCount(content []string) int {
-	// count lines that are not blank and are not comments
-	commitCount := 0
-	for _, line := range content {
-		if line != "" && !strings.HasPrefix(line, "#") {
-			commitCount++
-		}
-	}
-	return commitCount
+// MoveTodoDown moves a rebase todo item down by one position
+func (self *RebaseCommands) MoveTodoDown(commit *models.Commit) error {
+	fileName := filepath.Join(self.dotGitDir, "rebase-merge/git-rebase-todo")
+	return utils.MoveTodoDown(fileName, commit.Sha, commit.Action)
 }
 
 // MoveTodoDown moves a rebase todo item down by one position
-func (self *RebaseCommands) MoveTodoDown(index int) error {
+func (self *RebaseCommands) MoveTodoUp(commit *models.Commit) error {
 	fileName := filepath.Join(self.dotGitDir, "rebase-merge/git-rebase-todo")
-	bytes, err := os.ReadFile(fileName)
-	if err != nil {
-		return err
-	}
-
-	content := strings.Split(string(bytes), "\n")
-	commitCount := self.getTodoCommitCount(content)
-	contentIndex := commitCount - 1 - index
-
-	rearrangedContent := append(content[0:contentIndex-1], content[contentIndex], content[contentIndex-1])
-	rearrangedContent = append(rearrangedContent, content[contentIndex+1:]...)
-	result := strings.Join(rearrangedContent, "\n")
-
-	return os.WriteFile(fileName, []byte(result), 0o644)
+	return utils.MoveTodoUp(fileName, commit.Sha, commit.Action)
 }
 
 // SquashAllAboveFixupCommits squashes all fixup! commits above the given one

pkg/gui/controllers/local_commits_controller.go

@@ -382,7 +382,7 @@ func (self *LocalCommitsController) moveDown(commit *models.Commit) error {
 		self.c.LogAction(self.c.Tr.Actions.MoveCommitDown)
 		self.c.LogCommand(fmt.Sprintf("Moving commit %s down", commit.ShortSha()), false)
 
-		if err := self.git.Rebase.MoveTodoDown(index); err != nil {
+		if err := self.git.Rebase.MoveTodoDown(commit); err != nil {
 			return self.c.Error(err)
 		}
 		self.context().MoveSelectedLine(1)
@@ -420,7 +420,7 @@ func (self *LocalCommitsController) moveUp(commit *models.Commit) error {
 			false,
 		)
 
-		if err := self.git.Rebase.MoveTodoDown(index - 1); err != nil {
+		if err := self.git.Rebase.MoveTodoUp(self.model.Commits[index]); err != nil {
 			return self.c.Error(err)
 		}
 		self.context().MoveSelectedLine(-1)

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
+}

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)
+		},
+		)
+	}
+}

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
+}

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