Repository: jesseduffield/lazygit (language: go)

Issue:
Use "git cherry-pick" instead of interactive rebase to implement cherry-picking
We are currently using an interactive rebase to implement cherry-picking. The main reason for doing this is that when you cherry-pick multiple commits, and one of them conflicts, then you get lazygit's nice visualization of the rebase todo list to show you where in the sequence you are, what the conflicting commit is, how many are left etc. Another advantage is that this makes it easy to support cherry-picking in the middle of an interactive rebase, by just inserting the pasted commits as "pick" entries into `git-rebase-todo` at that point.

The downside is that cherry-picking merge commits is not supported this way, since the "pick" command doesn't work with merges (see [this mailing list thread](https://public-inbox.org/git/424f2e08-a2ad-4bb2-8a6b-136c426dc127@haller-berlin.de/)). Cherry-picking merge commits is a useful and requested feature though, see #1374.

If we add a way to visualize the `.git/sequencer/todo` file (which is used by git during cherry-picking) in a similar way to how we do for `.git/rebase-merge/git-rebase-todo` (just read-only), including the logic to show the fake "conflict" entry, then we could use `git cherry-pick -m1` for implementing copy/paste, which would then work for merge commits.

The downside is that it's much more complex to do; especially for the case that the cherry-pick happened in the middle of an interactive rebase, and conflicted. Our todo section would then have to show a merged view of both `.git/sequencer/todo` and `git-rebase-todo`.

All of this is very similar to what we would have to do to support #2442.


Discussion / hints:

Fix patch:
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
 }
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/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/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/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}
-	})
 }


Test patch:
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() {
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 ed051515f74..7ddbfc11977 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,