Skip to content

Commit

Permalink
Refactor git history parsing
Browse files Browse the repository at this point in the history
When running pint ci on a branch with merge commits these commits would make it look like there were more changes than in the actual diff.
  • Loading branch information
prymitive committed Jun 21, 2024
1 parent 847bc01 commit 6a13b5a
Show file tree
Hide file tree
Showing 13 changed files with 71 additions and 217 deletions.
4 changes: 2 additions & 2 deletions cmd/pint/tests/0028_ci_git_error.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions cmd/pint/tests/0065_ci_include.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0134_ci_base_branch_flag_path.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0135_ci_base_branch_config_path.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions cmd/pint/tests/0139_ci_exclude.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
2 changes: 0 additions & 2 deletions cmd/pint/tests/0140_ci_include_exclude.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
1 change: 1 addition & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
24 changes: 14 additions & 10 deletions internal/discovery/git_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
40 changes: 17 additions & 23 deletions internal/discovery/git_branch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
},
Expand All @@ -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)
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/git/changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Loading

0 comments on commit 6a13b5a

Please sign in to comment.