Avoid conflicts and duplicate lines when moving a patch to an earlier commit

When moving a custom patch to an earlier commit, we apply the patch to
the earlier commit and then let the rebase pick the commit that the
patch came from, unchanged. This relies on git noticing during the pick
that the patch's changes are already there. If the patch contains only
some of several adjacent added lines, git doesn't notice, and the pick
stops with a conflict. If git can place the patch's added lines in more
than one position, it may place them differently for the pick than for
the patch. It then adds them a second time, without any conflict.

Make the rebase pick a copy of the source commit instead. The copy has
the same tree, author and message, but its parent is the original
parent with the patch applied. Picking it only replays the source
commit's changes minus the patch, so git no longer has to recognize
the patch as already applied. Create the copy with plumbing commands
and a temporary index; this doesn't touch the working tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pkg/commands/git_commands/patch.go

@@ -211,6 +211,11 @@ func (self *PatchCommands) MovePatchToSelectedCommit(commits []*models.Commit, s
 			return err
 		}
 
+		if err := self.pickCopyWithPatchInParent(commits[sourceCommitIdx]); err != nil {
+			_ = self.rebase.AbortRebase()
+			return err
+		}
+
 		// apply each patch forward
 		if err := self.ApplyCustomPatch(false, false); err != nil {
 			// Don't abort the rebase here; this might cause conflicts, so give
@@ -427,6 +432,109 @@ func (self *PatchCommands) PullPatchIntoNewCommitBefore(
 	return self.rebase.ContinueRebase()
 }
 
+// When moving the patch to an earlier commit, we apply it to that commit, and
+// the rebase then picks the commit that the patch was built from. Picking that
+// commit unchanged would rely on git noticing that the patch is already
+// applied, and git doesn't always notice this. If the patch contains only some
+// of several adjacent added lines, the pick conflicts; if git can place the
+// patch's added lines in more than one position, it may add them a second
+// time. So we make the rebase pick a copy of the commit instead, whose parent
+// already contains the patch.
+func (self *PatchCommands) pickCopyWithPatchInParent(commit *models.Commit) error {
+	copyHash, err := self.copyCommitWithPatchInParent(commit)
+	if err != nil {
+		return err
+	}
+
+	return self.rebase.ChangeTodoCommit(commit.Hash(), copyHash)
+}
+
+// Creates a copy of the given commit, with the same tree, author, and message,
+// whose parent is the commit's original parent with the custom patch applied.
+// It does this without touching the working tree or the index.
+func (self *PatchCommands) copyCommitWithPatchInParent(commit *models.Commit) (string, error) {
+	indexDir, err := os.MkdirTemp(self.os.GetTempDir(), "index")
+	if err != nil {
+		return "", err
+	}
+	defer os.RemoveAll(indexDir)
+	indexFileEnvVar := "GIT_INDEX_FILE=" + filepath.Join(indexDir, "index")
+
+	cmd := self.cmd.New(NewGitCmd("read-tree").Arg(commit.Parents()[0]).ToArgv())
+	err = cmd.AddEnvVars(indexFileEnvVar).DontLog().Run()
+	if err != nil {
+		return "", err
+	}
+
+	patchFilePath, err := self.SaveTemporaryPatch(self.PatchBuilder.PatchToApply(false, false))
+	if err != nil {
+		return "", err
+	}
+	cmd = self.cmd.New(NewGitCmd("apply").Arg("--cached", patchFilePath).ToArgv())
+	err = cmd.AddEnvVars(indexFileEnvVar).DontLog().Run()
+	if err != nil {
+		return "", err
+	}
+
+	cmd = self.cmd.New(NewGitCmd("write-tree").ToArgv())
+	tree, _, err := cmd.AddEnvVars(indexFileEnvVar).DontLog().RunWithOutputs()
+	if err != nil {
+		return "", err
+	}
+
+	cmd = self.cmd.New(NewGitCmd("cat-file").Arg("commit", commit.Hash()).ToArgv())
+	commitObject, _, err := cmd.DontLog().RunWithOutputs()
+	if err != nil {
+		return "", err
+	}
+
+	parentHash, err := self.writeCommitObject(
+		commitObjectWithHeader(commitObject, "tree", strings.TrimSpace(tree)))
+	if err != nil {
+		return "", err
+	}
+
+	return self.writeCommitObject(commitObjectWithHeader(commitObject, "parent", parentHash))
+}
+
+func (self *PatchCommands) writeCommitObject(commitObject string) (string, error) {
+	cmd := self.cmd.New(
+		NewGitCmd("hash-object").
+			Arg("-t", "commit", "-w", "--stdin").ToArgv())
+	hash, _, err := cmd.
+		SetStdin(commitObject).
+		DontLog().
+		RunWithOutputs()
+	return strings.TrimSpace(hash), err
+}
+
+// Returns the given raw commit object with the value of the given header
+// replaced. The commit's signature is dropped, since it no longer matches.
+func commitObjectWithHeader(commitObject string, name string, value string) string {
+	headers, message, _ := strings.Cut(commitObject, "\n\n")
+
+	newHeaders := []string{}
+	isInSignature := false
+	for line := range strings.SplitSeq(headers, "\n") {
+		// Continuation lines of a multi-line header start with a space
+		if isInSignature && strings.HasPrefix(line, " ") {
+			continue
+		}
+
+		isInSignature = strings.HasPrefix(line, "gpgsig ") || strings.HasPrefix(line, "gpgsig-sha256 ")
+		if isInSignature {
+			continue
+		}
+
+		if strings.HasPrefix(line, name+" ") {
+			line = name + " " + value
+		}
+		newHeaders = append(newHeaders, line)
+	}
+
+	return strings.Join(newHeaders, "\n") + "\n\n" + message
+}
+
 // We have just applied a patch in reverse to discard it from a commit; if we
 // now try to apply the patch again to move it to a later commit, or to the
 // index, then this would conflict "with itself" in case the patch contained

pkg/commands/git_commands/patch_test.go

@@ -0,0 +1,76 @@
+package git_commands
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCommitObjectWithHeader(t *testing.T) {
+	scenarios := []struct {
+		name         string
+		commitObject string
+		headerName   string
+		value        string
+		expected     string
+	}{
+		{
+			name: "replace parent",
+			commitObject: "tree 1111\nparent 2222\n" +
+				"author A U Thor <author@example.com> 1700000000 +0100\n" +
+				"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
+				"\nsubject\n\nbody\n",
+			headerName: "parent",
+			value:      "3333",
+			expected: "tree 1111\nparent 3333\n" +
+				"author A U Thor <author@example.com> 1700000000 +0100\n" +
+				"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
+				"\nsubject\n\nbody\n",
+		},
+		{
+			name: "replace tree, keeping other headers",
+			commitObject: "tree 1111\nparent 2222\n" +
+				"author A U Thor <author@example.com> 1700000000 +0100\n" +
+				"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
+				"encoding ISO-8859-1\n" +
+				"\nsubject\n",
+			headerName: "tree",
+			value:      "3333",
+			expected: "tree 3333\nparent 2222\n" +
+				"author A U Thor <author@example.com> 1700000000 +0100\n" +
+				"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
+				"encoding ISO-8859-1\n" +
+				"\nsubject\n",
+		},
+		{
+			name: "drop signature",
+			commitObject: "tree 1111\nparent 2222\n" +
+				"author A U Thor <author@example.com> 1700000000 +0100\n" +
+				"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
+				"gpgsig -----BEGIN PGP SIGNATURE-----\n \n iQEz\n -----END PGP SIGNATURE-----\n" +
+				"gpgsig-sha256 -----BEGIN SSH SIGNATURE-----\n U1NI\n -----END SSH SIGNATURE-----\n" +
+				"some-other-header value\n" +
+				"\nsubject\n",
+			headerName: "parent",
+			value:      "3333",
+			expected: "tree 1111\nparent 3333\n" +
+				"author A U Thor <author@example.com> 1700000000 +0100\n" +
+				"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
+				"some-other-header value\n" +
+				"\nsubject\n",
+		},
+		{
+			name:         "empty message",
+			commitObject: "tree 1111\nparent 2222\n\n",
+			headerName:   "parent",
+			value:        "3333",
+			expected:     "tree 1111\nparent 3333\n\n",
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			assert.Equal(t, s.expected, commitObjectWithHeader(s.commitObject, s.headerName, s.value))
+		})
+	}
+}

