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>
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 @@
// 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 @@
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 @@
// 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 @@
}
}
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 @@
// 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 @@
)
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 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 @@
}
// 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 @@
<-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 @@
// 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 @@
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)
+ 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]
+ }
+ fmt.Fprintf(&buf, " %s", msg)
+ if g.Number != 0 {
+ tags = append(tags, fmt.Sprintf("CL %d%s", g.Number, codeReviewScores(g)))
+ }
+ } else {
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")
+ 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 @@
}
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 @@
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 @@
testPending(t, `
work REVHASH..REVHASH (current branch)
+ + REVHASH
msg
Change-Id: I123456789
@@ -83,7 +84,26 @@
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 @@
Files in this change:
file
- work2 REVHASH..REVHASH (current branch)
- some changes
-
- Files in this change:
- file
- file1
- Files staged:
- afile1
- file1
- Files unstaged:
- file
- file1
- Files untracked:
- bfile
-
`)
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
+
+ + REVHASH
+ some changes
+
+ Files in this change:
+ file
+ file1
+
+ `)
+
+ testPendingArgs(t, []string{"-c", "-s"}, `
+ work2 REVHASH..REVHASH (current branch)
+ + uncommitted changes
+ Files untracked:
+ bfile
Files unstaged:
file
file1
- Files untracked:
- bfile
+ Files staged:
+ afile1
+ file1
+ + REVHASH some changes
`)
}
@@ -140,12 +163,21 @@
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 @@
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 @@
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 @@
// Test error from Gerrit server.
testPending(t, `
work REVHASH..REVHASH (current branch)
+ + REVHASH
msg
Change-Id: I123456789
@@ -219,6 +266,7 @@
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 @@
`)
+ 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 @@
`)
+ 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 @@
file
`)
+ testPendingArgs(t, []string{"-l", "-s"}, `
+ work REVHASH..REVHASH (current branch, 1 behind)
+ + REVHASH msg
+
+ `)
}
func TestPendingGerritMultiChange(t *testing.T) {
@@ -284,12 +351,12 @@
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 @@
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 @@
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 @@
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
