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)
  }