From dbe58a13c5751a952e6dfdd92dfa16f83f748510 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 8 Oct 2024 17:46:54 +0200 Subject: [PATCH 1/4] Reference original commits in CherryPicking mode instead of synthesizing new ones Previously we would create new Commit objects to store in the CherryPicking mode which only contained a name and hash, all other fields were unset. I'm not sure why we did this; it's easier to just reference the original commits. Later on this branch we will need this because we need to determine whether a copied commit was a merge commit (by looking at its Parents field). --- pkg/gui/modes/cherrypicking/cherry_picking.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/gui/modes/cherrypicking/cherry_picking.go b/pkg/gui/modes/cherrypicking/cherry_picking.go index d84cec39ace..82dae33230b 100644 --- a/pkg/gui/modes/cherrypicking/cherry_picking.go +++ b/pkg/gui/modes/cherrypicking/cherry_picking.go @@ -59,11 +59,7 @@ func (self *CherryPicking) Remove(selectedCommit *models.Commit, commitsList []* } func (self *CherryPicking) update(selectedHashSet *set.Set[string], commitsList []*models.Commit) { - cherryPickedCommits := lo.Filter(commitsList, func(commit *models.Commit, _ int) bool { + self.CherryPickedCommits = lo.Filter(commitsList, func(commit *models.Commit, _ int) bool { return selectedHashSet.Includes(commit.Hash) }) - - self.CherryPickedCommits = lo.Map(cherryPickedCommits, func(commit *models.Commit, _ int) *models.Commit { - return &models.Commit{Name: commit.Name, Hash: commit.Hash} - }) } From 79f8af4bda6682dfb0449bc17b6660b707916416 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Jun 2024 19:32:27 +0200 Subject: [PATCH 2/4] Use "git cherry-pick" for implementing copy/paste of commits We do this because - it's closer to what you would do on the command line - it simplifies the code a bit - it will allow us to support cherry-picking merge commits. --- pkg/commands/git_commands/rebase.go | 36 ++++-------------- .../controllers/helpers/cherry_pick_helper.go | 38 +++++-------------- .../cherry_pick/cherry_pick_conflicts.go | 2 +- .../cherry_pick/cherry_pick_during_rebase.go | 4 +- 4 files changed, 21 insertions(+), 59 deletions(-) diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 1c0497da8a6..188fb72cd56 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -533,35 +533,15 @@ func (self *RebaseCommands) DiscardOldFileChanges(commits []*models.Commit, comm // CherryPickCommits begins an interactive rebase with the given hashes being cherry picked onto HEAD func (self *RebaseCommands) CherryPickCommits(commits []*models.Commit) error { - commitLines := lo.Map(commits, func(commit *models.Commit, _ int) string { - return fmt.Sprintf("%s %s", utils.ShortHash(commit.Hash), commit.Name) - }) - msg := utils.ResolvePlaceholderString( - self.Tr.Log.CherryPickCommits, - map[string]string{ - "commitLines": strings.Join(commitLines, "\n"), - }, - ) - self.os.LogCommand(msg, false) - - return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{ - baseHashOrRoot: "HEAD", - instruction: daemon.NewCherryPickCommitsInstruction(commits), - }).Run() -} - -// CherryPickCommitsDuringRebase simply prepends the given commits to the existing git-rebase-todo file -func (self *RebaseCommands) CherryPickCommitsDuringRebase(commits []*models.Commit) error { - todoLines := lo.Map(commits, func(commit *models.Commit, _ int) daemon.TodoLine { - return daemon.TodoLine{ - Action: "pick", - Commit: commit, - } - }) + hasMergeCommit := lo.SomeBy(commits, func(c *models.Commit) bool { return c.IsMerge() }) + cmdArgs := NewGitCmd("cherry-pick"). + Arg("--allow-empty"). + ArgIf(self.version.IsAtLeast(2, 45, 0), "--empty=keep", "--keep-redundant-commits"). + ArgIf(hasMergeCommit, "-m1"). + Arg(lo.Reverse(lo.Map(commits, func(c *models.Commit, _ int) string { return c.Hash }))...). + ToArgv() - todo := daemon.TodoLinesToString(todoLines) - filePath := filepath.Join(self.repoPaths.worktreeGitDirPath, "rebase-merge/git-rebase-todo") - return utils.PrependStrToTodoFile(filePath, []byte(todo)) + return self.cmd.New(cmdArgs).Run() } func (self *RebaseCommands) DropMergeCommit(commits []*models.Commit, commitIndex int) error { diff --git a/pkg/gui/controllers/helpers/cherry_pick_helper.go b/pkg/gui/controllers/helpers/cherry_pick_helper.go index 1cb2285d002..6b13b0fda74 100644 --- a/pkg/gui/controllers/helpers/cherry_pick_helper.go +++ b/pkg/gui/controllers/helpers/cherry_pick_helper.go @@ -77,41 +77,23 @@ func (self *CherryPickHelper) Paste() error { "numCommits": strconv.Itoa(len(self.getData().CherryPickedCommits)), }), HandleConfirm: func() error { - isInRebase, err := self.c.Git().Status.IsInRebase() - if err != nil { - return err - } - if isInRebase { - if err := self.c.Git().Rebase.CherryPickCommitsDuringRebase(self.getData().CherryPickedCommits); err != nil { - return err - } - err = self.c.Refresh(types.RefreshOptions{ - Mode: types.SYNC, Scope: []types.RefreshableView{types.REBASE_COMMITS}, - }) - if err != nil { - return err - } - - return self.Reset() - } - return self.c.WithWaitingStatus(self.c.Tr.CherryPickingStatus, func(gocui.Task) error { self.c.LogAction(self.c.Tr.Actions.CherryPick) - err := self.c.Git().Rebase.CherryPickCommits(self.getData().CherryPickedCommits) - err = self.rebaseHelper.CheckMergeOrRebase(err) + result := self.c.Git().Rebase.CherryPickCommits(self.getData().CherryPickedCommits) + err := self.rebaseHelper.CheckMergeOrRebase(result) if err != nil { - return err + return result } - // If we're in an interactive rebase at this point, it must + // If we're in the cherry-picking state at this point, it must // be because there were conflicts. Don't clear the copied - // commits in this case, since we might want to abort and - // try pasting them again. - isInRebase, err = self.c.Git().Status.IsInRebase() - if err != nil { - return err + // commits in this case, since we might want to abort and try + // pasting them again. + isInCherryPick, result := self.c.Git().Status.IsInCherryPick() + if result != nil { + return result } - if !isInRebase { + if !isInCherryPick { self.getData().DidPaste = true self.rerender() } diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go index 2f001ebe448..2f0df7498e5 100644 --- a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go +++ b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go @@ -69,7 +69,7 @@ var CherryPickConflicts = NewIntegrationTest(NewIntegrationTestArgs{ SelectNextItem(). PressPrimaryAction() - t.Common().ContinueOnConflictsResolved("rebase") + t.Common().ContinueOnConflictsResolved("cherry-pick") t.Views().Files().IsEmpty() diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_during_rebase.go b/pkg/integration/tests/cherry_pick/cherry_pick_during_rebase.go index e1fc115cf3b..1833337548b 100644 --- a/pkg/integration/tests/cherry_pick/cherry_pick_during_rebase.go +++ b/pkg/integration/tests/cherry_pick/cherry_pick_during_rebase.go @@ -75,8 +75,8 @@ var CherryPickDuringRebase = NewIntegrationTest(NewIntegrationTestArgs{ }). Lines( Contains("pick CI two"), - Contains("pick CI three"), - Contains(" CI <-- YOU ARE HERE --- one"), + Contains(" CI <-- YOU ARE HERE --- three"), + Contains(" CI one"), Contains(" CI base"), ). Tap(func() { From 0e96f17a39ec71aeae58b8d17d98b68e455a9a56 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Jun 2024 19:34:29 +0200 Subject: [PATCH 3/4] Remove unused cherry-picking code in daemon --- pkg/app/daemon/daemon.go | 36 ------------------------------------ 1 file changed, 36 deletions(-) diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index 782df5f1fed..16b9bf5e5ee 100644 --- a/pkg/app/daemon/daemon.go +++ b/pkg/app/daemon/daemon.go @@ -8,7 +8,6 @@ import ( "os/exec" "strconv" - "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/common" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" @@ -33,7 +32,6 @@ const ( DaemonKindExitImmediately DaemonKindRemoveUpdateRefsForCopiedBranch - DaemonKindCherryPick DaemonKindMoveTodosUp DaemonKindMoveTodosDown DaemonKindInsertBreak @@ -56,7 +54,6 @@ func getInstruction() Instruction { mapping := map[DaemonKind]func(string) Instruction{ DaemonKindExitImmediately: deserializeInstruction[*ExitImmediatelyInstruction], DaemonKindRemoveUpdateRefsForCopiedBranch: deserializeInstruction[*RemoveUpdateRefsForCopiedBranchInstruction], - DaemonKindCherryPick: deserializeInstruction[*CherryPickCommitsInstruction], DaemonKindChangeTodoActions: deserializeInstruction[*ChangeTodoActionsInstruction], DaemonKindDropMergeCommit: deserializeInstruction[*DropMergeCommitInstruction], DaemonKindMoveFixupCommitDown: deserializeInstruction[*MoveFixupCommitDownInstruction], @@ -180,39 +177,6 @@ func NewRemoveUpdateRefsForCopiedBranchInstruction() Instruction { return &RemoveUpdateRefsForCopiedBranchInstruction{} } -type CherryPickCommitsInstruction struct { - Todo string -} - -func NewCherryPickCommitsInstruction(commits []*models.Commit) Instruction { - todoLines := lo.Map(commits, func(commit *models.Commit, _ int) TodoLine { - return TodoLine{ - Action: "pick", - Commit: commit, - } - }) - - todo := TodoLinesToString(todoLines) - - return &CherryPickCommitsInstruction{ - Todo: todo, - } -} - -func (self *CherryPickCommitsInstruction) Kind() DaemonKind { - return DaemonKindCherryPick -} - -func (self *CherryPickCommitsInstruction) SerializedInstructions() string { - return serializeInstruction(self) -} - -func (self *CherryPickCommitsInstruction) run(common *common.Common) error { - return handleInteractiveRebase(common, func(path string) error { - return utils.PrependStrToTodoFile(path, []byte(self.Todo)) - }) -} - type ChangeTodoActionsInstruction struct { Changes []ChangeTodoAction } From 417080ebc6ea1b7eda4bd0a816ebfadc89486014 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 21 Jun 2024 10:19:10 +0200 Subject: [PATCH 4/4] Allow cherry-picking merge commits Now that we use git cherry-pick to implement it, there's no reason not to. --- .../controllers/basic_commits_controller.go | 4 - .../tests/cherry_pick/cherry_pick_merge.go | 80 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 3 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 pkg/integration/tests/cherry_pick/cherry_pick_merge.go diff --git a/pkg/gui/controllers/basic_commits_controller.go b/pkg/gui/controllers/basic_commits_controller.go index 925f9e9d585..76dd5500632 100644 --- a/pkg/gui/controllers/basic_commits_controller.go +++ b/pkg/gui/controllers/basic_commits_controller.go @@ -366,10 +366,6 @@ func (self *BasicCommitsController) canCopyCommits(selectedCommits []*models.Com if commit.Hash == "" { return &types.DisabledReason{Text: self.c.Tr.CannotCherryPickNonCommit, ShowErrorInPanel: true} } - - if commit.IsMerge() { - return &types.DisabledReason{Text: self.c.Tr.CannotCherryPickMergeCommit, ShowErrorInPanel: true} - } } return nil diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_merge.go b/pkg/integration/tests/cherry_pick/cherry_pick_merge.go new file mode 100644 index 00000000000..cc9ded0ba84 --- /dev/null +++ b/pkg/integration/tests/cherry_pick/cherry_pick_merge.go @@ -0,0 +1,80 @@ +package cherry_pick + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var CherryPickMerge = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Cherry pick a merge commit", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell. + EmptyCommit("base"). + NewBranch("first-branch"). + NewBranch("second-branch"). + Checkout("first-branch"). + Checkout("second-branch"). + CreateFileAndAdd("file1.txt", "content"). + Commit("one"). + CreateFileAndAdd("file2.txt", "content"). + Commit("two"). + Checkout("master"). + Merge("second-branch"). + Checkout("first-branch") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Focus(). + Lines( + Contains("first-branch"), + Contains("master"), + Contains("second-branch"), + ). + SelectNextItem(). + PressEnter() + + t.Views().SubCommits(). + IsFocused(). + Lines( + Contains("⏣─╮ Merge branch 'second-branch'").IsSelected(), + Contains("│ ◯ two"), + Contains("│ ◯ one"), + Contains("◯ ╯ base"), + ). + // copy the merge commit + Press(keys.Commits.CherryPickCopy) + + t.Views().Information().Content(Contains("1 commit copied")) + + t.Views().Commits(). + Focus(). + Lines( + Contains("base").IsSelected(), + ). + Press(keys.Commits.PasteCommits). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Cherry-pick")). + Content(Contains("Are you sure you want to cherry-pick the 1 copied commit(s) onto this branch?")). + Confirm() + }). + Tap(func() { + t.Views().Information().Content(DoesNotContain("commit copied")) + }). + Lines( + Contains("Merge branch 'second-branch'").IsSelected(), + Contains("base"), + ) + + t.Views().Main().ContainsLines( + Contains("Merge branch 'second-branch'"), + Contains("---"), + Contains("file1.txt | 1 +"), + Contains("file2.txt | 1 +"), + Contains("2 files changed, 2 insertions(+)"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 6add9f2a914..6447fe0dde1 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -82,6 +82,7 @@ var tests = []*components.IntegrationTest{ cherry_pick.CherryPick, cherry_pick.CherryPickConflicts, cherry_pick.CherryPickDuringRebase, + cherry_pick.CherryPickMerge, cherry_pick.CherryPickRange, commit.AddCoAuthor, commit.AddCoAuthorRange,