smarthttp: upload_pack: Use stats pack for packfile negotiation analysis

The smarthttp service keeps track of all "deepen" actions that are
requested via the upload-pack feature. In order to detect them, we have
a function `scanDeepen()` that parses all pktlines part of the
negotiation process and checks whether they are a "deepen" verb.

As we are going to extend our analysis to include additional features
like "have" and "filter" verbs, we have introduced PackfileNegotiation
statistics into our internal git/stats package. So let's drop the custom
implementation that scans for "deepen" instructions in favor of the more
general stats package.

No functional changes are expected as a result of this refactoring.

3 files changed, 11 insertions, 84 deletions

diff --git a/internal/service/smarthttp/scan_deepen.go b/internal/service/smarthttp/scan_deepen.go
deleted file mode 100644
index b6d997b57..000000000
--- a/internal/service/smarthttp/scan_deepen.go
+++ /dev/null
@@ -1,26 +0,0 @@
-package smarthttp
-
-import (
-	"bytes"
-	"io"
-	"io/ioutil"
-
-	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
-)
-
-func scanDeepen(body io.Reader) bool {
-	result := false
-
-	scanner := pktline.NewScanner(body)
-	for scanner.Scan() {
-		if bytes.HasPrefix(pktline.Data(scanner.Bytes()), []byte("deepen")) && scanner.Err() == nil {
-			result = true
-			break
-		}
-	}
-
-	// Because we are connected to another consumer via an io.Pipe and
-	// io.TeeReader we must consume all data.
-	io.Copy(ioutil.Discard, body)
-	return result
-}
diff --git a/internal/service/smarthttp/scan_deepen_test.go b/internal/service/smarthttp/scan_deepen_test.go
deleted file mode 100644
index 0a5f90ef3..000000000
--- a/internal/service/smarthttp/scan_deepen_test.go
+++ /dev/null
@@ -1,54 +0,0 @@
-package smarthttp
-
-import (
-	"bytes"
-	"fmt"
-	"strings"
-	"testing"
-)
-
-func TestSuccessfulScanDeepen(t *testing.T) {
-	examples := []struct {
-		input  string
-		output bool
-	}{
-		{"000dsomething000cdeepen 10000", true},
-		{"000dsomething0000000cdeepen 1", true}, // 0000 packet
-		{"000dsomething0001000cdeepen 1", true}, // 0001 packet
-		{"000dsomething0002000cdeepen 1", true}, // 0002 packet
-		{"000dsomething0003000cdeepen 1", true}, // 0003 packet
-		{"000dsomething0000000cdeepen 1" + strings.Repeat("garbage", 1000000), true},
-		{"ffff" + strings.Repeat("x", 65531) + "000cdeepen 1", true},
-		{"000dsomething0000", false},
-	}
-
-	for _, example := range examples {
-		t.Run(fmt.Sprintf("%.30s", example.input), func(t *testing.T) {
-			reader := bytes.NewReader([]byte(example.input))
-			hasDeepen := scanDeepen(reader)
-			if n := reader.Len(); n != 0 {
-				t.Fatalf("expected reader to be drained, found %d bytes left", n)
-			}
-
-			if hasDeepen != example.output {
-				t.Fatalf("expected %v, got %v", example.output, hasDeepen)
-			}
-		})
-	}
-}
-
-func TestFailedScanDeepen(t *testing.T) {
-	examples := []string{
-		"invalid data",
-		"deepen",
-		"000cdeepen",
-	}
-
-	for _, example := range examples {
-		t.Run(example, func(t *testing.T) {
-			if scanDeepen(bytes.NewReader([]byte(example))) {
-				t.Fatalf("scanDeepen %q: expected result to be false, got true", example)
-			}
-		})
-	}
-}
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index 7ed467281..e43add012 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -9,6 +9,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/service/inspect"
@@ -54,9 +55,13 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 	pr, pw := io.Pipe()
 	defer pw.Close()
 	stdin := io.TeeReader(stdinReader, pw)
-	deepenCh := make(chan bool, 1)
+	statsCh := make(chan stats.PackfileNegotiation, 1)
 	go func() {
-		deepenCh <- scanDeepen(pr)
+		defer close(statsCh)
+		stats := stats.PackfileNegotiation{}
+		if err := stats.Parse(pr); err == nil {
+			statsCh <- stats
+		}
 	}()
 
 	var respBytes int64
@@ -100,8 +105,10 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 	}
 
 	if err := cmd.Wait(); err != nil {
-		pw.Close() // ensure scanDeepen returns
-		if _, ok := command.ExitStatus(err); ok && <-deepenCh {
+		pw.Close() // ensure PackfileNegotiation parser returns
+		stats := <-statsCh
+
+		if _, ok := command.ExitStatus(err); ok && stats.Deepen != "" {
 			// We have seen a 'deepen' message in the request. It is expected that
 			// git-upload-pack has a non-zero exit status: don't treat this as an
 			// error.