OCPBUGS-60989: pkg/cli/admin/release/info: Mention elided first-parent, non-merge commits

pkg/cli/admin/release/git.go

@@ -215,34 +215,35 @@ 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)}
 	out, err := g.exec(args...)
 	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

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,14 +292,17 @@ 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
 			}
 			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)
+			}
 		})
 	}
 }

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"`
 }