diff --git a/pkg/cli/admin/release/git.go b/pkg/cli/admin/release/git.go index a6e44c8866..0ec5d682d3 100644 --- a/pkg/cli/admin/release/git.go +++ b/pkg/cli/admin/release/git.go @@ -215,9 +215,9 @@ var ( rePrefix = regexp.MustCompile(`^(\[[\w\.\-]+\]\s*)+`) ) -func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommit, error) { +func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommit, int, error) { if from == to { - return nil, nil + return nil, 0, nil } args := []string{"log", "--merges", "--topo-order", "--first-parent", "-z", "--pretty=format:%H %P%x1E%ct%x1E%s%x1E%b", fmt.Sprintf("%s..%s", from, to)} @@ -225,24 +225,25 @@ func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommi if err != nil { // retry once if there's a chance we haven't fetched the latest commits if !strings.Contains(out, "Invalid revision range") { - return nil, gitOutputToError(err, out) + return nil, 0, gitOutputToError(err, out) } if _, err := g.exec("fetch", "--all"); err != nil { - return nil, gitOutputToError(err, out) + return nil, 0, gitOutputToError(err, out) } if _, err := g.exec("cat-file", "-e", from+"^{commit}"); err != nil { - return nil, fmt.Errorf("from commit %s does not exist", from) + return nil, 0, fmt.Errorf("from commit %s does not exist", from) } if _, err := g.exec("cat-file", "-e", to+"^{commit}"); err != nil { - return nil, fmt.Errorf("to commit %s does not exist", to) + return nil, 0, fmt.Errorf("to commit %s does not exist", to) } out, err = g.exec(args...) if err != nil { - return nil, gitOutputToError(err, out) + return nil, 0, gitOutputToError(err, out) } } squash := false + elidedCommits := 0 if out == "" { // some repositories use squash merging(like insights-operator) and // out which is populated by --merges flag is empty. Thereby, @@ -251,9 +252,21 @@ func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommi args = []string{"log", "--no-merges", "--topo-order", "-z", "--pretty=format:%H %P%x1E%ct%x1E%s%x1E%b", fmt.Sprintf("%s..%s", from, to)} out, err = g.exec(args...) if err != nil { - return nil, gitOutputToError(err, out) + return nil, 0, gitOutputToError(err, out) } squash = true + } else { + // some repositories use both real merges and squash or rebase merging. + // Don't flood the output with single-commit noise, which might be poorly curated, + // but do at least mention the fact that there are non-merge commits in the + // first-parent line, so folks who are curious can click through to GitHub for + // details. + args := []string{"log", "--no-merges", "--first-parent", "-z", "--format=%H", fmt.Sprintf("%s..%s", from, to)} + out, err := g.exec(args...) + if err != nil { + return nil, 0, gitOutputToError(err, out) + } + elidedCommits = strings.Count(out, "\x00") } if klog.V(5).Enabled() { @@ -262,16 +275,16 @@ func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommi var commits []MergeCommit if len(out) == 0 { - return nil, nil + return nil, elidedCommits, nil } for _, entry := range strings.Split(out, "\x00") { records := strings.Split(entry, "\x1e") if len(records) != 4 { - return nil, fmt.Errorf("unexpected git log output width %d columns", len(records)) + return nil, elidedCommits, fmt.Errorf("unexpected git log output width %d columns", len(records)) } unixTS, err := strconv.ParseInt(records[1], 10, 64) if err != nil { - return nil, fmt.Errorf("unexpected timestamp: %v", err) + return nil, elidedCommits, fmt.Errorf("unexpected timestamp: %v", err) } commitValues := strings.Split(records[0], " ") @@ -304,7 +317,7 @@ func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommi } mergeCommit.PullRequest, err = strconv.Atoi(m[1]) if err != nil { - return nil, fmt.Errorf("could not extract PR number from %q: %v", mergeMsg, err) + return nil, elidedCommits, fmt.Errorf("could not extract PR number from %q: %v", mergeMsg, err) } if len(msg) == 0 { msg = "Merge" @@ -314,7 +327,7 @@ func mergeLogForRepo(g gitInterface, repo string, from, to string) ([]MergeCommi commits = append(commits, mergeCommit) } - return commits, nil + return commits, elidedCommits, nil } // ensureCloneForRepo ensures that the repo exists on disk, is cloned, and has remotes for diff --git a/pkg/cli/admin/release/git_test.go b/pkg/cli/admin/release/git_test.go index 4b1bbd3997..d3f6517675 100644 --- a/pkg/cli/admin/release/git_test.go +++ b/pkg/cli/admin/release/git_test.go @@ -22,14 +22,15 @@ func (g fakeGit) exec(commands ...string) (string, error) { func Test_mergeLogForRepo(t *testing.T) { tests := []struct { - name string - input string - repo string - from string - to string - squash bool - want []MergeCommit - wantErr bool + name string + input string + repo string + from string + to string + squash bool + want []MergeCommit + wantElidedCommits int + wantErr bool }{ { input: "abc\x1e1\x1eMerge pull request #145 from\x1eBug 1743564: test", @@ -291,7 +292,7 @@ func Test_mergeLogForRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := fakeGit{input: tt.input, squash: tt.squash} - got, err := mergeLogForRepo(g, tt.repo, "a", "b") + got, elidedCommits, err := mergeLogForRepo(g, tt.repo, "a", "b") if (err != nil) != tt.wantErr { t.Errorf("mergeLogForRepo() error = %v, wantErr %v", err, tt.wantErr) return @@ -299,6 +300,9 @@ func Test_mergeLogForRepo(t *testing.T) { if !reflect.DeepEqual(got, tt.want) { t.Errorf("mergeLogForRepo(): %s", diff.ObjectReflectDiff(tt.want, got)) } + if elidedCommits != tt.wantElidedCommits { + t.Errorf("mergeLogForRepo(): %d elided commits report differs from expected %d", elidedCommits, tt.wantElidedCommits) + } }) } } diff --git a/pkg/cli/admin/release/info.go b/pkg/cli/admin/release/info.go index ba7f58810f..2367751d1e 100644 --- a/pkg/cli/admin/release/info.go +++ b/pkg/cli/admin/release/info.go @@ -1794,16 +1794,17 @@ func describeChangelog(out, errOut io.Writer, releaseInfo *ReleaseInfo, diff *Re } } for _, change := range codeChanges { - u, commits, err := commitsForRepo(dir, change, out, errOut) + u, commits, elidedCommits, err := commitsForRepo(dir, change, out, errOut) if err != nil { fmt.Fprintf(errOut, "error: %v\n", err) hasError = true continue } - if len(commits) > 0 { + if len(commits) > 0 || elidedCommits > 0 { info := ChangeLogImageInfo{ - Name: strings.Join(change.ImagesAffected, ", "), - Commits: []CommitInfo{}, + Name: strings.Join(change.ImagesAffected, ", "), + Commits: []CommitInfo{}, + ElidedCommits: elidedCommits, } if u.Host == "github.com" { info.Path = fmt.Sprintf("https://github.com%s/tree/%s", u.Path, change.To) @@ -1903,13 +1904,13 @@ func describeChangelog(out, errOut io.Writer, releaseInfo *ReleaseInfo, diff *Re } for _, change := range codeChanges { - u, commits, err := commitsForRepo(dir, change, out, errOut) + u, commits, elidedCommits, err := commitsForRepo(dir, change, out, errOut) if err != nil { fmt.Fprintf(errOut, "error: %v\n", err) hasError = true continue } - if len(commits) > 0 { + if len(commits) > 0 || elidedCommits > 0 { if u.Host == "github.com" { fmt.Fprintf(out, "### [%s](https://github.com%s/tree/%s)\n\n", strings.Join(change.ImagesAffected, ", "), u.Path, change.To) } else { @@ -1930,6 +1931,9 @@ func describeChangelog(out, errOut io.Writer, releaseInfo *ReleaseInfo, diff *Re } fmt.Fprintf(out, "\n") } + if elidedCommits > 0 { + fmt.Fprintf(out, "* And %d elided commits (e.g. from squash or rebase merges)\n", elidedCommits) + } if u.Host == "github.com" { fmt.Fprintf(out, "* [Full changelog](%s)\n\n", fmt.Sprintf("https://%s%s/compare/%s...%s", u.Host, u.Path, change.From, change.To)) } else { @@ -1955,7 +1959,7 @@ func describeBugs(out, errOut io.Writer, diff *ReleaseDiff, dir string, format s bugIDs := make(map[string]Ref) for _, change := range codeChanges { - _, commits, err := commitsForRepo(dir, change, out, errOut) + _, commits, _, err := commitsForRepo(dir, change, out, errOut) if err != nil { fmt.Fprintf(errOut, "error: %v\n", err) hasError = true @@ -2479,20 +2483,20 @@ func (c CodeChange) ToShort() string { return c.To } -func commitsForRepo(dir string, change CodeChange, out, errOut io.Writer) (*url.URL, []MergeCommit, error) { +func commitsForRepo(dir string, change CodeChange, out, errOut io.Writer) (*url.URL, []MergeCommit, int, error) { u, err := sourceLocationAsURL(change.Repo) if err != nil { - return nil, nil, fmt.Errorf("The source repository cannot be parsed %s: %v", change.Repo, err) + return nil, nil, 0, fmt.Errorf("The source repository cannot be parsed %s: %v", change.Repo, err) } g, err := ensureCloneForRepo(dir, change.Repo, change.AlternateRepos, errOut, errOut) if err != nil { - return nil, nil, err + return u, nil, 0, err } - commits, err := mergeLogForRepo(g, change.Repo, change.From, change.To) + commits, elidedCommits, err := mergeLogForRepo(g, change.Repo, change.From, change.To) if err != nil { - return nil, nil, fmt.Errorf("Could not load commits for %s: %v", change.Repo, err) + return u, commits, elidedCommits, fmt.Errorf("Could not load commits for %s: %v", change.Repo, err) } - return u, commits, nil + return u, commits, elidedCommits, nil } func releaseDiffContentChanges(diff *ReleaseDiff) ([]CodeChange, []ImageChange, []string) { @@ -2733,6 +2737,7 @@ type ChangeLogImageInfo struct { Commit string `json:"commit,omitempty"` ImageRef string `json:"imageRef,omitempty"` Commits []CommitInfo `json:"commits,omitempty"` + ElidedCommits int `json:"elidedCommits,omitempty"` FullChangeLog string `json:"fullChangeLog,omitempty"` }