From 42c157a5e694c2b80ec91215155bb8747e5a3bb8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 4 Sep 2024 10:05:11 +0200 Subject: [PATCH 1/3] Add changeToFixup field to MoveFixupCommitDown --- pkg/app/daemon/daemon.go | 16 ++++++------ pkg/commands/git_commands/rebase.go | 2 +- pkg/utils/rebase_todo.go | 10 +++++--- pkg/utils/rebase_todo_test.go | 39 +++++++++++++++++++++++------ 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index 045491fce..575ceab88 100644 --- a/pkg/app/daemon/daemon.go +++ b/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()) }) } diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 56537e92f..de8e19c59 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -302,7 +302,7 @@ func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) e 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() } diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index d93eb4ac8..d08bd1bef 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -221,13 +221,13 @@ 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 } @@ -235,7 +235,7 @@ func MoveFixupCommitDown(fileName string, originalHash string, fixupHash string, 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 } diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go index 8265cf4cf..fbcc82d20 100644 --- a/pkg/utils/rebase_todo_test.go +++ b/pkg/utils/rebase_todo_test.go @@ -266,23 +266,40 @@ 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{ @@ -290,8 +307,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.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,6 +378,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) { }, originalHash: "original", fixupHash: "fixup", + changeToFixup: true, expectedTodos: nil, expectedErr: errors.New("Expected exactly one original hash, found 0"), }, @@ -363,7 +386,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) { 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) From 396215a5c95df3b113fc580d444e0827cfc4f3ef Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 6 Sep 2024 09:02:47 +0200 Subject: [PATCH 2/3] Extract helper function for getting the hash of the last commit made --- pkg/commands/git_commands/rebase.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index de8e19c59..ca2e1a968 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/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,9 +297,7 @@ 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 } From b22149d8322ae578c0a7a75ff52686ee8986ca72 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 4 Sep 2024 10:06:26 +0200 Subject: [PATCH 3/3] Create fixup commit at end of its branch when there's a stack of branches --- pkg/commands/git_commands/rebase.go | 13 +++++ .../controllers/local_commits_controller.go | 52 ++++++++++++++++++ .../create_fixup_commit_in_branch_stack.go | 53 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 4 files changed, 119 insertions(+) create mode 100644 pkg/integration/tests/commit/create_fixup_commit_in_branch_stack.go diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index ca2e1a968..a1362d725 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -309,6 +309,19 @@ func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) e }).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() +} + func todoFromCommit(commit *models.Commit) utils.Todo { if commit.Action == todo.UpdateRef { return utils.Todo{Ref: commit.Name, Action: commit.Action} diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 1e2d4c08e..937a4b1d4 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/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}) }) diff --git a/pkg/integration/tests/commit/create_fixup_commit_in_branch_stack.go b/pkg/integration/tests/commit/create_fixup_commit_in_branch_stack.go new file mode 100644 index 000000000..1c593cf29 --- /dev/null +++ b/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"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index dfc016a3a..301a5a530 100644 --- a/pkg/integration/tests/test_list.go +++ b/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,