git-review: add submit command
- add submit command. Fixes golang/go#9297.
- detect GitHub clones during git mail. Fixes golang/go#9266.
- expand api.go to be able to make authenticated Gerrit API calls.
- remove subcommand shorthands, since people don't invoke git-review directly.
the original functionality would now be implemented as short git aliases.
- check for unstaged changes during sync + suggest stash.
(git pull -r is going to whine anyway, might as well give a nicer error.)
- add testing hooks to capture death, stdout, stderr.
- add fake gerrit server for testing.
- add mail tests.
- add sync tests.
- add submit tests.
- fix change test.
Change-Id: I92a0113dca255c4205890776e2ceec6a1211606a
Reviewed-on: https://go-review.googlesource.com/1518
Reviewed-by: Andrew Gerrand <adg@golang.org>
diff --git a/git-review/api.go b/git-review/api.go
index ea42f6c..d5a9a2e 100644
--- a/git-review/api.go
+++ b/git-review/api.go
@@ -5,66 +5,277 @@
package main
import (
- "bufio"
+ "bytes"
"encoding/json"
- "errors"
"fmt"
+ "io"
+ "io/ioutil"
"net/http"
- "net/url"
+ "os"
"strings"
)
-const (
- sourceHost = ".googlesource.com"
- reviewSuffix = "-review"
-)
+// auth holds cached data about authentication to Gerrit.
+var auth struct {
+ host string // "go.googlesource.com"
+ url string // "https://go-review.googlesource.com"
+ project string // "go", "tools", "crypto", etc
-var notFound = errors.New("not found")
+ // Authentication information.
+ // Either cookie name + value from git cookie file
+ // or username and password from .netrc.
+ cookieName string
+ cookieValue string
+ user string
+ password string
+}
-func getChange(origin, id string) (*Change, error) {
- u, err := url.Parse(origin)
+// loadGerritOrigin loads the Gerrit host name from the origin remote.
+// If the origin remote does not appear to be a Gerrit server
+// (is missing, is GitHub, is not https, has too many path elements),
+// loadGerritOrigin dies.
+func loadGerritOrigin() {
+ if auth.host != "" {
+ return
+ }
+
+ origin := getOutput("git", "config", "remote.origin.url")
+ if origin == "" {
+ dief("git config remote.origin.url: origin not found")
+ }
+ if strings.Contains(origin, "//github.com/") {
+ dief("git origin must be a Gerrit host, not GitHub: %s", origin)
+ }
+
+ if !strings.HasPrefix(origin, "https://") {
+ dief("git origin must be an https:// URL: %s", origin)
+ }
+ // https:// prefix and then one slash between host and top-level name
+ if strings.Count(origin, "/") != 3 {
+ dief("git origin is malformed: %s", origin)
+ }
+ host := origin[len("https://"):strings.LastIndex(origin, "/")]
+
+ // In the case of Google's Gerrit, host is go.googlesource.com
+ // and apiURL uses go-review.googlesource.com, but the Gerrit
+ // setup instructions do not write down a cookie explicitly for
+ // go-review.googlesource.com, so we look for the non-review
+ // host name instead.
+ url := origin
+ if i := strings.Index(url, ".googlesource.com"); i >= 0 {
+ url = url[:i] + "-review" + url[i:]
+ }
+ i := strings.LastIndex(url, "/")
+ url, project := url[:i], url[i+1:]
+
+ auth.host = host
+ auth.url = url
+ auth.project = project
+}
+
+// loadAuth loads the authentication tokens for making API calls to
+// the Gerrit origin host.
+func loadAuth() {
+ if auth.user != "" || auth.cookieName != "" {
+ return
+ }
+
+ loadGerritOrigin()
+
+ // First look in Git's http.cookiefile, which is where Gerrit
+ // now tells users to store this information.
+ if cookieFile := getOutput("git", "config", "http.cookiefile"); cookieFile != "" {
+ data, _ := ioutil.ReadFile(cookieFile)
+ for _, line := range strings.Split(string(data), "\n") {
+ f := strings.Split(line, "\t")
+ if len(f) >= 7 && f[0] == auth.host {
+ auth.cookieName = f[5]
+ auth.cookieValue = f[6]
+ return
+ }
+ }
+ }
+
+ // If not there, then look in $HOME/.netrc, which is where Gerrit
+ // used to tell users to store the information, until the passwords
+ // got so long that old versions of curl couldn't handle them.
+ data, _ := ioutil.ReadFile(os.Getenv("HOME") + "/.netrc")
+ for _, line := range strings.Split(string(data), "\n") {
+ if i := strings.Index(line, "#"); i >= 0 {
+ line = line[:i]
+ }
+ f := strings.Fields(line)
+ if len(f) >= 6 && f[0] == "machine" && f[1] == auth.host && f[2] == "login" && f[4] == "password" {
+ auth.user = f[3]
+ auth.password = f[5]
+ return
+ }
+ }
+
+ dief("cannot find authentication info for %s", auth.host)
+}
+
+// gerritError is an HTTP error response served by Gerrit.
+type gerritError struct {
+ url string
+ statusCode int
+ status string
+ body string
+}
+
+func (e *gerritError) Error() string {
+ if e.statusCode == http.StatusNotFound {
+ return "change not found on Gerrit server"
+ }
+
+ extra := strings.TrimSpace(e.body)
+ if extra != "" {
+ extra = ": " + extra
+ }
+ return fmt.Sprintf("%s%s", e.status, extra)
+}
+
+// gerritAPI executes a GET or POST request to a Gerrit API endpoint.
+// It uses GET when requestBody is nil, otherwise POST. If target != nil,
+// gerritAPI expects to get a 200 response with a body consisting of an
+// anti-xss line (]})' or some such) followed by JSON.
+// If requestBody != nil, gerritAPI sets the Content-Type to application/json.
+func gerritAPI(path string, requestBody []byte, target interface{}) error {
+ // Strictly speaking, we might be able to use unauthenticated
+ // access, by removing the /a/ from the URL, but that assumes
+ // that all the information we care about is publicly visible.
+ // Using authentication makes it possible for this to work with
+ // non-public CLs or Gerrit hosts too.
+ loadAuth()
+
+ if !strings.HasPrefix(path, "/") {
+ dief("internal error: gerritAPI called with malformed path")
+ }
+
+ url := auth.url + path
+ method := "GET"
+ var reader io.Reader
+ if requestBody != nil {
+ method = "POST"
+ reader = bytes.NewReader(requestBody)
+ }
+ req, err := http.NewRequest(method, url, reader)
if err != nil {
- return nil, fmt.Errorf("parsing origin URL: %v", err)
+ return err
}
- if !strings.HasSuffix(u.Host, sourceHost) {
- return nil, fmt.Errorf("origin URL not on %v", sourceHost)
+ if requestBody != nil {
+ req.Header.Set("Content-Type", "application/json")
}
- u.Host = strings.TrimSuffix(u.Host, sourceHost) + reviewSuffix + sourceHost
- u.Path = "/changes/" + id
- r, err := http.Get(u.String())
+ if auth.cookieName != "" {
+ req.AddCookie(&http.Cookie{
+ Name: auth.cookieName,
+ Value: auth.cookieValue,
+ })
+ } else {
+ req.SetBasicAuth(auth.user, auth.password)
+ }
+
+ resp, err := http.DefaultClient.Do(req)
+ if err != nil {
+ return err
+ }
+ body, err := ioutil.ReadAll(resp.Body)
+ resp.Body.Close()
+
+ if err != nil {
+ return fmt.Errorf("reading response body: %v", err)
+ }
+ if resp.StatusCode != http.StatusOK {
+ return &gerritError{url, resp.StatusCode, resp.Status, string(body)}
+ }
+
+ if target != nil {
+ i := bytes.IndexByte(body, '\n')
+ if i < 0 {
+ return fmt.Errorf("%s: malformed json response", url)
+ }
+ body = body[i:]
+ if err := json.Unmarshal(body, target); err != nil {
+ return fmt.Errorf("%s: malformed json response", url)
+ }
+ }
+ return nil
+}
+
+// fullChangeID returns the unambigous Gerrit change ID for the pending change 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 {
+ loadGerritOrigin()
+ return auth.project + "~" + strings.TrimPrefix(b.OriginBranch(), "origin/") + "~" + b.ChangeID()
+}
+
+// readGerritChange reads the metadata about a change from the Gerrit server.
+// The changeID should use the syntax project~originbranch~Ihexhexhexhexhex returned
+// by fullChangeID. Using only Ihexhexhexhexhex will work provided it uniquely identifies
+// a single change on the server.
+// The changeID can have additional query parameters appended to it, as in "normalid?o=LABELS".
+// See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id for details.
+func readGerritChange(changeID string) (*GerritChange, error) {
+ var c GerritChange
+ err := gerritAPI("/a/changes/"+changeID, nil, &c)
if err != nil {
return nil, err
}
- defer r.Body.Close()
- if r.StatusCode == http.StatusNotFound {
- return nil, notFound
- }
- if r.StatusCode != http.StatusOK {
- return nil, fmt.Errorf("unexpected status: %v", r.Status)
- }
- br := bufio.NewReader(r.Body)
- br.ReadSlice('\n') // throw away first line
- var c Change
- if err := json.NewDecoder(br).Decode(&c); err != nil {
- return nil, err
- }
- u.Path = fmt.Sprintf("/%v", c.Number)
- c.URL = u.String()
return &c, nil
}
-type Change struct {
- ChangeId string `json:"change_id"`
- Subject string
- Status string
- Number int `json:"_number"`
- Messages []*Message
- URL string
+// GerritChange is the JSON struct returned by a Gerrit CL query.
+type GerritChange struct {
+ ID string
+ Project string
+ Branch string
+ ChangeId string `json:"change_id"`
+ Subject string
+ Status string
+ Created string
+ Updated string
+ Mergeable bool
+ Insertions int
+ Deletions int
+ Number int `json:"_number"`
+ Owner *GerritAccount
+ Labels map[string]*GerritLabel
+ CurrentRevision string `json:"current_revision"`
+ Revisions map[string]*GerritRevision
+ Messages []*GerritMessage
}
-type Message struct {
+// GerritMessage is the JSON struct for a Gerrit MessageInfo.
+type GerritMessage struct {
Author struct {
Name string
}
Message string
}
+
+// GerritLabel is the JSON struct for a Gerrit LabelInfo.
+type GerritLabel struct {
+ Optional bool
+ Blocking bool
+ Approved *GerritAccount
+ Rejected *GerritAccount
+}
+
+// GerritAccount is the JSON struct for a Gerrit AccountInfo.
+type GerritAccount struct {
+ ID int `json:"_account_id"`
+}
+
+// GerritRevision is the JSON struct for a Gerrit RevisionInfo.
+type GerritRevision struct {
+ Number int `json:"_number"`
+ Ref string
+ Fetch map[string]*GerritFetch
+}
+
+// GerritFetch is the JSON struct for a Gerrit FetchInfo
+type GerritFetch struct {
+ URL string
+ Ref string
+}
diff --git a/git-review/branch.go b/git-review/branch.go
index 812d574..054191f 100644
--- a/git-review/branch.go
+++ b/git-review/branch.go
@@ -15,9 +15,10 @@
// Branch describes a Git branch.
type Branch struct {
- Name string // branch name
- changeID string // Change-Id of pending commit ("" if nothing pending)
- subject string // first line of pending commit ("" if nothing pending)
+ Name string // branch name
+ changeID string // Change-Id of pending commit ("" if nothing pending)
+ subject string // first line of pending commit ("" if nothing pending)
+ commitHash string // commit hash of pending commit ("" if nothing pending)
}
// CurrentBranch returns the current branch.
@@ -26,8 +27,11 @@
return &Branch{Name: name}
}
+// OriginBranch returns the name of the origin branch that branch b tracks.
+// The returned name is like "origin/master" or "origin/dev.garbage" or
+// "origin/release-branch.go1.4".
func (b *Branch) OriginBranch() string {
- argv := []string{"git", "rev-parse", "--abbrev-ref", "@{u}"}
+ argv := []string{"git", "rev-parse", "--abbrev-ref", b.Name + "@{u}"}
out, err := exec.Command(argv[0], argv[1:]...).CombinedOutput()
if err == nil && len(out) > 0 {
return string(bytes.TrimSpace(out))
@@ -56,6 +60,7 @@
if b.HasPendingCommit() {
b.changeID = headChangeID(b.Name)
b.subject = commitSubject(b.Name)
+ b.commitHash = commitHash(b.Name)
}
}
return b.changeID
@@ -66,17 +71,22 @@
return b.subject
}
+func (b *Branch) CommitHash() string {
+ b.ChangeID() // page in commit hash
+ return b.commitHash
+}
+
func commitSubject(ref string) string {
- const f = "--format=format:%s"
- return getOutput("git", "log", "-n", "1", f, ref, "--")
+ return getOutput("git", "log", "-n", "1", "--format=format:%s", ref, "--")
+}
+
+func commitHash(ref string) string {
+ return getOutput("git", "log", "-n", "1", "--format=format:%H", ref, "--")
}
func headChangeID(branch string) string {
- const (
- p = "Change-Id: "
- f = "--format=format:%b"
- )
- for _, s := range getLines("git", "log", "-n", "1", f, branch, "--") {
+ const p = "Change-Id: "
+ for _, s := range getLines("git", "log", "-n", "1", "--format=format:%b", branch, "--") {
if strings.HasPrefix(s, p) {
return strings.TrimSpace(strings.TrimPrefix(s, p))
}
@@ -91,11 +101,22 @@
return len(getOutput("git", "log", "--grep", "Change-Id: "+id, b.OriginBranch())) > 0
}
-var stagedRe = regexp.MustCompile(`^[ACDMR] `)
+var stagedRE = regexp.MustCompile(`^[ACDMR] `)
func HasStagedChanges() bool {
for _, s := range getLines("git", "status", "-b", "--porcelain") {
- if stagedRe.MatchString(s) {
+ if stagedRE.MatchString(s) {
+ return true
+ }
+ }
+ return false
+}
+
+var unstagedRE = regexp.MustCompile(`^.[ACDMR] `)
+
+func HasUnstagedChanges() bool {
+ for _, s := range getLines("git", "status", "-b", "--porcelain") {
+ if unstagedRE.MatchString(s) {
return true
}
}
@@ -123,3 +144,12 @@
}
return branches
}
+
+// GerritChange returns the change metadata from the Gerrit server
+// for the branch's pending change.
+func (b *Branch) GerritChange() (*GerritChange, error) {
+ if !b.HasPendingCommit() {
+ return nil, fmt.Errorf("no pending commit")
+ }
+ return readGerritChange(fullChangeID(b) + "?o=LABELS&o=CURRENT_REVISION")
+}
diff --git a/git-review/change_test.go b/git-review/change_test.go
index 34c7a87..d055287 100644
--- a/git-review/change_test.go
+++ b/git-review/change_test.go
@@ -29,7 +29,7 @@
trun(t, gt.client, "git", "add", "file")
testMain(t, "change", "work")
testRan(t, "git checkout -q work",
- "git commit -q --allow-empty -m my commit msg")
+ "git commit -q --allow-empty -m foo: my commit msg")
t.Logf("master -> dev.branch")
testMain(t, "change", "dev.branch")
diff --git a/git-review/mail.go b/git-review/mail.go
index 93867d4..99821e3 100644
--- a/git-review/mail.go
+++ b/git-review/mail.go
@@ -45,7 +45,10 @@
"Use 'review change' to include them or 'review mail -f' to force it.")
}
- refSpec := "HEAD:refs/for/master"
+ // for side effect of dying with a good message if origin is GitHub
+ loadGerritOrigin()
+
+ refSpec := b.PushSpec()
start := "%"
if *rList != "" {
refSpec += mailList(start, "r", string(*rList))
@@ -70,6 +73,11 @@
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/")
+}
+
// mailAddressRE matches the mail addresses we admit. It's restrictive but admits
// all the addresses in the Go CONTRIBUTORS file at time of writing (tested separately).
var mailAddressRE = regexp.MustCompile(`^[a-zA-Z0-9][-_.a-zA-Z0-9]*@[-_.a-zA-Z0-9]+$`)
diff --git a/git-review/mail_test.go b/git-review/mail_test.go
new file mode 100644
index 0000000..8b80f9f
--- /dev/null
+++ b/git-review/mail_test.go
@@ -0,0 +1,37 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "testing"
+
+func TestMail(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+ gt.work(t)
+
+ // fake auth information to avoid Gerrit error
+ auth.host = "gerrit.fake"
+ auth.user = "not-a-user"
+ defer func() {
+ auth.host = ""
+ auth.user = ""
+ }()
+
+ testMain(t, "mail")
+ testRan(t,
+ "git push -q origin HEAD:refs/for/master",
+ "git tag -f work.mailed")
+}
+
+func TestMailGitHub(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+ gt.work(t)
+
+ trun(t, gt.client, "git", "config", "remote.origin.url", "https://github.com/golang/go")
+
+ testMainDied(t, "mail")
+ testPrintedStderr(t, "git origin must be a Gerrit host, not GitHub: https://github.com/golang/go")
+}
diff --git a/git-review/review.go b/git-review/review.go
index 6a6672d..d06c928 100644
--- a/git-review/review.go
+++ b/git-review/review.go
@@ -19,6 +19,7 @@
"bytes"
"flag"
"fmt"
+ "io"
"os"
"os/exec"
"strconv"
@@ -82,6 +83,9 @@
Show local branches and their head commits.
If -r is specified, show additional information from Gerrit.
+ submit
+ Submit the completed change commit into the repository.
+
sync
Fetch changes from the remote repository and merge them into
the current branch, rebasing the change commit on top of them.
@@ -109,7 +113,7 @@
installHook()
switch command {
- case "change", "c":
+ case "change":
change(args)
case "gofmt":
dief("gofmt not implemented")
@@ -117,9 +121,11 @@
// done - installHook already ran
case "mail", "m":
mail(args)
- case "pending", "p":
+ case "pending":
pending(args)
- case "sync", "s":
+ case "submit":
+ submit(args)
+ case "sync":
doSync(args)
default:
flags.Usage()
@@ -145,7 +151,7 @@
}
}
-var runLog []string
+var runLogTrap []string
func runErr(command string, args ...string) error {
if *verbose > 0 || *noRun {
@@ -154,13 +160,19 @@
if *noRun {
return nil
}
- if runLog != nil {
- runLog = append(runLog, strings.TrimSpace(command+" "+strings.Join(args, " ")))
+ if runLogTrap != nil {
+ runLogTrap = append(runLogTrap, strings.TrimSpace(command+" "+strings.Join(args, " ")))
}
cmd := exec.Command(command, args...)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
+ if stdoutTrap != nil {
+ cmd.Stdout = stdoutTrap
+ }
cmd.Stderr = os.Stderr
+ if stderrTrap != nil {
+ cmd.Stderr = stderrTrap
+ }
return cmd.Run()
}
@@ -215,8 +227,14 @@
}
}
+var stdoutTrap, stderrTrap *bytes.Buffer
+
func printf(format string, args ...interface{}) {
- fmt.Fprintf(os.Stderr, "%s: %s\n", os.Args[0], fmt.Sprintf(format, args...))
+ w := io.Writer(os.Stderr)
+ if stderrTrap != nil {
+ w = stderrTrap
+ }
+ fmt.Fprintf(w, "%s: %s\n", os.Args[0], fmt.Sprintf(format, args...))
}
// count is a flag.Value that is like a flag.Bool and a flag.Int.
diff --git a/git-review/submit.go b/git-review/submit.go
new file mode 100644
index 0000000..8db32ad
--- /dev/null
+++ b/git-review/submit.go
@@ -0,0 +1,142 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+ "sort"
+ "time"
+)
+
+// TODO(rsc): Add -tbr.
+
+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")
+ }
+ checkStaged("submit")
+
+ // 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.
+ checkUnstaged("submit")
+
+ // Fetch Gerrit information about this change.
+ ch, err := b.GerritChange()
+ if err != nil {
+ dief("%v", err)
+ }
+
+ // Check Gerrit change status.
+ switch ch.Status {
+ default:
+ dief("cannot submit: unexpected Gerrit change status %q", ch.Status)
+
+ case "NEW", "SUBMITTED":
+ // Not yet "MERGED", so try the submit.
+ // "SUBMITTED" is a weird state. It means that Submit has been clicked once,
+ // but it hasn't happened yet, usually because of a merge failure.
+ // The user may have done git sync and may now have a mergable
+ // copy waiting to be uploaded, so continue on as if it were "NEW".
+
+ case "MERGED":
+ // Can happen if moving between different clients.
+ dief("cannot submit: change already submitted, run 'git sync'")
+
+ case "ABANDONED":
+ dief("cannot submit: change abandoned")
+ }
+
+ // Check for label approvals (like CodeReview+2).
+ // The final submit will check these too, but it is better to fail now.
+ var names []string
+ for name := range ch.Labels {
+ names = append(names, name)
+ }
+ sort.Strings(names)
+ for _, name := range names {
+ label := ch.Labels[name]
+ if label.Optional {
+ continue
+ }
+ if label.Rejected != nil {
+ dief("cannot submit: change has %s rejection", name)
+ }
+ if label.Approved == nil {
+ dief("cannot submit: change missing %s approval", name)
+ }
+ }
+
+ // Upload most recent revision if not already on server.
+ if b.CommitHash() != ch.CurrentRevision {
+ run("git", "push", "-q", "origin", b.PushSpec())
+
+ // Refetch change information, especially mergeable.
+ ch, err = b.GerritChange()
+ if err != nil {
+ dief("%v", err)
+ }
+ }
+
+ // Don't bother if the server can't merge the changes.
+ if !ch.Mergeable {
+ // Server cannot merge; explicit sync is needed.
+ dief("cannot submit: conflicting changes submitted, run 'git sync'")
+ }
+
+ // Otherwise, try the submit. Sends back updated GerritChange,
+ // 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.
+ ch = new(GerritChange)
+ if err := gerritAPI("/a/changes/"+fullChangeID(b)+"/submit", []byte(`{"wait_for_merge": true}`), nil); err != nil {
+ dief("cannot submit: %v", err)
+ }
+
+ // It is common to get back "SUBMITTED" for a split second after the
+ // request is made. That indicates that the change has been queued for submit,
+ // but the first merge (the one wait_for_merge waited for)
+ // failed, possibly due to a spurious condition. We see this often, and the
+ // status usually changes to MERGED shortly thereafter.
+ // Wait a little while to see if we can get to a different state.
+ const steps = 6
+ const max = 2 * time.Second
+ for i := 0; i < steps; i++ {
+ time.Sleep(max * (1 << uint(i+1)) / (1 << steps))
+ ch, err = b.GerritChange()
+ if err != nil {
+ dief("%v", err)
+ }
+ if ch.Status != "SUBMITTED" {
+ break
+ }
+ }
+
+ switch ch.Status {
+ default:
+ dief("cannot submit: unexpected post-submit Gerrit change status %q", ch.Status)
+
+ case "MERGED":
+ // good
+
+ case "SUBMITTED":
+ // see above
+ dief("cannot submit: timed out waiting for change to be submitted by Gerrit")
+ }
+
+ // 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)
+ }
+
+ // Done! Change is submitted, branch is up to date, ready for new work.
+}
diff --git a/git-review/submit_test.go b/git-review/submit_test.go
new file mode 100644
index 0000000..c818e53
--- /dev/null
+++ b/git-review/submit_test.go
@@ -0,0 +1,166 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestSubmitErrors(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ srv := newGerritServer(t)
+ defer srv.done()
+
+ t.Logf("> no commit")
+ testMainDied(t, "submit")
+ testPrintedStderr(t, "cannot submit: no pending commit")
+ 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")
+
+ t.Logf("> staged changes")
+ write(t, gt.client+"/file1", "asdf")
+ trun(t, gt.client, "git", "add", "file1")
+ testMainDied(t, "submit")
+ testPrintedStderr(t, "cannot submit: staged changes exist",
+ "git status", "!git stash", "!git add", "git-review change")
+ testNoStdout(t)
+
+ t.Logf("> unstaged changes")
+ write(t, gt.client+"/file1", "actual content")
+ testMainDied(t, "submit")
+ testPrintedStderr(t, "cannot submit: unstaged changes exist",
+ "git status", "git stash", "git add", "git-review change")
+ testNoStdout(t)
+ testRan(t)
+ trun(t, gt.client, "git", "commit", "--amend", "--no-edit")
+
+ t.Logf("> not found")
+ 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})
+ }
+
+ t.Logf("> malformed json")
+ setJSON("XXX")
+ testMainDied(t, "submit")
+ testRan(t) // nothing
+ testPrintedStderr(t, "malformed json response")
+
+ t.Logf("> unexpected change status")
+ setJSON(`{"status": "UNEXPECTED"}`)
+ testMainDied(t, "submit")
+ testRan(t) // nothing
+ testPrintedStderr(t, "cannot submit: unexpected Gerrit change status \"UNEXPECTED\"")
+
+ t.Logf("> already merged")
+ setJSON(`{"status": "MERGED"}`)
+ testMainDied(t, "submit")
+ testRan(t) // nothing
+ testPrintedStderr(t, "cannot submit: change already submitted, run 'git sync'")
+
+ t.Logf("> abandoned")
+ setJSON(`{"status": "ABANDONED"}`)
+ testMainDied(t, "submit")
+ testRan(t) // nothing
+ testPrintedStderr(t, "cannot submit: change abandoned")
+
+ t.Logf("> missing approval")
+ setJSON(`{"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": {}}}}`)
+ 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": {}}}}`)
+ 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.setReply("/a/changes/proj~master~I123456789/submit", gerritReply{body: ")]}'\n" + newJSON})
+ testMainDied(t, "submit")
+ testRan(t, "git push -q origin HEAD:refs/for/master")
+ testPrintedStderr(t, "cannot submit: unexpected post-submit Gerrit change status \"NEW\"")
+}
+
+func TestSubmitTimeout(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+ srv := newGerritServer(t)
+ defer srv.done()
+
+ gt.work(t)
+
+ setJSON := func(json string) {
+ srv.setReply("/a/changes/proj~master~I123456789", gerritReply{body: ")]}'\n" + json})
+ }
+
+ t.Log("> submit with timeout")
+ const submittedJSON = `{"status": "SUBMITTED", "mergeable": true, "labels": {"Code-Review": {"approved": {}}}}`
+ setJSON(submittedJSON)
+ srv.setReply("/a/changes/proj~master~I123456789/submit", gerritReply{body: ")]}'\n" + submittedJSON})
+ testMainDied(t, "submit")
+ testRan(t, "git push -q origin HEAD:refs/for/master")
+ testPrintedStderr(t, "cannot submit: timed out waiting for change to be submitted by Gerrit")
+}
+
+func TestSubmit(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+ srv := newGerritServer(t)
+ defer srv.done()
+
+ gt.work(t)
+ trun(t, gt.client, "git", "tag", "-f", "work.mailed")
+ clientHead := strings.TrimSpace(trun(t, gt.client, "git", "log", "-n", "1", "--format=format:%H"))
+
+ write(t, gt.server+"/file", "another change")
+ trun(t, gt.server, "git", "add", "file")
+ trun(t, gt.server, "git", "commit", "-m", "conflict")
+ serverHead := strings.TrimSpace(trun(t, gt.server, "git", "log", "-n", "1", "--format=format:%H"))
+
+ t.Log("> submit")
+ var (
+ newJSON = `{"status": "NEW", "mergeable": true, "current_revision": "` + clientHead + `", "labels": {"Code-Review": {"approved": {}}}}`
+ submittedJSON = `{"status": "SUBMITTED", "mergeable": true, "current_revision": "` + clientHead + `", "labels": {"Code-Review": {"approved": {}}}}`
+ mergedJSON = `{"status": "MERGED", "mergeable": true, "current_revision": "` + serverHead + `", "labels": {"Code-Review": {"approved": {}}}}`
+ )
+ submitted := false
+ npoll := 0
+ srv.setReply("/a/changes/proj~master~I123456789", gerritReply{f: func() gerritReply {
+ if !submitted {
+ return gerritReply{body: ")]}'\n" + newJSON}
+ }
+ if npoll++; npoll <= 2 {
+ return gerritReply{body: ")]}'\n" + submittedJSON}
+ }
+ return gerritReply{body: ")]}'\n" + mergedJSON}
+ }})
+ srv.setReply("/a/changes/proj~master~I123456789/submit", gerritReply{f: func() gerritReply {
+ if submitted {
+ return gerritReply{status: 409}
+ }
+ submitted = true
+ return gerritReply{body: ")]}'\n" + submittedJSON}
+ }})
+ testMain(t, "submit")
+ testRan(t,
+ "git fetch -q",
+ "git checkout -q -B work "+serverHead+" --")
+}
diff --git a/git-review/sync.go b/git-review/sync.go
index c6618f9..5984df3 100644
--- a/git-review/sync.go
+++ b/git-review/sync.go
@@ -13,6 +13,11 @@
b := CurrentBranch()
id := b.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.
+ checkStaged("sync")
+ checkUnstaged("sync")
+
// Pull remote changes into local branch.
// We do this in one command so that people following along with 'git sync -v'
// see fewer commands to understand.
@@ -28,3 +33,20 @@
run("git", "reset", "HEAD^")
}
}
+
+func checkStaged(cmd string) {
+ if HasStagedChanges() {
+ dief("cannot %s: staged changes exist\n"+
+ "\trun 'git status' to see changes\n"+
+ "\trun 'git-review change' to commit staged changes", cmd)
+ }
+}
+
+func checkUnstaged(cmd string) {
+ if HasUnstagedChanges() {
+ dief("cannot %s: unstaged changes exist\n"+
+ "\trun 'git status' to see changes\n"+
+ "\trun 'git stash' to save unstaged changes\n"+
+ "\trun 'git add' and 'git-review change' to commit staged changes", cmd)
+ }
+}
diff --git a/git-review/sync_test.go b/git-review/sync_test.go
new file mode 100644
index 0000000..64ad7c6
--- /dev/null
+++ b/git-review/sync_test.go
@@ -0,0 +1,48 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "testing"
+
+func TestSync(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ testMain(t, "change", "work")
+
+ // check for error with unstaged changes
+ write(t, gt.client+"/file1", "")
+ trun(t, gt.client, "git", "add", "file1")
+ write(t, gt.client+"/file1", "actual content")
+ testMainDied(t, "sync")
+ testPrintedStderr(t, "cannot sync: unstaged changes exist",
+ "git status", "git stash", "git add", "git-review change")
+ testNoStdout(t)
+
+ // check for error with staged changes
+ trun(t, gt.client, "git", "add", "file1")
+ testMainDied(t, "sync")
+ testPrintedStderr(t, "cannot sync: staged changes exist",
+ "git status", "!git stash", "!git add", "git-review change")
+ testNoStdout(t)
+
+ // check for success after stash
+ trun(t, gt.client, "git", "stash")
+ testMain(t, "sync")
+ testNoStdout(t)
+ testNoStderr(t)
+
+ // make server 1 step ahead of client
+ write(t, gt.server+"/file", "new content")
+ trun(t, gt.server, "git", "add", "file")
+ trun(t, gt.server, "git", "commit", "-m", "msg")
+
+ // check for success
+ testMain(t, "sync")
+ testNoStdout(t)
+ testNoStderr(t)
+}
+
+// TODO: Add TestSyncRebase?
diff --git a/git-review/util_test.go b/git-review/util_test.go
index 2834563..0c5c1e5 100644
--- a/git-review/util_test.go
+++ b/git-review/util_test.go
@@ -5,12 +5,17 @@
package main
import (
+ "bytes"
+ "fmt"
"io/ioutil"
+ "net"
+ "net/http"
"os"
"os/exec"
"path/filepath"
"reflect"
"strings"
+ "sync"
"testing"
)
@@ -26,6 +31,16 @@
os.Chdir(gt.pwd)
}
+func (gt *gitTest) work(t *testing.T) {
+ trun(t, gt.client, "git", "checkout", "-b", "work")
+ trun(t, gt.client, "git", "branch", "--set-upstream-to", "origin/master")
+
+ // make local change on client
+ write(t, gt.client+"/file", "new content")
+ trun(t, gt.client, "git", "add", "file")
+ trun(t, gt.client, "git", "commit", "-m", "msg\n\nChange-Id: I123456789\n")
+}
+
func newGitTest(t *testing.T) *gitTest {
tmpdir, err := ioutil.TempDir("", "git-review-test")
if err != nil {
@@ -82,40 +97,188 @@
}
}
-func trun(t *testing.T, dir string, cmdline ...string) {
+func trun(t *testing.T, dir string, cmdline ...string) string {
cmd := exec.Command(cmdline[0], cmdline[1:]...)
cmd.Dir = dir
out, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("in %s/, ran %s: %v\n%s", filepath.Base(dir), cmdline, err, out)
}
+ return string(out)
+}
+
+var (
+ runLog []string
+ stderr *bytes.Buffer
+ stdout *bytes.Buffer
+ died bool
+)
+
+var mainCanDie bool
+
+func testMainDied(t *testing.T, args ...string) {
+ mainCanDie = true
+ testMain(t, args...)
+ if !died {
+ t.Fatalf("expected to die, did not\nstdout:\n%sstderr:\n%s", stdout, stderr)
+ }
+}
+
+func testMainCanDie(t *testing.T, args ...string) {
+ mainCanDie = true
+ testMain(t, args...)
}
func testMain(t *testing.T, args ...string) {
t.Logf("git-review %s", strings.Join(args, " "))
- runLog = []string{} // non-nil, to trigger saving of commands
+
+ canDie := mainCanDie
+ mainCanDie = false // reset for next invocation
defer func() {
+ runLog = runLogTrap
+ stdout = stdoutTrap
+ stderr = stderrTrap
+
+ dieTrap = nil
+ runLogTrap = nil
+ stdoutTrap = nil
+ stderrTrap = nil
if err := recover(); err != nil {
- runLog = nil
- dieTrap = nil
- t.Fatalf("panic: %v", err)
+ if died && canDie {
+ return
+ }
+ var msg string
+ if died {
+ msg = "died"
+ } else {
+ msg = fmt.Sprintf("panic: %v", err)
+ }
+ t.Fatalf("%s\nstdout:\n%sstderr:\n%s", msg, stdout, stderr)
}
}()
dieTrap = func() {
+ died = true
panic("died")
}
+ died = false
+ runLogTrap = []string{} // non-nil, to trigger saving of commands
+ stdoutTrap = new(bytes.Buffer)
+ stderrTrap = new(bytes.Buffer)
os.Args = append([]string{"git-review"}, args...)
main()
-
- dieTrap = nil
}
func testRan(t *testing.T, cmds ...string) {
+ if cmds == nil {
+ cmds = []string{}
+ }
if !reflect.DeepEqual(runLog, cmds) {
t.Errorf("ran:\n%s", strings.Join(runLog, "\n"))
t.Errorf("wanted:\n%s", strings.Join(cmds, "\n"))
}
}
+
+func testPrinted(t *testing.T, buf *bytes.Buffer, name string, messages ...string) {
+ all := buf.String()
+ var errors bytes.Buffer
+ for _, msg := range messages {
+ if strings.HasPrefix(msg, "!") {
+ if strings.Contains(all, msg[1:]) {
+ fmt.Fprintf(&errors, "%s does (but should not) contain %q\n", name, msg[1:])
+ }
+ continue
+ }
+ if !strings.Contains(all, msg) {
+ fmt.Fprintf(&errors, "%s does not contain %q\n", name, msg)
+ }
+ }
+ if errors.Len() > 0 {
+ t.Fatalf("wrong output\n%s%s:\n%s", &errors, name, all)
+ }
+}
+
+func testPrintedStdout(t *testing.T, messages ...string) {
+ testPrinted(t, stdout, "stdout", messages...)
+}
+
+func testPrintedStderr(t *testing.T, messages ...string) {
+ testPrinted(t, stderr, "stderr", messages...)
+}
+
+func testNoStdout(t *testing.T) {
+ if stdout.Len() != 0 {
+ t.Fatalf("unexpected stdout:\n%s", stdout)
+ }
+}
+
+func testNoStderr(t *testing.T) {
+ if stderr.Len() != 0 {
+ t.Fatalf("unexpected stderr:\n%s", stderr)
+ }
+}
+
+type gerritServer struct {
+ l net.Listener
+ mu sync.Mutex
+ reply map[string]gerritReply
+}
+
+func newGerritServer(t *testing.T) *gerritServer {
+ l, err := net.Listen("tcp", "127.0.0.1:0")
+ if err != nil {
+ t.Fatalf("starting fake gerrit: %v", err)
+ }
+
+ auth.host = l.Addr().String()
+ auth.url = "http://" + auth.host
+ auth.project = "proj"
+ auth.user = "gopher"
+ auth.password = "PASSWORD"
+
+ s := &gerritServer{l: l, reply: make(map[string]gerritReply)}
+ go http.Serve(l, s)
+ return s
+}
+
+func (s *gerritServer) done() {
+ s.l.Close()
+ auth.host = ""
+ auth.url = ""
+ auth.project = ""
+ auth.user = ""
+ auth.password = ""
+}
+
+type gerritReply struct {
+ status int
+ body string
+ f func() gerritReply
+}
+
+func (s *gerritServer) setReply(path string, reply gerritReply) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ s.reply[path] = reply
+}
+
+func (s *gerritServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ reply, ok := s.reply[req.URL.Path]
+ if !ok {
+ http.NotFound(w, req)
+ return
+ }
+ if reply.f != nil {
+ reply = reply.f()
+ }
+ if reply.status != 0 {
+ w.WriteHeader(reply.status)
+ }
+ if len(reply.body) > 0 {
+ w.Write([]byte(reply.body))
+ }
+}
