refactor moveFixupCommitDown

jesseduffield · Apr 29, 2023 · e638582 · e638582
1 parent 3fe4db9
commit e638582
Show file tree

Hide file tree

Showing 2 changed files with 138 additions and 23 deletions.
diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go
@@ -140,34 +140,41 @@ func MoveFixupCommitDown(fileName string, originalSha string, fixupSha string) e
 		return err
 	}
 
-	newTodos := []todo.Todo{}
-	numOriginalShaLinesFound := 0
-	numFixupShaLinesFound := 0
-
-	for _, t := range todos {
-		if t.Command == todo.Pick {
-			if equalShas(t.Commit, originalSha) {
-				numOriginalShaLinesFound += 1
-				// append the original commit, and then the fixup
-				newTodos = append(newTodos, t)
-				newTodos = append(newTodos, todo.Todo{Command: todo.Fixup, Commit: fixupSha})
-				continue
-			} else if equalShas(t.Commit, fixupSha) {
-				numFixupShaLinesFound += 1
-				// skip the fixup here
-				continue
-			}
-		}
+	newTodos, err := moveFixupCommitDown(todos, originalSha, fixupSha)
+	if err != nil {
+		return err
+	}
 
-		newTodos = append(newTodos, t)
+	return WriteRebaseTodoFile(fileName, newTodos)
+}
+
+func moveFixupCommitDown(todos []todo.Todo, originalSha string, fixupSha string) ([]todo.Todo, error) {
+	isOriginal := func(t todo.Todo) bool {
+		return t.Command == todo.Pick && equalShas(t.Commit, originalSha)
 	}
 
-	if numOriginalShaLinesFound != 1 || numFixupShaLinesFound != 1 {
-		return fmt.Errorf("Expected exactly one each of originalSha and fixupSha, got %d, %d",
-			numOriginalShaLinesFound, numFixupShaLinesFound)
+	isFixup := func(t todo.Todo) bool {
+		return t.Command == todo.Pick && equalShas(t.Commit, fixupSha)
 	}
 
-	return WriteRebaseTodoFile(fileName, newTodos)
+	originalShaCount := lo.CountBy(todos, isOriginal)
+	if originalShaCount != 1 {
+		return nil, fmt.Errorf("Expected exactly one original SHA, found %d", originalShaCount)
+	}
+
+	fixupShaCount := lo.CountBy(todos, isFixup)
+	if fixupShaCount != 1 {
+		return nil, fmt.Errorf("Expected exactly one fixup SHA, found %d", fixupShaCount)
+	}
+
+	_, fixupIndex, _ := lo.FindIndexOf(todos, isFixup)
+	_, originalIndex, _ := lo.FindIndexOf(todos, isOriginal)
+
+	newTodos := MoveElement(todos, fixupIndex, originalIndex+1)
+
+	newTodos[originalIndex+1].Command = todo.Fixup
+
+	return newTodos, nil
 }
 
 // We render a todo in the commits view if it's a commit or if it's an

diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go
@@ -1,6 +1,7 @@
 package utils
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/fsmiamoto/git-todo-parser/todo"
@@ -228,3 +229,110 @@ func TestRebaseCommands_moveTodoUp(t *testing.T) {
 		)
 	}
 }
+
+func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
+	scenarios := []struct {
+		name          string
+		todos         []todo.Todo
+		originalSha   string
+		fixupSha      string
+		expectedTodos []todo.Todo
+		expectedErr   error
+	}{
+		{
+			name: "fixup commit is the last commit",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			originalSha: "original",
+			fixupSha:    "fixup",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Fixup, Commit: "fixup"},
+			},
+			expectedErr: nil,
+		},
+		{
+			// TODO: is this something we actually want to support?
+			name: "fixup commit is separated from original commit",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "other"},
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			originalSha: "original",
+			fixupSha:    "fixup",
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Fixup, Commit: "fixup"},
+				{Command: todo.Pick, Commit: "other"},
+			},
+			expectedErr: nil,
+		},
+		{
+			name: "More original SHAs than expected",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			originalSha:   "original",
+			fixupSha:      "fixup",
+			expectedTodos: nil,
+			expectedErr:   errors.New("Expected exactly one original SHA, found 2"),
+		},
+		{
+			name: "More fixup SHAs than expected",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "fixup"},
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			originalSha:   "original",
+			fixupSha:      "fixup",
+			expectedTodos: nil,
+			expectedErr:   errors.New("Expected exactly one fixup SHA, found 2"),
+		},
+		{
+			name: "No fixup SHAs found",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+			},
+			originalSha:   "original",
+			fixupSha:      "fixup",
+			expectedTodos: nil,
+			expectedErr:   errors.New("Expected exactly one fixup SHA, found 0"),
+		},
+		{
+			name: "No original SHAs found",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			originalSha:   "original",
+			fixupSha:      "fixup",
+			expectedTodos: nil,
+			expectedErr:   errors.New("Expected exactly one original SHA, found 0"),
+		},
+	}
+
+	for _, scenario := range scenarios {
+		t.Run(scenario.name, func(t *testing.T) {
+			actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalSha, scenario.fixupSha)
+
+			if scenario.expectedErr == nil {
+				if !assert.NoError(t, actualErr) {
+					t.Errorf("Expected no error, got: %v", actualErr)
+				}
+			} else {
+				if !assert.EqualError(t, actualErr, scenario.expectedErr.Error()) {
+					t.Errorf("Expected err: %v, got: %v", scenario.expectedErr, actualErr)
+				}
+			}
+
+			if !assert.EqualValues(t, actualTodos, scenario.expectedTodos) {
+				t.Errorf("Expected todos: %v, got: %v", scenario.expectedTodos, actualTodos)
+			}
+		})
+	}
+}