stats: Refactor the `Get` structure

The `Get` structure hosts statistics about a GET request against
git-upload-pack(1). Without knowing details about how git-upload-pack(1)
works though, one wouldn't be able to tell what it actually represents:
everything related to the reference discovery.

Let's rename the structure to `HTTPReferenceDiscovery`, which more
clearly labels what it actually is about. While at it, this also
refactors the way it's constructed: instead of being constructed via the
`Clone` structure, it's now a standalone structure and can be
constructed via `performReferenceDiscovery()`. This disentangles
concerns and makes it easier to reason about the code.

5 files changed, 73 insertions, 69 deletions

diff --git a/.golangci.yml b/.golangci.yml
index d6475eb0e..182a85533 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -289,26 +289,6 @@ issues:
       text: "exported type `Clone` should have comment or be unexported"
     - linters:
         - golint
-      path: "internal/git/stats/analyzehttp.go"
-      text: "exported type `Get` should have comment or be unexported"
-    - linters:
-        - golint
-      path: "internal/git/stats/analyzehttp.go"
-      text: "exported method `Get.ResponseHeader` should have comment or be unexported"
-    - linters:
-        - golint
-      path: "internal/git/stats/analyzehttp.go"
-      text: "exported method `Get.HTTPStatus` should have comment or be unexported"
-    - linters:
-        - golint
-      path: "internal/git/stats/analyzehttp.go"
-      text: "exported method `Get.FirstGitPacket` should have comment or be unexported"
-    - linters:
-        - golint
-      path: "internal/git/stats/analyzehttp.go"
-      text: "exported method `Get.ResponseBody` should have comment or be unexported"
-    - linters:
-        - golint
       path: "internal/git/stats/packfile_negotiation.go"
       text: "exported type `PackfileNegotiation` should have comment or be unexported"
     - linters:
