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().