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>
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 @@
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 @@
}
// 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 @@
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 @@
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 @@
}
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 @@
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)
+ }
+}
