Merge pull request #1009 from cloudflare/merges

Refactor git history parsing

cmd/pint/tests/0028_ci_git_error.txt

@@ -28,8 +28,8 @@ level=DEBUG msg="Excluding git directory from glob results" path=.git glob=*
 level=DEBUG msg="File parsed" path=.pint.hcl rules=0
 level=DEBUG msg="File parsed" path=rules.yml rules=2
 level=DEBUG msg="Glob finder completed" count=2
-level=DEBUG msg="Running git command" args=["log","--format=%H","--no-abbrev-commit","--reverse","notmain..HEAD"]
-level=ERROR msg="Fatal error" err="failed to get the list of commits to scan: fatal: ambiguous argument 'notmain..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
+level=DEBUG msg="Running git command" args=["log","--reverse","--no-merges","--first-parent","--format=%H","--name-status","notmain..HEAD"]
+level=ERROR msg="Fatal error" err="failed to get the list of modified files from git: fatal: ambiguous argument 'notmain..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
 -- src/v1.yml --
 - record: rule1
   expr: sum(foo) by(job)

cmd/pint/tests/0065_ci_include.txt

@@ -23,8 +23,6 @@ exec git commit -am 'v2'
 pint.ok -l debug --no-color ci
 ! stdout .
 stderr 'level=DEBUG msg="Got branch information" base=main current=v2'
-stderr 'level=DEBUG msg="Found commit to scan" commit=.*'
-stderr 'level=DEBUG msg="Found commit to scan" commit=.*'
 stderr 'level=DEBUG msg="Git file change" change=A path=a.yml commit=.*'
 stderr 'level=DEBUG msg="Git file change" change=A path=b.yml commit=.*'
 stderr 'level=DEBUG msg="Skipping file due to include/exclude rules" path=a.yml'

cmd/pint/tests/0134_ci_base_branch_flag_path.txt

@@ -28,8 +28,8 @@ level=DEBUG msg="Excluding git directory from glob results" path=.git glob=*
 level=DEBUG msg="File parsed" path=.pint.hcl rules=0
 level=DEBUG msg="File parsed" path=rules.yml rules=1
 level=DEBUG msg="Glob finder completed" count=1
-level=DEBUG msg="Running git command" args=["log","--format=%H","--no-abbrev-commit","--reverse","origin/main..HEAD"]
-level=ERROR msg="Fatal error" err="failed to get the list of commits to scan: fatal: ambiguous argument 'origin/main..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
+level=DEBUG msg="Running git command" args=["log","--reverse","--no-merges","--first-parent","--format=%H","--name-status","origin/main..HEAD"]
+level=ERROR msg="Fatal error" err="failed to get the list of modified files from git: fatal: ambiguous argument 'origin/main..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
 -- src/v1.yml --
 - record: rule1
   expr: sum(foo) by(job)

cmd/pint/tests/0135_ci_base_branch_config_path.txt

@@ -28,8 +28,8 @@ level=DEBUG msg="Excluding git directory from glob results" path=.git glob=*
 level=DEBUG msg="File parsed" path=.pint.hcl rules=0
 level=DEBUG msg="File parsed" path=rules.yml rules=1
 level=DEBUG msg="Glob finder completed" count=1
-level=DEBUG msg="Running git command" args=["log","--format=%H","--no-abbrev-commit","--reverse","origin/main..HEAD"]
-level=ERROR msg="Fatal error" err="failed to get the list of commits to scan: fatal: ambiguous argument 'origin/main..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
+level=DEBUG msg="Running git command" args=["log","--reverse","--no-merges","--first-parent","--format=%H","--name-status","origin/main..HEAD"]
+level=ERROR msg="Fatal error" err="failed to get the list of modified files from git: fatal: ambiguous argument 'origin/main..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
 -- src/v1.yml --
 - record: rule1
   expr: sum(foo) by(job)

