diff --git a/pkg/commands/patch/hunk.go b/pkg/commands/patch/hunk.go index 568b312a7..e539f3a47 100644 --- a/pkg/commands/patch/hunk.go +++ b/pkg/commands/patch/hunk.go @@ -16,6 +16,11 @@ type Hunk struct { newStart int // the context at the end of the header line (' func (f *CommitFile) Description() string {' in the above example) headerContext string + // the lengths declared in the header line ('2' and '3' in the above example), + // kept so that we can check the parsed body against them (see + // Patch.IsWellFormed). Only set by Parse. + declaredOldLength int + declaredNewLength int // the body of the hunk, excluding the header line bodyLines []*PatchLine } diff --git a/pkg/commands/patch/parse.go b/pkg/commands/patch/parse.go index fee7d2918..a26332869 100644 --- a/pkg/commands/patch/parse.go +++ b/pkg/commands/patch/parse.go @@ -7,7 +7,9 @@ import ( "github.com/jesseduffield/lazygit/pkg/utils" ) -var hunkHeaderRegexp = regexp.MustCompile(`(?m)^@@ -(\d+)[^\+]+\+(\d+)[^@]+@@(.*)$`) +// Captures, in order: the old start, the old length (omitted by git when it is +// 1), the new start, the new length (likewise), and the trailing context. +var hunkHeaderRegexp = regexp.MustCompile(`(?m)^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)$`) func Parse(patchStr string) *Patch { // ignore trailing newline. @@ -19,13 +21,15 @@ func Parse(patchStr string) *Patch { var currentHunk *Hunk for _, line := range lines { if strings.HasPrefix(line, "@@") { - oldStart, newStart, headerContext := headerInfo(line) + oldStart, oldLength, newStart, newLength, headerContext := headerInfo(line) currentHunk = &Hunk{ - oldStart: oldStart, - newStart: newStart, - headerContext: headerContext, - bodyLines: []*PatchLine{}, + oldStart: oldStart, + newStart: newStart, + declaredOldLength: oldLength, + declaredNewLength: newLength, + headerContext: headerContext, + bodyLines: []*PatchLine{}, } hunks = append(hunks, currentHunk) } else if currentHunk != nil { @@ -41,14 +45,25 @@ func Parse(patchStr string) *Patch { } } -func headerInfo(header string) (int, int, string) { +func headerInfo(header string) (oldStart int, oldLength int, newStart int, newLength int, headerContext string) { match := hunkHeaderRegexp.FindStringSubmatch(header) - oldStart := utils.MustConvertToInt(match[1]) - newStart := utils.MustConvertToInt(match[2]) - headerContext := match[3] + oldStart = utils.MustConvertToInt(match[1]) + oldLength = declaredLength(match[2]) + newStart = utils.MustConvertToInt(match[3]) + newLength = declaredLength(match[4]) + headerContext = match[5] - return oldStart, newStart, headerContext + return oldStart, oldLength, newStart, newLength, headerContext +} + +// declaredLength parses a length capture of a hunk header, which git omits when +// it is 1 (e.g. "@@ -0,0 +1 @@"). +func declaredLength(match string) int { + if match == "" { + return 1 + } + return utils.MustConvertToInt(match) } func newHunkLine(line string) *PatchLine { diff --git a/pkg/commands/patch/patch.go b/pkg/commands/patch/patch.go index 8cff4d1fa..32c4788fb 100644 --- a/pkg/commands/patch/patch.go +++ b/pkg/commands/patch/patch.go @@ -79,6 +79,44 @@ func (self *Patch) HunkEndIdx(hunkIndex int) int { return self.HunkStartIdx(hunkIndex) + self.hunks[hunkIndex].lineCount() - 1 } +// IsWellFormed reports whether every hunk's body matches the lengths declared in +// its header. A faithful unified diff always satisfies this; a rendering that +// restructured the diff body does not — a diff renderer that puts line numbers in +// a gutter, say, shifts the +/- marker off the start of each line, so every body +// line reads as context and the computed lengths no longer match the header. That +// makes this the test for whether a rendered diff can be parsed as a unified diff +// at all, rather than trusting a mis-parse. Only meaningful for patches produced +// by Parse, which is where the declared lengths come from. +func (self *Patch) IsWellFormed() bool { + return self.isWellFormed(false) +} + +// IsWellFormedSoFar is IsWellFormed for a patch parsed from a diff we have only the +// beginning of. Its last hunk holds the first lines of a body that hasn't all arrived, +// so every hunk but the last has to match its header exactly, as before, while the last +// one only has to fit within what its header declares. +// +// The check exists to tell a faithful rendering from a restructured one, and it still +// does that. A rendering that moves the +/- marker off the start of the line makes us +// read a change as context, and a context line counts towards both lengths, so such a +// hunk comes out longer than its header declares rather than shorter. +func (self *Patch) IsWellFormedSoFar() bool { + return self.isWellFormed(true) +} + +func (self *Patch) isWellFormed(lastHunkMayBeIncomplete bool) bool { + for i, hunk := range self.hunks { + if lastHunkMayBeIncomplete && i == len(self.hunks)-1 { + return hunk.oldLength() <= hunk.declaredOldLength && + hunk.newLength() <= hunk.declaredNewLength + } + if hunk.oldLength() != hunk.declaredOldLength || hunk.newLength() != hunk.declaredNewLength { + return false + } + } + return true +} + func (self *Patch) ContainsChanges() bool { return lo.SomeBy(self.hunks, func(hunk *Hunk) bool { return hunk.containsChanges() diff --git a/pkg/commands/patch/patch_test.go b/pkg/commands/patch/patch_test.go index 9b2474bd1..1d925d399 100644 --- a/pkg/commands/patch/patch_test.go +++ b/pkg/commands/patch/patch_test.go @@ -696,6 +696,108 @@ func TestLineNumberOfLine(t *testing.T) { } } +func TestIsWellFormed(t *testing.T) { + // The body of a diff as rendered with the +/- markers moved out of the text + // and into a gutter: every body line now reads as context, so the lengths no + // longer match the header. + const gutterMangled = `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,4 +1,2 @@ + apple + grape + pear + lemon +` + + scenarios := []struct { + testName string + patchStr string + expected bool + }{ + {"simpleDiff", simpleDiff, true}, + {"renameWithModificationDiff", renameWithModificationDiff, true}, + {"addNewlineToEndOfFile", addNewlineToEndOfFile, true}, + {"twoHunks", twoHunks, true}, + {"consecutiveDeletions", consecutiveDeletions, true}, + {"newFile", newFile, true}, + {"deletedFile", deletedFile, true}, + {"addNewlineToPreviouslyEmptyFile", addNewlineToPreviouslyEmptyFile, true}, + {"exampleHunk", exampleHunk, true}, + {"gutterMangled", gutterMangled, false}, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + assert.Equal(t, s.expected, Parse(s.patchStr).IsWellFormed()) + }) + } +} + +func TestIsWellFormedSoFar(t *testing.T) { + // A diff read only as far as the middle of its second hunk. + const cutShort = `diff --git a/filename b/filename +index e48a11c..b2ab81b 100644 +--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape ++orange + ... + ... + ... +@@ -8,6 +8,8 @@ grape + ... + ... +` + + // The same diff cut short in its first hunk, so that the second is missing + // entirely rather than short. + const cutShortInTheFirstHunk = `diff --git a/filename b/filename +index e48a11c..b2ab81b 100644 +--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape +` + + // A rendering with the +/- markers moved into a gutter, cut short: reading the + // changes as context makes the hunk longer than its header declares, not shorter, + // so it doesn't pass for a diff we only have the beginning of. + const gutterMangledAndCutShort = `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,4 +1,2 @@ + apple + grape + pear + lemon + melon +` + + scenarios := []struct { + testName string + patchStr string + expected bool + }{ + {"simpleDiff", simpleDiff, true}, + {"twoHunks", twoHunks, true}, + {"cutShort", cutShort, true}, + {"cutShortInTheFirstHunk", cutShortInTheFirstHunk, true}, + {"gutterMangledAndCutShort", gutterMangledAndCutShort, false}, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + assert.Equal(t, s.expected, Parse(s.patchStr).IsWellFormedSoFar()) + }) + } +} + func TestOldLineNumberOfLine(t *testing.T) { type scenario struct { testName string