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 f9381706b0a65ff6c749a4539dd8a9dd2453cff4..f4aab415ae031b100b46c66b13f17b7a2a54d16e 100644
--- a/common/api/gitiles/gitiles.go
+++ b/common/api/gitiles/gitiles.go
@@ -21,9 +21,11 @@ import (
 	"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"
 )
@@ -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 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
@@ -67,44 +69,109 @@ func fixURL(repoURL, treeish string) (string, error) {
 		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")
+	}
+	if u.Path == "" || u.Path == "/" {
+		return "", fmt.Errorf("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.
+// 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
+		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 nil, fmt.Errorf("Failed to fetch %s, status code %d", URL, r.StatusCode)
+		return fmt.Errorf("Failed to fetch %s, status code %d", URL, r.StatusCode)
 	}
-	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
+	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()
+	}
+	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()
 	}
-	// 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
 }