From 3fe4db9316f2bcfb168c6585acee425831f9b04b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 19 Apr 2023 08:19:17 +0200 Subject: [PATCH] Make RebaseCommands.AmendTo more robust This fixes two problems with the "amend commit with staged changes" command: 1. Amending to a fixup commit didn't work (this would create a commmit with the title "fixup! fixup! original title" and keep that at the top of the branch) 2. Unrelated fixup commits would be squashed too. The added integration test verifies that both of these problems are fixed. --- pkg/app/daemon/daemon.go | 43 +++++++++++++--- pkg/commands/git_commands/rebase.go | 16 +++++- .../controllers/local_commits_controller.go | 2 +- .../interactive_rebase/amend_fixup_commit.go | 50 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + pkg/utils/rebase_todo.go | 36 +++++++++++++ 6 files changed, 139 insertions(+), 9 deletions(-) create mode 100644 pkg/integration/tests/interactive_rebase/amend_fixup_commit.go diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index d8f32e608..adc287309 100644 --- a/pkg/app/daemon/daemon.go +++ b/pkg/app/daemon/daemon.go @@ -37,6 +37,7 @@ const ( DaemonKindMoveTodoDown DaemonKindInsertBreak DaemonKindChangeTodoActions + DaemonKindMoveFixupCommitDown ) const ( @@ -50,12 +51,13 @@ func getInstruction() Instruction { jsonData := os.Getenv(DaemonInstructionEnvKey) mapping := map[DaemonKind]func(string) Instruction{ - DaemonKindExitImmediately: deserializeInstruction[*ExitImmediatelyInstruction], - DaemonKindCherryPick: deserializeInstruction[*CherryPickCommitsInstruction], - DaemonKindChangeTodoActions: deserializeInstruction[*ChangeTodoActionsInstruction], - DaemonKindMoveTodoUp: deserializeInstruction[*MoveTodoUpInstruction], - DaemonKindMoveTodoDown: deserializeInstruction[*MoveTodoDownInstruction], - DaemonKindInsertBreak: deserializeInstruction[*InsertBreakInstruction], + DaemonKindExitImmediately: deserializeInstruction[*ExitImmediatelyInstruction], + DaemonKindCherryPick: deserializeInstruction[*CherryPickCommitsInstruction], + DaemonKindChangeTodoActions: deserializeInstruction[*ChangeTodoActionsInstruction], + DaemonKindMoveFixupCommitDown: deserializeInstruction[*MoveFixupCommitDownInstruction], + DaemonKindMoveTodoUp: deserializeInstruction[*MoveTodoUpInstruction], + DaemonKindMoveTodoDown: deserializeInstruction[*MoveTodoDownInstruction], + DaemonKindInsertBreak: deserializeInstruction[*InsertBreakInstruction], } return mapping[getDaemonKind()](jsonData) @@ -206,6 +208,35 @@ func (self *ChangeTodoActionsInstruction) run(common *common.Common) error { }) } +// Takes the sha of some commit, and the sha 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" +type MoveFixupCommitDownInstruction struct { + OriginalSha string + FixupSha string +} + +func NewMoveFixupCommitDownInstruction(originalSha string, fixupSha string) Instruction { + return &MoveFixupCommitDownInstruction{ + OriginalSha: originalSha, + FixupSha: fixupSha, + } +} + +func (self *MoveFixupCommitDownInstruction) Kind() DaemonKind { + return DaemonKindMoveFixupCommitDown +} + +func (self *MoveFixupCommitDownInstruction) SerializedInstructions() string { + return serializeInstruction(self) +} + +func (self *MoveFixupCommitDownInstruction) run(common *common.Common) error { + return handleInteractiveRebase(common, func(path string) error { + return utils.MoveFixupCommitDown(path, self.OriginalSha, self.FixupSha) + }) +} + type MoveTodoUpInstruction struct { Sha string } diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index be5384773..50c599a66 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -214,12 +214,24 @@ func (self *RebaseCommands) PrepareInteractiveRebaseCommand(opts PrepareInteract } // AmendTo amends the given commit with whatever files are staged -func (self *RebaseCommands) AmendTo(commit *models.Commit) error { +func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) error { + commit := commits[commitIndex] + if err := self.commit.CreateFixupCommit(commit.Sha); err != nil { return err } - return self.SquashAllAboveFixupCommits(commit) + // Get the sha of the commit we just created + fixupSha, err := self.cmd.New("git rev-parse --verify HEAD").RunWithOutput() + if err != nil { + return err + } + + return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{ + baseShaOrRoot: getBaseShaOrRoot(commits, commitIndex+1), + overrideEditor: true, + instruction: daemon.NewMoveFixupCommitDownInstruction(commit.Sha, fixupSha), + }).Run() } // EditRebaseTodo sets the action for a given rebase commit in the git-rebase-todo file diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 082d24fce..285289121 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -466,7 +466,7 @@ func (self *LocalCommitsController) amendTo(commit *models.Commit) error { HandleConfirm: func() error { return self.c.WithWaitingStatus(self.c.Tr.AmendingStatus, func() error { self.c.LogAction(self.c.Tr.Actions.AmendCommit) - err := self.git.Rebase.AmendTo(commit) + err := self.git.Rebase.AmendTo(self.model.Commits, self.context().GetView().SelectedLineIdx()) return self.helpers.MergeAndRebase.CheckMergeOrRebase(err) }) }, diff --git a/pkg/integration/tests/interactive_rebase/amend_fixup_commit.go b/pkg/integration/tests/interactive_rebase/amend_fixup_commit.go new file mode 100644 index 000000000..0c39c756b --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/amend_fixup_commit.go @@ -0,0 +1,50 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var AmendFixupCommit = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Amends a staged file to a fixup commit, and checks that other unrelated fixup commits are not auto-squashed.", + ExtraCmdArgs: "", + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell. + CreateNCommits(1). + CreateFileAndAdd("first-fixup-file", "").Commit("fixup! commit 01"). + CreateNCommitsStartingAt(2, 2). + CreateFileAndAdd("unrelated-fixup-file", "fixup 03").Commit("fixup! commit 03"). + CreateFileAndAdd("fixup-file", "fixup 01") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("fixup! commit 03"), + Contains("commit 03"), + Contains("commit 02"), + Contains("fixup! commit 01"), + Contains("commit 01"), + ). + NavigateToLine(Contains("fixup! commit 01")). + Press(keys.Commits.AmendToCommit). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Amend Commit")). + Content(Contains("Are you sure you want to amend this commit with your staged files?")). + Confirm() + }). + Lines( + Contains("fixup! commit 03"), + Contains("commit 03"), + Contains("commit 02"), + Contains("fixup! commit 01").IsSelected(), + Contains("commit 01"), + ) + + t.Views().Main(). + Content(Contains("fixup 01")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index d4da79732..8b9ad8ff5 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -85,6 +85,7 @@ var tests = []*components.IntegrationTest{ filter_by_path.TypeFile, interactive_rebase.AdvancedInteractiveRebase, interactive_rebase.AmendFirstCommit, + interactive_rebase.AmendFixupCommit, interactive_rebase.AmendHeadCommitDuringRebase, interactive_rebase.AmendMerge, interactive_rebase.AmendNonHeadCommitDuringRebase, diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index f34e11774..2b8248224 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -134,6 +134,42 @@ func moveTodoUp(todos []todo.Todo, sha string, action todo.TodoCommand) ([]todo. return rearrangedTodos, nil } +func MoveFixupCommitDown(fileName string, originalSha string, fixupSha string) error { + todos, err := ReadRebaseTodoFile(fileName) + if err != nil { + 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 = append(newTodos, t) + } + + if numOriginalShaLinesFound != 1 || numFixupShaLinesFound != 1 { + return fmt.Errorf("Expected exactly one each of originalSha and fixupSha, got %d, %d", + numOriginalShaLinesFound, numFixupShaLinesFound) + } + + return WriteRebaseTodoFile(fileName, newTodos) +} + // 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 {