diff --git a/cmd/gitaly-debug/analyzehttp.go b/cmd/gitaly-debug/analyzehttp.go
index a61da43f5..d088ea45c 100644
--- a/cmd/gitaly-debug/analyzehttp.go
+++ b/cmd/gitaly-debug/analyzehttp.go
@@ -15,14 +15,14 @@ func analyzeHTTPClone(cloneURL string) {
 
 	noError(st.Perform(context.Background()))
 
-	fmt.Println("\n--- GET metrics:")
+	fmt.Println("\n--- Reference discovery metrics:")
 	for _, entry := range []metric{
-		{"response header time", st.Get.ResponseHeader()},
-		{"first Git packet", st.Get.FirstGitPacket()},
-		{"response body time", st.Get.ResponseBody()},
-		{"payload size", st.Get.PayloadSize()},
-		{"Git packets received", st.Get.Packets()},
-		{"refs advertised", len(st.Get.Refs())},
+		{"response header time", st.ReferenceDiscovery.ResponseHeader()},
+		{"first Git packet", st.ReferenceDiscovery.FirstGitPacket()},
+		{"response body time", st.ReferenceDiscovery.ResponseBody()},
+		{"payload size", st.ReferenceDiscovery.PayloadSize()},
+		{"Git packets received", st.ReferenceDiscovery.Packets()},
+		{"refs advertised", len(st.ReferenceDiscovery.Refs())},
 	} {
 		entry.print()
 	}
diff --git a/internal/blackbox/blackbox.go b/internal/blackbox/blackbox.go
index 1147b2c15..4eb16852e 100644
--- a/internal/blackbox/blackbox.go
+++ b/internal/blackbox/blackbox.go
@@ -122,9 +122,9 @@ func (b Blackbox) doProbe(probe Probe) {
 		gv.WithLabelValues(probe.Name).Set(value)
 	}
 
-	setGauge(b.getFirstPacket, clone.Get.FirstGitPacket().Seconds())
-	setGauge(b.getTotalTime, clone.Get.ResponseBody().Seconds())
-	setGauge(b.getAdvertisedRefs, float64(len(clone.Get.Refs())))
+	setGauge(b.getFirstPacket, clone.ReferenceDiscovery.FirstGitPacket().Seconds())
+	setGauge(b.getTotalTime, clone.ReferenceDiscovery.ResponseBody().Seconds())
+	setGauge(b.getAdvertisedRefs, float64(len(clone.ReferenceDiscovery.Refs())))
 	setGauge(b.wantedRefs, float64(clone.Post.RefsWanted()))
 	setGauge(b.postTotalTime, clone.Post.ResponseBody().Seconds())
 	setGauge(b.postFirstProgressPacket, clone.Post.BandFirstPacket("progress").Seconds())
diff --git a/internal/git/stats/analyzehttp.go b/internal/git/stats/analyzehttp.go
index c57cc41a2..7cef101da 100644
--- a/internal/git/stats/analyzehttp.go
+++ b/internal/git/stats/analyzehttp.go
@@ -20,20 +20,24 @@ type Clone struct {
 	User        string
 	Password    string
 
-	Get  Get
-	Post Post
+	// ReferenceDiscovery is the reference discovery performed as part of the clone.
+	ReferenceDiscovery HTTPReferenceDiscovery
+	Post               Post
 }
 
 // Perform does a Git HTTP clone, discarding cloned data to /dev/null.
 func (cl *Clone) Perform(ctx context.Context) error {
-	if err := cl.doGet(ctx); err != nil {
+	referenceDiscovery, err := performReferenceDiscovery(ctx, cl.URL, cl.User, cl.Password, cl.printInteractive)
+	if err != nil {
 		return ctxErr(ctx, err)
 	}
 
-	if err := cl.doPost(ctx, cl.Get.Refs()); err != nil {
+	if err := cl.doPost(ctx, referenceDiscovery.Refs()); err != nil {
 		return ctxErr(ctx, err)
 	}
 
+	cl.ReferenceDiscovery = referenceDiscovery
+
 	return nil
 }
 
@@ -44,39 +48,59 @@ func ctxErr(ctx context.Context, err error) error {
 	return err
 }
 
-type Get struct {
+// HTTPReferenceDiscovery is a ReferenceDiscovery obtained via a clone of a target repository via
+// HTTP. It contains additional information about the cloning process like status codes and
+// timings.
+type HTTPReferenceDiscovery struct {
 	start          time.Time
 	responseHeader time.Duration
 	httpStatus     int
 	stats          ReferenceDiscovery
 }
 
-func (g *Get) ResponseHeader() time.Duration { return g.responseHeader }
-func (g *Get) HTTPStatus() int               { return g.httpStatus }
-func (g *Get) FirstGitPacket() time.Duration { return g.stats.FirstPacket.Sub(g.start) }
-func (g *Get) ResponseBody() time.Duration   { return g.stats.LastPacket.Sub(g.start) }
+// ResponseHeader returns how long it took to receive the response header.
+func (d HTTPReferenceDiscovery) ResponseHeader() time.Duration { return d.responseHeader }
+
+// HTTPStatus returns the HTTP status code.
+func (d HTTPReferenceDiscovery) HTTPStatus() int { return d.httpStatus }
+
+// FirstGitPacket returns how long it took to receive the first Git packet.
+func (d HTTPReferenceDiscovery) FirstGitPacket() time.Duration {
+	return d.stats.FirstPacket.Sub(d.start)
+}
+
+// ResponseBody returns how long it took to receive the first bytes of the response body.
+func (d HTTPReferenceDiscovery) ResponseBody() time.Duration {
+	return d.stats.LastPacket.Sub(d.start)
+}
 
 // Refs returns all announced references.
-func (g *Get) Refs() []Reference { return g.stats.Refs }
+func (d HTTPReferenceDiscovery) Refs() []Reference { return d.stats.Refs }
 
 // Packets returns the number of Git packets received.
-func (g *Get) Packets() int { return g.stats.Packets }
+func (d HTTPReferenceDiscovery) Packets() int { return d.stats.Packets }
 
 // PayloadSize returns the total size of all pktlines' data.
-func (g *Get) PayloadSize() int64 { return g.stats.PayloadSize }
+func (d HTTPReferenceDiscovery) PayloadSize() int64 { return d.stats.PayloadSize }
 
 // Caps returns all announced capabilities.
-func (g *Get) Caps() []string { return g.stats.Caps }
+func (d HTTPReferenceDiscovery) Caps() []string { return d.stats.Caps }
 
-func (cl *Clone) doGet(ctx context.Context) error {
-	req, err := http.NewRequest("GET", cl.URL+"/info/refs?service=git-upload-pack", nil)
+func performReferenceDiscovery(
+	ctx context.Context,
+	url, user, password string,
+	reportProgress func(string, ...interface{}),
+) (HTTPReferenceDiscovery, error) {
+	var referenceDiscovery HTTPReferenceDiscovery
+
+	req, err := http.NewRequest("GET", url+"/info/refs?service=git-upload-pack", nil)
 	if err != nil {
-		return err
+		return HTTPReferenceDiscovery{}, err
 	}
 
 	req = req.WithContext(ctx)
-	if cl.User != "" {
-		req.SetBasicAuth(cl.User, cl.Password)
+	if user != "" {
+		req.SetBasicAuth(user, password)
 	}
 
 	for k, v := range map[string]string{
@@ -88,14 +112,14 @@ func (cl *Clone) doGet(ctx context.Context) error {
 		req.Header.Set(k, v)
 	}
 
-	cl.Get.start = time.Now()
-	cl.printInteractive("---\n")
-	cl.printInteractive("--- GET %v\n", req.URL)
-	cl.printInteractive("---\n")
+	referenceDiscovery.start = time.Now()
+	reportProgress("---\n")
+	reportProgress("--- GET %v\n", req.URL)
+	reportProgress("---\n")
 
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
-		return err
+		return HTTPReferenceDiscovery{}, err
 	}
 	defer func() {
 		io.Copy(ioutil.Discard, resp.Body)
@@ -103,27 +127,27 @@ func (cl *Clone) doGet(ctx context.Context) error {
 	}()
 
 	if code := resp.StatusCode; code < 200 || code >= 400 {
-		return fmt.Errorf("git http get: unexpected http status: %d", code)
+		return HTTPReferenceDiscovery{}, fmt.Errorf("git http get: unexpected http status: %d", code)
 	}
 
-	cl.Get.responseHeader = time.Since(cl.Get.start)
-	cl.Get.httpStatus = resp.StatusCode
-	cl.printInteractive("response code: %d\n", resp.StatusCode)
-	cl.printInteractive("response header: %v\n", resp.Header)
+	referenceDiscovery.responseHeader = time.Since(referenceDiscovery.start)
+	referenceDiscovery.httpStatus = resp.StatusCode
+	reportProgress("response code: %d\n", resp.StatusCode)
+	reportProgress("response header: %v\n", resp.Header)
 
 	body := resp.Body
 	if resp.Header.Get("Content-Encoding") == "gzip" {
 		body, err = gzip.NewReader(body)
 		if err != nil {
-			return err
+			return HTTPReferenceDiscovery{}, err
 		}
 	}
 
-	if err := cl.Get.stats.Parse(body); err != nil {
-		return err
+	if err := referenceDiscovery.stats.Parse(body); err != nil {
+		return HTTPReferenceDiscovery{}, err
 	}
 
-	return nil
+	return referenceDiscovery, nil
 }
 
 type Post struct {
diff --git a/internal/git/stats/analyzehttp_test.go b/internal/git/stats/analyzehttp_test.go
index e0cde04d4..ad89ad3f2 100644
--- a/internal/git/stats/analyzehttp_test.go
+++ b/internal/git/stats/analyzehttp_test.go
@@ -30,19 +30,19 @@ func TestClone(t *testing.T) {
 	const expectedRequests = 90 // based on contents of _support/gitlab-test.git-packed-refs
 	require.Greater(t, clone.Post.RefsWanted(), expectedRequests, "number of wanted refs")
 
-	require.Equal(t, 200, clone.Get.HTTPStatus(), "get status")
-	require.Greater(t, clone.Get.Packets(), 0, "number of get packets")
-	require.Greater(t, clone.Get.PayloadSize(), int64(0), "get payload size")
-	require.Greater(t, len(clone.Get.Caps()), 10, "get capabilities")
+	require.Equal(t, 200, clone.ReferenceDiscovery.HTTPStatus(), "get status")
+	require.Greater(t, clone.ReferenceDiscovery.Packets(), 0, "number of get packets")
+	require.Greater(t, clone.ReferenceDiscovery.PayloadSize(), int64(0), "get payload size")
+	require.Greater(t, len(clone.ReferenceDiscovery.Caps()), 10, "get capabilities")
 
 	previousValue := time.Duration(0)
 	for _, m := range []struct {
 		desc  string
 		value time.Duration
 	}{
-		{"time to receive response header", clone.Get.ResponseHeader()},
-		{"time to first packet", clone.Get.FirstGitPacket()},
-		{"time to receive response body", clone.Get.ResponseBody()},
+		{"time to receive response header", clone.ReferenceDiscovery.ResponseHeader()},
+		{"time to first packet", clone.ReferenceDiscovery.FirstGitPacket()},
+		{"time to receive response body", clone.ReferenceDiscovery.ResponseBody()},
 	} {
 		require.True(t, m.value > previousValue, "get: expect %s (%v) to be greater than previous value %v", m.desc, m.value, previousValue)
 		previousValue = m.value