gitiles.Log: implement paging.

common/api/gitiles/gitiles.go

Index: common/api/gitiles/gitiles.go
diff --git a/common/api/gitiles/gitiles.go b/common/api/gitiles/gitiles.go
index 39f71f52a378b8e0b9dc024384d8381147d7f9d8..bf7bd3341a646d844cf989773665f0f79b405474 100644
--- a/common/api/gitiles/gitiles.go
+++ b/common/api/gitiles/gitiles.go
@@ -24,8 +24,10 @@ import (
 	"strings"
 	"time"
 
-	"github.com/luci/luci-go/server/auth"
+	"github.com/luci/luci-go/common/errors"
+	"github.com/luci/luci-go/common/retry/transient"
 	"golang.org/x/net/context"
+	"golang.org/x/net/context/ctxhttp"
 )
 
 // User is the author or the committer returned from gitiles.
@@ -50,15 +52,15 @@ type Commit struct {
 	Message   string   `json:"message"`
 }
 
-// LogResponse is the JSON response from querying gitiles for a log request.
-type LogResponse struct {
-	Log  []Commit `json:"log"`
-	Next string   `json:"next"`
+// ValidateRepoURL validates gitiles repository URL for use in this package.
+func ValidateRepoURL(repoURL string) error {
+	_, err := NormalizeRepoURL(repoURL)
+	return err
 }
 
-// fixURL validates and normalizes a repoURL and treeish, and returns the
-// log JSON gitiles URL.
-func fixURL(repoURL, treeish string) (string, error) {
+// NormalizeRepoURL returns canonical for gitiles URL of the repo including "a/" path prefix.
+// error is returned if validation fails.
+func NormalizeRepoURL(repoURL string) (string, error) {
 	u, err := url.Parse(repoURL)
 	if err != nil {
 		return "", err
@@ -67,46 +69,139 @@ func fixURL(repoURL, treeish string) (string, error) {
 		return "", fmt.Errorf("%s should start with https://", repoURL)
 	}
 	if !strings.HasSuffix(u.Host, ".googlesource.com") {
-		return "", fmt.Errorf("Only .googlesource.com repos supported")
+		return "", errors.New("only .googlesource.com repos supported")
+	}
+	if u.Fragment != "" {
+		return "", errors.New("no fragments allowed in repoURL")
+	}
+	if u.Path == "" || u.Path == "/" {
+		return "", errors.New("path to repo is empty")
+	}
+	if !strings.HasPrefix(u.Path, "/") {
+		u.Path = "/" + u.Path
 	}
-	// Use the authenticated URL
-	u.Path = "a/" + u.Path
-	URL := fmt.Sprintf("%s/+log/%s?format=JSON", u.String(), treeish)
-	return URL, nil
+	if !strings.HasPrefix(u.Path, "/a/") {
+		// Use the authenticated URL
+		u.Path = "/a" + u.Path
+	}
+
+	u.Path = strings.TrimRight(u.Path, "/")
+	u.Path = strings.TrimSuffix(u.Path, ".git")
+	return u.String(), nil
 }
 
-// Log returns a list of commits based on a repo and treeish (usually
-// a branch). This should be equivilent of a "git log <treeish>" call in
-// that repository.
-func Log(c context.Context, repoURL, treeish string, limit int) ([]Commit, error) {
-	// TODO(hinoka): Respect the limit.
-	URL, err := fixURL(repoURL, treeish)
+// New returns new gitiles client.
+func New(c *http.Client) *Client {

[Vadim Sh., 2017/07/20 21:04:47]

just drop this function, it's totally normal in go to initialize objects
directly 

c := gitiles.Client{
  Client: http.DefaultClient,
}

[tandrii(chromium), 2017/07/20 21:11:14]

Done.

+	return &Client{Client: c}
+}
+
+// Client is Gitiles client.
+type Client struct {
+	Client *http.Client
+
+	// Used for testing only.
+	mockRepoURL string
+}
+
+// Log returns a list of commits based on a repo and treeish.
+// This should be equivalent of a "git log <treeish>" call in that repository.
+//
+// treeish can be either:
+//   (1) a git revision as 40-char string or its prefix so long as its unique in repo.
+//   (2) a ref such as "refs/heads/branch" or just "branch"
+//   (3) a ref defined as n-th parent of R in the form "R~n".
+//       For example, "master~2" or "deadbeef~1".
+//   (4) a range between two revisions in the form "CHILD..PREDECESSOR", where
+//       CHILD and PREDECESSOR are each specified in either (1), (2) or (3)
+//       formats listed above.
+//       For example, "foo..ba1", "master..refs/branch-heads/1.2.3",
+//         or even "master~5..master~9".
+//
+//
+// If the returned log has a commit with 2+ parents, the order of commits after
+// that is whatever Gitiles returns, which currently means ordered
+// by topological sort first, and then by commit timestamps.
+//
+// This means that if Log(C) contains commit A, Log(A) will not necessarily return
+// a subsequence of Log(C) (though definitely a subset). For example,
+//
+//     common... -> base ------> A ----> C
+//                       \            /
+//                        --> B ------
+//
+//     ----commit timestamp increases--->
+//
+//  Log(A) = [A, base, common...]
+//  Log(B) = [B, base, common...]
+//  Log(C) = [C, A, B, base, common...]
+//
+func (c *Client) Log(ctx context.Context, repoURL, treeish string, limit int) ([]Commit, error) {
+	repoURL, err := NormalizeRepoURL(repoURL)
 	if err != nil {
 		return nil, err
 	}
-	t, err := auth.GetRPCTransport(c, auth.AsSelf, auth.WithScopes(
-		"https://www.googleapis.com/auth/gerritcodereview",
-	))
-	if err != nil {
+	if limit < 1 {
+		return nil, fmt.Errorf("limit must be at least 1, but %d provided", limit)
+	}
+	subPath := fmt.Sprintf("+log/%s?format=JSON", url.PathEscape(treeish))
+	resp := &logResponse{}
+	if err := c.get(ctx, repoURL, subPath, resp); err != nil {
 		return nil, err
 	}
-	client := http.Client{Transport: t}
-	r, err := client.Get(URL)
+	result := resp.Log
+	for {
+		if resp.Next == "" || len(result) >= limit {
+			if len(result) > limit {
+				result = result[:limit]
+			}
+			return result, nil
+		}
+		nextPath := subPath + "&s=" + resp.Next
+		resp = &logResponse{}
+		if err := c.get(ctx, repoURL, nextPath, resp); err != nil {
+			return nil, err
+		}
+		result = append(result, resp.Log...)
+	}
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+// logResponse is the JSON response from querying gitiles for a log request.
+type logResponse struct {
+	Log  []Commit `json:"log"`
+	Next string   `json:"next"`
+}
+
+func (c *Client) get(ctx context.Context, repoURL, subPath string, result interface{}) error {
+	URL := fmt.Sprintf("%s/%s", repoURL, subPath)
+	if c.mockRepoURL != "" {
+		URL = fmt.Sprintf("%s/%s", c.mockRepoURL, subPath)
+	}
+	r, err := ctxhttp.Get(ctx, c.Client, URL)
 	if err != nil {
-		return nil, err
+		return transient.Tag.Apply(err)
 	}
+	defer r.Body.Close()
 	if r.StatusCode != 200 {
-		return nil, fmt.Errorf("Failed to fetch %s, status code %d", URL, r.StatusCode)
+		err = fmt.Errorf("failed to fetch %s, status code %d", URL, r.StatusCode)
+		if r.StatusCode >= 500 {
+			// TODO(tandrii): consider retrying.
+			err = transient.Tag.Apply(err)
+		}
+		return err
 	}
-	defer r.Body.Close()
 	// Strip out the jsonp header, which is ")]}'"
 	trash := make([]byte, 4)
-	r.Body.Read(trash) // Read the jsonp header
-	commits := LogResponse{}
-	if err := json.NewDecoder(r.Body).Decode(&commits); err != nil {
-		return nil, err
+	cnt, err := r.Body.Read(trash)
+	if err != nil {
+		return errors.Annotate(err, "unexpected response from Gitiles").Err()
+	}
+	if cnt != 4 || ")]}'" != string(trash) {
+		return errors.New("unexpected response from Gitiles")
+	}
+	if err = json.NewDecoder(r.Body).Decode(result); err != nil {
+		return errors.Annotate(err, "failed to decode Gitiles response into %T", result).Err()
 	}
-	// TODO(hinoka): If there is a page and we have gotten less than the limit,
-	// keep making requests for the next page until we have enough commits.
-	return commits.Log, nil
+	return nil
 }