From 5a50bfd1792f1518c3442ebfec836ce95b785683 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 26 Feb 2023 09:53:02 +0100 Subject: [PATCH 01/13] Fix opening the current test file from the integration test gui --- pkg/integration/clients/tui.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/integration/clients/tui.go b/pkg/integration/clients/tui.go index f93f4589a..6778bc012 100644 --- a/pkg/integration/clients/tui.go +++ b/pkg/integration/clients/tui.go @@ -121,7 +121,7 @@ func RunTUI() { return nil } - cmd := secureexec.Command("sh", "-c", fmt.Sprintf("code -r pkg/integration/tests/%s", currentTest.Name())) + cmd := secureexec.Command("sh", "-c", fmt.Sprintf("code -r pkg/integration/tests/%s.go", currentTest.Name())) if err := cmd.Run(); err != nil { return err } From f76cc2795601a14ce8e622a28f1d9f94028a3d06 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 26 Feb 2023 13:20:10 +0100 Subject: [PATCH 02/13] Bundle the reverse and keepOriginalHeader flags into a PatchOptions struct We are going to add one more flag in the next commit. Note that we are not using the struct inside patch_manager.go; we keep passing the individual flags there. The reason for this will become more obvious later in this branch. --- pkg/commands/patch/patch_manager.go | 3 ++- pkg/commands/patch/patch_modifier.go | 28 +++++++++++++++-------- pkg/commands/patch/patch_modifier_test.go | 3 ++- pkg/gui/controllers/staging_controller.go | 9 +++++--- 4 files changed, 29 insertions(+), 14 deletions(-) diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index 91adfecb4..71d6116d8 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -176,7 +176,8 @@ func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool, ke return info.diff case PART: // generate a new diff with just the selected lines - return ModifiedPatchForLines(p.Log, filename, info.diff, info.includedLineIndices, reverse, keepOriginalHeader) + return ModifiedPatchForLines(p.Log, filename, info.diff, info.includedLineIndices, + PatchOptions{Reverse: reverse, KeepOriginalHeader: keepOriginalHeader}) default: return "" } diff --git a/pkg/commands/patch/patch_modifier.go b/pkg/commands/patch/patch_modifier.go index fe0a896b1..5d9da3b60 100644 --- a/pkg/commands/patch/patch_modifier.go +++ b/pkg/commands/patch/patch_modifier.go @@ -13,6 +13,16 @@ var ( patchHeaderRegexp = regexp.MustCompile(`(?ms)(^diff.*?)^@@`) ) +type PatchOptions struct { + // Create a reverse patch; in other words, flip all the '+' and '-' while + // generating the patch. + Reverse bool + + // Whether to keep or discard the original diff header including the + // "index deadbeef..fa1afe1 100644" line. + KeepOriginalHeader bool +} + func GetHeaderFromDiff(diff string) string { match := patchHeaderRegexp.FindStringSubmatch(diff) if len(match) <= 1 { @@ -76,7 +86,7 @@ func NewPatchModifier(log *logrus.Entry, filename string, diffText string) *Patc } } -func (d *PatchModifier) ModifiedPatchForLines(lineIndices []int, reverse bool, keepOriginalHeader bool) string { +func (d *PatchModifier) ModifiedPatchForLines(lineIndices []int, opts PatchOptions) string { // step one is getting only those hunks which we care about hunksInRange := []*PatchHunk{} outer: @@ -95,7 +105,7 @@ outer: formattedHunks := "" var formattedHunk string for _, hunk := range hunksInRange { - startOffset, formattedHunk = hunk.formatWithChanges(lineIndices, reverse, startOffset) + startOffset, formattedHunk = hunk.formatWithChanges(lineIndices, opts.Reverse, startOffset) formattedHunks += formattedHunk } @@ -108,7 +118,7 @@ outer: // it makes git confused e.g. when dealing with deleted/added files // but with building and applying patches the original header gives git // information it needs to cleanly apply patches - if keepOriginalHeader { + if opts.KeepOriginalHeader { fileHeader = d.header } else { fileHeader = fmt.Sprintf("--- a/%s\n+++ b/%s\n", d.filename, d.filename) @@ -117,13 +127,13 @@ outer: return fileHeader + formattedHunks } -func (d *PatchModifier) ModifiedPatchForRange(firstLineIdx int, lastLineIdx int, reverse bool, keepOriginalHeader bool) string { +func (d *PatchModifier) ModifiedPatchForRange(firstLineIdx int, lastLineIdx int, opts PatchOptions) string { // generate array of consecutive line indices from our range selectedLines := []int{} for i := firstLineIdx; i <= lastLineIdx; i++ { selectedLines = append(selectedLines, i) } - return d.ModifiedPatchForLines(selectedLines, reverse, keepOriginalHeader) + return d.ModifiedPatchForLines(selectedLines, opts) } func (d *PatchModifier) OriginalPatchLength() int { @@ -134,14 +144,14 @@ func (d *PatchModifier) OriginalPatchLength() int { return d.hunks[len(d.hunks)-1].LastLineIdx() } -func ModifiedPatchForRange(log *logrus.Entry, filename string, diffText string, firstLineIdx int, lastLineIdx int, reverse bool, keepOriginalHeader bool) string { +func ModifiedPatchForRange(log *logrus.Entry, filename string, diffText string, firstLineIdx int, lastLineIdx int, opts PatchOptions) string { p := NewPatchModifier(log, filename, diffText) - return p.ModifiedPatchForRange(firstLineIdx, lastLineIdx, reverse, keepOriginalHeader) + return p.ModifiedPatchForRange(firstLineIdx, lastLineIdx, opts) } -func ModifiedPatchForLines(log *logrus.Entry, filename string, diffText string, includedLineIndices []int, reverse bool, keepOriginalHeader bool) string { +func ModifiedPatchForLines(log *logrus.Entry, filename string, diffText string, includedLineIndices []int, opts PatchOptions) string { p := NewPatchModifier(log, filename, diffText) - return p.ModifiedPatchForLines(includedLineIndices, reverse, keepOriginalHeader) + return p.ModifiedPatchForLines(includedLineIndices, opts) } // I want to know, given a hunk, what line a given index is on diff --git a/pkg/commands/patch/patch_modifier_test.go b/pkg/commands/patch/patch_modifier_test.go index ec79cbe32..618473d4b 100644 --- a/pkg/commands/patch/patch_modifier_test.go +++ b/pkg/commands/patch/patch_modifier_test.go @@ -513,7 +513,8 @@ func TestModifyPatchForRange(t *testing.T) { for _, s := range scenarios { s := s t.Run(s.testName, func(t *testing.T) { - result := ModifiedPatchForRange(nil, s.filename, s.diffText, s.firstLineIndex, s.lastLineIndex, s.reverse, false) + result := ModifiedPatchForRange(nil, s.filename, s.diffText, s.firstLineIndex, s.lastLineIndex, + PatchOptions{Reverse: s.reverse, KeepOriginalHeader: false}) if !assert.Equal(t, s.expected, result) { fmt.Println(result) } diff --git a/pkg/gui/controllers/staging_controller.go b/pkg/gui/controllers/staging_controller.go index 0cddfb841..840c18a18 100644 --- a/pkg/gui/controllers/staging_controller.go +++ b/pkg/gui/controllers/staging_controller.go @@ -181,7 +181,8 @@ func (self *StagingController) applySelection(reverse bool) error { } firstLineIdx, lastLineIdx := state.SelectedRange() - patch := patch.ModifiedPatchForRange(self.c.Log, path, state.GetDiff(), firstLineIdx, lastLineIdx, reverse, false) + patch := patch.ModifiedPatchForRange(self.c.Log, path, state.GetDiff(), firstLineIdx, lastLineIdx, + patch.PatchOptions{Reverse: reverse, KeepOriginalHeader: false}) if patch == "" { return nil @@ -227,7 +228,8 @@ func (self *StagingController) editHunk() error { hunk := state.CurrentHunk() patchText := patch.ModifiedPatchForRange( - self.c.Log, path, state.GetDiff(), hunk.FirstLineIdx, hunk.LastLineIdx(), self.staged, false, + self.c.Log, path, state.GetDiff(), hunk.FirstLineIdx, hunk.LastLineIdx(), + patch.PatchOptions{Reverse: self.staged, KeepOriginalHeader: false}, ) patchFilepath, err := self.git.WorkingTree.SaveTemporaryPatch(patchText) if err != nil { @@ -249,7 +251,8 @@ func (self *StagingController) editHunk() error { lineCount := strings.Count(editedPatchText, "\n") + 1 newPatchText := patch.ModifiedPatchForRange( - self.c.Log, path, editedPatchText, 0, lineCount, false, false, + self.c.Log, path, editedPatchText, 0, lineCount, + patch.PatchOptions{Reverse: false, KeepOriginalHeader: false}, ) if err := self.git.WorkingTree.ApplyPatch(newPatchText, "cached"); err != nil { return self.c.Error(err) From c79e3605840cf5c48c2008113ec006ad671f8c24 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 26 Feb 2023 13:48:10 +0100 Subject: [PATCH 03/13] Add patch option WillBeAppliedReverse It's not used yet, but covered with tests already. --- pkg/commands/patch/hunk.go | 16 +++-- pkg/commands/patch/patch_modifier.go | 12 +++- pkg/commands/patch/patch_modifier_test.go | 73 ++++++++++++++++++++--- 3 files changed, 86 insertions(+), 15 deletions(-) diff --git a/pkg/commands/patch/hunk.go b/pkg/commands/patch/hunk.go index 98d932126..a2727f2c9 100644 --- a/pkg/commands/patch/hunk.go +++ b/pkg/commands/patch/hunk.go @@ -45,7 +45,7 @@ func headerInfo(header string) (int, int, string) { return oldStart, newStart, heading } -func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool) []string { +func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool, willBeAppliedReverse bool) []string { skippedNewlineMessageIndex := -1 newLines := []string{} @@ -58,7 +58,7 @@ func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool) []string { isLineSelected := lo.Contains(lineIndices, lineIdx) firstChar, content := line[:1], line[1:] - transformedFirstChar := transformedFirstChar(firstChar, reverse, isLineSelected) + transformedFirstChar := transformedFirstChar(firstChar, reverse, willBeAppliedReverse, isLineSelected) if isLineSelected || (transformedFirstChar == "\\" && skippedNewlineMessageIndex != lineIdx) || transformedFirstChar == " " { newLines = append(newLines, transformedFirstChar+content) @@ -74,7 +74,7 @@ func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool) []string { return newLines } -func transformedFirstChar(firstChar string, reverse bool, isLineSelected bool) string { +func transformedFirstChar(firstChar string, reverse bool, willBeAppliedReverse bool, isLineSelected bool) string { if reverse { if !isLineSelected && firstChar == "+" { return " " @@ -87,7 +87,11 @@ func transformedFirstChar(firstChar string, reverse bool, isLineSelected bool) s } } - if !isLineSelected && firstChar == "-" { + linesToKeepInPatchContext := "-" + if willBeAppliedReverse { + linesToKeepInPatchContext = "+" + } + if !isLineSelected && firstChar == linesToKeepInPatchContext { return " " } @@ -98,8 +102,8 @@ func (hunk *PatchHunk) formatHeader(oldStart int, oldLength int, newStart int, n return fmt.Sprintf("@@ -%d,%d +%d,%d @@%s\n", oldStart, oldLength, newStart, newLength, heading) } -func (hunk *PatchHunk) formatWithChanges(lineIndices []int, reverse bool, startOffset int) (int, string) { - bodyLines := hunk.updatedLines(lineIndices, reverse) +func (hunk *PatchHunk) formatWithChanges(lineIndices []int, reverse bool, willBeAppliedReverse bool, startOffset int) (int, string) { + bodyLines := hunk.updatedLines(lineIndices, reverse, willBeAppliedReverse) startOffset, header, ok := hunk.updatedHeader(bodyLines, startOffset, reverse) if !ok { return startOffset, "" diff --git a/pkg/commands/patch/patch_modifier.go b/pkg/commands/patch/patch_modifier.go index 5d9da3b60..fa20c7917 100644 --- a/pkg/commands/patch/patch_modifier.go +++ b/pkg/commands/patch/patch_modifier.go @@ -18,6 +18,15 @@ type PatchOptions struct { // generating the patch. Reverse bool + // If true, we're building a patch that we are going to apply using + // "git apply --reverse". In other words, we are not flipping the '+' and + // '-' ourselves while creating the patch, but git is going to do that when + // applying. This has consequences for which lines we need to keep or + // discard when filtering lines from partial hunks. + // + // Currently incompatible with Reverse. + WillBeAppliedReverse bool + // Whether to keep or discard the original diff header including the // "index deadbeef..fa1afe1 100644" line. KeepOriginalHeader bool @@ -105,7 +114,8 @@ outer: formattedHunks := "" var formattedHunk string for _, hunk := range hunksInRange { - startOffset, formattedHunk = hunk.formatWithChanges(lineIndices, opts.Reverse, startOffset) + startOffset, formattedHunk = hunk.formatWithChanges( + lineIndices, opts.Reverse, opts.WillBeAppliedReverse, startOffset) formattedHunks += formattedHunk } diff --git a/pkg/commands/patch/patch_modifier_test.go b/pkg/commands/patch/patch_modifier_test.go index 618473d4b..c97fd89be 100644 --- a/pkg/commands/patch/patch_modifier_test.go +++ b/pkg/commands/patch/patch_modifier_test.go @@ -69,6 +69,20 @@ index e48a11c..b2ab81b 100644 ... ` +const twoChangesInOneHunk = `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape ++kiwi + orange +-pear ++banana + lemon +` + const newFile = `diff --git a/newfile b/newfile new file mode 100644 index 0000000..4e680cc @@ -101,13 +115,14 @@ const exampleHunk = `@@ -1,5 +1,5 @@ // TestModifyPatchForRange is a function. func TestModifyPatchForRange(t *testing.T) { type scenario struct { - testName string - filename string - diffText string - firstLineIndex int - lastLineIndex int - reverse bool - expected string + testName string + filename string + diffText string + firstLineIndex int + lastLineIndex int + reverse bool + willBeAppliedReverse bool + expected string } scenarios := []scenario{ @@ -506,6 +521,44 @@ func TestModifyPatchForRange(t *testing.T) { @@ -1,1 +0,0 @@ -new line \ No newline at end of file +`, + }, + { + testName: "adding part of a hunk", + filename: "filename", + firstLineIndex: 6, + lastLineIndex: 7, + reverse: false, + willBeAppliedReverse: false, + diffText: twoChangesInOneHunk, + expected: `--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape ++kiwi + orange + pear + lemon +`, + }, + { + testName: "adding part of a hunk, will-be-applied-reverse", + filename: "filename", + firstLineIndex: 6, + lastLineIndex: 7, + reverse: false, + willBeAppliedReverse: true, + diffText: twoChangesInOneHunk, + expected: `--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape ++kiwi + orange + banana + lemon `, }, } @@ -514,7 +567,11 @@ func TestModifyPatchForRange(t *testing.T) { s := s t.Run(s.testName, func(t *testing.T) { result := ModifiedPatchForRange(nil, s.filename, s.diffText, s.firstLineIndex, s.lastLineIndex, - PatchOptions{Reverse: s.reverse, KeepOriginalHeader: false}) + PatchOptions{ + Reverse: s.reverse, + WillBeAppliedReverse: s.willBeAppliedReverse, + KeepOriginalHeader: false, + }) if !assert.Equal(t, s.expected, result) { fmt.Println(result) } From 9cc33c479b6da6ef0db46ebf5a75e8b06c39310e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 23 Feb 2023 18:44:52 +0100 Subject: [PATCH 04/13] Use forward patches and --reverse flag for partial patches too There's no reason to have two different ways of applying patches for whole-file patches and partial patches; use --reverse for both. Not only does this simplify the code a bit, but it fixes an actual problem: when reverseOnGenerate and keepOriginalHeader are both true, the generated patch header is broken (the two blobs in the line `index 6d1959b..6dc5f84 100644` are swapped). Git fails to do a proper three-way merge in that case, as it expects the first of the two blobs to be the common ancestor. It would be possible to fix this by extending ModifiedPatchForLines to swap the two blobs in this case; but this would prevent us from concatenating all patches and apply them in one go, which we are going to do later in the branch. --- pkg/commands/patch/patch_manager.go | 33 +++++++++++++--------------- pkg/commands/patch/patch_modifier.go | 4 ++++ pkg/gui/refresh.go | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index 71d6116d8..a4b61dcd6 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -162,7 +162,7 @@ func (p *PatchManager) RemoveFileLineRange(filename string, firstLineIdx, lastLi return nil } -func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool, keepOriginalHeader bool) string { +func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool, willBeAppliedReverse bool, keepOriginalHeader bool) string { info, err := p.getFileInfo(filename) if err != nil { p.Log.Error(err) @@ -177,14 +177,18 @@ func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool, ke case PART: // generate a new diff with just the selected lines return ModifiedPatchForLines(p.Log, filename, info.diff, info.includedLineIndices, - PatchOptions{Reverse: reverse, KeepOriginalHeader: keepOriginalHeader}) + PatchOptions{ + Reverse: reverse, + WillBeAppliedReverse: willBeAppliedReverse, + KeepOriginalHeader: keepOriginalHeader, + }) default: return "" } } -func (p *PatchManager) RenderPatchForFile(filename string, plain bool, reverse bool, keepOriginalHeader bool) string { - patch := p.renderPlainPatchForFile(filename, reverse, keepOriginalHeader) +func (p *PatchManager) RenderPatchForFile(filename string, plain bool, reverse bool, willBeAppliedReverse bool, keepOriginalHeader bool) string { + patch := p.renderPlainPatchForFile(filename, reverse, willBeAppliedReverse, keepOriginalHeader) if plain { return patch } @@ -200,7 +204,7 @@ func (p *PatchManager) renderEachFilePatch(plain bool) []string { sort.Strings(filenames) patches := slices.Map(filenames, func(filename string) string { - return p.RenderPatchForFile(filename, plain, false, true) + return p.RenderPatchForFile(filename, plain, false, false, true) }) output := slices.Filter(patches, func(patch string) bool { return patch != "" @@ -241,27 +245,20 @@ func (p *PatchManager) GetFileIncLineIndices(filename string) ([]int, error) { } func (p *PatchManager) ApplyPatches(reverse bool) error { - // for whole patches we'll apply the patch in reverse - // but for part patches we'll apply a reverse patch forwards + applyFlags := []string{"index", "3way"} + if reverse { + applyFlags = append(applyFlags, "reverse") + } + for filename, info := range p.fileInfoMap { if info.mode == UNSELECTED { continue } - applyFlags := []string{"index", "3way"} - reverseOnGenerate := false - if reverse { - if info.mode == WHOLE { - applyFlags = append(applyFlags, "reverse") - } else { - reverseOnGenerate = true - } - } - var err error // first run we try with the original header, then without for _, keepOriginalHeader := range []bool{true, false} { - patch := p.RenderPatchForFile(filename, true, reverseOnGenerate, keepOriginalHeader) + patch := p.RenderPatchForFile(filename, true, false, reverse, keepOriginalHeader) if patch == "" { continue } diff --git a/pkg/commands/patch/patch_modifier.go b/pkg/commands/patch/patch_modifier.go index fa20c7917..985743b76 100644 --- a/pkg/commands/patch/patch_modifier.go +++ b/pkg/commands/patch/patch_modifier.go @@ -96,6 +96,10 @@ func NewPatchModifier(log *logrus.Entry, filename string, diffText string) *Patc } func (d *PatchModifier) ModifiedPatchForLines(lineIndices []int, opts PatchOptions) string { + if opts.Reverse && opts.KeepOriginalHeader { + panic("reverse and keepOriginalHeader are not compatible") + } + // step one is getting only those hunks which we care about hunksInRange := []*PatchHunk{} outer: diff --git a/pkg/gui/refresh.go b/pkg/gui/refresh.go index 9ee5ea0bd..60ca0accb 100644 --- a/pkg/gui/refresh.go +++ b/pkg/gui/refresh.go @@ -672,7 +672,7 @@ func (gui *Gui) refreshPatchBuildingPanel(opts types.OnFocusOpts) error { return err } - secondaryDiff := gui.git.Patch.PatchManager.RenderPatchForFile(path, false, false, true) + secondaryDiff := gui.git.Patch.PatchManager.RenderPatchForFile(path, false, false, false, true) if err != nil { return err } From 5d692e89610166fc0c4458de8b4f5acabc6e4b6f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 23 Feb 2023 18:45:38 +0100 Subject: [PATCH 05/13] Remove the keepOriginalHeader retry loop The loop is pointless for two reasons: - git apply --3way has this fallback built in already. If it can't do a three-way merge, it will fall back to applying the patch normally. - However, the only situation where it does this is when it can't do a 3-way merge at all because it can't find the necessary ancestor blob. This can only happen if you transfer a patch between different repos that don't have the same blobs available; we are applying the patch to the same repo that is was just generated from, so a 3-way merge is always possible. (Now that we fixed the bug in the previous commit, that is.) But the retry loop is not only pointless, it was actually harmful, because when a 3-way patch fails with a conflict, git will put conflict markers in the patched file and then exit with a non-zero exit status. So the retry loop would try to patch the already patched file again, and this almost certainly fails, but with a cryptic error message such as "error: main.go: does not exist in index". --- pkg/commands/patch/patch_manager.go | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index a4b61dcd6..7e1721b7f 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -255,21 +255,12 @@ func (p *PatchManager) ApplyPatches(reverse bool) error { continue } - var err error - // first run we try with the original header, then without - for _, keepOriginalHeader := range []bool{true, false} { - patch := p.RenderPatchForFile(filename, true, false, reverse, keepOriginalHeader) - if patch == "" { - continue + patch := p.RenderPatchForFile(filename, true, false, reverse, true) + if patch != "" { + err := p.applyPatch(patch, applyFlags...) + if err != nil { + return err } - if err = p.applyPatch(patch, applyFlags...); err != nil { - continue - } - break - } - - if err != nil { - return err } } From 6bd1c1d06807fced80ca0bdbc736c2da39ae3dcf Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 23 Feb 2023 18:50:53 +0100 Subject: [PATCH 06/13] Remove parameters that are no longer needed All callers in this file now use reverseOnGenerate=false and keepOriginalHeader=true, so hard-code that in the call to ModifiedPatchForLines and get rid of the parameters. --- pkg/commands/patch/patch_manager.go | 14 +++++++------- pkg/gui/refresh.go | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index 7e1721b7f..e3f6cd454 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -162,7 +162,7 @@ func (p *PatchManager) RemoveFileLineRange(filename string, firstLineIdx, lastLi return nil } -func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool, willBeAppliedReverse bool, keepOriginalHeader bool) string { +func (p *PatchManager) renderPlainPatchForFile(filename string, willBeAppliedReverse bool) string { info, err := p.getFileInfo(filename) if err != nil { p.Log.Error(err) @@ -178,17 +178,17 @@ func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool, wi // generate a new diff with just the selected lines return ModifiedPatchForLines(p.Log, filename, info.diff, info.includedLineIndices, PatchOptions{ - Reverse: reverse, + Reverse: false, WillBeAppliedReverse: willBeAppliedReverse, - KeepOriginalHeader: keepOriginalHeader, + KeepOriginalHeader: true, }) default: return "" } } -func (p *PatchManager) RenderPatchForFile(filename string, plain bool, reverse bool, willBeAppliedReverse bool, keepOriginalHeader bool) string { - patch := p.renderPlainPatchForFile(filename, reverse, willBeAppliedReverse, keepOriginalHeader) +func (p *PatchManager) RenderPatchForFile(filename string, plain bool, willBeAppliedReverse bool) string { + patch := p.renderPlainPatchForFile(filename, willBeAppliedReverse) if plain { return patch } @@ -204,7 +204,7 @@ func (p *PatchManager) renderEachFilePatch(plain bool) []string { sort.Strings(filenames) patches := slices.Map(filenames, func(filename string) string { - return p.RenderPatchForFile(filename, plain, false, false, true) + return p.RenderPatchForFile(filename, plain, false) }) output := slices.Filter(patches, func(patch string) bool { return patch != "" @@ -255,7 +255,7 @@ func (p *PatchManager) ApplyPatches(reverse bool) error { continue } - patch := p.RenderPatchForFile(filename, true, false, reverse, true) + patch := p.RenderPatchForFile(filename, true, reverse) if patch != "" { err := p.applyPatch(patch, applyFlags...) if err != nil { diff --git a/pkg/gui/refresh.go b/pkg/gui/refresh.go index 60ca0accb..0c6315767 100644 --- a/pkg/gui/refresh.go +++ b/pkg/gui/refresh.go @@ -672,7 +672,7 @@ func (gui *Gui) refreshPatchBuildingPanel(opts types.OnFocusOpts) error { return err } - secondaryDiff := gui.git.Patch.PatchManager.RenderPatchForFile(path, false, false, false, true) + secondaryDiff := gui.git.Patch.PatchManager.RenderPatchForFile(path, false, false) if err != nil { return err } From 4ca012dbfbe0c6be33da36f442a4d7d10019fafd Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 25 Feb 2023 21:00:29 +0100 Subject: [PATCH 07/13] Add test for reverse-applying a patch that conflicts The patch contains changes to two files; the first one conflicts, the second doesn't. Note how it only applies changes to the first file at this point in the branch; we'll fix this in the next commit. This test would fail on master for multiple reasons. --- .../apply_in_reverse_with_conflict.go | 91 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 92 insertions(+) create mode 100644 pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go diff --git a/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go b/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go new file mode 100644 index 000000000..8c3e61a86 --- /dev/null +++ b/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go @@ -0,0 +1,91 @@ +package patch_building + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var ApplyInReverseWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Apply a custom patch in reverse, resulting in a conflict", + ExtraCmdArgs: "", + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file1", "file1 content\n") + shell.CreateFileAndAdd("file2", "file2 content\n") + shell.Commit("first commit") + shell.UpdateFileAndAdd("file1", "file1 content\nmore file1 content\n") + shell.UpdateFileAndAdd("file2", "file2 content\nmore file2 content\n") + shell.Commit("second commit") + shell.UpdateFileAndAdd("file1", "file1 content\nmore file1 content\neven more file1\n") + shell.Commit("third commit") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("third commit").IsSelected(), + Contains("second commit"), + Contains("first commit"), + ). + NavigateToLine(Contains("second commit")). + PressEnter() + + t.Views().CommitFiles(). + IsFocused(). + Lines( + Contains("M").Contains("file1").IsSelected(), + Contains("M").Contains("file2"), + ). + // Add both files to the patch; the first will conflict, the second won't + PressPrimaryAction(). + SelectNextItem(). + PressPrimaryAction() + + t.Views().Information().Content(Contains("building patch")) + + t.Views().PatchBuildingSecondary().Content( + Contains("+more file1 content").Contains("+more file2 content")) + + t.Common().SelectPatchOption(Contains("apply patch in reverse")) + + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("Applied patch to 'file1' with conflicts."). + DoesNotContain("Applied patch to 'file2' cleanly.")). + Confirm() + + t.Views().Files(). + Focus(). + Lines( + Contains("UU").Contains("file1").IsSelected(), + ). + PressPrimaryAction() + + t.Views().MergeConflicts(). + IsFocused(). + ContainsLines( + Contains("file1 content"), + Contains("<<<<<<< ours").IsSelected(), + Contains("more file1 content").IsSelected(), + Contains("even more file1").IsSelected(), + Contains("=======").IsSelected(), + Contains(">>>>>>> theirs"), + ). + SelectNextItem(). + PressPrimaryAction() + + t.Views().Files(). + Focus(). + Lines( + Contains("M").Contains("file1").IsSelected(), + ) + + t.Views().Main(). + ContainsLines( + Contains(" file1 content"), + Contains("-more file1 content"), + Contains("-even more file1"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index dd1dfd70f..3a0fa8269 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -99,6 +99,7 @@ var tests = []*components.IntegrationTest{ misc.InitialOpen, patch_building.Apply, patch_building.ApplyInReverse, + patch_building.ApplyInReverseWithConflict, patch_building.CopyPatchToClipboard, patch_building.MoveToIndex, patch_building.MoveToIndexPartial, From a68cd6af9c572a83cbaf511a72b8a89c1d534e0b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 23 Feb 2023 22:51:49 +0100 Subject: [PATCH 08/13] Concatenate patches to apply them all at once This fixes the problem that patching would stop at the first file that has a conflict. We always want to patch all files. Also, it's faster for large patches, and the code is a little bit simpler too. --- pkg/commands/patch/patch_manager.go | 12 ++++-------- .../patch_building/apply_in_reverse_with_conflict.go | 3 ++- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index e3f6cd454..e50c0fd3a 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -245,6 +245,8 @@ func (p *PatchManager) GetFileIncLineIndices(filename string) ([]int, error) { } func (p *PatchManager) ApplyPatches(reverse bool) error { + patch := "" + applyFlags := []string{"index", "3way"} if reverse { applyFlags = append(applyFlags, "reverse") @@ -255,16 +257,10 @@ func (p *PatchManager) ApplyPatches(reverse bool) error { continue } - patch := p.RenderPatchForFile(filename, true, reverse) - if patch != "" { - err := p.applyPatch(patch, applyFlags...) - if err != nil { - return err - } - } + patch += p.RenderPatchForFile(filename, true, reverse) } - return nil + return p.applyPatch(patch, applyFlags...) } // clears the patch diff --git a/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go b/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go index 8c3e61a86..04d160a01 100644 --- a/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go +++ b/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go @@ -52,7 +52,7 @@ var ApplyInReverseWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ t.ExpectPopup().Alert(). Title(Equals("Error")). Content(Contains("Applied patch to 'file1' with conflicts."). - DoesNotContain("Applied patch to 'file2' cleanly.")). + Contains("Applied patch to 'file2' cleanly.")). Confirm() t.Views().Files(). @@ -79,6 +79,7 @@ var ApplyInReverseWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ Focus(). Lines( Contains("M").Contains("file1").IsSelected(), + Contains("M").Contains("file2"), ) t.Views().Main(). From bf6e9a1bd3f7a3084644b3658264631b156d725c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 25 Feb 2023 14:06:00 +0100 Subject: [PATCH 09/13] Reenable failing test --- .../tests/patch_building/move_to_index_with_conflict.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/integration/tests/patch_building/move_to_index_with_conflict.go b/pkg/integration/tests/patch_building/move_to_index_with_conflict.go index bdeb321c4..75ecff9a4 100644 --- a/pkg/integration/tests/patch_building/move_to_index_with_conflict.go +++ b/pkg/integration/tests/patch_building/move_to_index_with_conflict.go @@ -8,7 +8,7 @@ import ( var MoveToIndexWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ Description: "Move a patch from a commit to the index, causing a conflict", ExtraCmdArgs: "", - Skip: true, // Skipping until https://github.com/jesseduffield/lazygit/pull/2471 is merged + Skip: false, SetupConfig: func(config *config.AppConfig) {}, SetupRepo: func(shell *Shell) { shell.CreateFileAndAdd("file1", "file1 content") From e4659145e89f8b84e4c12078eca5361ce0715611 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 7 Mar 2023 08:55:48 +0100 Subject: [PATCH 10/13] Use WillBeAppliedReverse (and git apply --reverse) in the staging panel too It's simpler to have only one way of reversing a patch. --- pkg/gui/controllers/staging_controller.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/gui/controllers/staging_controller.go b/pkg/gui/controllers/staging_controller.go index 840c18a18..b29361aca 100644 --- a/pkg/gui/controllers/staging_controller.go +++ b/pkg/gui/controllers/staging_controller.go @@ -182,7 +182,7 @@ func (self *StagingController) applySelection(reverse bool) error { firstLineIdx, lastLineIdx := state.SelectedRange() patch := patch.ModifiedPatchForRange(self.c.Log, path, state.GetDiff(), firstLineIdx, lastLineIdx, - patch.PatchOptions{Reverse: reverse, KeepOriginalHeader: false}) + patch.PatchOptions{Reverse: false, WillBeAppliedReverse: reverse, KeepOriginalHeader: false}) if patch == "" { return nil @@ -191,6 +191,9 @@ func (self *StagingController) applySelection(reverse bool) error { // apply the patch then refresh this panel // create a new temp file with the patch, then call git apply with that patch applyFlags := []string{} + if reverse { + applyFlags = append(applyFlags, "reverse") + } if !reverse || self.staged { applyFlags = append(applyFlags, "cached") } @@ -229,7 +232,7 @@ func (self *StagingController) editHunk() error { hunk := state.CurrentHunk() patchText := patch.ModifiedPatchForRange( self.c.Log, path, state.GetDiff(), hunk.FirstLineIdx, hunk.LastLineIdx(), - patch.PatchOptions{Reverse: self.staged, KeepOriginalHeader: false}, + patch.PatchOptions{Reverse: false, WillBeAppliedReverse: self.staged, KeepOriginalHeader: false}, ) patchFilepath, err := self.git.WorkingTree.SaveTemporaryPatch(patchText) if err != nil { @@ -254,7 +257,12 @@ func (self *StagingController) editHunk() error { self.c.Log, path, editedPatchText, 0, lineCount, patch.PatchOptions{Reverse: false, KeepOriginalHeader: false}, ) - if err := self.git.WorkingTree.ApplyPatch(newPatchText, "cached"); err != nil { + + applyFlags := []string{"cached"} + if self.staged { + applyFlags = append(applyFlags, "reverse") + } + if err := self.git.WorkingTree.ApplyPatch(newPatchText, applyFlags...); err != nil { return self.c.Error(err) } From 45cf993982f9b56afedd5fd6585c6d6fcd858181 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 7 Mar 2023 09:31:38 +0100 Subject: [PATCH 11/13] Remove the PatchOptions.Reverse option All callers pass false now (except for the tests, which we simply remove), so we don't need the option any more. --- pkg/commands/patch/hunk.go | 33 ++--- pkg/commands/patch/patch_manager.go | 1 - pkg/commands/patch/patch_modifier.go | 12 +- pkg/commands/patch/patch_modifier_test.go | 161 ---------------------- pkg/gui/controllers/staging_controller.go | 6 +- 5 files changed, 12 insertions(+), 201 deletions(-) diff --git a/pkg/commands/patch/hunk.go b/pkg/commands/patch/hunk.go index a2727f2c9..e0aeb4157 100644 --- a/pkg/commands/patch/hunk.go +++ b/pkg/commands/patch/hunk.go @@ -45,7 +45,7 @@ func headerInfo(header string) (int, int, string) { return oldStart, newStart, heading } -func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool, willBeAppliedReverse bool) []string { +func (hunk *PatchHunk) updatedLines(lineIndices []int, willBeAppliedReverse bool) []string { skippedNewlineMessageIndex := -1 newLines := []string{} @@ -58,7 +58,7 @@ func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool, willBeAppli isLineSelected := lo.Contains(lineIndices, lineIdx) firstChar, content := line[:1], line[1:] - transformedFirstChar := transformedFirstChar(firstChar, reverse, willBeAppliedReverse, isLineSelected) + transformedFirstChar := transformedFirstChar(firstChar, willBeAppliedReverse, isLineSelected) if isLineSelected || (transformedFirstChar == "\\" && skippedNewlineMessageIndex != lineIdx) || transformedFirstChar == " " { newLines = append(newLines, transformedFirstChar+content) @@ -74,19 +74,7 @@ func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool, willBeAppli return newLines } -func transformedFirstChar(firstChar string, reverse bool, willBeAppliedReverse bool, isLineSelected bool) string { - if reverse { - if !isLineSelected && firstChar == "+" { - return " " - } else if firstChar == "-" { - return "+" - } else if firstChar == "+" { - return "-" - } else { - return firstChar - } - } - +func transformedFirstChar(firstChar string, willBeAppliedReverse bool, isLineSelected bool) string { linesToKeepInPatchContext := "-" if willBeAppliedReverse { linesToKeepInPatchContext = "+" @@ -102,16 +90,16 @@ func (hunk *PatchHunk) formatHeader(oldStart int, oldLength int, newStart int, n return fmt.Sprintf("@@ -%d,%d +%d,%d @@%s\n", oldStart, oldLength, newStart, newLength, heading) } -func (hunk *PatchHunk) formatWithChanges(lineIndices []int, reverse bool, willBeAppliedReverse bool, startOffset int) (int, string) { - bodyLines := hunk.updatedLines(lineIndices, reverse, willBeAppliedReverse) - startOffset, header, ok := hunk.updatedHeader(bodyLines, startOffset, reverse) +func (hunk *PatchHunk) formatWithChanges(lineIndices []int, willBeAppliedReverse bool, startOffset int) (int, string) { + bodyLines := hunk.updatedLines(lineIndices, willBeAppliedReverse) + startOffset, header, ok := hunk.updatedHeader(bodyLines, startOffset) if !ok { return startOffset, "" } return startOffset, header + strings.Join(bodyLines, "") } -func (hunk *PatchHunk) updatedHeader(newBodyLines []string, startOffset int, reverse bool) (int, string, bool) { +func (hunk *PatchHunk) updatedHeader(newBodyLines []string, startOffset int) (int, string, bool) { changeCount := nLinesWithPrefix(newBodyLines, []string{"+", "-"}) oldLength := nLinesWithPrefix(newBodyLines, []string{" ", "-"}) newLength := nLinesWithPrefix(newBodyLines, []string{"+", " "}) @@ -121,12 +109,7 @@ func (hunk *PatchHunk) updatedHeader(newBodyLines []string, startOffset int, rev return startOffset, "", false } - var oldStart int - if reverse { - oldStart = hunk.newStart - } else { - oldStart = hunk.oldStart - } + oldStart := hunk.oldStart var newStartOffset int // if the hunk went from zero to positive length, we need to increment the starting point by one diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index e50c0fd3a..7c7197583 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -178,7 +178,6 @@ func (p *PatchManager) renderPlainPatchForFile(filename string, willBeAppliedRev // generate a new diff with just the selected lines return ModifiedPatchForLines(p.Log, filename, info.diff, info.includedLineIndices, PatchOptions{ - Reverse: false, WillBeAppliedReverse: willBeAppliedReverse, KeepOriginalHeader: true, }) diff --git a/pkg/commands/patch/patch_modifier.go b/pkg/commands/patch/patch_modifier.go index 985743b76..c8e97e42d 100644 --- a/pkg/commands/patch/patch_modifier.go +++ b/pkg/commands/patch/patch_modifier.go @@ -14,17 +14,11 @@ var ( ) type PatchOptions struct { - // Create a reverse patch; in other words, flip all the '+' and '-' while - // generating the patch. - Reverse bool - // If true, we're building a patch that we are going to apply using // "git apply --reverse". In other words, we are not flipping the '+' and // '-' ourselves while creating the patch, but git is going to do that when // applying. This has consequences for which lines we need to keep or // discard when filtering lines from partial hunks. - // - // Currently incompatible with Reverse. WillBeAppliedReverse bool // Whether to keep or discard the original diff header including the @@ -96,10 +90,6 @@ func NewPatchModifier(log *logrus.Entry, filename string, diffText string) *Patc } func (d *PatchModifier) ModifiedPatchForLines(lineIndices []int, opts PatchOptions) string { - if opts.Reverse && opts.KeepOriginalHeader { - panic("reverse and keepOriginalHeader are not compatible") - } - // step one is getting only those hunks which we care about hunksInRange := []*PatchHunk{} outer: @@ -119,7 +109,7 @@ outer: var formattedHunk string for _, hunk := range hunksInRange { startOffset, formattedHunk = hunk.formatWithChanges( - lineIndices, opts.Reverse, opts.WillBeAppliedReverse, startOffset) + lineIndices, opts.WillBeAppliedReverse, startOffset) formattedHunks += formattedHunk } diff --git a/pkg/commands/patch/patch_modifier_test.go b/pkg/commands/patch/patch_modifier_test.go index c97fd89be..55fdfb547 100644 --- a/pkg/commands/patch/patch_modifier_test.go +++ b/pkg/commands/patch/patch_modifier_test.go @@ -120,7 +120,6 @@ func TestModifyPatchForRange(t *testing.T) { diffText string firstLineIndex int lastLineIndex int - reverse bool willBeAppliedReverse bool expected string } @@ -131,7 +130,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: -1, lastLineIndex: -1, - reverse: false, diffText: simpleDiff, expected: "", }, @@ -140,7 +138,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 5, lastLineIndex: 5, - reverse: false, diffText: simpleDiff, expected: "", }, @@ -149,7 +146,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 0, lastLineIndex: 11, - reverse: false, diffText: simpleDiff, expected: `--- a/filename +++ b/filename @@ -167,7 +163,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 6, lastLineIndex: 6, - reverse: false, diffText: simpleDiff, expected: `--- a/filename +++ b/filename @@ -184,7 +179,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 7, lastLineIndex: 7, - reverse: false, diffText: simpleDiff, expected: `--- a/filename +++ b/filename @@ -202,7 +196,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: -100, lastLineIndex: 100, - reverse: false, diffText: simpleDiff, expected: `--- a/filename +++ b/filename @@ -213,59 +206,6 @@ func TestModifyPatchForRange(t *testing.T) { ... ... ... -`, - }, - { - testName: "whole range reversed", - filename: "filename", - firstLineIndex: 0, - lastLineIndex: 11, - reverse: true, - diffText: simpleDiff, - expected: `--- a/filename -+++ b/filename -@@ -1,5 +1,5 @@ - apple -+orange --grape - ... - ... - ... -`, - }, - { - testName: "removal reversed", - filename: "filename", - firstLineIndex: 6, - lastLineIndex: 6, - reverse: true, - diffText: simpleDiff, - expected: `--- a/filename -+++ b/filename -@@ -1,5 +1,6 @@ - apple -+orange - grape - ... - ... - ... -`, - }, - { - testName: "removal reversed", - filename: "filename", - firstLineIndex: 7, - lastLineIndex: 7, - reverse: true, - diffText: simpleDiff, - expected: `--- a/filename -+++ b/filename -@@ -1,5 +1,4 @@ - apple --grape - ... - ... - ... `, }, { @@ -273,7 +213,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: -100, lastLineIndex: 100, - reverse: false, diffText: addNewlineToEndOfFile, expected: `--- a/filename +++ b/filename @@ -284,40 +223,6 @@ func TestModifyPatchForRange(t *testing.T) { -last line \ No newline at end of file +last line -`, - }, - { - testName: "add newline to end of file, addition only", - filename: "filename", - firstLineIndex: 8, - lastLineIndex: 8, - reverse: true, - diffText: addNewlineToEndOfFile, - expected: `--- a/filename -+++ b/filename -@@ -60,4 +60,5 @@ grape - ... - ... - ... -+last line -\ No newline at end of file - last line -`, - }, - { - testName: "add newline to end of file, removal only", - filename: "filename", - firstLineIndex: 10, - lastLineIndex: 10, - reverse: true, - diffText: addNewlineToEndOfFile, - expected: `--- a/filename -+++ b/filename -@@ -60,4 +60,3 @@ grape - ... - ... - ... --last line `, }, { @@ -325,7 +230,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: -100, lastLineIndex: 100, - reverse: false, diffText: removeNewlinefromEndOfFile, expected: `--- a/filename +++ b/filename @@ -343,7 +247,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 8, lastLineIndex: 8, - reverse: false, diffText: removeNewlinefromEndOfFile, expected: `--- a/filename +++ b/filename @@ -359,7 +262,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 9, lastLineIndex: 9, - reverse: false, diffText: removeNewlinefromEndOfFile, expected: `--- a/filename +++ b/filename @@ -377,7 +279,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: -100, lastLineIndex: 100, - reverse: false, diffText: twoHunks, expected: `--- a/filename +++ b/filename @@ -404,7 +305,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 7, lastLineIndex: 15, - reverse: false, diffText: twoHunks, expected: `--- a/filename +++ b/filename @@ -423,32 +323,6 @@ func TestModifyPatchForRange(t *testing.T) { ... ... ... -`, - }, - { - testName: "staging part of both hunks, reversed", - filename: "filename", - firstLineIndex: 7, - lastLineIndex: 15, - reverse: true, - diffText: twoHunks, - expected: `--- a/filename -+++ b/filename -@@ -1,5 +1,4 @@ - apple --orange - ... - ... - ... -@@ -8,8 +7,7 @@ grape - ... - ... - ... --pear - lemon - ... - ... - ... `, }, { @@ -456,7 +330,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "newfile", firstLineIndex: -100, lastLineIndex: 100, - reverse: false, diffText: newFile, expected: `--- a/newfile +++ b/newfile @@ -471,28 +344,12 @@ func TestModifyPatchForRange(t *testing.T) { filename: "newfile", firstLineIndex: 6, lastLineIndex: 7, - reverse: false, diffText: newFile, expected: `--- a/newfile +++ b/newfile @@ -0,0 +1,2 @@ +apple +orange -`, - }, - { - testName: "adding a new file, reversed", - filename: "newfile", - firstLineIndex: -100, - lastLineIndex: 100, - reverse: true, - diffText: newFile, - expected: `--- a/newfile -+++ b/newfile -@@ -1,3 +0,0 @@ --apple --orange --grape `, }, { @@ -500,27 +357,12 @@ func TestModifyPatchForRange(t *testing.T) { filename: "newfile", firstLineIndex: -100, lastLineIndex: 100, - reverse: false, diffText: addNewlineToPreviouslyEmptyFile, expected: `--- a/newfile +++ b/newfile @@ -0,0 +1,1 @@ +new line \ No newline at end of file -`, - }, - { - testName: "adding a new line to a previously empty file, reversed", - filename: "newfile", - firstLineIndex: -100, - lastLineIndex: 100, - reverse: true, - diffText: addNewlineToPreviouslyEmptyFile, - expected: `--- a/newfile -+++ b/newfile -@@ -1,1 +0,0 @@ --new line -\ No newline at end of file `, }, { @@ -528,7 +370,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 6, lastLineIndex: 7, - reverse: false, willBeAppliedReverse: false, diffText: twoChangesInOneHunk, expected: `--- a/filename @@ -547,7 +388,6 @@ func TestModifyPatchForRange(t *testing.T) { filename: "filename", firstLineIndex: 6, lastLineIndex: 7, - reverse: false, willBeAppliedReverse: true, diffText: twoChangesInOneHunk, expected: `--- a/filename @@ -568,7 +408,6 @@ func TestModifyPatchForRange(t *testing.T) { t.Run(s.testName, func(t *testing.T) { result := ModifiedPatchForRange(nil, s.filename, s.diffText, s.firstLineIndex, s.lastLineIndex, PatchOptions{ - Reverse: s.reverse, WillBeAppliedReverse: s.willBeAppliedReverse, KeepOriginalHeader: false, }) diff --git a/pkg/gui/controllers/staging_controller.go b/pkg/gui/controllers/staging_controller.go index b29361aca..2fef137d0 100644 --- a/pkg/gui/controllers/staging_controller.go +++ b/pkg/gui/controllers/staging_controller.go @@ -182,7 +182,7 @@ func (self *StagingController) applySelection(reverse bool) error { firstLineIdx, lastLineIdx := state.SelectedRange() patch := patch.ModifiedPatchForRange(self.c.Log, path, state.GetDiff(), firstLineIdx, lastLineIdx, - patch.PatchOptions{Reverse: false, WillBeAppliedReverse: reverse, KeepOriginalHeader: false}) + patch.PatchOptions{WillBeAppliedReverse: reverse, KeepOriginalHeader: false}) if patch == "" { return nil @@ -232,7 +232,7 @@ func (self *StagingController) editHunk() error { hunk := state.CurrentHunk() patchText := patch.ModifiedPatchForRange( self.c.Log, path, state.GetDiff(), hunk.FirstLineIdx, hunk.LastLineIdx(), - patch.PatchOptions{Reverse: false, WillBeAppliedReverse: self.staged, KeepOriginalHeader: false}, + patch.PatchOptions{WillBeAppliedReverse: self.staged, KeepOriginalHeader: false}, ) patchFilepath, err := self.git.WorkingTree.SaveTemporaryPatch(patchText) if err != nil { @@ -255,7 +255,7 @@ func (self *StagingController) editHunk() error { lineCount := strings.Count(editedPatchText, "\n") + 1 newPatchText := patch.ModifiedPatchForRange( self.c.Log, path, editedPatchText, 0, lineCount, - patch.PatchOptions{Reverse: false, KeepOriginalHeader: false}, + patch.PatchOptions{KeepOriginalHeader: false}, ) applyFlags := []string{"cached"} From 4bd1322941af599f4cf9e406cc72adf7a5bc6ff4 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 7 Mar 2023 10:16:30 +0100 Subject: [PATCH 12/13] Rename WillBeAppliedReverse to Reverse This is the only "reverse"-related option that is left, so use a less clumsy name for it. --- pkg/commands/patch/hunk.go | 12 +++---- pkg/commands/patch/patch_manager.go | 10 +++--- pkg/commands/patch/patch_modifier.go | 14 ++++---- pkg/commands/patch/patch_modifier_test.go | 42 +++++++++++------------ pkg/gui/controllers/staging_controller.go | 4 +-- 5 files changed, 41 insertions(+), 41 deletions(-) diff --git a/pkg/commands/patch/hunk.go b/pkg/commands/patch/hunk.go index e0aeb4157..605c473c1 100644 --- a/pkg/commands/patch/hunk.go +++ b/pkg/commands/patch/hunk.go @@ -45,7 +45,7 @@ func headerInfo(header string) (int, int, string) { return oldStart, newStart, heading } -func (hunk *PatchHunk) updatedLines(lineIndices []int, willBeAppliedReverse bool) []string { +func (hunk *PatchHunk) updatedLines(lineIndices []int, reverse bool) []string { skippedNewlineMessageIndex := -1 newLines := []string{} @@ -58,7 +58,7 @@ func (hunk *PatchHunk) updatedLines(lineIndices []int, willBeAppliedReverse bool isLineSelected := lo.Contains(lineIndices, lineIdx) firstChar, content := line[:1], line[1:] - transformedFirstChar := transformedFirstChar(firstChar, willBeAppliedReverse, isLineSelected) + transformedFirstChar := transformedFirstChar(firstChar, reverse, isLineSelected) if isLineSelected || (transformedFirstChar == "\\" && skippedNewlineMessageIndex != lineIdx) || transformedFirstChar == " " { newLines = append(newLines, transformedFirstChar+content) @@ -74,9 +74,9 @@ func (hunk *PatchHunk) updatedLines(lineIndices []int, willBeAppliedReverse bool return newLines } -func transformedFirstChar(firstChar string, willBeAppliedReverse bool, isLineSelected bool) string { +func transformedFirstChar(firstChar string, reverse bool, isLineSelected bool) string { linesToKeepInPatchContext := "-" - if willBeAppliedReverse { + if reverse { linesToKeepInPatchContext = "+" } if !isLineSelected && firstChar == linesToKeepInPatchContext { @@ -90,8 +90,8 @@ func (hunk *PatchHunk) formatHeader(oldStart int, oldLength int, newStart int, n return fmt.Sprintf("@@ -%d,%d +%d,%d @@%s\n", oldStart, oldLength, newStart, newLength, heading) } -func (hunk *PatchHunk) formatWithChanges(lineIndices []int, willBeAppliedReverse bool, startOffset int) (int, string) { - bodyLines := hunk.updatedLines(lineIndices, willBeAppliedReverse) +func (hunk *PatchHunk) formatWithChanges(lineIndices []int, reverse bool, startOffset int) (int, string) { + bodyLines := hunk.updatedLines(lineIndices, reverse) startOffset, header, ok := hunk.updatedHeader(bodyLines, startOffset) if !ok { return startOffset, "" diff --git a/pkg/commands/patch/patch_manager.go b/pkg/commands/patch/patch_manager.go index 7c7197583..3a379b97e 100644 --- a/pkg/commands/patch/patch_manager.go +++ b/pkg/commands/patch/patch_manager.go @@ -162,7 +162,7 @@ func (p *PatchManager) RemoveFileLineRange(filename string, firstLineIdx, lastLi return nil } -func (p *PatchManager) renderPlainPatchForFile(filename string, willBeAppliedReverse bool) string { +func (p *PatchManager) renderPlainPatchForFile(filename string, reverse bool) string { info, err := p.getFileInfo(filename) if err != nil { p.Log.Error(err) @@ -178,16 +178,16 @@ func (p *PatchManager) renderPlainPatchForFile(filename string, willBeAppliedRev // generate a new diff with just the selected lines return ModifiedPatchForLines(p.Log, filename, info.diff, info.includedLineIndices, PatchOptions{ - WillBeAppliedReverse: willBeAppliedReverse, - KeepOriginalHeader: true, + Reverse: reverse, + KeepOriginalHeader: true, }) default: return "" } } -func (p *PatchManager) RenderPatchForFile(filename string, plain bool, willBeAppliedReverse bool) string { - patch := p.renderPlainPatchForFile(filename, willBeAppliedReverse) +func (p *PatchManager) RenderPatchForFile(filename string, plain bool, reverse bool) string { + patch := p.renderPlainPatchForFile(filename, reverse) if plain { return patch } diff --git a/pkg/commands/patch/patch_modifier.go b/pkg/commands/patch/patch_modifier.go index c8e97e42d..79f7b7d31 100644 --- a/pkg/commands/patch/patch_modifier.go +++ b/pkg/commands/patch/patch_modifier.go @@ -14,12 +14,12 @@ var ( ) type PatchOptions struct { - // If true, we're building a patch that we are going to apply using - // "git apply --reverse". In other words, we are not flipping the '+' and - // '-' ourselves while creating the patch, but git is going to do that when - // applying. This has consequences for which lines we need to keep or - // discard when filtering lines from partial hunks. - WillBeAppliedReverse bool + // Create a patch that will applied in reverse with `git apply --reverse`. + // This affects how unselected lines are treated when only parts of a hunk + // are selected: usually, for unselected lines we change '-' lines to + // context lines and remove '+' lines, but when Reverse is true we need to + // turn '+' lines into context lines and remove '-' lines. + Reverse bool // Whether to keep or discard the original diff header including the // "index deadbeef..fa1afe1 100644" line. @@ -109,7 +109,7 @@ outer: var formattedHunk string for _, hunk := range hunksInRange { startOffset, formattedHunk = hunk.formatWithChanges( - lineIndices, opts.WillBeAppliedReverse, startOffset) + lineIndices, opts.Reverse, startOffset) formattedHunks += formattedHunk } diff --git a/pkg/commands/patch/patch_modifier_test.go b/pkg/commands/patch/patch_modifier_test.go index 55fdfb547..a6dfc2716 100644 --- a/pkg/commands/patch/patch_modifier_test.go +++ b/pkg/commands/patch/patch_modifier_test.go @@ -115,13 +115,13 @@ const exampleHunk = `@@ -1,5 +1,5 @@ // TestModifyPatchForRange is a function. func TestModifyPatchForRange(t *testing.T) { type scenario struct { - testName string - filename string - diffText string - firstLineIndex int - lastLineIndex int - willBeAppliedReverse bool - expected string + testName string + filename string + diffText string + firstLineIndex int + lastLineIndex int + reverse bool + expected string } scenarios := []scenario{ @@ -366,12 +366,12 @@ func TestModifyPatchForRange(t *testing.T) { `, }, { - testName: "adding part of a hunk", - filename: "filename", - firstLineIndex: 6, - lastLineIndex: 7, - willBeAppliedReverse: false, - diffText: twoChangesInOneHunk, + testName: "adding part of a hunk", + filename: "filename", + firstLineIndex: 6, + lastLineIndex: 7, + reverse: false, + diffText: twoChangesInOneHunk, expected: `--- a/filename +++ b/filename @@ -1,5 +1,5 @@ @@ -384,12 +384,12 @@ func TestModifyPatchForRange(t *testing.T) { `, }, { - testName: "adding part of a hunk, will-be-applied-reverse", - filename: "filename", - firstLineIndex: 6, - lastLineIndex: 7, - willBeAppliedReverse: true, - diffText: twoChangesInOneHunk, + testName: "adding part of a hunk, reverse", + filename: "filename", + firstLineIndex: 6, + lastLineIndex: 7, + reverse: true, + diffText: twoChangesInOneHunk, expected: `--- a/filename +++ b/filename @@ -1,5 +1,5 @@ @@ -408,8 +408,8 @@ func TestModifyPatchForRange(t *testing.T) { t.Run(s.testName, func(t *testing.T) { result := ModifiedPatchForRange(nil, s.filename, s.diffText, s.firstLineIndex, s.lastLineIndex, PatchOptions{ - WillBeAppliedReverse: s.willBeAppliedReverse, - KeepOriginalHeader: false, + Reverse: s.reverse, + KeepOriginalHeader: false, }) if !assert.Equal(t, s.expected, result) { fmt.Println(result) diff --git a/pkg/gui/controllers/staging_controller.go b/pkg/gui/controllers/staging_controller.go index 2fef137d0..78c271640 100644 --- a/pkg/gui/controllers/staging_controller.go +++ b/pkg/gui/controllers/staging_controller.go @@ -182,7 +182,7 @@ func (self *StagingController) applySelection(reverse bool) error { firstLineIdx, lastLineIdx := state.SelectedRange() patch := patch.ModifiedPatchForRange(self.c.Log, path, state.GetDiff(), firstLineIdx, lastLineIdx, - patch.PatchOptions{WillBeAppliedReverse: reverse, KeepOriginalHeader: false}) + patch.PatchOptions{Reverse: reverse, KeepOriginalHeader: false}) if patch == "" { return nil @@ -232,7 +232,7 @@ func (self *StagingController) editHunk() error { hunk := state.CurrentHunk() patchText := patch.ModifiedPatchForRange( self.c.Log, path, state.GetDiff(), hunk.FirstLineIdx, hunk.LastLineIdx(), - patch.PatchOptions{WillBeAppliedReverse: self.staged, KeepOriginalHeader: false}, + patch.PatchOptions{Reverse: self.staged, KeepOriginalHeader: false}, ) patchFilepath, err := self.git.WorkingTree.SaveTemporaryPatch(patchText) if err != nil { From 0bda93d4c3aea5f66c6a50322afc3b5a7e0ff75c Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Wed, 8 Mar 2023 09:19:20 +1100 Subject: [PATCH 13/13] Add more unit tests --- pkg/commands/patch/patch_modifier_test.go | 85 +++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/pkg/commands/patch/patch_modifier_test.go b/pkg/commands/patch/patch_modifier_test.go index a6dfc2716..131490ece 100644 --- a/pkg/commands/patch/patch_modifier_test.go +++ b/pkg/commands/patch/patch_modifier_test.go @@ -223,6 +223,24 @@ func TestModifyPatchForRange(t *testing.T) { -last line \ No newline at end of file +last line +`, + }, + { + testName: "add newline to end of file, reversed", + filename: "filename", + firstLineIndex: -100, + lastLineIndex: 100, + reverse: true, + diffText: addNewlineToEndOfFile, + expected: `--- a/filename ++++ b/filename +@@ -60,4 +60,4 @@ grape + ... + ... + ... +-last line +\ No newline at end of file ++last line `, }, { @@ -240,6 +258,24 @@ func TestModifyPatchForRange(t *testing.T) { -last line +last line \ No newline at end of file +`, + }, + { + testName: "remove newline from end of file, reversed", + filename: "filename", + firstLineIndex: -100, + lastLineIndex: 100, + reverse: true, + diffText: removeNewlinefromEndOfFile, + expected: `--- a/filename ++++ b/filename +@@ -60,4 +60,4 @@ grape + ... + ... + ... +-last line ++last line +\ No newline at end of file `, }, { @@ -255,6 +291,24 @@ func TestModifyPatchForRange(t *testing.T) { ... ... -last line +`, + }, + { + testName: "remove newline from end of file, removal only, reversed", + filename: "filename", + firstLineIndex: 8, + lastLineIndex: 8, + reverse: true, + diffText: removeNewlinefromEndOfFile, + expected: `--- a/filename ++++ b/filename +@@ -60,5 +60,4 @@ grape + ... + ... + ... +-last line + last line +\ No newline at end of file `, }, { @@ -272,6 +326,23 @@ func TestModifyPatchForRange(t *testing.T) { last line +last line \ No newline at end of file +`, + }, + { + testName: "remove newline from end of file, addition only, reversed", + filename: "filename", + firstLineIndex: 9, + lastLineIndex: 9, + reverse: true, + diffText: removeNewlinefromEndOfFile, + expected: `--- a/filename ++++ b/filename +@@ -60,3 +60,4 @@ grape + ... + ... + ... ++last line +\ No newline at end of file `, }, { @@ -363,6 +434,20 @@ func TestModifyPatchForRange(t *testing.T) { @@ -0,0 +1,1 @@ +new line \ No newline at end of file +`, + }, + { + testName: "adding a new line to a previously empty file, reversed", + filename: "newfile", + firstLineIndex: -100, + lastLineIndex: 100, + diffText: addNewlineToPreviouslyEmptyFile, + reverse: true, + expected: `--- a/newfile ++++ b/newfile +@@ -0,0 +1,1 @@ ++new line +\ No newline at end of file `, }, {