pkg/commands/git_commands/rebase.go

@@ -352,6 +352,17 @@ func (self *RebaseCommands) EditRebaseTodo(commits []*models.Commit, action todo
 	)
 }
 
+// Makes the todo for the given commit in the git-rebase-todo file use a
+// different commit instead
+func (self *RebaseCommands) ChangeTodoCommit(hash string, newHash string) error {
+	return utils.ChangeTodoCommit(
+		filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/git-rebase-todo"),
+		hash,
+		newHash,
+		self.config.GetCoreCommentChar(),
+	)
+}
+
 func (self *RebaseCommands) DeleteUpdateRefTodos(commits []*models.Commit) error {
 	todosToDelete := lo.Map(commits, func(commit *models.Commit, _ int) utils.Todo {
 		return todoFromCommit(commit)

pkg/integration/tests/main_view/move_ambiguously_placed_added_lines_to_earlier_commit.go

@@ -63,10 +63,7 @@ var MoveAmbiguouslyPlacedAddedLinesToEarlierCommit = NewIntegrationTest(NewInteg
 
 		t.FileSystem().FileContent(
 			"file1",
-			/* EXPECTED:
 			Equals("first line\nextra line\n}\nadded line\n}\nlast line\n"),
-			ACTUAL: */
-			Equals("first line\nextra line\n}\nadded line\n}\nadded line\n}\nlast line\n"),
 		)
 	},
 })

pkg/integration/tests/main_view/move_part_of_adjacent_added_lines_to_earlier_commit.go

@@ -48,7 +48,6 @@ var MovePartOfAdjacentAddedLinesToEarlierCommit = NewIntegrationTest(NewIntegrat
 		t.Views().Commits().Focus().SelectNextItem()
 		t.Common().SelectPatchOption(Contains("Move patch to selected commit"))
 
-		/* EXPECTED:
 		t.Views().Commits().
 			IsFocused().
 			Lines(
@@ -63,13 +62,5 @@ var MovePartOfAdjacentAddedLinesToEarlierCommit = NewIntegrationTest(NewIntegrat
 			Tap(func() {
 				t.Views().Main().Content(Contains(" 1st line\n+2nd line"))
 			})
-		ACTUAL: */
-		t.Common().AcknowledgeConflicts()
-
-		t.Views().Files().
-			IsFocused().
-			Lines(
-				Contains("UU").Contains("file1"),
-			)
 	},
 })

