With stacked branches, create fixup commit at the end of the branch it belongs to (jesseduffield#3892)

- **PR Description**

When working with stacked branches, and creating a fixup commit for a
commit in one of the lower branches of the stack, the fixup was created
at the top of the stack and the user needed to move it down to the right
branch manually. This is unnecessary extra work; create it at the end of
the right branch automatically.

Author: stefanhaller

pkg/app/daemon/daemon.go

@@ -246,16 +246,18 @@ func (self *ChangeTodoActionsInstruction) run(common *common.Common) error {
 
 // Takes the hash of some commit, and the hash of a fixup commit that was created
 // at the end of the branch, then moves the fixup commit down to right after the
-// original commit, changing its type to "fixup"
+// original commit, changing its type to "fixup" (only if ChangeToFixup is true)
 type MoveFixupCommitDownInstruction struct {
-	OriginalHash string
-	FixupHash    string
+	OriginalHash  string
+	FixupHash     string
+	ChangeToFixup bool
 }
 
-func NewMoveFixupCommitDownInstruction(originalHash string, fixupHash string) Instruction {
+func NewMoveFixupCommitDownInstruction(originalHash string, fixupHash string, changeToFixup bool) Instruction {
 	return &MoveFixupCommitDownInstruction{
-		OriginalHash: originalHash,
-		FixupHash:    fixupHash,
+		OriginalHash:  originalHash,
+		FixupHash:     fixupHash,
+		ChangeToFixup: changeToFixup,
 	}
 }
 
@@ -269,7 +271,7 @@ func (self *MoveFixupCommitDownInstruction) SerializedInstructions() string {
 
 func (self *MoveFixupCommitDownInstruction) run(common *common.Common) error {
 	return handleInteractiveRebase(common, func(path string) error {
-		return utils.MoveFixupCommitDown(path, self.OriginalHash, self.FixupHash, getCommentChar())
+		return utils.MoveFixupCommitDown(path, self.OriginalHash, self.FixupHash, self.ChangeToFixup, getCommentChar())
 	})
 }
 

pkg/commands/git_commands/rebase.go

@@ -284,6 +284,11 @@ func (self *RebaseCommands) GitRebaseEditTodo(todosFileContent []byte) error {
 	return cmdObj.Run()
 }
 
+func (self *RebaseCommands) getHashOfLastCommitMade() (string, error) {
+	cmdArgs := NewGitCmd("rev-parse").Arg("--verify", "HEAD").ToArgv()
+	return self.cmd.New(cmdArgs).RunWithOutput()
+}
+
 // AmendTo amends the given commit with whatever files are staged
 func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) error {
 	commit := commits[commitIndex]
@@ -292,17 +297,28 @@ func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) e
 		return err
 	}
 
-	// Get the hash of the commit we just created
-	cmdArgs := NewGitCmd("rev-parse").Arg("--verify", "HEAD").ToArgv()
-	fixupHash, err := self.cmd.New(cmdArgs).RunWithOutput()
+	fixupHash, err := self.getHashOfLastCommitMade()
 	if err != nil {
 		return err
 	}
 
 	return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
 		baseHashOrRoot: getBaseHashOrRoot(commits, commitIndex+1),
 		overrideEditor: true,
-		instruction:    daemon.NewMoveFixupCommitDownInstruction(commit.Hash, fixupHash),
+		instruction:    daemon.NewMoveFixupCommitDownInstruction(commit.Hash, fixupHash, true),
+	}).Run()
+}
+
+func (self *RebaseCommands) MoveFixupCommitDown(commits []*models.Commit, targetCommitIndex int) error {
+	fixupHash, err := self.getHashOfLastCommitMade()
+	if err != nil {
+		return err
+	}
+
+	return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
+		baseHashOrRoot: getBaseHashOrRoot(commits, targetCommitIndex+1),
+		overrideEditor: true,
+		instruction:    daemon.NewMoveFixupCommitDownInstruction(commits[targetCommitIndex].Hash, fixupHash, false),
 	}).Run()
 }
 

pkg/gui/controllers/local_commits_controller.go

@@ -895,6 +895,10 @@ func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) err
 								return err
 							}
 
+							if err := self.moveFixupCommitToOwnerStackedBranch(commit); err != nil {
+								return err
+							}
+
 							self.context().MoveSelectedLine(1)
 							return self.c.Refresh(types.RefreshOptions{Mode: types.SYNC})
 						})
@@ -924,6 +928,50 @@ func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) err
 	})
 }
 
