stats: Disentangle parameters and results for `Clone()`

The `Clone` structure currently holds both parameters and results. Such
usage is always a code smell as it conflates two different concerns and
makes it harder to use correctly for the caller.

Let's disentangle both concerns by instead accepting parameters as part
of a refactored `PerformClone()` function. This function is not a
receiver anymore, but instead returns a newly constructed `Clone`
structure which only contains results.

4 files changed, 33 insertions, 47 deletions

diff --git a/cmd/gitaly-debug/analyzehttp.go b/cmd/gitaly-debug/analyzehttp.go
index c3c6d2d5a..dda0cb11e 100644
--- a/cmd/gitaly-debug/analyzehttp.go
+++ b/cmd/gitaly-debug/analyzehttp.go
@@ -8,12 +8,8 @@ import (
 )
 
 func analyzeHTTPClone(cloneURL string) {
-	st := &stats.Clone{
-		URL:         cloneURL,
-		Interactive: true,
-	}
-
-	noError(st.Perform(context.Background()))
+	st, err := stats.PerformClone(context.Background(), cloneURL, "", "", true)
+	noError(err)
 
 	fmt.Println("\n--- Reference discovery metrics:")
 	for _, entry := range []metric{
diff --git a/internal/blackbox/blackbox.go b/internal/blackbox/blackbox.go
index a8ecf4aff..dd77d60f8 100644
--- a/internal/blackbox/blackbox.go
+++ b/internal/blackbox/blackbox.go
@@ -105,13 +105,8 @@ func (b Blackbox) doProbe(probe Probe) {
 	entry := log.WithField("probe", probe.Name)
 	entry.Info("starting probe")
 
-	clone := &stats.Clone{
-		URL:      probe.URL,
-		User:     probe.User,
-		Password: probe.Password,
-	}
-
-	if err := clone.Perform(ctx); err != nil {
+	clone, err := stats.PerformClone(ctx, probe.URL, probe.User, probe.Password, false)
+	if err != nil {
 		entry.WithError(err).Error("probe failed")
 		return
 	}
diff --git a/internal/git/stats/analyzehttp.go b/internal/git/stats/analyzehttp.go
index 875b660a0..48b801297 100644
--- a/internal/git/stats/analyzehttp.go
+++ b/internal/git/stats/analyzehttp.go
@@ -15,11 +15,6 @@ import (
 )
 
 type Clone struct {
-	URL         string
-	Interactive bool
-	User        string
-	Password    string
-
 	// ReferenceDiscovery is the reference discovery performed as part of the clone.
 	ReferenceDiscovery HTTPReferenceDiscovery
 	// FetchPack is the response to a git-fetch-pack(1) request which computes transmits the
@@ -27,23 +22,31 @@ type Clone struct {
 	FetchPack HTTPFetchPack
 }
 
-// Perform does a Git HTTP clone, discarding cloned data to /dev/null.
-func (cl *Clone) Perform(ctx context.Context) error {
-	referenceDiscovery, err := performReferenceDiscovery(ctx, cl.URL, cl.User, cl.Password, cl.printInteractive)
-	if err != nil {
-		return ctxErr(ctx, err)
+// PerformClone does a Git HTTP clone, discarding cloned data to /dev/null.
+func PerformClone(ctx context.Context, url, user, password string, interactive bool) (Clone, error) {
+	printInteractive := func(format string, a ...interface{}) {
+		if interactive {
+			// Ignore any errors returned by this given that we only use it as a
+			// debugging aid to write to stdout.
+			fmt.Printf(format, a...)
+		}
 	}
 
-	fetchPack, err := performFetchPack(ctx, cl.URL, cl.User, cl.Password,
-		referenceDiscovery.Refs(), cl.printInteractive)
+	referenceDiscovery, err := performReferenceDiscovery(ctx, url, user, password, printInteractive)
 	if err != nil {
-		return ctxErr(ctx, err)
+		return Clone{}, ctxErr(ctx, err)
 	}
 
-	cl.ReferenceDiscovery = referenceDiscovery
-	cl.FetchPack = fetchPack
+	fetchPack, err := performFetchPack(ctx, url, user, password,
+		referenceDiscovery.Refs(), printInteractive)
+	if err != nil {
+		return Clone{}, ctxErr(ctx, err)
+	}
 
-	return nil
+	return Clone{
+		ReferenceDiscovery: referenceDiscovery,
+		FetchPack:          fetchPack,
+	}, nil
 }
 
 func ctxErr(ctx context.Context, err error) error {
@@ -297,13 +300,3 @@ func performFetchPack(
 
 	return fetchPack, nil
 }
-
-func (cl *Clone) printInteractive(format string, a ...interface{}) {
-	if !cl.Interactive {
-		return
-	}
-
-	// Ignore any errors returned by this given that we only use it as a debugging aid
-	// to write to stdout.
-	fmt.Printf(format, a...)
-}
diff --git a/internal/git/stats/analyzehttp_test.go b/internal/git/stats/analyzehttp_test.go
index 87618dbf9..4e25cea93 100644
--- a/internal/git/stats/analyzehttp_test.go
+++ b/internal/git/stats/analyzehttp_test.go
@@ -24,8 +24,8 @@ func TestClone(t *testing.T) {
 		require.NoError(t, stopGitServer())
 	}()
 
-	clone := Clone{URL: fmt.Sprintf("http://localhost:%d/%s", serverPort, filepath.Base(repoPath))}
-	require.NoError(t, clone.Perform(ctx), "perform analysis clone")
+	clone, err := PerformClone(ctx, fmt.Sprintf("http://localhost:%d/%s", serverPort, filepath.Base(repoPath)), "", "", false)
+	require.NoError(t, err, "perform analysis clone")
 
 	const expectedRequests = 90 // based on contents of _support/gitlab-test.git-packed-refs
 	require.Greater(t, clone.FetchPack.RefsWanted(), expectedRequests, "number of wanted refs")
@@ -100,12 +100,14 @@ func TestCloneWithAuth(t *testing.T) {
 		require.NoError(t, stopGitServer())
 	}()
 
-	clone := Clone{
-		URL:      fmt.Sprintf("http://localhost:%d/%s", serverPort, filepath.Base(repoPath)),
-		User:     user,
-		Password: password,
-	}
-	require.NoError(t, clone.Perform(ctx), "perform analysis clone")
+	_, err := PerformClone(
+		ctx,
+		fmt.Sprintf("http://localhost:%d/%s", serverPort, filepath.Base(repoPath)),
+		user,
+		password,
+		false,
+	)
+	require.NoError(t, err, "perform analysis clone")
 
 	require.True(t, authWasChecked, "authentication middleware should have gotten triggered")
 }