pkg/integration/tests/main_view/move_patch_from_added_file_to_earlier_commit.go

@@ -44,7 +44,6 @@ var MovePatchFromAddedFileToEarlierCommit = NewIntegrationTest(NewIntegrationTes
 		t.Views().Commits().Focus().SelectNextItem()
 		t.Common().SelectPatchOption(Contains("Move patch to selected commit"))
 
-		/* EXPECTED:
 		t.Views().Commits().
 			IsFocused().
 			Lines(
@@ -53,42 +52,6 @@ var MovePatchFromAddedFileToEarlierCommit = NewIntegrationTest(NewIntegrationTes
 				Contains("first commit"),
 			).
 			PressEnter()
-		ACTUAL: */
-		// The source commit tries to add a file that the moved line now causes to exist.
-		t.Common().AcknowledgeConflicts()
-
-		t.Views().Files().
-			IsFocused().
-			Lines(
-				Contains("AA").Contains("file"),
-			).
-			PressEnter()
-
-		t.Views().MergeConflicts().
-			IsFocused().
-			TopLines(
-				Contains("<<<<<<< HEAD"),
-				Contains("2nd line"),
-				Contains("======="),
-				Contains("1st line"),
-				Contains("2nd line"),
-				Contains("3rd line"),
-				Contains(">>>>>>>"),
-			).
-			SelectNextItem().
-			PressPrimaryAction()
-
-		t.Common().ContinueOnConflictsResolved("rebase")
-
-		t.Views().Commits().
-			Focus().
-			Lines(
-				Contains("commit to move from").IsSelected(),
-				Contains("destination commit"),
-				Contains("first commit"),
-			).
-			NavigateToLine(Contains("destination commit")).
-			PressEnter()
 
 		t.Views().CommitFiles().
 			IsFocused().