Skip to content

Commit bbca3df

Browse files
stefanhallerstefanhaller
authored
Avoid conflicts and duplicate lines when moving a patch to an earlier commit (#6167)
When moving a custom patch to an earlier commit, or into a new commit before the original one, lazygit applies the patch to the earlier commit and then replayed the original commit unchanged. It relied on git noticing during the replay that the patch's changes are already there. This doesn't always work: - If the patch contains only some of several adjacent added lines, replaying the original commit stops with a conflict. - If git can place the patch's added lines in two positions because they repeat the line after them, the replay may put them in the other position than the patch did. Git then keeps both copies, and the file ends up containing the patch's lines twice. There's no conflict, so this goes unnoticed. Fix this by making 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.
2 parents 0002c03 + fabc14d commit bbca3df

16 files changed

Lines changed: 805 additions & 67 deletions

‎pkg/commands/git_commands/patch.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,11 @@ func (self *PatchCommands) MovePatchToSelectedCommit(commits []*models.Commit, s
211211
return err
212212
}
213213

214+
if err := self.pickCopyWithPatchInParent(commits[sourceCommitIdx]); err != nil {
215+
_ = self.rebase.AbortRebase()
216+
return err
217+
}
218+
214219
// apply each patch forward
215220
if err := self.ApplyCustomPatch(false, false); err != nil {
216221
// Don't abort the rebase here; this might cause conflicts, so give
@@ -414,6 +419,11 @@ func (self *PatchCommands) PullPatchIntoNewCommitBefore(
414419
return err
415420
}
416421

422+
if err := self.pickCopyWithPatchInParent(commits[commitIdx]); err != nil {
423+
_ = self.rebase.AbortRebase()
424+
return err
425+
}
426+
417427
if err := self.ApplyCustomPatch(false, false); err != nil {
418428
_ = self.rebase.AbortRebase()
419429
return err
@@ -427,6 +437,110 @@ func (self *PatchCommands) PullPatchIntoNewCommitBefore(
427437
return self.rebase.ContinueRebase()
428438
}
429439

440+
// When moving the patch to an earlier commit, or into a new commit before the
441+
// one it was built from, we first apply it there, and the rebase then picks
442+
// the commit that the patch was built from. Picking that commit unchanged
443+
// would rely on git noticing that the patch is already applied, and git
444+
// doesn't always notice this. If the patch contains only some of several
445+
// adjacent added lines, the pick conflicts; if git can place the patch's added
446+
// lines in more than one position, it may add them a second time. So we make
447+
// the rebase pick a copy of the commit instead, whose parent already contains
448+
// the patch.
449+
func (self *PatchCommands) pickCopyWithPatchInParent(commit *models.Commit) error {
450+
copyHash, err := self.copyCommitWithPatchInParent(commit)
451+
if err != nil {
452+
return err
453+
}
454+
455+
return self.rebase.ChangeTodoCommit(commit.Hash(), copyHash)
456+
}
457+
458+
// Creates a copy of the given commit, with the same tree, author, and message,
459+
// whose parent is the commit's original parent with the custom patch applied.
460+
// It does this without touching the working tree or the index.
461+
func (self *PatchCommands) copyCommitWithPatchInParent(commit *models.Commit) (string, error) {
462+
indexDir, err := os.MkdirTemp(self.os.GetTempDir(), "index")
463+
if err != nil {
464+
return "", err
465+
}
466+
defer os.RemoveAll(indexDir)
467+
indexFileEnvVar := "GIT_INDEX_FILE=" + filepath.Join(indexDir, "index")
468+
469+
cmd := self.cmd.New(NewGitCmd("read-tree").Arg(commit.Parents()[0]).ToArgv())
470+
err = cmd.AddEnvVars(indexFileEnvVar).DontLog().Run()
471+
if err != nil {
472+
return "", err
473+
}
474+
475+
patchFilePath, err := self.SaveTemporaryPatch(self.PatchBuilder.PatchToApply(false, false))
476+
if err != nil {
477+
return "", err
478+
}
479+
cmd = self.cmd.New(NewGitCmd("apply").Arg("--cached", patchFilePath).ToArgv())
480+
err = cmd.AddEnvVars(indexFileEnvVar).DontLog().Run()
481+
if err != nil {
482+
return "", err
483+
}
484+
485+
cmd = self.cmd.New(NewGitCmd("write-tree").ToArgv())
486+
tree, _, err := cmd.AddEnvVars(indexFileEnvVar).DontLog().RunWithOutputs()
487+
if err != nil {
488+
return "", err
489+
}
490+
491+
cmd = self.cmd.New(NewGitCmd("cat-file").Arg("commit", commit.Hash()).ToArgv())
492+
commitObject, _, err := cmd.DontLog().RunWithOutputs()
493+
if err != nil {
494+
return "", err
495+
}
496+
497+
parentHash, err := self.writeCommitObject(
498+
commitObjectWithHeader(commitObject, "tree", strings.TrimSpace(tree)))
499+
if err != nil {
500+
return "", err
501+
}
502+
503+
return self.writeCommitObject(commitObjectWithHeader(commitObject, "parent", parentHash))
504+
}
505+
506+
func (self *PatchCommands) writeCommitObject(commitObject string) (string, error) {
507+
cmd := self.cmd.New(
508+
NewGitCmd("hash-object").
509+
Arg("-t", "commit", "-w", "--stdin").ToArgv())
510+
hash, _, err := cmd.
511+
SetStdin(commitObject).
512+
DontLog().
513+
RunWithOutputs()
514+
return strings.TrimSpace(hash), err
515+
}
516+
517+
// Returns the given raw commit object with the value of the given header
518+
// replaced. The commit's signature is dropped, since it no longer matches.
519+
func commitObjectWithHeader(commitObject string, name string, value string) string {
520+
headers, message, _ := strings.Cut(commitObject, "\n\n")
521+
522+
newHeaders := []string{}
523+
isInSignature := false
524+
for line := range strings.SplitSeq(headers, "\n") {
525+
// Continuation lines of a multi-line header start with a space
526+
if isInSignature && strings.HasPrefix(line, " ") {
527+
continue
528+
}
529+
530+
isInSignature = strings.HasPrefix(line, "gpgsig ") || strings.HasPrefix(line, "gpgsig-sha256 ")
531+
if isInSignature {
532+
continue
533+
}
534+
535+
if strings.HasPrefix(line, name+" ") {
536+
line = name + " " + value
537+
}
538+
newHeaders = append(newHeaders, line)
539+
}
540+
541+
return strings.Join(newHeaders, "\n") + "\n\n" + message
542+
}
543+
430544
// We have just applied a patch in reverse to discard it from a commit; if we
431545
// now try to apply the patch again to move it to a later commit, or to the
432546
// index, then this would conflict "with itself" in case the patch contained

‎pkg/commands/git_commands/patch_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package git_commands
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
func TestCommitObjectWithHeader(t *testing.T) {
10+
scenarios := []struct {
11+
name string
12+
commitObject string
13+
headerName string
14+
value string
15+
expected string
16+
}{
17+
{
18+
name: "replace parent",
19+
commitObject: "tree 1111\nparent 2222\n" +
20+
"author A U Thor <author@example.com> 1700000000 +0100\n" +
21+
"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
22+
"\nsubject\n\nbody\n",
23+
headerName: "parent",
24+
value: "3333",
25+
expected: "tree 1111\nparent 3333\n" +
26+
"author A U Thor <author@example.com> 1700000000 +0100\n" +
27+
"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
28+
"\nsubject\n\nbody\n",
29+
},
30+
{
31+
name: "replace tree, keeping other headers",
32+
commitObject: "tree 1111\nparent 2222\n" +
33+
"author A U Thor <author@example.com> 1700000000 +0100\n" +
34+
"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
35+
"encoding ISO-8859-1\n" +
36+
"\nsubject\n",
37+
headerName: "tree",
38+
value: "3333",
39+
expected: "tree 3333\nparent 2222\n" +
40+
"author A U Thor <author@example.com> 1700000000 +0100\n" +
41+
"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
42+
"encoding ISO-8859-1\n" +
43+
"\nsubject\n",
44+
},
45+
{
46+
name: "drop signature",
47+
commitObject: "tree 1111\nparent 2222\n" +
48+
"author A U Thor <author@example.com> 1700000000 +0100\n" +
49+
"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
50+
"gpgsig -----BEGIN PGP SIGNATURE-----\n \n iQEz\n -----END PGP SIGNATURE-----\n" +
51+
"gpgsig-sha256 -----BEGIN SSH SIGNATURE-----\n U1NI\n -----END SSH SIGNATURE-----\n" +
52+
"some-other-header value\n" +
53+
"\nsubject\n",
54+
headerName: "parent",
55+
value: "3333",
56+
expected: "tree 1111\nparent 3333\n" +
57+
"author A U Thor <author@example.com> 1700000000 +0100\n" +
58+
"committer C O Mitter <committer@example.com> 1700000001 +0100\n" +
59+
"some-other-header value\n" +
60+
"\nsubject\n",
61+
},
62+
{
63+
name: "empty message",
64+
commitObject: "tree 1111\nparent 2222\n\n",
65+
headerName: "parent",
66+
value: "3333",
67+
expected: "tree 1111\nparent 3333\n\n",
68+
},
69+
}
70+
71+
for _, s := range scenarios {
72+
t.Run(s.name, func(t *testing.T) {
73+
assert.Equal(t, s.expected, commitObjectWithHeader(s.commitObject, s.headerName, s.value))
74+
})
75+
}
76+
}

‎pkg/commands/git_commands/rebase.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,17 @@ func (self *RebaseCommands) EditRebaseTodo(commits []*models.Commit, action todo
352352
)
353353
}
354354

355+
// Makes the todo for the given commit in the git-rebase-todo file use a
356+
// different commit instead
357+
func (self *RebaseCommands) ChangeTodoCommit(hash string, newHash string) error {
358+
return utils.ChangeTodoCommit(
359+
filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/git-rebase-todo"),
360+
hash,
361+
newHash,
362+
self.config.GetCoreCommentChar(),
363+
)
364+
}
365+
355366
func (self *RebaseCommands) DeleteUpdateRefTodos(commits []*models.Commit) error {
356367
todosToDelete := lo.Map(commits, func(commit *models.Commit, _ int) utils.Todo {
357368
return todoFromCommit(commit)

‎pkg/gui/controllers/custom_patch_options_menu_action.go‎

Lines changed: 37 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,11 @@ func (self *CustomPatchOptionsMenuAction) Call() error {
4848
}
4949

5050
if self.c.Git().Patch.PatchBuilder.CanRebase && self.c.Git().Status.WorkingTreeState().None() {
51+
var disabledReasonForMergeCommit *types.DisabledReason
52+
if self.isPatchFromMergeCommit() {
53+
disabledReasonForMergeCommit = &types.DisabledReason{Text: self.c.Tr.NotSupportedForPatchFromMergeCommit}
54+
}
55+
5156
menuItems = append(menuItems, []*types.MenuItem{
5257
{
5358
Label: fmt.Sprintf(self.c.Tr.RemovePatchFromOriginalCommit, utils.ShortHash(self.c.Git().Patch.PatchBuilder.To)),
@@ -68,18 +73,22 @@ func (self *CustomPatchOptionsMenuAction) Call() error {
6873
Keys: menuKey('n'),
6974
},
7075
{
71-
Label: self.c.Tr.MovePatchIntoNewCommitBefore,
72-
Tooltip: self.c.Tr.MovePatchIntoNewCommitBeforeTooltip,
73-
OnPress: self.handlePullPatchIntoNewCommitBefore,
74-
Keys: menuKey('N'),
76+
Label: self.c.Tr.MovePatchIntoNewCommitBefore,
77+
Tooltip: self.c.Tr.MovePatchIntoNewCommitBeforeTooltip,
78+
OnPress: self.handlePullPatchIntoNewCommitBefore,
79+
Keys: menuKey('N'),
80+
DisabledReason: disabledReasonForMergeCommit,
7581
},
7682
}...)
7783

7884
if self.c.Context().Current().GetKey() == self.c.Contexts().LocalCommits.GetKey() {
7985
selectedCommit := self.c.Contexts().LocalCommits.GetSelected()
8086
if selectedCommit != nil && self.c.Git().Patch.PatchBuilder.To != selectedCommit.Hash() {
8187

82-
var disabledReason *types.DisabledReason
88+
disabledReason := disabledReasonForMergeCommit
89+
if selectedCommit.IsMerge() {
90+
disabledReason = &types.DisabledReason{Text: self.c.Tr.CannotMovePatchToMergeCommit}
91+
}
8392
if self.c.Contexts().LocalCommits.AreMultipleItemsSelected() {
8493
disabledReason = &types.DisabledReason{Text: self.c.Tr.RangeSelectNotSupported}
8594
}
@@ -123,6 +132,11 @@ func (self *CustomPatchOptionsMenuAction) getPatchCommitIndex() int {
123132
return -1
124133
}
125134

135+
func (self *CustomPatchOptionsMenuAction) isPatchFromMergeCommit() bool {
136+
commitIndex := self.getPatchCommitIndex()
137+
return commitIndex != -1 && self.c.Model().Commits[commitIndex].IsMerge()
138+
}
139+
126140
func (self *CustomPatchOptionsMenuAction) handleDeletePatchFromCommit() error {
127141
commits := self.c.Model().Commits
128142
commitIndex := self.getPatchCommitIndex()
@@ -162,39 +176,14 @@ func (self *CustomPatchOptionsMenuAction) handleMovePatchIntoWorkingTree() error
162176
}
163177

164178
func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommit() error {
165-
commitIndex := self.getPatchCommitIndex()
166-
self.c.Helpers().Commits.OpenCommitMessagePanel(
167-
&helpers.OpenCommitMessagePanelOpts{
168-
// Pass a commit index of one less than the moved-from commit, so that
169-
// you can press up arrow once to recall the original commit message:
170-
CommitIndex: commitIndex - 1,
171-
InitialMessage: "",
172-
SummaryTitle: self.c.Tr.CommitSummaryTitle,
173-
DescriptionTitle: self.c.Tr.CommitDescriptionTitle,
174-
PreserveMessage: false,
175-
OnConfirm: func(summary string, description string) error {
176-
commits := self.c.Model().Commits
177-
self.c.Helpers().Commits.CloseCommitMessagePanel()
178-
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
179-
self.c.LogAction(self.c.Tr.Actions.MovePatchIntoNewCommit)
180-
err := self.c.Git().Patch.PullPatchIntoNewCommit(commits, commitIndex, summary, description)
181-
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
182-
return err
183-
}
184-
self.c.OnUIThread(func() error {
185-
self.c.Context().Push(self.c.Contexts().LocalCommits, types.OnFocusOpts{})
186-
return nil
187-
})
188-
return nil
189-
})
190-
},
191-
},
192-
)
193-
194-
return nil
179+
return self.pullPatchIntoNewCommit(false)
195180
}
196181

197182
func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommitBefore() error {
183+
return self.pullPatchIntoNewCommit(true)
184+
}
185+
186+
func (self *CustomPatchOptionsMenuAction) pullPatchIntoNewCommit(beforeOriginalCommit bool) error {
198187
commitIndex := self.getPatchCommitIndex()
199188
self.c.Helpers().Commits.OpenCommitMessagePanel(
200189
&helpers.OpenCommitMessagePanelOpts{
@@ -210,10 +199,21 @@ func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommitBefore() e
210199
self.c.Helpers().Commits.CloseCommitMessagePanel()
211200
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
212201
self.c.LogAction(self.c.Tr.Actions.MovePatchIntoNewCommit)
213-
err := self.c.Git().Patch.PullPatchIntoNewCommitBefore(commits, commitIndex, summary, description)
214-
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
202+
var result error
203+
if beforeOriginalCommit {
204+
result = self.c.Git().Patch.PullPatchIntoNewCommitBefore(commits, commitIndex, summary, description)
205+
} else {
206+
result = self.c.Git().Patch.PullPatchIntoNewCommit(commits, commitIndex, summary, description)
207+
}
208+
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(result); err != nil {
215209
return err
216210
}
211+
// If the rebase stopped at conflicts, CheckMergeOrRebase has asked the
212+
// user how to handle them; focusing the commits view would close that
213+
// popup.
214+
if result != nil {
215+
return nil
216+
}
217217
self.c.OnUIThread(func() error {
218218
self.c.Context().Push(self.c.Contexts().LocalCommits, types.OnFocusOpts{})
219219
return nil

‎pkg/i18n/english.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,6 +1013,8 @@ type TranslationSet struct {
10131013
RangeSelectUp string
10141014
RangeSelectDown string
10151015
RangeSelectNotSupported string
1016+
NotSupportedForPatchFromMergeCommit string
1017+
CannotMovePatchToMergeCommit string
10161018
NoItemSelected string
10171019
SelectedItemIsNotABranch string
10181020
SelectedItemDoesNotHaveFiles string
@@ -2230,6 +2232,8 @@ func EnglishTranslationSet() *TranslationSet {
22302232
RangeSelectUp: "Range select up",
22312233
RangeSelectDown: "Range select down",
22322234
RangeSelectNotSupported: "Action does not support range selection, please select a single item",
2235+
NotSupportedForPatchFromMergeCommit: "Not supported for a patch from a merge commit",
2236+
CannotMovePatchToMergeCommit: "Cannot move a patch to a merge commit",
22332237
NoItemSelected: "No item selected",
22342238
SelectedItemIsNotABranch: "Selected item is not a branch",
22352239
SelectedItemDoesNotHaveFiles: "Selected item does not have files to view",

0 commit comments

Comments
 (0)