git-codereview: batch GerritChange info fetches during pending
This speeds pending a bit by reducing the number of round trips
to the Gerrit server when you have a stack of CLs.
Change-Id: I456e1a8739b9b6586f4e05c1a5442f402e440a79
Reviewed-on: https://go-review.googlesource.com/67571
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
diff --git a/git-codereview/api.go b/git-codereview/api.go
index a81fc9e..77d2c08 100644
--- a/git-codereview/api.go
+++ b/git-codereview/api.go
@@ -269,7 +269,7 @@
if target != nil {
i := bytes.IndexByte(body, '\n')
if i < 0 {
- return fmt.Errorf("%s: malformed json response", url)
+ return fmt.Errorf("%s: malformed json response - bad header", url)
}
body = body[i:]
if err := json.Unmarshal(body, target); err != nil {
@@ -302,6 +302,63 @@
return &c, nil
}
+// readGerritChanges is like readGerritChange but expects changeID
+// to be a query parameter list like q=change:XXX&q=change:YYY&o=OPTIONS,
+// and it expects to receive a JSON array of GerritChanges, not just one.
+func readGerritChanges(query string) ([][]*GerritChange, error) {
+ // The Gerrit server imposes a limit of at most 10 q= parameters.
+ v, err := url.ParseQuery(query)
+ if err != nil {
+ return nil, err
+ }
+ var results []chan gerritChangeResult
+ for len(v["q"]) > 0 {
+ n := len(v["q"])
+ if n > 10 {
+ n = 10
+ }
+ all := v["q"]
+ v["q"] = all[:n]
+ query := v.Encode()
+ v["q"] = all[n:]
+ ch := make(chan gerritChangeResult, 1)
+ go readGerritChangesBatch(query, n, ch)
+ results = append(results, ch)
+ }
+
+ var c [][]*GerritChange
+ for _, ch := range results {
+ res := <-ch
+ if res.err != nil {
+ return nil, res.err
+ }
+ c = append(c, res.c...)
+ }
+ return c, nil
+}
+
+type gerritChangeResult struct {
+ c [][]*GerritChange
+ err error
+}
+
+func readGerritChangesBatch(query string, n int, ch chan gerritChangeResult) {
+ var c [][]*GerritChange
+ // If there are multiple q=, the server sends back an array of arrays of results.
+ // If there is a single q=, it only sends back an array of results; in that case
+ // we need to do the wrapping ourselves.
+ var arg interface{} = &c
+ if n == 1 {
+ c = append(c, nil)
+ arg = &c[0]
+ }
+ err := gerritAPI("/a/changes/?"+query, nil, arg)
+ if len(c) != n && err == nil {
+ err = fmt.Errorf("gerrit result count mismatch")
+ }
+ ch <- gerritChangeResult{c, err}
+}
+
// GerritChange is the JSON struct returned by a Gerrit CL query.
type GerritChange struct {
ID string
diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 13193ff..982c739 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -7,6 +7,7 @@
import (
"bytes"
"fmt"
+ "net/url"
"os"
"os/exec"
"regexp"
@@ -322,6 +323,33 @@
return readGerritChange(id)
}
+// GerritChange returns the change metadata from the Gerrit server
+// for the given changes, which each be be the result of fullChangeID(b, c) for some c.
+// 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) GerritChanges(ids []string, extra ...string) ([][]*GerritChange, error) {
+ q := ""
+ for _, id := range ids {
+ if q != "" {
+ q += "&"
+ }
+ if strings.HasSuffix(id, "~") {
+ // result of fullChangeID(b, c) with missing Change-Id; don't send
+ q += "q=is:closed+is:open" // cannot match anything
+ continue
+ }
+ q += "q=change:" + url.QueryEscape(id)
+ }
+ if q == "" {
+ return nil, fmt.Errorf("no changes found")
+ }
+ for _, x := range extra {
+ q += "&o=" + url.QueryEscape(x)
+ }
+ return readGerritChanges(q)
+}
+
// CommitByRev finds a unique pending commit by its git <rev>.
// It dies if rev cannot be resolved to a commit or that commit is not
// pending on b using the action ("mail", "submit") in the failure message.
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index 0219a20..83725c8 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -42,11 +42,37 @@
if b.current {
b.staged, b.unstaged, b.untracked = LocalChanges()
}
+ var changeIDs []string
+ var commits []*Commit
for _, c := range b.Pending() {
c.committed = ListFiles(c)
- if !pendingLocal {
- c.g, c.gerr = b.GerritChange(c, "DETAILED_LABELS", "CURRENT_REVISION", "MESSAGES", "DETAILED_ACCOUNTS")
+ if c.ChangeID == "" {
+ c.gerr = fmt.Errorf("missing Change-Id in commit message")
+ } else {
+ changeIDs = append(changeIDs, fullChangeID(b.Branch, c))
+ commits = append(commits, c)
}
+ }
+ if !pendingLocal {
+ gs, err := b.GerritChanges(changeIDs, "DETAILED_LABELS", "CURRENT_REVISION", "MESSAGES", "DETAILED_ACCOUNTS")
+ if len(gs) != len(commits) && err == nil {
+ err = fmt.Errorf("invalid response from Gerrit server - %d queries but %d results", len(changeIDs), len(gs))
+ }
+ if err != nil {
+ for _, c := range commits {
+ if c.gerr != nil {
+ c.gerr = err
+ }
+ }
+ } else {
+ for i, c := range commits {
+ if len(gs[i]) == 1 {
+ c.g = gs[i][0]
+ }
+ }
+ }
+ }
+ for _, c := range b.Pending() {
if c.g == nil {
c.g = new(GerritChange) // easier for formatting code
}
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index e9a6b02..e29d0eb 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -5,6 +5,7 @@
package main
import (
+ "fmt"
"io/ioutil"
"os"
"os/exec"
@@ -402,8 +403,48 @@
`)
}
+func TestPendingGerritMultiChange15(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+ srv := newGerritServer(t)
+ defer srv.done()
+
+ gt.work(t)
+ hash1 := CurrentBranch().Pending()[0].Hash
+ testPendingReply(srv, "I123456789", hash1, "MERGED")
+
+ for i := 1; i < 15; i++ {
+ write(t, gt.client+"/file", fmt.Sprintf("v%d", i))
+ trun(t, gt.client, "git", "commit", "-a", "-m", fmt.Sprintf("v%d\n\nChange-Id: I%010d", i, i))
+ hash2 := CurrentBranch().Pending()[0].Hash
+ testPendingReply(srv, fmt.Sprintf("I%010d", i), hash2, "NEW")
+ }
+
+ testPendingArgs(t, []string{"-s"}, `
+ work REVHASH..REVHASH (current branch, all mailed)
+ + REVHASH v14 (CL 1234 -2 +1, mailed)
+ + REVHASH v13 (CL 1234 -2 +1, mailed)
+ + REVHASH v12 (CL 1234 -2 +1, mailed)
+ + REVHASH v11 (CL 1234 -2 +1, mailed)
+ + REVHASH v10 (CL 1234 -2 +1, mailed)
+ + REVHASH v9 (CL 1234 -2 +1, mailed)
+ + REVHASH v8 (CL 1234 -2 +1, mailed)
+ + REVHASH v7 (CL 1234 -2 +1, mailed)
+ + REVHASH v6 (CL 1234 -2 +1, mailed)
+ + REVHASH v5 (CL 1234 -2 +1, mailed)
+ + REVHASH v4 (CL 1234 -2 +1, mailed)
+ + REVHASH v3 (CL 1234 -2 +1, mailed)
+ + REVHASH v2 (CL 1234 -2 +1, mailed)
+ + REVHASH v1 (CL 1234 -2 +1, mailed)
+ + REVHASH msg (CL 1234 -2 +1, mailed, submitted)
+
+ `)
+}
+
func testPendingReply(srv *gerritServer, id, rev, status string) {
srv.setJSON(id, `{
+ "id": "proj~master~`+id+`",
+ "project": "proj",
"current_revision": "`+rev+`",
"status": "`+status+`",
"_number": 1234,
@@ -455,10 +496,12 @@
}
func testPending(t *testing.T, want string) {
+ t.Helper()
testPendingArgs(t, nil, want)
}
func testPendingArgs(t *testing.T, args []string, want string) {
+ t.Helper()
// fake auth information to avoid Gerrit error
if auth.host == "" {
auth.host = "gerrit.fake"
diff --git a/git-codereview/review.go b/git-codereview/review.go
index ab5a795..2e67396 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -18,6 +18,7 @@
"os/exec"
"strconv"
"strings"
+ "time"
)
var (
@@ -207,8 +208,13 @@
var runLogTrap []string
func runDirErr(dir, command string, args ...string) error {
- if *verbose > 0 || *noRun {
+ if *noRun || *verbose == 1 {
fmt.Fprintln(stderr(), commandString(command, args))
+ } else if *verbose > 1 {
+ start := time.Now()
+ defer func() {
+ fmt.Fprintf(stderr(), "%s # %.3fs\n", commandString(command, args), time.Since(start).Seconds())
+ }()
}
if *noRun {
return nil
@@ -272,7 +278,10 @@
// the git repo" commands, which is confusing if you are just trying to find
// out what git sync means.
if *verbose > 1 {
- fmt.Fprintln(stderr(), commandString(command, args))
+ start := time.Now()
+ defer func() {
+ fmt.Fprintf(stderr(), "%s # %.3fs\n", commandString(command, args), time.Since(start).Seconds())
+ }()
}
cmd := exec.Command(command, args...)
if dir != "." {
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index 7f088e8..89753d3 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -419,6 +419,10 @@
}
func (s *gerritServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+ if req.URL.Path == "/a/changes/" {
+ s.serveChangesQuery(w, req)
+ return
+ }
s.mu.Lock()
defer s.mu.Unlock()
reply, ok := s.reply[req.URL.Path]
@@ -443,3 +447,45 @@
w.Write([]byte(reply.body))
}
}
+
+func (s *gerritServer) serveChangesQuery(w http.ResponseWriter, req *http.Request) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ qs := req.URL.Query()["q"]
+ if len(qs) > 10 {
+ http.Error(w, "too many queries", 500)
+ }
+ var buf bytes.Buffer
+ fmt.Fprintf(&buf, ")]}'\n")
+ end := ""
+ if len(qs) > 1 {
+ fmt.Fprintf(&buf, "[")
+ end = "]"
+ }
+ sep := ""
+ for _, q := range qs {
+ fmt.Fprintf(&buf, "%s[", sep)
+ if strings.HasPrefix(q, "change:") {
+ reply, ok := s.reply[req.URL.Path+strings.TrimPrefix(q, "change:")]
+ if ok {
+ if reply.json != nil {
+ body, err := json.Marshal(reply.json)
+ if err != nil {
+ dief("%v", err)
+ }
+ reply.body = ")]}'\n" + string(body)
+ }
+ body := reply.body
+ i := strings.Index(body, "\n")
+ if i > 0 {
+ body = body[i+1:]
+ }
+ fmt.Fprintf(&buf, "%s", body)
+ }
+ }
+ fmt.Fprintf(&buf, "]")
+ sep = ","
+ }
+ fmt.Fprintf(&buf, "%s", end)
+ w.Write(buf.Bytes())
+}