git-codereview: begin to handle multiple-change branches

Gerrit supports multiple-change branches, and we'd like to make
git-codereview useful for people using this mode of work.
This CL is the first step.

- remove error message on detecting a multiple-change branch
- require 'git submit hash' in multiple-change branch
- make git submit, git sync cleanup safe for multiple-change branch
- adjust git pending output to show full information about multiple-change branch
- add pending -c to show only current branch, since output is getting long

We're not advertising or supporting this mode yet. For now the only
way to enter it is to run 'git commit' to create the second commit.
Perhaps eventually we will support something like 'git change -new'.

Change-Id: I8284a7c230503061d3e6d7cce0be7d8d05c9b2a3
Reviewed-on: https://go-review.googlesource.com/2110
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Austin Clements <austin@google.com>

1 files changed, 174 insertions, 52 deletions

diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index de9e4f5..150d91f 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -14,19 +14,19 @@ import (
 	"time"
 )
 
-var pendingLocal bool // -l flag, use only local operations (no network)
+var (
+	pendingLocal   bool // -l flag, use only local operations (no network)
+	pendingCurrent bool // -c flag, show only current branch
+)
 
 // A pendingBranch collects information about a single pending branch.
 // We overlap the reading of this information for each branch.
 type pendingBranch struct {
-	*Branch                 // standard Branch functionality
-	g         *GerritChange // state loaded from Gerrit
-	gerr      error         // error loading state from Gerrit
-	current   bool          // is this the current branch?
-	committed []string      // files committed on this branch
-	staged    []string      // files in staging area, only if current==true
-	unstaged  []string      // files unstaged in local directory, only if current==true
-	untracked []string      // files untracked in local directory, only if current==true
+	*Branch            // standard Branch functionality
+	current   bool     // is this the current branch?
+	staged    []string // files in staging area, only if current==true
+	unstaged  []string // files unstaged in local directory, only if current==true
+	untracked []string // files untracked in local directory, only if current==true
 }
 
 // load populates b with information about the branch.
