git-codereview: revise pending

- Use multiple commit output form always.
  We're going to start suggesting the use of multiple commits,
  and it is confusing to flip between the two displays
  based on the number of commits.

- Document pending -c option (current branch only).

- Add pending -s option (short form).

While doing this, I got my client into a state where I had a tag
and a branch with the same name. Make things work in that mode too.

Change-Id: I4a3d73ce88be78b04d5bc4e56f1e3bed435cfde7
Reviewed-on: https://go-review.googlesource.com/3621
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

6 files changed, 221 insertions, 119 deletions

diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 1193da9..48ba8bb 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -41,7 +41,7 @@ type Commit struct {
 
 // CurrentBranch returns the current branch.
 func CurrentBranch() *Branch {
-	name := trim(cmdOutput("git", "rev-parse", "--abbrev-ref", "HEAD"))
+	name := strings.TrimPrefix(trim(cmdOutput("git", "rev-parse", "--abbrev-ref", "HEAD")), "heads/")
 	return &Branch{Name: name}
 }
 
@@ -82,6 +82,13 @@ func (b *Branch) OriginBranch() string {
 	panic("not reached")
 }
 
+func (b *Branch) FullName() string {
+	if b.Name != "HEAD" {
+		return "refs/heads/" + b.Name
+	}
+	return b.Name
+}
+
 // IsLocalOnly reports whether b is a local work branch (only local, not known to remote server).
 func (b *Branch) IsLocalOnly() bool {
 	return "origin/"+b.Name != b.OriginBranch()
@@ -122,7 +129,7 @@ func (b *Branch) loadPending() {
 	// Note: --topo-order means child first, then parent.
 	origin := b.OriginBranch()
 	const numField = 5
-	all := trim(cmdOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", origin+".."+b.Name, "--"))
+	all := trim(cmdOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", origin+".."+b.FullName(), "--"))
 	fields := strings.Split(all, "\x00")
 	if len(fields) < numField {
 		return // nothing pending
@@ -175,7 +182,7 @@ func (b *Branch) loadPending() {
 		}
 	}
 	b.commitsAhead = len(b.pending)
-	b.commitsBehind = len(trim(cmdOutput("git", "log", "--format=format:x", b.Name+".."+b.OriginBranch(), "--")))
+	b.commitsBehind = len(trim(cmdOutput("git", "log", "--format=format:x", b.FullName()+".."+b.OriginBranch(), "--")))
 }
 
 // Submitted reports whether some form of b's pending commit
diff --git a/git-codereview/gofmt.go b/git-codereview/gofmt.go
index 96b5e86..4608a3a 100644
--- a/git-codereview/gofmt.go
+++ b/git-codereview/gofmt.go
@@ -120,7 +120,7 @@ func runGofmt(flags int) (files []string, stderrText string) {
 
 	// Find files modified in the index compared to the branchpoint.
 	branchpt := b.Branchpoint()
-	if strings.Contains(cmdOutput("git", "branch", "-r", "--contains", b.Name), "origin/") {
+	if strings.Contains(cmdOutput("git", "branch", "-r", "--contains", b.FullName()), "origin/") {
 		// This is a branch tag move, not an actual change.
 		// Use HEAD as branch point, so nothing will appear changed.
 		// We don't want to think about gofmt on already published
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index 1a58fe2..3239d23 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -15,8 +15,9 @@ import (
 )
 
 var (
-	pendingLocal   bool // -l flag, use only local operations (no network)
-	pendingCurrent bool // -c flag, show only current branch
+	pendingLocal       bool // -l flag, use only local operations (no network)
+	pendingCurrentOnly bool // -c flag, show only current branch
+	pendingShort       bool // -s flag, short display
 )
 
 // A pendingBranch collects information about a single pending branch.
@@ -52,11 +53,12 @@ func (b *pendingBranch) load() {
 }
 
 func pending(args []string) {
-	flags.BoolVar(&pendingCurrent, "c", false, "show only current branch")
+	flags.BoolVar(&pendingCurrentOnly, "c", false, "show only current branch")
 	flags.BoolVar(&pendingLocal, "l", false, "use only local information - no network operations")
+	flags.BoolVar(&pendingShort, "s", false, "show short listing")
 	flags.Parse(args)
 	if len(flags.Args()) > 0 {
-		fmt.Fprintf(stderr(), "Usage: %s pending %s [-l]\n", os.Args[0], globalFlags)
+		fmt.Fprintf(stderr(), "Usage: %s pending %s [-c] [-l] [-s]\n", os.Args[0], globalFlags)
 		os.Exit(2)
 	}
 
@@ -67,13 +69,15 @@ func pending(args []string) {
 	}
 
 	// Build list of pendingBranch structs to be filled in.
+	// The current branch is always first.
 	var branches []*pendingBranch
-	if pendingCurrent {
-		branches = []*pendingBranch{{Branch: CurrentBranch(), current: true}}
-	} else {
+	branches = []*pendingBranch{{Branch: CurrentBranch(), current: true}}
+	if !pendingCurrentOnly {
 		current := CurrentBranch().Name
 		for _, b := range LocalBranches() {
-			branches = append(branches, &pendingBranch{Branch: b, current: b.Name == current})
+			if b.Name != current {
+				branches = append(branches, &pendingBranch{Branch: b})
+			}
 		}
 	}
 
@@ -110,45 +114,11 @@ func pending(args []string) {
 		<-done
 	}
 
-	// Print output, like:
-	//	pending 2378abf..d8fcb99 https://go-review.googlesource.com/1620 (current branch, mailed, submitted, 1 behind)
-	//		git-codereview: expand pending output
-	//
-	//		for pending:
-	//		- show full commit message
-	//		- show information about being behind upstream
-	//		- show list of modified files
-	//		- for current branch, show staged, unstaged, untracked files
-	//		- warn about being ahead of upstream on master
-	//		- warn about being multiple commits ahead of upstream
-	//
-	//		- add same warnings to change
-	//		- add change -a (mostly unrelated, but prompted by this work)
-	//
-	//		Change-Id: Ie480ba5b66cc07faffca421ee6c9623d35204696
-	//
-	//		Code-Review:
-	//			+2 Andrew Gerrand, Rob Pike
-	//		Files in this change:
-	//			git-codereview/api.go
-	//			git-codereview/branch.go
-	//			git-codereview/change.go
-	//			git-codereview/pending.go
-	//			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
-	//
+	// Print output.
 	// 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)
+	//	wbshadow 7a524a1..a496c1e (current branch, all mailed, 23 behind, tracking master)
 	//	+ uncommitted changes
 	//		Files unstaged:
 	//			src/runtime/proc1.go
@@ -189,13 +159,22 @@ func pending(args []string) {
 	//			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 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, what remote branch
+	// it is tracking.
 	// 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.
+	//
+	// The short view compresses the listing down to two lines per commit:
+	//	wbshadow 7a524a1..a496c1e (current branch, all mailed, 23 behind, tracking master)
+	//	+ uncommitted changes
+	//		Files unstaged:
+	//			src/runtime/proc1.go
+	//	+ a496c1e runtime: add missing write barriers in append's copy of slice data (CL 2064, mailed)
+	//	+ 95390c7 runtime: add GODEBUG wbshadow for finding missing write barriers (CL 2061, mailed)
 
 	var buf bytes.Buffer
 	printFileList := func(name string, list []string) {
@@ -219,66 +198,79 @@ func pending(args []string) {
 		if len(work) > 0 {
 			fmt.Fprintf(&buf, " %.7s..%s", b.branchpoint, work[0].ShortHash)
 		}
-		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 allMailed(work) {
-			if len(work) == 1 {
-				tags = append(tags, "mailed")
-			} else if len(work) > 1 {
-				tags = append(tags, "all mailed")
-			}
+		if allMailed(work) && len(work) > 0 {
+			tags = append(tags, "all mailed")
 		}
-		if allSubmitted(work) {
-			if len(work) == 1 {
-				tags = append(tags, "submitted")
-			} else if len(work) > 1 {
-				tags = append(tags, "all submitted")
-			}
+		if allSubmitted(work) && len(work) > 0 {
+			tags = append(tags, "all submitted")
 		}
 		if b.commitsBehind > 0 {
 			tags = append(tags, fmt.Sprintf("%d behind", b.commitsBehind))
 		}
+		if b.OriginBranch() != "origin/master" {
+			tags = append(tags, "tracking "+strings.TrimPrefix(b.OriginBranch(), "origin/"))
+		}
 		if len(tags) > 0 {
 			fmt.Fprintf(&buf, " (%s)", strings.Join(tags, ", "))
 		}
 		fmt.Fprintf(&buf, "\n")
+		printed := false
 		if text := b.errors(); text != "" {
-			fmt.Fprintf(&buf, "\tERROR: %s\n\n", strings.Replace(strings.TrimSpace(text), "\n", "\n\t", -1))
+			fmt.Fprintf(&buf, "\tERROR: %s\n", strings.Replace(strings.TrimSpace(text), "\n", "\n\t", -1))
+			if !pendingShort {
+				printed = true
+				fmt.Fprintf(&buf, "\n")
+			}
 		}
 
-		if b.current && len(work) > 1 && len(b.staged)+len(b.unstaged)+len(b.untracked) > 0 {
+		if b.current && len(b.staged)+len(b.unstaged)+len(b.untracked) > 0 {
+			printed = true
 			fmt.Fprintf(&buf, "+ uncommitted changes\n")
-			printFileList("staged", b.staged)
-			printFileList("unstaged", b.unstaged)
 			printFileList("untracked", b.untracked)
-			fmt.Fprintf(&buf, "\n")
+			printFileList("unstaged", b.unstaged)
+			printFileList("staged", b.staged)
+			if !pendingShort {
+				fmt.Fprintf(&buf, "\n")
+			}
 		}
 
 		for _, c := range work {
+			printed = true
 			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")
+			msg := strings.TrimRight(c.Message, "\r\n")
+			fmt.Fprintf(&buf, "+ %s", c.ShortHash)
+			var tags []string
+			if pendingShort {
+				if i := strings.Index(msg, "\n"); i >= 0 {
+					msg = msg[:i]
 				}
-				if g.Status == "MERGED" {
-					tags = append(tags, "submitted")
+				fmt.Fprintf(&buf, " %s", msg)
+				if g.Number != 0 {
+					tags = append(tags, fmt.Sprintf("CL %d%s", g.Number, codeReviewScores(g)))
 				}
-				if len(tags) > 0 {
-					fmt.Fprintf(&buf, " (%s)", strings.Join(tags, ", "))
+			} else {
+				if g.Number != 0 {
+					fmt.Fprintf(&buf, " %s/%d", auth.url, g.Number)
 				}
-				fmt.Fprintf(&buf, "\n")
 			}
-			msg := strings.TrimRight(c.Message, "\r\n")
+			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")
+			if pendingShort {
+				continue
+			}
+
 			fmt.Fprintf(&buf, "\t%s\n", strings.Replace(msg, "\n", "\n\t", -1))
 			fmt.Fprintf(&buf, "\n")
 
@@ -312,16 +304,9 @@ func pending(args []string) {
 			}
 
 			printFileList("in this change", c.committed)
-			if b.current && len(work) == 1 {
-				// staged file list will be printed next
-			} else {
-				fmt.Fprintf(&buf, "\n")
-			}
+			fmt.Fprintf(&buf, "\n")
 		}
-		if b.current && len(work) <= 1 {
-			printFileList("staged", b.staged)
-			printFileList("unstaged", b.unstaged)
-			printFileList("untracked", b.untracked)
+		if pendingShort || !printed {
 			fmt.Fprintf(&buf, "\n")
 		}
 	}
@@ -329,6 +314,32 @@ func pending(args []string) {
 	stdout().Write(buf.Bytes())
 }
 
+// codeReviewScores reports the code review scores as tags for the short output.
+func codeReviewScores(g *GerritChange) string {
+	label := g.Labels["Code-Review"]
+	if label == nil {
+		return ""
+	}
+	minValue := 10000
+	maxValue := -10000
+	for _, x := range label.All {
+		if minValue > x.Value {
+			minValue = x.Value
+		}
+		if maxValue < x.Value {
+			maxValue = x.Value
+		}
+	}
+	var scores string
+	if minValue < 0 {
+		scores += fmt.Sprintf(" %d", minValue)
+	}
+	if maxValue > 0 {
+		scores += fmt.Sprintf(" %+d", maxValue)
+	}
+	return scores
+}
+
 // allMailed reports whether all commits in work have been posted to Gerrit.
 func allMailed(work []*Commit) bool {
 	for _, c := range work {
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index f22d8d0..a52cdb8 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -42,6 +42,7 @@ func TestPendingBasic(t *testing.T) {
 
 	testPending(t, `
 		work REVHASH..REVHASH (current branch)
+		+ REVHASH
 			msg
 			
 			Change-Id: I123456789
@@ -83,7 +84,26 @@ func TestPendingComplex(t *testing.T) {
 	write(t, gt.client+"/bfile", "untracked")
 
 	testPending(t, `
+		work2 REVHASH..REVHASH (current branch)
+		+ uncommitted changes
+			Files untracked:
+				bfile
+			Files unstaged:
+				file
+				file1
+			Files staged:
+				afile1
+				file1
+
+		+ REVHASH
+			some changes
+
+			Files in this change:
+				file
+				file1
+
 		work REVHASH..REVHASH (5 behind)
+		+ REVHASH
 			msg
 			
 			Change-Id: I123456789
@@ -91,38 +111,41 @@ func TestPendingComplex(t *testing.T) {
 			Files in this change:
 				file
 
+	`)
+
+	testPendingArgs(t, []string{"-c"}, `
 		work2 REVHASH..REVHASH (current branch)
-			some changes
-		
-			Files in this change:
+		+ uncommitted changes
+			Files untracked:
+				bfile
+			Files unstaged:
 				file
 				file1
 			Files staged:
 				afile1
 				file1
-			Files unstaged:
+
+		+ REVHASH
+			some changes
+		
+			Files in this change:
 				file
 				file1
-			Files untracked:
-				bfile
 		
 	`)
 
-	testPendingArgs(t, []string{"-c"}, `
+	testPendingArgs(t, []string{"-c", "-s"}, `
 		work2 REVHASH..REVHASH (current branch)
-			some changes
-		
-			Files in this change:
+		+ uncommitted changes
+			Files untracked:
+				bfile
+			Files unstaged:
 				file
 				file1
 			Files staged:
 				afile1
 				file1
-			Files unstaged:
-				file
-				file1
-			Files untracked:
-				bfile
+		+ REVHASH some changes
 		
 	`)
 }
@@ -140,12 +163,21 @@ func TestPendingErrors(t *testing.T) {
 			ERROR: Branch contains 1 commit not on origin/master.
 				Do not commit directly to master branch.
 		
+		+ REVHASH
 			v3
 		
 			Files in this change:
 				file
 
 	`)
+
+	testPendingArgs(t, []string{"-s"}, `
+		master REVHASH..REVHASH (current branch)
+			ERROR: Branch contains 1 commit not on origin/master.
+				Do not commit directly to master branch.
+		+ REVHASH v3
+
+	`)
 }
 
 func TestPendingMultiChange(t *testing.T) {
@@ -165,12 +197,12 @@ func TestPendingMultiChange(t *testing.T) {
 	testPending(t, `
 		work REVHASH..REVHASH (current branch)
 		+ uncommitted changes
-			Files staged:
-				file
-			Files unstaged:
-				file
 			Files untracked:
 				file2
+			Files unstaged:
+				file
+			Files staged:
+				file
 		
 		+ REVHASH
 			v2
@@ -187,6 +219,20 @@ func TestPendingMultiChange(t *testing.T) {
 				file
 
 	`)
+
+	testPendingArgs(t, []string{"-s"}, `
+		work REVHASH..REVHASH (current branch)
+		+ uncommitted changes
+			Files untracked:
+				file2
+			Files unstaged:
+				file
+			Files staged:
+				file
+		+ REVHASH v2
+		+ REVHASH msg
+
+	`)
 }
 
 func TestPendingGerrit(t *testing.T) {
@@ -200,6 +246,7 @@ func TestPendingGerrit(t *testing.T) {
 	// Test error from Gerrit server.
 	testPending(t, `
 		work REVHASH..REVHASH (current branch)
+		+ REVHASH
 			msg
 			
 			Change-Id: I123456789
@@ -219,6 +266,7 @@ func TestPendingGerrit(t *testing.T) {
 	trun(t, gt.server, "git", "commit", "-m", "msg")
 	testPendingArgs(t, []string{"-l"}, `
 		work REVHASH..REVHASH (current branch)
+		+ REVHASH
 			msg
 			
 			Change-Id: I123456789
@@ -228,9 +276,16 @@ func TestPendingGerrit(t *testing.T) {
 
 	`)
 
+	testPendingArgs(t, []string{"-l", "-s"}, `
+		work REVHASH..REVHASH (current branch)
+		+ REVHASH msg
+
+	`)
+
 	// Without -l, the 1 behind should appear, as should Gerrit information.
 	testPending(t, `
-		work REVHASH..REVHASH http://127.0.0.1:PORT/1234 (current branch, mailed, submitted, 1 behind)
+		work REVHASH..REVHASH (current branch, all mailed, all submitted, 1 behind)
+		+ REVHASH http://127.0.0.1:PORT/1234 (mailed, submitted)
 			msg
 			
 			Change-Id: I123456789
@@ -245,9 +300,16 @@ func TestPendingGerrit(t *testing.T) {
 
 	`)
 
+	testPendingArgs(t, []string{"-s"}, `
+		work REVHASH..REVHASH (current branch, all mailed, all submitted, 1 behind)
+		+ REVHASH msg (CL 1234 -2 +1, mailed, submitted)
+
+	`)
+
 	// Since pending did a fetch, 1 behind should show up even with -l.
 	testPendingArgs(t, []string{"-l"}, `
 		work REVHASH..REVHASH (current branch, 1 behind)
+		+ REVHASH
 			msg
 			
 			Change-Id: I123456789
@@ -256,6 +318,11 @@ func TestPendingGerrit(t *testing.T) {
 				file
 
 	`)
+	testPendingArgs(t, []string{"-l", "-s"}, `
+		work REVHASH..REVHASH (current branch, 1 behind)
+		+ REVHASH msg
+
+	`)
 }
 
 func TestPendingGerritMultiChange(t *testing.T) {
@@ -284,12 +351,12 @@ func TestPendingGerritMultiChange(t *testing.T) {
 	testPending(t, `
 		work REVHASH..REVHASH (current branch, all mailed)
 		+ uncommitted changes
-			Files staged:
-				file
-			Files unstaged:
-				file
 			Files untracked:
 				file2
+			Files unstaged:
+				file
+			Files staged:
+				file
 		
 		+ REVHASH http://127.0.0.1:PORT/1234 (mailed)
 			v2
@@ -318,6 +385,20 @@ func TestPendingGerritMultiChange(t *testing.T) {
 				file
 
 	`)
+
+	testPendingArgs(t, []string{"-s"}, `
+		work REVHASH..REVHASH (current branch, all mailed)
+		+ uncommitted changes
+			Files untracked:
+				file2
+			Files unstaged:
+				file
+			Files staged:
+				file
+		+ REVHASH v2 (CL 1234 -2 +1, mailed)
+		+ REVHASH msg (CL 1234 -2 +1, mailed, submitted)
+
+	`)
 }
 
 func testPendingReply(srv *gerritServer, id, rev, status string) {
diff --git a/git-codereview/review.go b/git-codereview/review.go
index b99aee5..964ba1e 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -88,10 +88,12 @@ Available commands:
 	mail -diff
 		Show the changes but do not send mail or upload.
 
-	pending [-l]
+	pending [-c] [-l] [-s]
 		Show the status of all pending changes and staged, unstaged,
 		and untracked files in the local repository.
+		If -c is specified, show only changes on the current branch.
 		If -l is specified, only use locally available information.
+		If -s is specified, show short output.
 
 	submit
 		Push the pending change to the Gerrit server and tell Gerrit to
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index cd62f33..1fd2505 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -64,6 +64,7 @@ func (gt *gitTest) work(t *testing.T) {
 	if gt.nwork == 0 {
 		trun(t, gt.client, "git", "checkout", "-b", "work")
 		trun(t, gt.client, "git", "branch", "--set-upstream-to", "origin/master")
+		trun(t, gt.client, "git", "tag", "work") // make sure commands do the right thing when there is a tag of the same name
 	}
 
 	// make local change on client