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>

6 files changed, 214 insertions, 5 deletions

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 @@ func gerritAPI(path string, requestBody []byte, target interface{}) error {
 	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 @@ func readGerritChange(changeID string) (*GerritChange, error) {
 	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 @@ package main
 import (
 	"bytes"
 	"fmt"
+	"net/url"
 	"os"
 	"os/exec"
 	"regexp"
@@ -322,6 +323,33 @@ func (b *Branch) GerritChange(c *Commit, extra ...string) (*GerritChange, error)
 	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 @@ func (b *pendingBranch) load() {
 	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 TestPendingGerritMultiChange(t *testing.T) {
 	`)
 }
 
+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 testPendingReply(srv *gerritServer, id, rev, status string) {
 }
 
 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 @@ import (
 	"os/exec"
 	"strconv"
 	"strings"
+	"time"
 )
 
 var (
@@ -207,8 +208,13 @@ func runErr(command string, args ...string) error {
 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 @@ func cmdOutputDirErr(dir, command string, args ...string) (string, error) {
 	// 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) setJSON(id, json string) {
 }
 
 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 @@ func (s *gerritServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		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())
+}