pkg/oc/cli/admin/release: Issues from non-fist-parent commits

Sometimes a single pull request will address multiple issues, and
listing them all in a PR subject is not feasible.  With this commit, I
iterate over all commits in the given range (merge or not), but only
return a slice for the first-parent chain (which in most cases will be
a series of merges to master).  For commits that are part of the
retrieved graph but which lie outside the first-parent chain, I find
the nearest first-parent ancestor and attach any referenced issues to
that ancestor.  The new 'issue' structure sets the stage for future
work to also support references to GitHub and other issue stores,
although I haven't added extractors for those yet.

pkg/oc/cli/admin/release/git.go

@@ -2,19 +2,20 @@ package release
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"net/url"
 	"os/exec"
 	"path"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
 
 	"github.com/golang/glog"
+	"github.com/pkg/errors"
 )
 
 // git is a wrapper to invoke git safely, similar to
@@ -111,16 +112,19 @@ func sourceLocationAsRelativePath(dir, location string) (string, error) {
 	return basePath, nil
 }
 
-type MergeCommit struct {
-	CommitDate time.Time
+type commit struct {
+	// Merkle graph information
+	Hash          string
+	Parents       []string
+	KnownChildren []string
 
-	Commit        string
-	ParentCommits []string
+	// Local Merkle-graph node information
+	Subject       string
+	CommitterDate time.Time
 
+	// Extracted metadata
 	PullRequest int
-	Bug         int
-
-	Subject string
+	Issues      []*issue
 }
 
 func gitOutputToError(err error, out string) error {
@@ -134,7 +138,7 @@ func gitOutputToError(err error, out string) error {
 	return fmt.Errorf(out)
 }
 
-func mergeLogForRepo(g *git, from, to string) ([]MergeCommit, error) {
+func firstParentLogForRepo(g *git, from, to string) ([]*commit, error) {
 	if from == to {
 		return nil, nil
 	}
@@ -148,7 +152,7 @@ func mergeLogForRepo(g *git, from, to string) ([]MergeCommit, error) {
 		return nil, err
 	}
 
-	args := []string{"log", "--merges", "--topo-order", "-z", "--pretty=format:%H %P%x1E%ct%x1E%s%x1E%b", "--reverse", fmt.Sprintf("%s..%s", from, to)}
+	args := []string{"log", "--topo-order", "-z", "--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
@@ -174,52 +178,137 @@ func mergeLogForRepo(g *git, from, to string) ([]MergeCommit, error) {
 		glog.Infof("Got commit info:\n%s", strconv.Quote(out))
 	}
 
-	var commits []MergeCommit
+	var tip *commit
+	commitMap := map[string]*commit{}
 	for _, entry := range strings.Split(out, "\x00") {
+		if entry == "" {
+			continue
+		}
+
 		records := strings.Split(entry, "\x1e")
 		if len(records) != 4 {
 			return nil, fmt.Errorf("unexpected git log output width %d columns", len(records))
 		}
+
+		commitHashes := strings.Split(records[0], " ")
 		unixTS, err := strconv.ParseInt(records[1], 10, 64)
 		if err != nil {
 			return nil, fmt.Errorf("unexpected timestamp: %v", err)
 		}
-		commitValues := strings.Split(records[0], " ")
 
-		mergeCommit := MergeCommit{
-			CommitDate:    time.Unix(unixTS, 0).UTC(),
-			Commit:        commitValues[0],
-			ParentCommits: commitValues[1:],
+		cmt := &commit{
+			CommitterDate: time.Unix(unixTS, 0).UTC(),
+			Hash:          commitHashes[0],
+			Parents:       commitHashes[1:],
+			Subject:       records[2],
+		}
+		for _, potentialChild := range commitMap {
+			for _, hash := range potentialChild.Parents {
+				if hash == cmt.Hash {
+					cmt.KnownChildren = append(cmt.KnownChildren, potentialChild.Hash)
+					break
+				}
+			}
 		}
 
-		msg := records[3]
-		if m := reBug.FindStringSubmatch(msg); m != nil {
-			mergeCommit.Subject = msg[len(m[0]):]
-			mergeCommit.Bug, err = strconv.Atoi(m[1])
-			if err != nil {
-				return nil, fmt.Errorf("could not extract bug number from %q: %v", msg, err)
+		commitMap[cmt.Hash] = cmt
+		if tip == nil {
+			tip = cmt
+		}
+
+		body := records[3]
+		if len(cmt.Parents) > 1 { // merge commit
+			if m := rePR.FindStringSubmatch(cmt.Subject); m != nil {
+				cmt.PullRequest, err = strconv.Atoi(m[1])
+				if err != nil {
+					return nil, fmt.Errorf("could not extract PR number from %q: %v", cmt.Subject, err)
+				}
 			}
-		} else {
-			mergeCommit.Subject = msg
+
+			cmt.Subject = strings.SplitN(strings.TrimSpace(body), "\n", 2)[0]
 		}
-		mergeCommit.Subject = strings.TrimSpace(mergeCommit.Subject)
-		mergeCommit.Subject = strings.SplitN(mergeCommit.Subject, "\n", 2)[0]
 
-		mergeMsg := records[2]
-		if m := rePR.FindStringSubmatch(mergeMsg); m != nil {
-			mergeCommit.PullRequest, err = strconv.Atoi(m[1])
+		if m := reBug.FindStringSubmatch(cmt.Subject); m != nil {
+			bug, err := strconv.Atoi(m[1])
 			if err != nil {
-				return nil, fmt.Errorf("could not extract PR number from %q: %v", mergeMsg, err)
+				return nil, fmt.Errorf("could not extract bug number from %q: %v", cmt.Subject, err)
 			}
-		} else {
-			glog.V(2).Infof("Omitted commit %s which has no pull-request", mergeCommit.Commit)
+			cmt.Subject = cmt.Subject[len(m[0]):]
+			cmt.Issues = append(cmt.Issues, &issue{
+				Store: "rhbz",
+				ID:    bug,
+				URI:   fmt.Sprintf("https://bugzilla.redhat.com/show_bug.cgi?id=%d", bug),
+			})
+		}
+	}
+
+	exists := struct{}{}
+	firstParents := map[string]struct{}{}
+	commits := []*commit{}
+	for cmt := tip; cmt != nil; cmt = commitMap[cmt.Parents[0]] { // [0] is ok, because there will be no orphans in a from..to log
+		commits = append(commits, cmt)
+		firstParents[cmt.Hash] = exists
+	}
+
+	// collect issue information from commits outside the first-parent chain
+	for _, cmt := range commitMap {
+		if len(cmt.Issues) == 0 {
+			continue
+		}
+
+		if _, ok := firstParents[cmt.Hash]; ok {
 			continue
 		}
-		if len(mergeCommit.Subject) == 0 {
-			mergeCommit.Subject = "Merge"
+
+		// figure out which first-parent is closest
+		var closestFirstParent *commit
+		for generation := cmt.KnownChildren; closestFirstParent == nil && len(generation) > 0; {
+			nextGeneration := []string{}
+			for _, hash := range generation {
+				child := commitMap[hash]
+				if _, ok := firstParents[hash]; ok {
+					closestFirstParent = child
+					break
+				}
+				for _, grandChild := range child.KnownChildren {
+					nextGeneration = append(nextGeneration, grandChild)
+				}
+			}
+
+			generation = nextGeneration
 		}
 
-		commits = append(commits, mergeCommit)
+		if closestFirstParent == nil {
+			return nil, errors.Errorf("no first-parent found for %s", cmt.Hash)
+		}
+
+		for _, issue := range cmt.Issues {
+			alreadyOnClosestFirstParent := false
+			for _, mergeIssue := range closestFirstParent.Issues {
+				if mergeIssue.URI == issue.URI {
+					alreadyOnClosestFirstParent = true
+					break
+				}
+			}
+
+			if !alreadyOnClosestFirstParent {
+				closestFirstParent.Issues = append(closestFirstParent.Issues, issue)
+			}
+		}
+	}
+
+	for _, cmt := range commits {
+		sort.Slice(cmt.Issues, func(i, j int) bool {
+			if cmt.Issues[i].Store != cmt.Issues[j].Store {
+				return cmt.Issues[i].Store < cmt.Issues[j].Store
+			}
+
+			if cmt.Issues[i].ID != cmt.Issues[j].ID {
+				return cmt.Issues[i].ID < cmt.Issues[j].ID
+			}
+
+			return cmt.Issues[i].URI < cmt.Issues[j].URI
+		})
 	}
 
 	return commits, nil

pkg/oc/cli/admin/release/info.go

@@ -1039,7 +1039,7 @@ func describeChangelog(out, errOut io.Writer, diff *ReleaseDiff, dir string) err
 	}
 
 	for _, change := range codeChanges {
-		u, commits, err := commitsForRepo(dir, change, out, errOut)
+		u, commits, err := firstParentCommitsForRepo(dir, change, out, errOut)
 		if err != nil {
 			fmt.Fprintf(errOut, "error: %v\n", err)
 			hasError = true
@@ -1052,32 +1052,30 @@ func describeChangelog(out, errOut io.Writer, diff *ReleaseDiff, dir string) err
 				fmt.Fprintf(out, "### %s\n\n", strings.Join(change.ImagesAffected, ", "))
 			}
 			for _, commit := range commits {
-				var suffix string
+				entries := []string{}
+
+				if len(commit.Subject) > 0 {
+					entries = append(entries, commit.Subject)
+				}
+
 				switch {
 				case commit.PullRequest > 0:
-					suffix = fmt.Sprintf("[#%d](%s)", commit.PullRequest, fmt.Sprintf("https://%s%s/pull/%d", u.Host, u.Path, commit.PullRequest))
+					entries = append(entries, fmt.Sprintf("[#%d](%s)", commit.PullRequest, fmt.Sprintf("https://%s%s/pull/%d", u.Host, u.Path, commit.PullRequest)))
 				case u.Host == "github.com":
-					commit := commit.Commit[:8]
-					suffix = fmt.Sprintf("[%s](%s)", commit, fmt.Sprintf("https://%s%s/commit/%s", u.Host, u.Path, commit))
+					commit := commit.Hash[:8]
+					entries = append(entries, fmt.Sprintf("[%s](%s)", commit, fmt.Sprintf("https://%s%s/commit/%s", u.Host, u.Path, commit)))
 				default:
-					suffix = commit.Commit[:8]
+					entries = append(entries, commit.Hash[:8])
 				}
-				switch {
-				case commit.Bug > 0:
-					fmt.Fprintf(out,
-						"* [Bug %d](%s): %s %s\n",
-						commit.Bug,
-						fmt.Sprintf("https://bugzilla.redhat.com/show_bug.cgi?id=%d", commit.Bug),
-						commit.Subject,
-						suffix,
-					)
-				default:
-					fmt.Fprintf(out,
-						"* %s %s\n",
-						commit.Subject,
-						suffix,
-					)
+
+				for _, issue := range commit.Issues {
+					entries = append(entries, issue.Markdown())
 				}
+
+				fmt.Fprintf(out,
+					"* %s\n",
+					strings.Join(entries, " "),
+				)
 			}
 			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))
@@ -1103,17 +1101,18 @@ func describeBugs(out, errOut io.Writer, diff *ReleaseDiff, dir string, format s
 
 	bugIDs := sets.NewInt()
 	for _, change := range codeChanges {
-		_, commits, err := commitsForRepo(dir, change, out, errOut)
+		_, commits, err := firstParentCommitsForRepo(dir, change, out, errOut)
 		if err != nil {
 			fmt.Fprintf(errOut, "error: %v\n", err)
 			hasError = true
 			continue
 		}
 		for _, commit := range commits {
-			if commit.Bug == 0 {
-				continue
+			for _, issue := range commit.Issues {
+				if issue.Store == "rhbz" {
+					bugIDs.Insert(issue.ID)
+				}
 			}
-			bugIDs.Insert(commit.Bug)
 		}
 	}
 
@@ -1249,7 +1248,7 @@ func (c CodeChange) ToShort() string {
 	return c.To
 }
 
-func commitsForRepo(dir string, change CodeChange, out, errOut io.Writer) (*url.URL, []MergeCommit, error) {
+func firstParentCommitsForRepo(dir string, change CodeChange, out, errOut io.Writer) (*url.URL, []*commit, 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)
@@ -1258,7 +1257,7 @@ func commitsForRepo(dir string, change CodeChange, out, errOut io.Writer) (*url.
 	if err != nil {
 		return nil, nil, err
 	}
-	commits, err := mergeLogForRepo(g, change.From, change.To)
+	commits, err := firstParentLogForRepo(g, change.From, change.To)
 	if err != nil {
 		return nil, nil, fmt.Errorf("Could not load commits for %s: %v", change.Repo, err)
 	}

pkg/oc/cli/admin/release/issue.go

@@ -0,0 +1,20 @@
+package release
+
+import (
+	"fmt"
+)
+
+type issue struct {
+	// Store for the issue, e.g. "rhbz" for https://bugzilla.redhat.com, or "origin" for https://github.com/openshift/origin/issues.
+	Store string
+
+	// ID for the issue, e.g. 123.
+	ID int
+
+	// URI for the issue, e.g. https://bugzilla.redhat.com/show_bug.cgi?id=123.
+	URI string
+}
+
+func (i *issue) Markdown() string {
+	return fmt.Sprintf("[%s#%d](%s)", i.Store, i.ID, i.URI) // TODO: proper escaping
+}