cmd/pint/tests/0139_ci_exclude.txt

@@ -23,8 +23,6 @@ exec git commit -am 'v2'
 pint.ok -l debug --no-color ci
 ! stdout .
 stderr 'level=DEBUG msg="Got branch information" base=main current=v2'
-stderr 'level=DEBUG msg="Found commit to scan" commit=.*'
-stderr 'level=DEBUG msg="Found commit to scan" commit=.*'
 stderr 'level=DEBUG msg="Git file change" change=A path=a.yml commit=.*'
 stderr 'level=DEBUG msg="Git file change" change=A path=b.yml commit=.*'
 stderr 'level=DEBUG msg="Skipping file due to include/exclude rules" path=a.yml'

cmd/pint/tests/0140_ci_include_exclude.txt

@@ -23,8 +23,6 @@ exec git commit -am 'v2'
 pint.ok -l debug --no-color ci
 ! stdout .
 stderr 'level=DEBUG msg="Got branch information" base=main current=v2'
-stderr 'level=DEBUG msg="Found commit to scan" commit=.*'
-stderr 'level=DEBUG msg="Found commit to scan" commit=.*'
 stderr 'level=DEBUG msg="Git file change" change=A path=a.yml commit=.*'
 stderr 'level=DEBUG msg="Git file change" change=A path=b.yml commit=.*'
 stderr 'level=DEBUG msg="Skipping file due to include/exclude rules" path=a.yml'

docs/changelog.md

@@ -5,6 +5,7 @@
 ### Fixed
 
 - Don't suggest using `humanize` when alert template is already using printf on format the `$value`.
+- Fixed git history parsing when running `pint ci` on a branch that include merge commits.
 
 ## v0.60.0
 

internal/discovery/git_branch.go

@@ -39,19 +39,13 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
 		allEntries[i].State = Excluded
 	}
 
-	cr, err := git.CommitRange(f.gitCmd, f.baseBranch)
+	changes, err := git.Changes(f.gitCmd, f.baseBranch, f.filter)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get the list of commits to scan: %w", err)
+		return nil, err
 	}
-	slog.Debug("Got commit range from git", slog.String("from", cr.From), slog.String("to", cr.To))
 
-	if len(cr.Commits) > f.maxCommits {
-		return nil, fmt.Errorf("number of commits to check (%d) is higher than maxCommits (%d), exiting", len(cr.Commits), f.maxCommits)
-	}
-
-	changes, err := git.Changes(f.gitCmd, cr, f.filter)
-	if err != nil {
-		return nil, err
+	if totalCommits := countCommits(changes); totalCommits > f.maxCommits {
+		return nil, fmt.Errorf("number of commits to check (%d) is higher than maxCommits (%d), exiting", totalCommits, f.maxCommits)
 	}
 
 	shouldSkip, err := f.shouldSkipAllChecks(changes)
@@ -321,3 +315,13 @@ func findRulesByName(entries []Entry, name string, typ parser.RuleType) (nomatch
 	}
 	return nomatch, match
 }
+
+func countCommits(changes []*git.FileChange) int {
+	commits := map[string]struct{}{}
+	for _, change := range changes {
+		for _, commit := range change.Commits {
+			commits[commit] = struct{}{}
+		}
+	}
+	return len(commits)
+}

internal/discovery/git_branch_test.go

@@ -80,7 +80,7 @@ func TestGitBranchFinder(t *testing.T) {
 				50,
 			),
 			entries: nil,
