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"
+        "reflect"
         "strings"
         "time"
 
+        "github.com/luci/luci-go/common/errors"
         "github.com/luci/luci-go/server/auth"
         "golang.org/x/net/context"
 )
 
 // 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 is validation fails.

[Vadim Sh., 2017/07/19 17:09:24]

if validation fails

[tandrii(chromium), 2017/07/19 20:12:30]

Done.

+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") {

[Vadim Sh., 2017/07/19 17:09:24]

this reminds me of general uselessness of luci as open source project :)

[tandrii(chromium), 2017/07/19 20:12:31]

yeah, i had the same thought :(

-                return "", fmt.Errorf("Only .googlesource.com repos supported")
+                return "", fmt.Errorf("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.Path == "" || u.Path == "/" {
+                return "", fmt.Errorf("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.
+// 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 Log(c context.Context, repoURL, treeish string, limit int) ([]Commit, error) {
-        // TODO(hinoka): Respect the limit.
-        URL, err := fixURL(repoURL, treeish)
+        logURL, err := NormalizeRepoURL(repoURL)
         if err != nil {
                 return nil, err
         }
+        logURL = fmt.Sprintf("%s/+log/%s?format=JSON", logURL, treeish)

[Vadim Sh., 2017/07/19 17:09:24]

url-encode treeish to be safe (assuming gitiles is ok with that)

[tandrii(chromium), 2017/07/19 20:12:31]

tested gitiles, they seem OK.

         t, err := auth.GetRPCTransport(c, auth.NoAuth)

[Vadim Sh., 2017/07/19 17:09:24]

strictly speaking, this library will be much more usable (and will deserve the
right to be in 'common', which is supposedly usable from both client and server
environments), if http.Client can be passed from outside:

type Client struct {
  Client *http.Client
}

func (c *Client) Log(context.Context, ....) (....) {
 ...
}

Then we can call gitiles from client-side tools. And on server side callers can
decide for themselves what authentication to use.

[tandrii(chromium), 2017/07/19 20:12:31]

Yay, I thought of this too bcz it'd have made testing so much easier + maybe
keepalive can shelve some latency. Mind if I do this change as a follow up CL?

[Vadim Sh., 2017/07/19 23:02:30]

ok

[tandrii(chromium), 2017/07/20 16:19:14]

Actually, Done here.

         if err != nil {
                 return nil, err
         }
         client := http.Client{Transport: t}
+        resp := &logResponse{}
+        if err = get(client, logURL, resp); err != nil {
+                return nil, err
+        }
+        result := resp.Log
+        for {
+                if resp.Next == "" || len(result) >= limit {
+                        return result, nil

[Vadim Sh., 2017/07/19 17:09:24]

do we want to trim this to the limit? It may be surprising for the caller to
receive more than 'limit'.

[tandrii(chromium), 2017/07/19 20:12:31]

trimmed.

+                }
+                nextURL := logURL + "&s=" + resp.Next
+                resp = &logResponse{}
+                if err = get(client, nextURL, 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 get(client http.Client, URL string, result interface{}) error {
         r, err := client.Get(URL)

[Vadim Sh., 2017/07/19 17:09:24]

please use https://godoc.org/golang.org/x/net/context/ctxhttp#Get

[tandrii(chromium), 2017/07/19 20:12:31]

Good point. done.

         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 err

[Vadim Sh., 2017/07/19 17:09:24]

we should probably retry transient errors (like HTTP 500), or at least annotate
them, so that callers know when its safe to retry.

[tandrii(chromium), 2017/07/19 20:12:30]

annotated + todo.

         }
         defer r.Body.Close()
+        if r.StatusCode != 200 {
+                return fmt.Errorf("Failed to fetch %s, status code %d", URL, r.StatusCode)
+        }
         // 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
+        if c, err := r.Body.Read(trash); err != nil || c != 4 {

[Vadim Sh., 2017/07/19 17:09:24]

please assert that 'trash' is indeed ")]}'"

[tandrii(chromium), 2017/07/19 20:12:31]

Done.

+                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 err := json.NewDecoder(r.Body).Decode(result); err != nil {
+                return errors.Annotate(err, "failed to decode Gitiles response into %v", reflect.TypeOf(result)).Err()
+        }
+        return nil
 }