+func (self *LocalCommitsController) moveFixupCommitToOwnerStackedBranch(targetCommit *models.Commit) error {
+	if self.c.Git().Version.IsOlderThan(2, 38, 0) {
+		// Git 2.38.0 introduced the `rebase.updateRefs` config option. Don't
+		// move the commit down with older versions, as it would break the stack.
+		return nil
+	}
+
+	if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE {
+		// Can't move commits while rebasing
+		return nil
+	}
+
+	if targetCommit.Status == models.StatusMerged {
+		// Target commit is already on main. It's a bit questionable that we
+		// allow creating a fixup commit for it in the first place, but we
+		// always did, so why restrict that now; however, it doesn't make sense
+		// to move the created fixup commit down in that case.
+		return nil
+	}
+
+	if !self.c.Git().Config.GetRebaseUpdateRefs() {
+		// If the user has disabled rebase.updateRefs, we don't move the fixup
+		// because this would break the stack of branches (presumably they like
+		// to manage it themselves manually, or something).
+		return nil
+	}
+
+	headOfOwnerBranchIdx := -1
+	for i := self.context().GetSelectedLineIdx(); i > 0; i-- {
+		if lo.SomeBy(self.c.Model().Branches, func(b *models.Branch) bool {
+			return b.CommitHash == self.c.Model().Commits[i].Hash
+		}) {
+			headOfOwnerBranchIdx = i
+			break
+		}
+	}
+
+	if headOfOwnerBranchIdx == -1 {
+		return nil
+	}
+
+	return self.c.Git().Rebase.MoveFixupCommitDown(self.c.Model().Commits, headOfOwnerBranchIdx)
+}
+
 func (self *LocalCommitsController) createAmendCommit(commit *models.Commit, includeFileChanges bool) error {
 	commitMessage, err := self.c.Git().Commit.GetCommitMessage(commit.Hash)
 	if err != nil {
@@ -947,6 +995,10 @@ func (self *LocalCommitsController) createAmendCommit(commit *models.Commit, inc
 						return err
 					}
 
+					if err := self.moveFixupCommitToOwnerStackedBranch(commit); err != nil {
+						return err
+					}
+
 					self.context().MoveSelectedLine(1)
 					return self.c.Refresh(types.RefreshOptions{Mode: types.SYNC})
 				})

pkg/integration/tests/commit/create_fixup_commit_in_branch_stack.go

