uploadpack: Improve test coverage for pack-objects hook

We've recently grown support for setting up the pack-objects hook. The
test coverage is kind of lacking though: we do have several tests which
now set the accompanying feature flag, but we don't assert that the hook
is being executed in the first place. Furthermore, only the smarthttp
service is tested right now, with no tests for the ssh service.

This commit improves coverage by adding two tests which assert that
the hook is executed correctly and adding additional testcases which set
up the feature flag for the ssh service.

2 files changed, 114 insertions, 6 deletions

diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 7b4d418c3..d02bef953 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -245,6 +246,55 @@ func testSuccessfulUploadPackDeepenRequest(t *testing.T, ctx context.Context) {
 	assert.Equal(t, `0034shallow e63f41fe459e62e1228fcef60d7189127aeba95a0000`, response.String())
 }
 
+func TestUploadPackWithPackObjectsHook(t *testing.T) {
+	hookDir, cleanup := testhelper.TempDir(t)
+	defer cleanup()
+
+	defer func(old string) {
+		config.Config.BinDir = old
+	}(config.Config.BinDir)
+	config.Config.BinDir = hookDir
+
+	outputPath := filepath.Join(hookDir, "output")
+	script := fmt.Sprintf("#!/bin/sh\necho 'I was invoked' >%s\nexit 1\n", outputPath)
+
+	// We're using a custom pack-objects hook for git-upload-pack. In order
+	// to assure that it's getting executed as expected, we're writing a
+	// custom script which replaces the hook binary. It doesn't do anything
+	// special, but writes a message into a status file and then errors
+	// out. In the best case we'd have just printed the error to stderr and
+	// check the return error message. But it's unfortunately not
+	// transferred back.
+	cleanup = testhelper.WriteExecutable(t, filepath.Join(hookDir, "gitaly-hooks"), []byte(script))
+	defer cleanup()
+
+	serverSocketPath, stop := runSmartHTTPServer(t)
+	defer stop()
+
+	repo, repoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	oldHead := bytes.TrimSpace(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", "master~"))
+	newHead := bytes.TrimSpace(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", "master"))
+
+	requestBuffer := &bytes.Buffer{}
+	pktline.WriteString(requestBuffer, fmt.Sprintf("want %s %s\n", newHead, clientCapabilities))
+	pktline.WriteFlush(requestBuffer)
+	pktline.WriteString(requestBuffer, fmt.Sprintf("have %s\n", oldHead))
+	pktline.WriteFlush(requestBuffer)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	_, err := makePostUploadPackRequest(ctx, t, serverSocketPath, &gitalypb.PostUploadPackRequest{
+		Repository: repo,
+	}, requestBuffer)
+	require.Error(t, err)
+
+	contents := testhelper.MustReadFile(t, outputPath)
+	require.Equal(t, "I was invoked\n", string(contents))
+}
+
 func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
 		featureflag.UploadPackGitalyHooks,
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 358d9a1b8..1f777ca3f 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -19,6 +19,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -210,9 +211,10 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 	defer cleanup()
 
 	tests := []struct {
-		cmd    *exec.Cmd
-		desc   string
-		deepen float64
+		cmd          *exec.Cmd
+		desc         string
+		deepen       float64
+		featureFlags []string
 	}{
 		{
 			cmd:    exec.Command(config.Config.Git.BinPath, "clone", "git@localhost:test/test.git", localRepoPath),
@@ -224,6 +226,22 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 			desc:   "shallow clone",
 			deepen: 1,
 		},
+		{
+			cmd:    exec.Command(config.Config.Git.BinPath, "clone", "git@localhost:test/test.git", localRepoPath),
+			desc:   "full clone with hook",
+			deepen: 0,
+			featureFlags: []string{
+				featureflag.UploadPackGitalyHooks.Name,
+			},
+		},
+		{
+			cmd:    exec.Command(config.Config.Git.BinPath, "clone", "--depth", "1", "git@localhost:test/test.git", localRepoPath),
+			desc:   "shallow clone with hook",
+			deepen: 1,
+			featureFlags: []string{
+				featureflag.UploadPackGitalyHooks.Name,
+			},
+		},
 	}
 
 	testRepo, _, cleanup := testhelper.NewTestRepo(t)
@@ -231,10 +249,13 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.desc, func(t *testing.T) {
+			negotiationMetrics.Reset()
+
 			cmd := cloneCommand{
-				repository: testRepo,
-				command:    tc.cmd,
-				server:     serverSocketPath,
+				repository:   testRepo,
+				command:      tc.cmd,
+				featureFlags: tc.featureFlags,
+				server:       serverSocketPath,
 			}
 			lHead, rHead, _, _ := cmd.test(t, localRepoPath)
 			require.Equal(t, lHead, rHead, "local and remote head not equal")
@@ -246,6 +267,43 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 	}
 }
 
+func TestUploadPackWithPackObjectsHook(t *testing.T) {
+	filterDir, cleanup := testhelper.TempDir(t)
+	defer cleanup()
+
+	defer func(old string) {
+		config.Config.BinDir = old
+	}(config.Config.BinDir)
+	config.Config.BinDir = filterDir
+
+	// We're using a custom pack-objetcs hook for git-upload-pack. In order
+	// to assure that it's getting executed as expected, we're writing a
+	// custom script which replaces the hook binary. It doesn't do anything
+	// special, but writes an error message and errors out and should thus
+	// cause the clone to fail with this error message.
+	testhelper.WriteExecutable(t, filepath.Join(filterDir, "gitaly-hooks"),
+		[]byte("#!/bin/sh\necho 'I was invoked' >&2\nexit 73\n"))
+
+	serverSocketPath, stop := runSSHServer(t)
+	defer stop()
+
+	localRepoPath, cleanup := testhelper.TempDir(t)
+	defer cleanup()
+
+	testRepo, _, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	err := cloneCommand{
+		repository: testRepo,
+		command:    exec.Command(config.Config.Git.BinPath, "clone", "git@localhost:test/test.git", localRepoPath),
+		featureFlags: []string{
+			featureflag.UploadPackGitalyHooks.Name,
+		},
+		server: serverSocketPath,
+	}.execute(t)
+	require.Contains(t, err.Error(), "remote: I was invoked")
+}
+
 func TestUploadPackWithoutSideband(t *testing.T) {
 	serverSocketPath, stop := runSSHServer(t)
 	defer stop()