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>
diff --git a/git-codereview/api.go b/git-codereview/api.go
index 5ccc358..ac61cba 100644
--- a/git-codereview/api.go
+++ b/git-codereview/api.go
@@ -208,12 +208,12 @@
return nil
}
-// fullChangeID returns the unambigous Gerrit change ID for the pending change on branch b.
+// fullChangeID returns the unambigous Gerrit change ID for the commit c on branch b.
// The retruned ID has the form project~originbranch~Ihexhexhexhexhex.
// See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id for details.
-func fullChangeID(b *Branch) string {
+func fullChangeID(b *Branch, c *Commit) string {
loadGerritOrigin()
- return auth.project + "~" + strings.TrimPrefix(b.OriginBranch(), "origin/") + "~" + b.ChangeID()
+ return auth.project + "~" + strings.TrimPrefix(b.OriginBranch(), "origin/") + "~" + c.ChangeID
}
// readGerritChange reads the metadata about a change from the Gerrit server.
@@ -253,9 +253,9 @@
}
// LabelNames returns the label names for the change, in lexicographic order.
-func (ch *GerritChange) LabelNames() []string {
+func (g *GerritChange) LabelNames() []string {
var names []string
- for name := range ch.Labels {
+ for name := range g.Labels {
names = append(names, name)
}
sort.Strings(names)
diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 128a577..1aa5427 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -14,17 +14,28 @@
// Branch describes a Git branch.
type Branch struct {
- Name string // branch name
- loadedPending bool // following fields are valid
- changeID string // Change-Id of pending commit ("" if nothing pending)
- subject string // first line of pending commit ("" if nothing pending)
- message string // commit message
- commitHash string // commit hash of pending commit ("" if nothing pending)
- shortCommitHash string // abbreviated commitHash ("" if nothing pending)
- parentHash string // parent hash of pending commit ("" if nothing pending)
- commitsAhead int // number of commits ahead of origin branch
- commitsBehind int // number of commits behind origin branch
- originBranch string // upstream origin branch
+ Name string // branch name
+ loadedPending bool // following fields are valid
+ originBranch string // upstream origin branch
+ commitsAhead int // number of commits ahead of origin branch
+ commitsBehind int // number of commits behind origin branch
+ branchpoint string // latest commit hash shared with origin branch
+ pending []*Commit // pending commits, newest first (children before parents)
+}
+
+// A Commit describes a single pending commit on a Git branch.
+type Commit struct {
+ Hash string // commit hash
+ ShortHash string // abbreviated commit hash
+ Parent string // parent hash
+ Message string // commit message
+ Subject string // first line of commit message
+ ChangeID string // Change-Id in commit message ("" if missing)
+
+ // For use by pending command.
+ g *GerritChange // associated Gerrit change data
+ gerr error // error loading Gerrit data
+ committed []string // list of files in this commit
}
// CurrentBranch returns the current branch.
@@ -70,40 +81,28 @@
panic("not reached")
}
+// 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()
}
+// HasPendingCommit reports whether b has any pending commits.
func (b *Branch) HasPendingCommit() bool {
b.loadPending()
- return b.commitHash != ""
+ return b.commitsAhead > 0
}
-func (b *Branch) ChangeID() string {
+// Pending returns b's pending commits, newest first (children before parents).
+func (b *Branch) Pending() []*Commit {
b.loadPending()
- return b.changeID
-}
-
-func (b *Branch) Subject() string {
- b.loadPending()
- return b.subject
-}
-
-func (b *Branch) CommitHash() string {
- b.loadPending()
- return b.commitHash
+ return b.pending
}
// Branchpoint returns an identifier for the latest revision
// common to both this branch and its upstream branch.
-// If this branch has not split from upstream,
-// Branchpoint returns "HEAD".
func (b *Branch) Branchpoint() string {
b.loadPending()
- if b.parentHash == "" {
- return "HEAD"
- }
- return b.parentHash
+ return b.branchpoint
}
func (b *Branch) loadPending() {
@@ -112,41 +111,46 @@
}
b.loadedPending = true
+ // In case of early return.
+ b.branchpoint = getOutput("git", "rev-parse", "HEAD")
+
if b.DetachedHead() {
return
}
+ // Note: --topo-order means child first, then parent.
const numField = 5
- all := getOutput("git", "log", "--format=format:%H%x00%h%x00%P%x00%s%x00%B%x00", b.OriginBranch()+".."+b.Name, "--")
+ all := getOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", b.OriginBranch()+".."+b.Name, "--")
fields := strings.Split(all, "\x00")
if len(fields) < numField {
return // nothing pending
}
+ for i, field := range fields {
+ fields[i] = strings.TrimLeft(field, "\r\n")
+ }
for i := 0; i+numField <= len(fields); i += numField {
- hash := fields[i]
- shortHash := fields[i+1]
- parent := fields[i+2]
- subject := fields[i+3]
- msg := fields[i+4]
-
- // Overwrite each time through the loop.
- // We want to save the info about the *first* commit
- // after the branch point, and the log is ordered
- // starting at the most recent and working backward.
- b.commitHash = strings.TrimSpace(hash)
- b.shortCommitHash = strings.TrimSpace(shortHash)
- b.parentHash = strings.TrimSpace(parent)
- b.subject = subject
- b.message = msg
- for _, line := range strings.Split(msg, "\n") {
+ c := &Commit{
+ Hash: fields[i],
+ ShortHash: fields[i+1],
+ Parent: strings.TrimSpace(fields[i+2]), // %P starts with \n for some reason
+ Message: fields[i+3],
+ Subject: fields[i+4],
+ }
+ 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
+ // there is a commit message quoting another
+ // commit message.
+ // I'm not sure this can come up at all, but just in case.
if strings.HasPrefix(line, "Change-Id: ") {
- b.changeID = line[len("Change-Id: "):]
- break
+ c.ChangeID = line[len("Change-Id: "):]
}
}
- b.commitsAhead++
+
+ b.pending = append(b.pending, c)
+ b.branchpoint = c.Parent
}
- b.commitsAhead = len(fields) / numField
+ b.commitsAhead = len(b.pending)
b.commitsBehind = len(getOutput("git", "log", "--format=format:x", b.Name+".."+b.OriginBranch(), "--"))
}
@@ -163,6 +167,7 @@
var stagedRE = regexp.MustCompile(`^[ACDMR] `)
+// HasStagedChanges reports whether the working directory contains staged changes.
func HasStagedChanges() bool {
for _, s := range getLines("git", "status", "-b", "--porcelain") {
if stagedRE.MatchString(s) {
@@ -174,6 +179,7 @@
var unstagedRE = regexp.MustCompile(`^.[ACDMR]`)
+// HasUnstagedChanges reports whether the working directory contains unstaged changes.
func HasUnstagedChanges() bool {
for _, s := range getLines("git", "status", "-b", "--porcelain") {
if unstagedRE.MatchString(s) {
@@ -209,6 +215,9 @@
return
}
+// LocalBranches returns a list of all known local branches.
+// If the current directory is in detached HEAD mode, one returned
+// branch will have Name == "HEAD" and DetachedHead() == true.
func LocalBranches() []*Branch {
var branches []*Branch
current := CurrentBranch()
@@ -250,11 +259,11 @@
// The extra strings are passed to the Gerrit API request as o= parameters,
// to enable additional information. Typical values include "LABELS" and "CURRENT_REVISION".
// See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html for details.
-func (b *Branch) GerritChange(extra ...string) (*GerritChange, error) {
+func (b *Branch) GerritChange(c *Commit, extra ...string) (*GerritChange, error) {
if !b.HasPendingCommit() {
- return nil, fmt.Errorf("no pending commit")
+ return nil, fmt.Errorf("no changes pending")
}
- id := fullChangeID(b)
+ id := fullChangeID(b, c)
for i, x := range extra {
if i == 0 {
id += "?"
@@ -265,3 +274,45 @@
}
return readGerritChange(id)
}
+
+const minHashLen = 4 // git minimum hash length accepted on command line
+
+// CommitByHash finds a unique pending commit by its hash prefix.
+// It dies if the hash cannot be resolved to a pending commit,
+// using the action ("mail", "submit") in the failure message.
+func (b *Branch) CommitByHash(action, hash string) *Commit {
+ if len(hash) < minHashLen {
+ dief("cannot %s: commit hash %q must be at least %d digits long", action, hash, minHashLen)
+ }
+ var c *Commit
+ for _, c1 := range b.Pending() {
+ if strings.HasPrefix(c1.Hash, hash) {
+ if c != nil {
+ dief("cannot %s: commit hash %q is ambiguous in the current branch", action, hash)
+ }
+ c = c1
+ }
+ }
+ if c == nil {
+ dief("cannot %s: commit hash %q not found in the current branch", action, hash)
+ }
+ return c
+}
+
+// DefaultCommit returns the default pending commit for this branch.
+// It dies if there is not exactly one pending commit,
+// using the action ("mail", "submit") in the failure message.
+func (b *Branch) DefaultCommit(action string) *Commit {
+ work := b.Pending()
+ if len(work) == 0 {
+ dief("cannot %s: no changes pending", action)
+ }
+ if len(work) >= 2 {
+ var buf bytes.Buffer
+ for _, c := range work {
+ fmt.Fprintf(&buf, "\n\t%s %s", c.ShortHash, c.Subject)
+ }
+ dief("cannot %s: multiple changes pending; must specify commit hash on command line:%s", action, buf.String())
+ }
+ return work[0]
+}
diff --git a/git-codereview/branch_test.go b/git-codereview/branch_test.go
index 6bc52f3..f206d9e 100644
--- a/git-codereview/branch_test.go
+++ b/git-codereview/branch_test.go
@@ -57,11 +57,14 @@
if x := b.HasPendingCommit(); x != hasPending {
t.Errorf("b.HasPendingCommit() = %v, want %v", x, isLocal)
}
- if x := b.ChangeID(); x != changeID {
- t.Errorf("b.ChangeID() = %q, want %q", x, changeID)
- }
- if x := b.Subject(); x != subject {
- t.Errorf("b.Subject() = %q, want %q", x, subject)
+ if work := b.Pending(); len(work) > 0 {
+ c := work[0]
+ if x := c.ChangeID; x != changeID {
+ t.Errorf("b.Pending()[0].ChangeID = %q, want %q", x, changeID)
+ }
+ if x := c.Subject; x != subject {
+ t.Errorf("b.Pending()[0].Subject = %q, want %q", x, subject)
+ }
}
}
diff --git a/git-codereview/change.go b/git-codereview/change.go
index 4281b6a..1741b4f 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -29,7 +29,7 @@
if target != "" {
checkoutOrCreate(target)
b := CurrentBranch()
- if HasStagedChanges() && b.IsLocalOnly() && b.ChangeID() == "" {
+ if HasStagedChanges() && b.IsLocalOnly() && !b.HasPendingCommit() {
commitChanges(false)
}
b.check()
@@ -42,7 +42,7 @@
dief("can't commit to %s branch (use '%s change branchname').", b.Name, os.Args[0])
}
- amend := b.ChangeID() != ""
+ amend := b.HasPendingCommit()
commitChanges(amend)
b.loadedPending = false // force reload after commitChanges
b.check()
diff --git a/git-codereview/mail.go b/git-codereview/mail.go
index c6eee7e..c5ec7ce 100644
--- a/git-codereview/mail.go
+++ b/git-codereview/mail.go
@@ -23,21 +23,25 @@
flags.Var(ccList, "cc", "comma-separated list of people to CC:")
flags.Usage = func() {
- fmt.Fprintf(stderr(), "Usage: %s mail %s [-r reviewer,...] [-cc mail,...]\n", os.Args[0], globalFlags)
+ fmt.Fprintf(stderr(), "Usage: %s mail %s [-r reviewer,...] [-cc mail,...] [commit-hash]\n", os.Args[0], globalFlags)
}
flags.Parse(args)
- if len(flags.Args()) != 0 {
+ if len(flags.Args()) > 1 {
flags.Usage()
os.Exit(2)
}
b := CurrentBranch()
- if b.ChangeID() == "" {
- dief("no pending change; can't mail.")
+
+ var c *Commit
+ if len(flags.Args()) == 1 {
+ c = b.CommitByHash("mail", flags.Arg(0))
+ } else {
+ c = b.DefaultCommit("mail")
}
if *diff {
- run("git", "diff", b.Branchpoint()+"..HEAD", "--")
+ run("git", "diff", b.Branchpoint()[:7]+".."+c.ShortHash, "--")
return
}
@@ -49,7 +53,7 @@
// for side effect of dying with a good message if origin is GitHub
loadGerritOrigin()
- refSpec := b.PushSpec()
+ refSpec := b.PushSpec(c)
start := "%"
if *rList != "" {
refSpec += mailList(start, "r", string(*rList))
@@ -74,9 +78,14 @@
run("git", "tag", "-f", b.Name+".mailed")
}
-// PushSpec returns the spec for a Gerrit push command to publish the change in b.
-func (b *Branch) PushSpec() string {
- return "HEAD:refs/for/" + strings.TrimPrefix(b.OriginBranch(), "origin/")
+// PushSpec returns the spec for a Gerrit push command to publish the change c in b.
+// If c is nil, PushSpec returns a spec for pushing all changes in b.
+func (b *Branch) PushSpec(c *Commit) string {
+ local := "HEAD"
+ if c != nil && (len(b.Pending()) == 0 || b.Pending()[0].Hash != c.Hash) {
+ local = c.ShortHash
+ }
+ return local + ":refs/for/" + strings.TrimPrefix(b.OriginBranch(), "origin/")
}
// mailAddressRE matches the mail addresses we admit. It's restrictive but admits
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 @@
"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 @@
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 @@
}
// 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 @@
}
// 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 @@
// 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 @@
}
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 @@
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 @@
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", b.committed)
- printFileList("staged", b.staged)
- printFileList("unstaged", b.unstaged)
- printFileList("untracked", b.untracked)
- fmt.Fprintf(&buf, "\n")
+ printFileList("in this change", c.committed)
+ if b.current && len(work) == 1 {
+ // staged file list will be printed next
+ } else {
+ 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 @@
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()
}
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index 68c0f00..f22d8d0 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -41,7 +41,7 @@
gt.work(t)
testPending(t, `
- work REVHASH (current branch)
+ work REVHASH..REVHASH (current branch)
msg
Change-Id: I123456789
@@ -83,7 +83,7 @@
write(t, gt.client+"/bfile", "untracked")
testPending(t, `
- work REVHASH (5 behind)
+ work REVHASH..REVHASH (5 behind)
msg
Change-Id: I123456789
@@ -91,7 +91,25 @@
Files in this change:
file
- work2 REVHASH (current branch)
+ 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:
@@ -113,16 +131,12 @@
gt := newGitTest(t)
defer gt.done()
- gt.work(t)
- write(t, gt.client+"/file", "v2")
- trun(t, gt.client, "git", "commit", "-a", "-m", "v2")
-
trun(t, gt.client, "git", "checkout", "master")
write(t, gt.client+"/file", "v3")
trun(t, gt.client, "git", "commit", "-a", "-m", "v3")
testPending(t, `
- master REVHASH (current branch)
+ master REVHASH..REVHASH (current branch)
ERROR: Branch contains 1 commit not on origin/master.
Do not commit directly to master branch.
@@ -130,13 +144,41 @@
Files in this change:
file
+
+ `)
+}
+
+func TestPendingMultiChange(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ gt.work(t)
+ write(t, gt.client+"/file", "v2")
+ trun(t, gt.client, "git", "commit", "-a", "-m", "v2")
+
+ write(t, gt.client+"/file", "v4")
+ trun(t, gt.client, "git", "add", "file")
+
+ write(t, gt.client+"/file", "v5")
+ write(t, gt.client+"/file2", "v6")
+
+ testPending(t, `
+ work REVHASH..REVHASH (current branch)
+ + uncommitted changes
+ Files staged:
+ file
+ Files unstaged:
+ file
+ Files untracked:
+ file2
- work REVHASH
- ERROR: Branch contains 2 commits not on origin/origin/master.
- There should be at most one.
- Use 'git change', not 'git commit'.
- Run 'git log origin/master..work' to list commits.
+ + REVHASH
+ v2
+ Files in this change:
+ file
+
+ + REVHASH
msg
Change-Id: I123456789
@@ -144,7 +186,7 @@
Files in this change:
file
-`)
+ `)
}
func TestPendingGerrit(t *testing.T) {
@@ -157,7 +199,7 @@
// Test error from Gerrit server.
testPending(t, `
- work REVHASH (current branch)
+ work REVHASH..REVHASH (current branch)
msg
Change-Id: I123456789
@@ -167,13 +209,121 @@
`)
- setJSON := func(json string) {
- srv.setReply("/a/changes/proj~master~I123456789", gerritReply{body: ")]}'\n" + json})
- }
+ testPendingReply(srv, "I123456789", CurrentBranch().Pending()[0].Hash, "MERGED")
- setJSON(`{
- "current_revision": "` + CurrentBranch().CommitHash() + `",
- "status": "MERGED",
+ // Test local mode does not talk to any server.
+ // Make client 1 behind server.
+ // The '1 behind' should not show up, nor any Gerrit information.
+ write(t, gt.server+"/file", "v4")
+ trun(t, gt.server, "git", "add", "file")
+ trun(t, gt.server, "git", "commit", "-m", "msg")
+ testPendingArgs(t, []string{"-l"}, `
+ work REVHASH..REVHASH (current branch)
+ msg
+
+ Change-Id: I123456789
+
+ Files in this change:
+ file
+
+ `)
+
+ // 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)
+ msg
+
+ Change-Id: I123456789
+
+ Code-Review:
+ +1 Grace Emlin
+ -2 George Opher
+ Other-Label:
+ +2 The Owner
+ Files in this change:
+ file
+
+ `)
+
+ // Since pending did a fetch, 1 behind should show up even with -l.
+ testPendingArgs(t, []string{"-l"}, `
+ work REVHASH..REVHASH (current branch, 1 behind)
+ msg
+
+ Change-Id: I123456789
+
+ Files in this change:
+ file
+
+ `)
+}
+
+func TestPendingGerritMultiChange(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ gt.work(t)
+ hash1 := CurrentBranch().Pending()[0].Hash
+
+ write(t, gt.client+"/file", "v2")
+ trun(t, gt.client, "git", "commit", "-a", "-m", "v2\n\nChange-Id: I2345")
+ hash2 := CurrentBranch().Pending()[0].Hash
+
+ write(t, gt.client+"/file", "v4")
+ trun(t, gt.client, "git", "add", "file")
+
+ write(t, gt.client+"/file", "v5")
+ write(t, gt.client+"/file2", "v6")
+
+ srv := newGerritServer(t)
+ defer srv.done()
+
+ testPendingReply(srv, "I123456789", hash1, "MERGED")
+ testPendingReply(srv, "I2345", hash2, "NEW")
+
+ testPending(t, `
+ work REVHASH..REVHASH (current branch, all mailed)
+ + uncommitted changes
+ Files staged:
+ file
+ Files unstaged:
+ file
+ Files untracked:
+ file2
+
+ + REVHASH http://127.0.0.1:PORT/1234 (mailed)
+ v2
+
+ Change-Id: I2345
+
+ Code-Review:
+ +1 Grace Emlin
+ -2 George Opher
+ Other-Label:
+ +2 The Owner
+ Files in this change:
+ file
+
+ + REVHASH http://127.0.0.1:PORT/1234 (mailed, submitted)
+ msg
+
+ Change-Id: I123456789
+
+ Code-Review:
+ +1 Grace Emlin
+ -2 George Opher
+ Other-Label:
+ +2 The Owner
+ Files in this change:
+ file
+
+ `)
+}
+
+func testPendingReply(srv *gerritServer, id, rev, status string) {
+ srv.setJSON(id, `{
+ "current_revision": "`+rev+`",
+ "status": "`+status+`",
"_number": 1234,
"owner": {"_id": 42},
"labels": {
@@ -206,26 +356,13 @@
}
}
}`)
-
- testPending(t, `
- work REVHASH http://127.0.0.1:PORT/1234 (current branch, mailed, submitted)
- msg
-
- Change-Id: I123456789
-
- Code-Review:
- +1 Grace Emlin
- -2 George Opher
- Other-Label:
- +2 The Owner
- Files in this change:
- file
-
- `)
-
}
func testPending(t *testing.T, want string) {
+ testPendingArgs(t, nil, want)
+}
+
+func testPendingArgs(t *testing.T, args []string, want string) {
// fake auth information to avoid Gerrit error
if auth.host == "" {
auth.host = "gerrit.fake"
@@ -240,7 +377,7 @@
want = strings.Replace(want, "\n\t", "\n", -1)
want = strings.TrimPrefix(want, "\n")
- testMain(t, "pending")
+ testMain(t, append([]string{"pending"}, args...)...)
out := testStdout.Bytes()
out = regexp.MustCompile(`\b[0-9a-f]{7}\b`).ReplaceAllLiteral(out, []byte("REVHASH"))
diff --git a/git-codereview/review.go b/git-codereview/review.go
index 6d1f6ac..c8ed27a 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
+// TODO(rsc): Document multi-change branch behavior.
+
// Command git-codereview provides a simple command-line user interface for
// working with git repositories and the Gerrit code review system.
// See "git-codereview help" for details.
diff --git a/git-codereview/submit.go b/git-codereview/submit.go
index 61189e9..a50b214 100644
--- a/git-codereview/submit.go
+++ b/git-codereview/submit.go
@@ -4,35 +4,49 @@
package main
-import "time"
+import (
+ "fmt"
+ "os"
+ "time"
+)
// TODO(rsc): Add -tbr, along with standard exceptions (doc/go1.5.txt)
func submit(args []string) {
- expectZeroArgs(args, "submit")
-
- // Must have pending change, no staged changes.
- b := CurrentBranch()
- if !b.HasPendingCommit() {
- dief("cannot submit: no pending commit")
+ flags.Usage = func() {
+ fmt.Fprintf(stderr(), "Usage: %s submit %s [commit-hash]\n", os.Args[0], globalFlags)
}
- checkStaged("submit")
+ flags.Parse(args)
+ if n := len(flags.Args()); n > 1 {
+ flags.Usage()
+ os.Exit(2)
+ }
+ b := CurrentBranch()
+ var c *Commit
+ if len(flags.Args()) == 1 {
+ c = b.CommitByHash("submit", flags.Arg(0))
+ } else {
+ c = b.DefaultCommit("submit")
+ }
+
+ // No staged changes.
// Also, no unstaged changes, at least for now.
// This makes sure the sync at the end will work well.
// We can relax this later if there is a good reason.
+ checkStaged("submit")
checkUnstaged("submit")
// Fetch Gerrit information about this change.
- ch, err := b.GerritChange("LABELS", "CURRENT_REVISION")
+ g, err := b.GerritChange(c, "LABELS", "CURRENT_REVISION")
if err != nil {
dief("%v", err)
}
// Check Gerrit change status.
- switch ch.Status {
+ switch g.Status {
default:
- dief("cannot submit: unexpected Gerrit change status %q", ch.Status)
+ dief("cannot submit: unexpected Gerrit change status %q", g.Status)
case "NEW", "SUBMITTED":
// Not yet "MERGED", so try the submit.
@@ -51,8 +65,8 @@
// Check for label approvals (like CodeReview+2).
// The final submit will check these too, but it is better to fail now.
- for _, name := range ch.LabelNames() {
- label := ch.Labels[name]
+ for _, name := range g.LabelNames() {
+ label := g.Labels[name]
if label.Optional {
continue
}
@@ -65,18 +79,19 @@
}
// Upload most recent revision if not already on server.
- if b.CommitHash() != ch.CurrentRevision {
- run("git", "push", "-q", "origin", b.PushSpec())
+
+ if c.Hash != g.CurrentRevision {
+ run("git", "push", "-q", "origin", b.PushSpec(c))
// Refetch change information, especially mergeable.
- ch, err = b.GerritChange("LABELS", "CURRENT_REVISION")
+ g, err = b.GerritChange(c, "LABELS", "CURRENT_REVISION")
if err != nil {
dief("%v", err)
}
}
// Don't bother if the server can't merge the changes.
- if !ch.Mergeable {
+ if !g.Mergeable {
// Server cannot merge; explicit sync is needed.
dief("cannot submit: conflicting changes submitted, run 'git sync'")
}
@@ -89,7 +104,7 @@
// but we need extended information and the reply is in the
// "SUBMITTED" state anyway, so ignore the GerritChange
// in the response and fetch a new one below.
- if err := gerritAPI("/a/changes/"+fullChangeID(b)+"/submit", []byte(`{"wait_for_merge": true}`), nil); err != nil {
+ if err := gerritAPI("/a/changes/"+fullChangeID(b, c)+"/submit", []byte(`{"wait_for_merge": true}`), nil); err != nil {
dief("cannot submit: %v", err)
}
@@ -103,18 +118,18 @@
const max = 2 * time.Second
for i := 0; i < steps; i++ {
time.Sleep(max * (1 << uint(i+1)) / (1 << steps))
- ch, err = b.GerritChange("LABELS", "CURRENT_REVISION")
+ g, err = b.GerritChange(c, "LABELS", "CURRENT_REVISION")
if err != nil {
dief("waiting for merge: %v", err)
}
- if ch.Status != "SUBMITTED" {
+ if g.Status != "SUBMITTED" {
break
}
}
- switch ch.Status {
+ switch g.Status {
default:
- dief("submit error: unexpected post-submit Gerrit change status %q", ch.Status)
+ dief("submit error: unexpected post-submit Gerrit change status %q", g.Status)
case "MERGED":
// good
@@ -127,10 +142,14 @@
// Sync client to revision that Gerrit committed, but only if we can do it cleanly.
// Otherwise require user to run 'git sync' themselves (if they care).
run("git", "fetch", "-q")
- if err := runErr("git", "checkout", "-q", "-B", b.Name, ch.CurrentRevision, "--"); err != nil {
- dief("submit succeeded, but cannot sync local branch\n"+
- "\trun 'git sync' to sync, or\n"+
- "\trun 'git branch -D %s; git change master; git sync' to discard local branch", b.Name)
+ if len(b.Pending()) == 1 {
+ if err := runErr("git", "checkout", "-q", "-B", b.Name, g.CurrentRevision, "--"); err != nil {
+ dief("submit succeeded, but cannot sync local branch\n"+
+ "\trun 'git sync' to sync, or\n"+
+ "\trun 'git branch -D %s; git change master; git sync' to discard local branch", b.Name)
+ }
+ } else {
+ printf("submit succeeded; run 'git sync' to sync")
}
// Done! Change is submitted, branch is up to date, ready for new work.
diff --git a/git-codereview/submit_test.go b/git-codereview/submit_test.go
index 2f87bbf..8e91e42 100644
--- a/git-codereview/submit_test.go
+++ b/git-codereview/submit_test.go
@@ -18,7 +18,7 @@
t.Logf("> no commit")
testMainDied(t, "submit")
- testPrintedStderr(t, "cannot submit: no pending commit")
+ testPrintedStderr(t, "cannot submit: no changes pending")
write(t, gt.client+"/file1", "")
trun(t, gt.client, "git", "add", "file1")
trun(t, gt.client, "git", "commit", "-m", "msg\n\nChange-Id: I123456789\n")
@@ -45,55 +45,53 @@
testMainDied(t, "submit")
testPrintedStderr(t, "change not found on Gerrit server")
- setJSON := func(json string) {
- srv.setReply("/a/changes/proj~master~I123456789", gerritReply{body: ")]}'\n" + json})
- }
+ const id = "I123456789"
t.Logf("> malformed json")
- setJSON("XXX")
+ srv.setJSON(id, "XXX")
testMainDied(t, "submit")
testRan(t) // nothing
testPrintedStderr(t, "malformed json response")
t.Logf("> unexpected change status")
- setJSON(`{"status": "UNEXPECTED"}`)
+ srv.setJSON(id, `{"status": "UNEXPECTED"}`)
testMainDied(t, "submit")
testRan(t) // nothing
testPrintedStderr(t, "cannot submit: unexpected Gerrit change status \"UNEXPECTED\"")
t.Logf("> already merged")
- setJSON(`{"status": "MERGED"}`)
+ srv.setJSON(id, `{"status": "MERGED"}`)
testMainDied(t, "submit")
testRan(t) // nothing
testPrintedStderr(t, "cannot submit: change already submitted, run 'git sync'")
t.Logf("> abandoned")
- setJSON(`{"status": "ABANDONED"}`)
+ srv.setJSON(id, `{"status": "ABANDONED"}`)
testMainDied(t, "submit")
testRan(t) // nothing
testPrintedStderr(t, "cannot submit: change abandoned")
t.Logf("> missing approval")
- setJSON(`{"status": "NEW", "labels": {"Code-Review": {}}}`)
+ srv.setJSON(id, `{"status": "NEW", "labels": {"Code-Review": {}}}`)
testMainDied(t, "submit")
testRan(t) // nothing
testPrintedStderr(t, "cannot submit: change missing Code-Review approval")
t.Logf("> rejection")
- setJSON(`{"status": "NEW", "labels": {"Code-Review": {"rejected": {}}}}`)
+ srv.setJSON(id, `{"status": "NEW", "labels": {"Code-Review": {"rejected": {}}}}`)
testMainDied(t, "submit")
testRan(t) // nothing
testPrintedStderr(t, "cannot submit: change has Code-Review rejection")
t.Logf("> unmergeable")
- setJSON(`{"status": "NEW", "mergeable": false, "labels": {"Code-Review": {"approved": {}}}}`)
+ srv.setJSON(id, `{"status": "NEW", "mergeable": false, "labels": {"Code-Review": {"approved": {}}}}`)
testMainDied(t, "submit")
testRan(t, "git push -q origin HEAD:refs/for/master")
testPrintedStderr(t, "cannot submit: conflicting changes submitted, run 'git sync'")
t.Logf("> submit with unexpected status")
const newJSON = `{"status": "NEW", "mergeable": true, "labels": {"Code-Review": {"approved": {}}}}`
- setJSON(newJSON)
+ srv.setJSON(id, newJSON)
srv.setReply("/a/changes/proj~master~I123456789/submit", gerritReply{body: ")]}'\n" + newJSON})
testMainDied(t, "submit")
testRan(t, "git push -q origin HEAD:refs/for/master")
diff --git a/git-codereview/sync.go b/git-codereview/sync.go
index f11e26c..652b0bf 100644
--- a/git-codereview/sync.go
+++ b/git-codereview/sync.go
@@ -11,7 +11,10 @@
// Get current branch and commit ID for fixup after pull.
b := CurrentBranch()
- id := b.ChangeID()
+ var id string
+ if work := b.Pending(); len(work) > 0 {
+ id = work[len(work)-1].ChangeID
+ }
// Don't sync with staged or unstaged changes.
// rebase is going to complain if we don't, and we can give a nicer error.
@@ -29,8 +32,8 @@
// If the change commit has been submitted,
// roll back change leaving any changes unstaged.
// Pull should have done this for us, but check just in case.
- b.loadedPending = false
- if b.Submitted(id) && b.HasPendingCommit() {
+ b = CurrentBranch() // discard any cached information
+ if len(b.Pending()) == 1 && b.Submitted(id) {
run("git", "reset", b.Branchpoint())
}
}
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index bc44e2f..e38c364 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -331,6 +331,10 @@
s.reply[path] = reply
}
+func (s *gerritServer) setJSON(id, json string) {
+ s.setReply("/a/changes/proj~master~"+id, gerritReply{body: ")]}'\n" + json})
+}
+
func (s *gerritServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
s.mu.Lock()
defer s.mu.Unlock()