@@ -0,0 +1,53 @@
+package commit
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var CreateFixupCommitInBranchStack = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Create a fixup commit in a stack of branches, verify that it is created at the end of the branch it belongs to",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	GitVersion:   AtLeast("2.38.0"),
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.NewBranch("branch1")
+		shell.EmptyCommit("branch1 commit 1")
+		shell.EmptyCommit("branch1 commit 2")
+		shell.EmptyCommit("branch1 commit 3")
+		shell.NewBranch("branch2")
+		shell.EmptyCommit("branch2 commit 1")
+		shell.EmptyCommit("branch2 commit 2")
+		shell.CreateFileAndAdd("fixup-file", "fixup content")
+
+		shell.SetConfig("rebase.updateRefs", "true")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("CI ◯ branch2 commit 2"),
+				Contains("CI ◯ branch2 commit 1"),
+				Contains("CI ◯ * branch1 commit 3"),
+				Contains("CI ◯ branch1 commit 2"),
+				Contains("CI ◯ branch1 commit 1"),
+			).
+			NavigateToLine(Contains("branch1 commit 2")).
+			Press(keys.Commits.CreateFixupCommit).
+			Tap(func() {
+				t.ExpectPopup().Menu().
+					Title(Equals("Create fixup commit")).
+					Select(Contains("fixup! commit")).
+					Confirm()
+			}).
+			Lines(
+				Contains("CI ◯ branch2 commit 2"),
+				Contains("CI ◯ branch2 commit 1"),
+				Contains("CI ◯ * fixup! branch1 commit 2"),
+				Contains("CI ◯ branch1 commit 3"),
+				Contains("CI ◯ branch1 commit 2"),
+				Contains("CI ◯ branch1 commit 1"),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -87,6 +87,7 @@ var tests = []*components.IntegrationTest{
 	commit.CommitWithNonMatchingBranchName,
 	commit.CommitWithPrefix,
 	commit.CreateAmendCommit,
+	commit.CreateFixupCommitInBranchStack,
 	commit.CreateTag,
 	commit.DiscardOldFileChanges,
 	commit.FindBaseCommitForFixup,

pkg/utils/rebase_todo.go

@@ -221,21 +221,21 @@ func moveTodosUp(todos []todo.Todo, todosToMove []Todo) ([]todo.Todo, error) {
 	return todos, nil
 }
 
-func MoveFixupCommitDown(fileName string, originalHash string, fixupHash string, commentChar byte) error {
+func MoveFixupCommitDown(fileName string, originalHash string, fixupHash string, changeToFixup bool, commentChar byte) error {
 	todos, err := ReadRebaseTodoFile(fileName, commentChar)
 	if err != nil {
 		return err
 	}
 
-	newTodos, err := moveFixupCommitDown(todos, originalHash, fixupHash)
+	newTodos, err := moveFixupCommitDown(todos, originalHash, fixupHash, changeToFixup)
 	if err != nil {
 		return err
 	}
 
 	return WriteRebaseTodoFile(fileName, newTodos, commentChar)
 }
 
-func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash string) ([]todo.Todo, error) {
+func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash string, changeToFixup bool) ([]todo.Todo, error) {
 	isOriginal := func(t todo.Todo) bool {
 		return (t.Command == todo.Pick || t.Command == todo.Merge) && equalHash(t.Commit, originalHash)
 	}
@@ -259,7 +259,9 @@ func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash strin
 
 	newTodos := MoveElement(todos, fixupIndex, originalIndex+1)
 
-	newTodos[originalIndex+1].Command = todo.Fixup
+	if changeToFixup {
+		newTodos[originalIndex+1].Command = todo.Fixup
+	}
 
 	return newTodos, nil
 }

pkg/utils/rebase_todo_test.go

@@ -266,32 +266,50 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 		todos         []todo.Todo
 		originalHash  string
 		fixupHash     string
+		changeToFixup bool
 		expectedTodos []todo.Todo
 		expectedErr   error
 	}{
 		{
-			name: "fixup commit is the last commit",
+			name: "fixup commit is the last commit (change to fixup)",
 			todos: []todo.Todo{
 				{Command: todo.Pick, Commit: "original"},
 				{Command: todo.Pick, Commit: "fixup"},
 			},
-			originalHash: "original",
-			fixupHash:    "fixup",
+			originalHash:  "original",
+			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: []todo.Todo{
 				{Command: todo.Pick, Commit: "original"},
 				{Command: todo.Fixup, Commit: "fixup"},
 			},
 			expectedErr: nil,
 		},
+		{
+			name: "fixup commit is the last commit (don't change to fixup)",
+			todos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			originalHash:  "original",
+			fixupHash:     "fixup",
+			changeToFixup: false,
+			expectedTodos: []todo.Todo{
+				{Command: todo.Pick, Commit: "original"},
+				{Command: todo.Pick, Commit: "fixup"},
+			},
+			expectedErr: nil,
+		},
 		{
 			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"},
 			},
-			originalHash: "original",
-			fixupHash:    "fixup",
+			originalHash:  "original",
+			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: []todo.Todo{
 				{Command: todo.Pick, Commit: "original"},
 				{Command: todo.Fixup, Commit: "fixup"},
@@ -306,8 +324,9 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 				{Command: todo.Pick, Commit: "other"},
 				{Command: todo.Pick, Commit: "fixup"},
 			},
-			originalHash: "original",
-			fixupHash:    "fixup",
+			originalHash:  "original",
+			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: []todo.Todo{
 				{Command: todo.Merge, Commit: "original"},
 				{Command: todo.Fixup, Commit: "fixup"},
@@ -324,6 +343,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 			},
 			originalHash:  "original",
 			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: nil,
 			expectedErr:   errors.New("Expected exactly one original hash, found 2"),
 		},
@@ -336,6 +356,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 			},
 			originalHash:  "original",
 			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: nil,
 			expectedErr:   errors.New("Expected exactly one fixup hash, found 2"),
 		},
@@ -346,6 +367,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 			},
 			originalHash:  "original",
 			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: nil,
 			expectedErr:   errors.New("Expected exactly one fixup hash, found 0"),
 		},
@@ -356,14 +378,15 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
 			},
 			originalHash:  "original",
 			fixupHash:     "fixup",
+			changeToFixup: true,
 			expectedTodos: nil,
 			expectedErr:   errors.New("Expected exactly one original hash, found 0"),
 		},
 	}
 
 	for _, scenario := range scenarios {
 		t.Run(scenario.name, func(t *testing.T) {
-			actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalHash, scenario.fixupHash)
+			actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalHash, scenario.fixupHash, scenario.changeToFixup)
 
 			if scenario.expectedErr == nil {
 				assert.NoError(t, actualErr)