Fix patch size calculations to not include headers

4 files changed, 109 insertions, 22 deletions

diff --git a/changelogs/unreleased/fix-collapse-lines.yml b/changelogs/unreleased/fix-collapse-lines.yml
new file mode 100644
index 000000000..87cb3e491
--- /dev/null
+++ b/changelogs/unreleased/fix-collapse-lines.yml
@@ -0,0 +1,5 @@
+---
+title: Fix patch size calculations to not include headers
+merge_request: 859
+author:
+type: fixed
diff --git a/internal/diff/diff.go b/internal/diff/diff.go
index acc3e574f..1cc02aeaa 100644
--- a/internal/diff/diff.go
+++ b/internal/diff/diff.go
@@ -122,7 +122,7 @@ func (parser *Parser) Parse() bool {
 			}
 
 			if len(line) > 0 && len(line) < 10 {
-				parser.consumeChunkLine()
+				parser.consumeChunkLine(true)
 			}
 
 			break
@@ -134,11 +134,11 @@ func (parser *Parser) Parse() bool {
 		if bytes.HasPrefix(line, []byte("diff --git")) {
 			break
 		} else if bytes.HasPrefix(line, []byte("@@")) {
-			parser.consumeChunkLine()
+			parser.consumeChunkLine(false)
 		} else if helper.ByteSliceHasAnyPrefix(line, "---", "+++") && !parser.isParsingChunkLines() {
-			parser.consumeLine(true)
+			parser.consumeLine(false)
 		} else if helper.ByteSliceHasAnyPrefix(line, "-", "+", " ", "\\", "Binary") {
-			parser.consumeChunkLine()
+			parser.consumeChunkLine(true)
 		} else {
 			parser.consumeLine(false)
 		}
@@ -313,7 +313,7 @@ func parseRawLine(line []byte, diff *Diff) error {
 	return nil
 }
 
-func (parser *Parser) consumeChunkLine() {
+func (parser *Parser) consumeChunkLine(updateLineStats bool) {
 	var line []byte
 	var err error
 
@@ -333,9 +333,12 @@ func (parser *Parser) consumeChunkLine() {
 	}
 
 	parser.currentDiff.Patch = append(parser.currentDiff.Patch, line...)
-	parser.currentDiff.lineCount++
-	parser.linesProcessed++
-	parser.bytesProcessed += len(line)
+
+	if updateLineStats {
+		parser.bytesProcessed += len(line)
+		parser.currentDiff.lineCount++
+		parser.linesProcessed++
+	}
 }
 
 func (parser *Parser) consumeLine(updateStats bool) {
diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go
index a052719d9..d7eccb1f8 100644
--- a/internal/diff/diff_test.go
+++ b/internal/diff/diff_test.go
@@ -38,12 +38,7 @@ index 0000000000000000000000000000000000000000..3be11c69355948412925fa5e073d76d5
 		MaxLines:      10000000,
 		CollapseDiffs: true,
 	}
-	diffParser := NewDiffParser(strings.NewReader(rawDiff), limits)
-
-	diffs := []*Diff{}
-	for diffParser.Parse() {
-		diffs = append(diffs, diffParser.Diff())
-	}
+	diffs := getDiffs(rawDiff, limits)
 
 	expectedDiffs := []*Diff{
 		&Diff{
@@ -53,9 +48,9 @@ index 0000000000000000000000000000000000000000..3be11c69355948412925fa5e073d76d5
 			ToID:      "4cc7061661b8f52891bc1b39feb4d856b21a1067",
 			FromPath:  []byte("big.txt"),
 			ToPath:    []byte("big.txt"),
-			Status:    65,
+			Status:    'A',
 			Collapsed: true,
-			lineCount: 100003,
+			lineCount: 100000,
 		},
 		&Diff{
 			OldMode:   0,
@@ -64,12 +59,96 @@ index 0000000000000000000000000000000000000000..3be11c69355948412925fa5e073d76d5
 			ToID:      "3be11c69355948412925fa5e073d76d58ff3afd2",
 			FromPath:  []byte("file-00.txt"),
 			ToPath:    []byte("file-00.txt"),
-			Status:    65,
+			Status:    'A',
 			Collapsed: false,
 			Patch:     []byte("@@ -0,0 +1 @@\n+Lorem ipsum\n"),
-			lineCount: 4,
+			lineCount: 1,
+		},
+	}
+
+	require.Equal(t, expectedDiffs, diffs)
+}
+
+func TestDiffParserWithLargeDiffOfSmallPatches(t *testing.T) {
+	patch := "@@ -0,0 +1,5 @@\n" + strings.Repeat("+Lorem\n", 5)
+	rawDiff := `:000000 100644 0000000000000000000000000000000000000000 b6507e5b5ce18077e3ec8aaa2291404e5051d45d A	expand-collapse/file-0.txt
+:000000 100644 0000000000000000000000000000000000000000 b6507e5b5ce18077e3ec8aaa2291404e5051d45d A	expand-collapse/file-1.txt
+:000000 100644 0000000000000000000000000000000000000000 b6507e5b5ce18077e3ec8aaa2291404e5051d45d A	expand-collapse/file-2.txt
+
+`
+
+	// Create 3 files of 5 lines. The first two files added together surpass
+	// the limits, which should cause the last one to be collpased.
+	for i := 0; i < 3; i++ {
+		rawDiff += fmt.Sprintf(`diff --git a/expand-collapse/file-%d.txt b/expand-collapse/file-%d.txt
+new file mode 100644
+index 0000000000000000000000000000000000000000..b6507e5b5ce18077e3ec8aaa2291404e5051d45d
+--- /dev/null
++++ b/expand-collapse/file-%d.txt
+%s`, i, i, i, patch)
+	}
+
+	limits := Limits{
+		EnforceLimits: true,
+		SafeMaxLines:  10, // This is the one we care about here
+		SafeMaxFiles:  10000000,
+		SafeMaxBytes:  10000000,
+		MaxFiles:      10000000,
+		MaxBytes:      10000000,
+		MaxLines:      10000000,
+		CollapseDiffs: true,
+	}
+	diffs := getDiffs(rawDiff, limits)
+
+	expectedDiffs := []*Diff{
+		&Diff{
+			OldMode:   0,
+			NewMode:   0100644,
+			FromID:    "0000000000000000000000000000000000000000",
+			ToID:      "b6507e5b5ce18077e3ec8aaa2291404e5051d45d",
+			FromPath:  []byte("expand-collapse/file-0.txt"),
+			ToPath:    []byte("expand-collapse/file-0.txt"),
+			Status:    'A',
+			Collapsed: false,
+			Patch:     []byte(patch),
+			lineCount: 5,
+		},
+		&Diff{
+			OldMode:   0,
+			NewMode:   0100644,
+			FromID:    "0000000000000000000000000000000000000000",
+			ToID:      "b6507e5b5ce18077e3ec8aaa2291404e5051d45d",
+			FromPath:  []byte("expand-collapse/file-1.txt"),
+			ToPath:    []byte("expand-collapse/file-1.txt"),
+			Status:    'A',
+			Collapsed: false,
+			Patch:     []byte(patch),
+			lineCount: 5,
+		},
+		&Diff{
+			OldMode:   0,
+			NewMode:   0100644,
+			FromID:    "0000000000000000000000000000000000000000",
+			ToID:      "b6507e5b5ce18077e3ec8aaa2291404e5051d45d",
+			FromPath:  []byte("expand-collapse/file-2.txt"),
+			ToPath:    []byte("expand-collapse/file-2.txt"),
+			Status:    'A',
+			Collapsed: true,
+			Patch:     nil,
+			lineCount: 5,
 		},
 	}
 
 	require.Equal(t, expectedDiffs, diffs)
 }
+
+func getDiffs(rawDiff string, limits Limits) []*Diff {
+	diffParser := NewDiffParser(strings.NewReader(rawDiff), limits)
+
+	diffs := []*Diff{}
+	for diffParser.Parse() {
+		diffs = append(diffs, diffParser.Diff())
+	}
+
+	return diffs
+}
diff --git a/internal/service/diff/commit_test.go b/internal/service/diff/commit_test.go
index 551f4e151..17baf59c9 100644
--- a/internal/service/diff/commit_test.go
+++ b/internal/service/diff/commit_test.go
@@ -478,7 +478,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 			request: pb.CommitDiffRequest{
 				EnforceLimits: true,
 				MaxFiles:      5,
-				MaxLines:      100,
+				MaxLines:      90,
 				MaxBytes:      5 * 5 * 1024,
 			},
 			result: []diffAttributes{
@@ -493,7 +493,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 				EnforceLimits: true,
 				MaxFiles:      5,
 				MaxLines:      1000,
-				MaxBytes:      7650,
+				MaxBytes:      6900,
 			},
 			result: []diffAttributes{
 				{path: "CHANGELOG"},
@@ -550,7 +550,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 				MaxLines:      100,
 				MaxBytes:      5 * 5 * 1024,
 				SafeMaxFiles:  5,
-				SafeMaxLines:  45,
+				SafeMaxLines:  40,
 				SafeMaxBytes:  5 * 5 * 1024,
 			},
 			result: []diffAttributes{
@@ -610,7 +610,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 				require.Equal(t, requestAndResult.result[i].path, string(diff.FromPath), "path")
 
 				collapsed := requestAndResult.result[i].collapsed
-				require.Equal(t, collapsed, diff.Collapsed, "collapsed")
+				require.Equal(t, collapsed, diff.Collapsed, "%s collapsed", requestAndResult.result[i].path)
 				if collapsed {
 					require.Empty(t, diff.Patch, "patch")
 				}