-			err:     "failed to get the list of commits to scan: mock git error: [log --format=%H --no-abbrev-commit --reverse main..HEAD]",
+			err:     "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status main..HEAD]",
 		},
 		{
 			title: "git list PR commits error - master",
@@ -94,24 +94,22 @@ func TestGitBranchFinder(t *testing.T) {
 				50,
 			),
 			entries: nil,
-			err:     "failed to get the list of commits to scan: mock git error: [log --format=%H --no-abbrev-commit --reverse master..HEAD]",
+			err:     "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status master..HEAD]",
 		},
 		{
 			title: "too many commits",
-			setup: func(_ *testing.T) {},
-			finder: discovery.NewGitBranchFinder(
-				func(args ...string) ([]byte, error) {
-					switch strings.Join(args, " ") {
-					case "log --format=%H --no-abbrev-commit --reverse main..HEAD":
-						return []byte("c1\nc2\nc3\nc4\n"), nil
-					default:
-						return nil, fmt.Errorf("mock git error: %v", args)
-					}
-				},
-				git.NewPathFilter(includeAll, nil, nil),
-				"main",
-				3,
-			),
+			setup: func(t *testing.T) {
+				commitFile(t, "rules.yml", "# v1\n", "v1")
+
+				_, err := git.RunGit("checkout", "-b", "v2")
+				require.NoError(t, err, "git checkout v2")
+
+				commitFile(t, "rules.yml", "# v2-1\n", "v2-1")
+				commitFile(t, "rules.yml", "# v2-2\n", "v2-2")
+				commitFile(t, "rules.yml", "# v2-3\n", "v2-3")
+				commitFile(t, "rules.yml", "# v2-4\n", "v2-4")
+			},
+			finder:  discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 3),
 			entries: nil,
 			err:     "number of commits to check (4) is higher than maxCommits (3), exiting",
 		},
@@ -132,17 +130,15 @@ func TestGitBranchFinder(t *testing.T) {
 				4,
 			),
 			entries: nil,
-			err:     "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status c1^..c4]",
+			err:     "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status main..HEAD]",
 		},
 		{
 			title: "git get commit message error",
 			setup: func(_ *testing.T) {},
 			finder: discovery.NewGitBranchFinder(
 				func(args ...string) ([]byte, error) {
 					switch strings.Join(args, " ") {
-					case "log --format=%H --no-abbrev-commit --reverse main..HEAD":
-						return []byte("c1\nc2\nc3\nc4\n"), nil
-					case "log --reverse --no-merges --first-parent --format=%H --name-status c1^..c4":
+					case "log --reverse --no-merges --first-parent --format=%H --name-status main..HEAD":
 						return []byte("c1\nA\trules.yml\n"), nil
 					default:
 						return nil, fmt.Errorf("mock git error: %v", args)
@@ -161,9 +157,7 @@ func TestGitBranchFinder(t *testing.T) {
 			finder: discovery.NewGitBranchFinder(
 				func(args ...string) ([]byte, error) {
 					switch strings.Join(args, " ") {
-					case "log --format=%H --no-abbrev-commit --reverse main..HEAD":
-						return []byte("c1\nc2\nc3\nc4\n"), nil
-					case "log --reverse --no-merges --first-parent --format=%H --name-status c1^..c4":
+					case "log --reverse --no-merges --first-parent --format=%H --name-status main..HEAD":
 						return []byte("c1\nA\trules.yml\n"), nil
 					case "ls-tree c1^ rules.yml":
 						return []byte("100644 blob c0\trules.yml"), nil

internal/git/changes.go

@@ -68,8 +68,8 @@ type FileChange struct {
 	Status  FileStatus
 }
 
-func Changes(cmd CommandRunner, cr CommitRangeResults, filter PathFilter) ([]*FileChange, error) {
-	out, err := cmd("log", "--reverse", "--no-merges", "--first-parent", "--format=%H", "--name-status", cr.String())
+func Changes(cmd CommandRunner, baseBranch string, filter PathFilter) ([]*FileChange, error) {
+	out, err := cmd("log", "--reverse", "--no-merges", "--first-parent", "--format=%H", "--name-status", fmt.Sprintf("%s..HEAD", baseBranch))
 	if err != nil {
 		return nil, fmt.Errorf("failed to get the list of modified files from git: %w", err)
 	}