From 507b65065f6c73fc3472f860d43dd0f50d44d034 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 30 Oct 2026 15:19:25 +0100 Subject: [PATCH] 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 --- pkg/commands/git_commands/patch.go | 108 ++++++++++++++++++ pkg/commands/git_commands/patch_test.go | 76 ++++++++++++ pkg/commands/git_commands/rebase.go | 11 ++ ...ly_placed_added_lines_to_earlier_commit.go | 3 - ..._adjacent_added_lines_to_earlier_commit.go | 9 -- ...patch_from_added_file_to_earlier_commit.go | 37 ------ 6 files changed, 195 insertions(+), 49 deletions(-) create mode 100644 pkg/commands/git_commands/patch_test.go diff --git a/pkg/commands/git_commands/patch.go b/pkg/commands/git_commands/patch.go index 39f60478a..f3d6d9647 100644 --- a/pkg/commands/git_commands/patch.go +++ b/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 diff --git a/pkg/commands/git_commands/patch_test.go b/pkg/commands/git_commands/patch_test.go new file mode 100644 index 000000000..ab7518df9 --- /dev/null +++ b/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 1700000000 +0100\n" + + "committer C O Mitter 1700000001 +0100\n" + + "\nsubject\n\nbody\n", + headerName: "parent", + value: "3333", + expected: "tree 1111\nparent 3333\n" + + "author A U Thor 1700000000 +0100\n" + + "committer C O Mitter 1700000001 +0100\n" + + "\nsubject\n\nbody\n", + }, + { + name: "replace tree, keeping other headers", + commitObject: "tree 1111\nparent 2222\n" + + "author A U Thor 1700000000 +0100\n" + + "committer C O Mitter 1700000001 +0100\n" + + "encoding ISO-8859-1\n" + + "\nsubject\n", + headerName: "tree", + value: "3333", + expected: "tree 3333\nparent 2222\n" + + "author A U Thor 1700000000 +0100\n" + + "committer C O Mitter 1700000001 +0100\n" + + "encoding ISO-8859-1\n" + + "\nsubject\n", + }, + { + name: "drop signature", + commitObject: "tree 1111\nparent 2222\n" + + "author A U Thor 1700000000 +0100\n" + + "committer C O Mitter 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 1700000000 +0100\n" + + "committer C O Mitter 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)) + }) + } +} diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 74278b18d..c79105c1f 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/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) diff --git a/pkg/integration/tests/main_view/move_ambiguously_placed_added_lines_to_earlier_commit.go b/pkg/integration/tests/main_view/move_ambiguously_placed_added_lines_to_earlier_commit.go index 7c2ec2c07..3a18daff6 100644 --- a/pkg/integration/tests/main_view/move_ambiguously_placed_added_lines_to_earlier_commit.go +++ b/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"), ) }, }) diff --git a/pkg/integration/tests/main_view/move_part_of_adjacent_added_lines_to_earlier_commit.go b/pkg/integration/tests/main_view/move_part_of_adjacent_added_lines_to_earlier_commit.go index db30538cc..ee959326c 100644 --- a/pkg/integration/tests/main_view/move_part_of_adjacent_added_lines_to_earlier_commit.go +++ b/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"), - ) }, }) diff --git a/pkg/integration/tests/main_view/move_patch_from_added_file_to_earlier_commit.go b/pkg/integration/tests/main_view/move_patch_from_added_file_to_earlier_commit.go index a726af2c5..2c583af90 100644 --- a/pkg/integration/tests/main_view/move_patch_from_added_file_to_earlier_commit.go +++ b/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().