@@ -40,15 +40,19 @@ func (b *pendingBranch) load() {
 	if b.current {
 		b.staged, b.unstaged, b.untracked = LocalChanges()
 	}
-	if b.parentHash != "" && b.commitHash != "" {
-		b.committed = getLines("git", "diff", "--name-only", b.parentHash, b.commitHash, "--")
-	}
-	if !pendingLocal {
-		b.g, b.gerr = b.GerritChange("DETAILED_LABELS", "CURRENT_REVISION", "MESSAGES", "DETAILED_ACCOUNTS")
+	for _, c := range b.Pending() {
+		c.committed = getLines("git", "diff", "--name-only", c.Parent, c.Hash, "--")
+		if !pendingLocal {
+			c.g, c.gerr = b.GerritChange(c, "DETAILED_LABELS", "CURRENT_REVISION", "MESSAGES", "DETAILED_ACCOUNTS")
+		}
+		if c.g == nil {
+			c.g = new(GerritChange) // easier for formatting code
+		}
 	}
 }
 
 func pending(args []string) {
+	flags.BoolVar(&pendingCurrent, "c", false, "show only current branch")
 	flags.BoolVar(&pendingLocal, "l", false, "use only local information - no network operations")
 	flags.Parse(args)
 	if len(flags.Args()) > 0 {
@@ -63,10 +67,14 @@ func pending(args []string) {
 	}
 
 	// Build list of pendingBranch structs to be filled in.
-	current := CurrentBranch().Name
 	var branches []*pendingBranch
-	for _, b := range LocalBranches() {
-		branches = append(branches, &pendingBranch{Branch: b, current: b.Name == current})
+	if pendingCurrent {
+		branches = []*pendingBranch{{Branch: CurrentBranch(), current: true}}
+	} else {
+		current := CurrentBranch().Name
+		for _, b := range LocalBranches() {
+			branches = append(branches, &pendingBranch{Branch: b, current: b.Name == current})
+		}
 	}
 
 	// The various data gathering is a little slow,
@@ -103,7 +111,7 @@ func pending(args []string) {
 	}
 
 	// Print output, like:
-	//	pending d8fcb99 https://go-review.googlesource.com/1620 (current branch, 1 behind)
+	//	pending 2378abf..d8fcb99 https://go-review.googlesource.com/1620 (current branch, mailed, submitted, 1 behind)
 	//		git-codereview: expand pending output
 	//
 	//		for pending:
@@ -129,11 +137,77 @@ func pending(args []string) {
 	//			git-codereview/review.go
 	//			git-codereview/submit.go
 	//			git-codereview/sync.go
+	//		Files staged:
+	//			git-codereview/sync.go
+	//		Files unstaged:
+	//			git-codereview/submit.go
 	//		Files untracked:
 	//			git-codereview/doc.go
 	//			git-codereview/savedmail.go.txt
 	//
+	// If there are multiple changes in the current branch, the output splits them out into separate sections,
+	// in reverse commit order, to match git log output.
+	//
+	//	wbshadow 7a524a1..a496c1e (current branch, all mailed, 23 behind)
+	//	+ uncommitted changes
+	//		Files unstaged:
+	//			src/runtime/proc1.go
+	//
+	//	+ a496c1e https://go-review.googlesource.com/2064 (mailed)
+	//		runtime: add missing write barriers in append's copy of slice data
+	//
+	//		Found with GODEBUG=wbshadow=1 mode.
+	//		Eventually that will run automatically, but right now
+	//		it still detects other missing write barriers.
+	//
+	//		Change-Id: Ic8624401d7c8225a935f719f96f2675c6f5c0d7c
+	//
+	//		Code-Review:
+	//			+0 Austin Clements, Rick Hudson
+	//		Files in this change:
+	//			src/runtime/slice.go
+	//
+	//	+ 95390c7 https://go-review.googlesource.com/2061 (mailed)
+	//		runtime: add GODEBUG wbshadow for finding missing write barriers
+	//
+	//		This is the detection code. It works well enough that I know of
+	//		a handful of missing write barriers. However, those are subtle
+	//		enough that I'll address them in separate followup CLs.
+	//
+	//		Change-Id: If863837308e7c50d96b5bdc7d65af4969bf53a6e
+	//
+	//		Code-Review:
+	//			+0 Austin Clements, Rick Hudson
+	//		Files in this change:
+	//			src/runtime/extern.go
+	//			src/runtime/malloc1.go
+	//			src/runtime/malloc2.go
+	//			src/runtime/mgc.go
+	//			src/runtime/mgc0.go
+	//			src/runtime/proc1.go
+	//			src/runtime/runtime1.go
+	//			src/runtime/runtime2.go
+	//			src/runtime/stack1.go
+	//
+	// In multichange mode, the first line only gives information that applies to the entire
+	// branch: the name, the commit range, whether this is the current branch, whether
+	// all the commits are mailed/submitted, how far behind.
+	// The individual change sections have per-change information: the hash of that
+	// commit, the URL on the Gerrit server, whether it is mailed/submitted, the list of
+	// files in that commit. The uncommitted file modifications are shown as a separate
+	// section, at the beginning, to fit better into the reverse commit order.
+
 	var buf bytes.Buffer
+	printFileList := func(name string, list []string) {
+		if len(list) == 0 {
+			return
+		}
+		fmt.Fprintf(&buf, "\tFiles %s:\n", name)
+		for _, file := range list {
+			fmt.Fprintf(&buf, "\t\t%s\n", file)
+		}
+	}
+
 	for _, b := range branches {
 		if !b.current && b.commitsAhead == 0 {
 			// Hide branches with no work on them.
@@ -141,21 +215,30 @@ func pending(args []string) {
 		}
 
 		fmt.Fprintf(&buf, "%s", b.Name)
-		if b.shortCommitHash != "" {
-			fmt.Fprintf(&buf, " %s", b.shortCommitHash)
+		work := b.Pending()
+		if len(work) > 0 {
+			fmt.Fprintf(&buf, " %.7s..%s", b.branchpoint, work[0].ShortHash)
 		}
-		if b.g != nil && b.g.Number != 0 {
-			fmt.Fprintf(&buf, " %s/%d", auth.url, b.g.Number)
+		if len(work) == 1 && work[0].g.Number != 0 {
+			fmt.Fprintf(&buf, " %s/%d", auth.url, work[0].g.Number)
 		}
 		var tags []string
 		if b.current {
 			tags = append(tags, "current branch")
 		}
-		if b.g != nil && b.g.CurrentRevision == b.commitHash {
-			tags = append(tags, "mailed")
+		if allMailed(work) {
+			if len(work) == 1 {
+				tags = append(tags, "mailed")
+			} else if len(work) > 1 {
+				tags = append(tags, "all mailed")
+			}
 		}
-		if b.g != nil && b.g.Status == "MERGED" {
-			tags = append(tags, "submitted")
+		if allSubmitted(work) {
+			if len(work) == 1 {
+				tags = append(tags, "submitted")
+			} else if len(work) > 1 {
+				tags = append(tags, "all submitted")
+			}
 		}
 		if b.commitsBehind > 0 {
 			tags = append(tags, fmt.Sprintf("%d behind", b.commitsBehind))
@@ -167,20 +250,46 @@ func pending(args []string) {
 		if text := b.errors(); text != "" {
 			fmt.Fprintf(&buf, "\tERROR: %s\n\n", strings.Replace(strings.TrimSpace(text), "\n", "\n\t", -1))
 		}
-		if b.message != "" {
-			msg := strings.TrimRight(b.message, "\r\n")
-			fmt.Fprintf(&buf, "\t%s\n", strings.Replace(msg, "\n", "\n\t", -1))
+
+		if b.current && len(work) > 1 && len(b.staged)+len(b.unstaged)+len(b.untracked) > 0 {
+			fmt.Fprintf(&buf, "+ uncommitted changes\n")
+			printFileList("staged", b.staged)
+			printFileList("unstaged", b.unstaged)
+			printFileList("untracked", b.untracked)
 			fmt.Fprintf(&buf, "\n")
 		}
-		if b.g != nil {
-			for _, name := range b.g.LabelNames() {
-				label := b.g.Labels[name]
+
+		for _, c := range work {
+			g := c.g
+			if len(work) > 1 {
+				fmt.Fprintf(&buf, "+ %s", c.ShortHash)
+				if g.Number != 0 {
+					fmt.Fprintf(&buf, " %s/%d", auth.url, g.Number)
+				}
+				var tags []string
+				if g.CurrentRevision == c.Hash {
+					tags = append(tags, "mailed")
+				}
+				if g.Status == "MERGED" {
+					tags = append(tags, "submitted")
+				}
+				if len(tags) > 0 {
+					fmt.Fprintf(&buf, " (%s)", strings.Join(tags, ", "))
+				}
+				fmt.Fprintf(&buf, "\n")
+			}
+			msg := strings.TrimRight(c.Message, "\r\n")
+			fmt.Fprintf(&buf, "\t%s\n", strings.Replace(msg, "\n", "\n\t", -1))
+			fmt.Fprintf(&buf, "\n")
+
+			for _, name := range g.LabelNames() {
+				label := g.Labels[name]
 				minValue := 10000
 				maxValue := -10000
 				byScore := map[int][]string{}
 				for _, x := range label.All {
 					// Hide CL owner unless owner score is nonzero.
-					if b.g.Owner != nil && x.ID == b.g.Owner.ID && x.Value == 0 {
+					if g.Owner != nil && x.ID == g.Owner.ID && x.Value == 0 {
 						continue
 					}
 					byScore[x.Value] = append(byScore[x.Value], x.Name)
@@ -201,27 +310,45 @@ func pending(args []string) {
 					fmt.Fprintf(&buf, "\t\t%+d %s\n", score, strings.Join(who, ", "))
 				}
 			}
-		}
-		printFileList := func(name string, list []string) {
-			if len(list) == 0 {
-				return
-			}
-			fmt.Fprintf(&buf, "\tFiles %s:\n", name)
-			for _, file := range list {
-				fmt.Fprintf(&buf, "\t\t%s\n", file)
+
+			printFileList("in this change", c.committed)
+			if b.current && len(work) == 1 {
+				// staged file list will be printed next
+			} else {
+				fmt.Fprintf(&buf, "\n")
 			}
 		}
-		printFileList("in this change", b.committed)
-		printFileList("staged", b.staged)
-		printFileList("unstaged", b.unstaged)
-		printFileList("untracked", b.untracked)
-
-		fmt.Fprintf(&buf, "\n")
+		if b.current && len(work) <= 1 {
+			printFileList("staged", b.staged)
+			printFileList("unstaged", b.unstaged)
+			printFileList("untracked", b.untracked)
+			fmt.Fprintf(&buf, "\n")
+		}
 	}
 
 	stdout().Write(buf.Bytes())
 }
 
+// allMailed reports whether all commits in work have been posted to Gerrit.
+func allMailed(work []*Commit) bool {
+	for _, c := range work {
+		if c.Hash != c.g.CurrentRevision {
+			return false
+		}
+	}
+	return true
+}
+
+// allSubmitted reports whether all commits in work have been submitted to the origin branch.
+func allSubmitted(work []*Commit) bool {
+	for _, c := range work {
+		if c.g.Status != "MERGED" {
+			return false
+		}
+	}
+	return true
+}
+
 // errors returns any errors that should be displayed
 // about the state of the current branch, diagnosing common mistakes.
 func (b *Branch) errors() string {
@@ -230,11 +357,6 @@ func (b *Branch) errors() string {
 	if !b.IsLocalOnly() && b.commitsAhead > 0 {
 		fmt.Fprintf(&buf, "Branch contains %d commit%s not on origin/%s.\n", b.commitsAhead, suffix(b.commitsAhead, "s"), b.Name)
 		fmt.Fprintf(&buf, "\tDo not commit directly to %s branch.\n", b.Name)
-	} else if b.commitsAhead > 1 {
-		fmt.Fprintf(&buf, "Branch contains %d commits not on origin/%s.\n", b.commitsAhead, b.OriginBranch())
-		fmt.Fprintf(&buf, "\tThere should be at most one.\n")
-		fmt.Fprintf(&buf, "\tUse 'git change', not 'git commit'.\n")
-		fmt.Fprintf(&buf, "\tRun 'git log %s..%s' to list commits.\n", b.OriginBranch(), b.Name)
 	}
 	return buf.String()
 }