Merge pull request #1025 from cloudflare/git-discovery

Don't report removed rules on syntax errors

docs/changelog.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## v0.61.2
+
+### Fixed
+
+- Fixed false positive reports about removed rules when running `pint ci` on a branch that
+  contains YAML syntax errors.
+
 ## v0.61.1
 
 ### Fixed

internal/discovery/git_branch.go

@@ -76,6 +76,9 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
 		if err != nil {
 			return nil, fmt.Errorf("invalid file syntax: %w", err)
 		}
+
+		failedEntries := entriesWithPathErrors(entriesAfter)
+
 		slog.Debug(
 			"Parsing git file change",
 			slog.Any("commits", change.Commits),
@@ -134,7 +137,7 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
 					me.after.ModifiedLines = commonLines(change.Body.ModifiedLines, me.after.ModifiedLines)
 				}
 				entries = append(entries, me.after)
-			case me.hasBefore && !me.hasAfter:
+			case me.hasBefore && !me.hasAfter && len(failedEntries) == 0:
 				me.before.State = Removed
 				ml := commonLines(change.Body.ModifiedLines, me.before.ModifiedLines)
 				if len(ml) > 0 {
@@ -149,6 +152,15 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
 					slog.String("modifiedLines", output.FormatLineRangeString(me.before.ModifiedLines)),
 				)
 				entries = append(entries, me.before)
+			case me.hasBefore && !me.hasAfter && len(failedEntries) > 0:
+				slog.Debug(
+					"Rule not present on HEAD branch but there are parse errors",
+					slog.String("name", me.before.Rule.Name()),
+					slog.String("state", me.before.State.String()),
+					slog.String("path", me.before.Path.Name),
+					slog.String("ruleLines", me.before.Rule.Lines.String()),
+					slog.String("modifiedLines", output.FormatLineRangeString(me.before.ModifiedLines)),
+				)
 			default:
 				slog.Warn(
 					"Unknown rule state",
@@ -325,3 +337,12 @@ func countCommits(changes []*git.FileChange) int {
 	}
 	return len(commits)
 }
+
+func entriesWithPathErrors(entries []Entry) (match []Entry) {
+	for _, entry := range entries {
+		if entry.PathError != nil {
+			match = append(match, entry)
+		}
+	}
+	return match
+}

internal/discovery/git_branch_test.go

@@ -1017,6 +1017,60 @@ groups:
 				},
 			},
 		},
+		{
+			title: "rule broken",
+			setup: func(t *testing.T) {
+				commitFile(t, "rules.yml", `
+groups:
+- name: v1
+  rules:
+  - record: rule1
+    expr: sum(up)
+  - record: rule2
+    expr: sum(up)
+  - record: rule3
+    expr: sum(up)
+  - record: rule4
+    expr: sum(up)
+`, "v1")
+
+				_, err := git.RunGit("checkout", "-b", "v2")
+				require.NoError(t, err, "git checkout v2")
+
+				commitFile(t, "rules.yml", `
+groups:
+- name: v2
+  rules:
+  - record: rule1
+    expr: sum(up)
+  - record: rule2
+    expr: sum(up)
+  - record: rule3
+    expr: sum(up)
+    +
+    sum(up)
+  - record: rule4
+    expr: sum(up)
+  - record: rule5
+    expr: sum(up)
+`, "v2")
+			},
+			finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4),
+			entries: []discovery.Entry{
+				{
+					State: discovery.Added,
+					Path: discovery.Path{
+						Name:          "rules.yml",
+						SymlinkTarget: "rules.yml",
+					},
+					ModifiedLines: []int{3, 11, 12, 13, 14, 15, 16},
+					PathError: parser.ParseError{
+						Line: 11,
+						Err:  errors.New("could not find expected ':'"),
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {