pkg/cli/admin/release/info: Mention elided first-parent, non-merge co…

…mmits

Originally, the change-log generation focused on merge commits, which
are common in many repositories.  For example, in this 'oc' repository:

  $ git log --graph --oneline -10
  *   559c67e (HEAD -> main, origin/main) Merge pull request #2083 from wking/oc-adm-upgrade-recommend-certificate-authority
  |\
  | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc.
  * |   e7900fe Merge pull request #2082 from ardaguclu/code-rabbit-test
  |\ \
  | |/
  |/|
  | * ffe752e Remove mentioning about IRC channel from README
  * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (#2063)
  * |   1c54ec0 Merge pull request #2077 from hongkailiu/OTA-1601
  |\ \
  | * | 5f1dbb8 Move not-lines-related table.Write out of the loop
  | * | a352662 Truncate long reasons
  | * | 11f9f47 Reproduce the long reason issue
  | * | 970f883 status: improve line break handling for CO Message

4299014 (Add squash-merge support into oc adm release info,
2022-04-26, #1116), and mentioned the insights operator:

  $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator
  $ cd insights-operator
  $ git log --graph --oneline -5
  * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (#1110)
  * 737b59b fix: incorrect anonymization of domains (#1111)
  * b5185da feat: add permissions to gather clusterrole (#1106)
  * 3099eb0 feat: Allow on-demand gathering during initial periodic run (#1109)
  * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (#1104)

But some repositories also use a mixture of real merges and squash or
rebase merges:

  $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller
  $ cd operator-framework-operator-controller
  $ git log --graph --oneline -11
  * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests
  * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present.
  * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests
  * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin
  * 28299f38 UPSTREAM: <carry>: remove obsolete owners
  * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests
  * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present.
  * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation
  *   a7d35272 Merge pull request #453 from openshift-bot/synchronize-upstream
  |\
  | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker
  | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration

Before this commit, the logic would find the merge commits, assume
that all content came in via merge commits, and not mention the other
changes:

  $ git log --first-parent --merges --oneline
  a7d3527 Merge pull request #453 from openshift-bot/synchronize-upstream
  4cae159 Merge pull request #449 from openshift-bot/synchronize-upstream
  82f63dc Merge pull request #444 from openshift-bot/synchronize-upstream
  6f91d84 Merge pull request #435 from openshift-bot/synchronize-upstream

With this commit, we'll continue to include a line for each of those
merges.  But we'll also include the count of any elided, non-merge,
first-parent commits:

  $ git log --first-parent --no-merges --oneline | wc -l
  16

Folks interested in what exactly was elided will have to click through
to 'Full changelog' or otherwise go digging.  But at least they'll get
the count of elided commits to know that there is more information
there to see behind that click.

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