From dc4e88f8a48bd52160a76b79da56e13af7b9ffc0 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 4 Apr 2023 10:23:50 +0200 Subject: [PATCH] Make moving todo commits more robust --- pkg/commands/git_commands/rebase.go | 31 +-- .../controllers/local_commits_controller.go | 4 +- pkg/utils/rebaseTodo.go | 74 ++++++ pkg/utils/rebaseTodo_test.go | 230 ++++++++++++++++++ pkg/utils/slice.go | 21 ++ pkg/utils/slice_test.go | 82 +++++++ 6 files changed, 415 insertions(+), 27 deletions(-) create mode 100644 pkg/utils/rebaseTodo_test.go diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 27b5a6ba4..e4c20426f 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/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 diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 72169a5f0..48fafd4e6 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/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) 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) + }) + }) +}