gitiles.Log: implement paging.

common/api/gitiles/gitiles.go

 // Copyright 2016 The LUCI Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //      http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 package gitiles
 
 // TODO(tandrii): add tests.
 
 import (
         "encoding/json"
         "fmt"
         "net/http"
         "net/url"
         "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.
 type User struct {
         Name  string `json:"name"`
         Email string `json:"email"`
         Time  string `json:"time"`
 }
 
 // GetTime returns the Time field as real data!
 func (u *User) GetTime() (time.Time, error) {
         return time.Parse(time.ANSIC, u.Time)
 }
 
 // Commit is the information of a commit returned from gitiles.
 type Commit struct {
         Commit    string   `json:"commit"`
         Tree      string   `json:"tree"`
         Parents   []string `json:"parents"`
         Author    User     `json:"author"`
         Committer User     `json:"committer"`
         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
         }
         if u.Scheme != "https" {
                 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")
         }
-        // Use the authenticated URL
-        u.Path = "a/" + u.Path
-        URL := fmt.Sprintf("%s/+log/%s?format=JSON", u.String(), treeish)
-        return URL, nil
+        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
+        }
+        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 bound to a specific repository.
+// Returns errors if repoURL is not valid.
+func New(c *http.Client, repoURL string) (*Client, error) {

[Vadim Sh., 2017/07/20 16:48:12]

I'm not a fan of such ambiguous constructors. How do I know I should use New
instead of initializing Client{} struct manually? All fields are public and
there are no "complex" fields - one would assume Client{} is usable without New.

Also, binding Client to a particular repo is unnecessarily strict. I can easily
imagine situations where we need to fetch stuff from multiple repos at the same
time. Constructing Client's for each of them may feel heavy.

IMHO, it'll be cleaner to keep Client constructor-less and move repoURL as
argument to Log.

[tandrii(chromium), 2017/07/20 19:04:10]

Done!

+        u, 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 {
+        return &Client{c, u}, nil
+}
+
+// Client is Gitiles client bound to a specific Git repository.
+type Client struct {
+        Client  *http.Client
+        RepoURL string // Normalized RepoURL
+}
+
+// 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, treeish string, limit int) ([]Commit, error) {
+        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, 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, 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, subPath string, result interface{}) error {
+        URL := fmt.Sprintf("%s/%s", c.RepoURL, subPath)
+        r, err := ctxhttp.Get(ctx, c.Client, URL)
         if err != nil {
-                return nil, err
-        }
-        if r.StatusCode != 200 {
-                return nil, fmt.Errorf("Failed to fetch %s, status code %d", URL, r.StatusCode)
+                return transient.Tag.Apply(err)
         }
         defer r.Body.Close()
+        if r.StatusCode != 200 {
+                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
+        }
         // 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()
         }
-        // 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
+        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()
+        }
+        return nil
 }