git-codereview: parse branch commits correctly during b.Pending

Change-Id: I71b087ec51f6101022e950f783e09e42f7e4e57d
Reviewed-on: https://go-review.googlesource.com/2787
Reviewed-by: Andrew Gerrand <adg@golang.org>

2 files changed, 54 insertions, 2 deletions

diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index cdec9c3..2a69dcd 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -28,6 +28,7 @@ type Commit struct {
 	Hash      string // commit hash
 	ShortHash string // abbreviated commit hash
 	Parent    string // parent hash
+	Merge     string // for merges, hash of commit being merged into Parent
 	Message   string // commit message
 	Subject   string // first line of commit message
 	ChangeID  string // Change-Id in commit message ("" if missing)
@@ -119,8 +120,9 @@ func (b *Branch) loadPending() {
 	}
 
 	// Note: --topo-order means child first, then parent.
+	origin := b.OriginBranch()
 	const numField = 5
-	all := getOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", b.OriginBranch()+".."+b.Name, "--")
+	all := getOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", origin+".."+b.Name, "--")
 	fields := strings.Split(all, "\x00")
 	if len(fields) < numField {
 		return // nothing pending
@@ -128,6 +130,7 @@ func (b *Branch) loadPending() {
 	for i, field := range fields {
 		fields[i] = strings.TrimLeft(field, "\r\n")
 	}
+	foundMergeBranchpoint := false
 	for i := 0; i+numField <= len(fields); i += numField {
 		c := &Commit{
 			Hash:      fields[i],
@@ -136,6 +139,25 @@ func (b *Branch) loadPending() {
 			Message:   fields[i+3],
 			Subject:   fields[i+4],
 		}
+		if j := strings.Index(c.Parent, " "); j >= 0 {
+			c.Parent, c.Merge = c.Parent[:j], c.Parent[j+1:]
+			// Found merge point.
+			// Merges break the invariant that the last shared commit (the branchpoint)
+			// is the parent of the final commit in the log output.
+			// If c.Parent is on the origin branch, then since we are reading the log
+			// in (reverse) topological order, we know that c.Parent is the actual branchpoint,
+			// even if we later see additional commits on a different branch leading down to
+			// a lower location on the same origin branch.
+			// Check c.Merge (the second parent) too, so we don't depend on the parent order.
+			if strings.Contains(getOutput("git", "branch", "-a", "--contains", c.Parent), " "+origin+"\n") {
+				foundMergeBranchpoint = true
+				b.branchpoint = c.Parent
+			}
+			if strings.Contains(getOutput("git", "branch", "-a", "--contains", c.Merge), " "+origin+"\n") {
+				foundMergeBranchpoint = true
+				b.branchpoint = c.Merge
+			}
+		}
 		for _, line := range strings.Split(c.Message, "\n") {
 			// Note: Keep going even if we find one, so that
 			// we take the last Change-Id line, just in case
@@ -148,7 +170,9 @@ func (b *Branch) loadPending() {
 		}
 
 		b.pending = append(b.pending, c)
-		b.branchpoint = c.Parent
+		if !foundMergeBranchpoint {
+			b.branchpoint = c.Parent
+		}
 	}
 	b.commitsAhead = len(b.pending)
 	b.commitsBehind = len(getOutput("git", "log", "--format=format:x", b.Name+".."+b.OriginBranch(), "--"))
diff --git a/git-codereview/branch_test.go b/git-codereview/branch_test.go
index fdfb044..9a3ab5c 100644
--- a/git-codereview/branch_test.go
+++ b/git-codereview/branch_test.go
@@ -128,3 +128,31 @@ func TestBranchpoint(t *testing.T) {
 		gt.work(t)
 	}
 }
+
+func TestBranchpointMerge(t *testing.T) {
+	gt := newGitTest(t)
+	defer gt.done()
+
+	// commit more work on master
+	write(t, gt.server+"/file", "more work")
+	trun(t, gt.server, "git", "commit", "-m", "work", "file")
+
+	// update client
+	trun(t, gt.client, "git", "checkout", "master")
+	trun(t, gt.client, "git", "pull")
+
+	hash := strings.TrimSpace(trun(t, gt.client, "git", "rev-parse", "HEAD"))
+
+	// merge dev.branch
+	testMain(t, "change", "work")
+	trun(t, gt.client, "git", "merge", "-m", "merge", "origin/dev.branch")
+
+	// check branchpoint is old head (despite this commit having two parents)
+	bp := CurrentBranch().Branchpoint()
+	if bp != hash {
+		t.Logf("branches:\n%s", trun(t, gt.client, "git", "branch", "-a", "-v"))
+		t.Logf("log:\n%s", trun(t, gt.client, "git", "log", "--graph", "--decorate"))
+		t.Logf("log origin/master..HEAD:\n%s", trun(t, gt.client, "git", "log", "origin/master..HEAD"))
+		t.Fatalf("branchpoint=%q, want %q", bp, hash)
+	}
+}