Bundle-uri capability: Only advertise when bundle exist

This commit modify the way `bundle-uri` capability is
advertied to the client to only make it so when a bundle
exist for the given repository.

For SSH, this change does not provide much benefit since
the whole packfile negociation flow is performed on a single
connection. In other words, the flow starts when the client
starts the upload-pack service, and it ends when the client
received all the objects it needed.

For SmartHTTP, however, this is different. Each request/command
sent by the client (`ls-refs`, `bundle-uri`, `fetch`) is sent
on different, stateless, RPC calls. There is no way to know
what command the client is sending to the git-upload-pack process
until that process is actually started, which means that the Git
config injected into the process must be computed in advance for
every request; this implies computing the SignedURL of the bundle
if it exist. By advertising `bundle-uri` capability only
when a bundle exists for the given repository, we make sure
the client won't send a `bundle-uri` command if no bundle exist.

This not only reduces by 1 the number of round-trip request
and the number of Git config computation, but it also make it easier
to monitor the use of `bundle-uri` feature, since now we can be sure
that when a client sends the `bundle-uri` command, it is because:
1. A bundle exists
2. The server advertised the capability
3. The client support the capability

Thus, we now have a way to know if a client is using `bundle-uri`
or not.

References:
#6572

internal/bundleuri/git_config.go

@@ -2,61 +2,45 @@ package bundleuri
 
 import (
 	"context"
-	"errors"
 	"fmt"
+	"strconv"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 )
 
 // CapabilitiesGitConfig returns a slice of gitcmd.ConfigPairs that can be injected
-// into the Git config to make it aware the bundle-URI capabilities are
-// supported.
-// This can be used when spawning git-upload-pack(1) --advertise-refs in
+// into the Git config to enable or disable bundle-uri capability.
+//
+// If the feature flag is OFF, this config disables bundle-uri regardless of the `enable` parameter.
+//
+// Explicitly disabling bundle-uri capability is used during calls to git-upload-pack(1)
+// when no bundle exists for the given repository. This prevents the client
+// from requesting a non-existing URI.
+//
+// This function is also used when spawning git-upload-pack(1) --advertise-refs in
 // response to the GET /info/refs request.
-func CapabilitiesGitConfig(ctx context.Context) []gitcmd.ConfigPair {
-	if featureflag.BundleURI.IsDisabled(ctx) {
-		return []gitcmd.ConfigPair{}
+func CapabilitiesGitConfig(ctx context.Context, enable bool) []gitcmd.ConfigPair {
+	cfg := gitcmd.ConfigPair{
+		Key:   "uploadpack.advertiseBundleURIs",
+		Value: strconv.FormatBool(enable),
 	}
 
-	return []gitcmd.ConfigPair{
-		{
-			Key:   "uploadpack.advertiseBundleURIs",
-			Value: "true",
-		},
+	if featureflag.BundleURI.IsDisabled(ctx) {
+		cfg.Value = strconv.FormatBool(false)
 	}
+	return []gitcmd.ConfigPair{cfg}
 }
 
 // UploadPackGitConfig return a slice of gitcmd.ConfigPairs you can inject into the
 // call to git-upload-pack(1) to advertise the available bundle to the client
 // who clones/fetches from the repository.
-func UploadPackGitConfig(
-	ctx context.Context,
-	manager *GenerationManager,
-	repo storage.Repository,
-) ([]gitcmd.ConfigPair, error) {
-	if featureflag.BundleURI.IsDisabled(ctx) {
-		return []gitcmd.ConfigPair{}, nil
-	}
-
-	if manager == nil || manager.sink == nil {
-		return CapabilitiesGitConfig(ctx), errors.New("bundle-URI sink missing")
+func UploadPackGitConfig(ctx context.Context, signedURL string) []gitcmd.ConfigPair {
+	if featureflag.BundleURI.IsDisabled(ctx) || signedURL == "" {
+		return CapabilitiesGitConfig(ctx, false)
 	}
 
-	uri, err := manager.SignedURL(ctx, repo)
-	if err != nil {
-		return nil, err
-	}
-
-	log.AddFields(ctx, log.Fields{"bundle_uri": true})
-
-	return []gitcmd.ConfigPair{
-		{
-			Key:   "uploadpack.advertiseBundleURIs",
-			Value: "true",
-		},
+	config := []gitcmd.ConfigPair{
 		{
 			Key:   "bundle.version",
 			Value: "1",
@@ -71,7 +55,7 @@ func UploadPackGitConfig(
 		},
 		{
 			Key:   fmt.Sprintf("bundle.%s.uri", defaultBundle),
-			Value: uri,
+			Value: signedURL,
 		},
 		{
 			// Gitaly uses only one bundle URI bundle, and it's a
@@ -91,5 +75,6 @@ func UploadPackGitConfig(
 			Key:   fmt.Sprintf("bundle.%s.creationToken", defaultBundle),
 			Value: "1",
 		},
-	}, nil
+	}
+	return append(config, CapabilitiesGitConfig(ctx, true)...)
 }

internal/bundleuri/git_config_test.go

@@ -2,118 +2,82 @@ package bundleuri
 
 import (
 	"context"
-	"fmt"
-	"os"
-	"path/filepath"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 )
 
-func TestUploadPackGitConfig(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.BundleURI).
-		Run(t, testUploadPackGitConfig)
-}
-
-func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
+func Test_capabilitiesGitConfig(t *testing.T) {
 	t.Parallel()
+	featureFlagSet := testhelper.NewFeatureSets(featureflag.BundleURI)
 
-	cfg := testcfg.Build(t)
-	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-		SkipCreationViaService: true,
-	})
-	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-
-	gittest.WriteCommit(t, cfg, repoPath,
-		gittest.WithTreeEntries(gittest.TreeEntry{Mode: "100644", Path: "README", Content: "much"}),
-		gittest.WithBranch("main"))
-
-	tempDir := testhelper.TempDir(t)
-	keyFile, err := os.Create(filepath.Join(tempDir, "secret.key"))
-	require.NoError(t, err)
-	_, err = keyFile.WriteString("super-secret-key")
-	require.NoError(t, err)
-	require.NoError(t, keyFile.Close())
-
-	type setupData struct {
-		manager *GenerationManager
-	}
-
-	for _, tc := range []struct {
-		desc           string
-		setup          func(t *testing.T) setupData
-		expectedConfig []gitcmd.ConfigPair
-		expectedErr    error
+	tests := []struct {
+		desc     string
+		enabled  bool
+		expected []gitcmd.ConfigPair
 	}{
 		{
-			desc: "no bundle found",
-			setup: func(t *testing.T) setupData {
-				sinkDir := t.TempDir()
-				sink, err := NewSink(ctx, "file://"+sinkDir+"?base_url=http://example.com&secret_key_path="+keyFile.Name())
-				require.NoError(t, err)
-
-				manager, err := NewGenerationManager(sink, testhelper.NewLogger(t), 1, 0, nil)
-				require.NoError(t, err)
-
-				return setupData{
-					manager: manager,
-				}
+			desc:    "when bundle-uri enabled",
+			enabled: true,
+			expected: []gitcmd.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
 			},
-			expectedConfig: nil,
-			expectedErr:    structerr.NewNotFound("no bundle available"),
 		},
 		{
-			desc: "not signed",
-			setup: func(t *testing.T) setupData {
-				sinkDir := t.TempDir()
-				sink, err := NewSink(ctx, "file://"+sinkDir)
-				require.NoError(t, err)
-
-				manager, err := NewGenerationManager(sink, testhelper.NewLogger(t), 1, 0, nil)
-				require.NoError(t, err)
-
-				require.NoError(t, manager.Generate(ctx, repo))
-
-				return setupData{
-					manager: manager,
-				}
+			desc:    "when bundle-uri disabled",
+			enabled: false,
+			expected: []gitcmd.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "false",
+				},
 			},
-			expectedConfig: nil,
-			expectedErr:    fmt.Errorf("signed URL: fileblob.SignedURL: bucket does not have an Options.URLSigner (code=Unimplemented)"),
 		},
-		{
-			desc: "success",
-			setup: func(t *testing.T) setupData {
-				sinkDir := t.TempDir()
-				sink, err := NewSink(ctx, "file://"+sinkDir+"?base_url=http://example.com&secret_key_path="+keyFile.Name())
-				require.NoError(t, err)
-
-				manager, err := NewGenerationManager(sink, testhelper.NewLogger(t), 1, 0, nil)
-				require.NoError(t, err)
+	}
+	for _, tt := range tests {
+		featureFlagSet.Run(t, func(t *testing.T, ctx context.Context) {
+			got := CapabilitiesGitConfig(ctx, tt.enabled)
+			if featureflag.BundleURI.IsEnabled(ctx) {
+				require.ElementsMatch(t, tt.expected, got)
+			} else {
+				require.ElementsMatch(t, []gitcmd.ConfigPair{
+					{
+						Key:   "uploadpack.advertiseBundleURIs",
+						Value: "false",
+					},
+				}, got)
+			}
+		})
+	}
+}
 
-				require.NoError(t, manager.Generate(ctx, repo))
+func Test_uploadPackGitConfig(t *testing.T) {
+	t.Parallel()
+	featureFlagSet := testhelper.NewFeatureSets(featureflag.BundleURI)
 
-				return setupData{
-					manager: manager,
-				}
-			},
-			expectedConfig: []gitcmd.ConfigPair{
-				{
-					Key:   "uploadpack.advertiseBundleURIs",
-					Value: "true",
-				},
+	tests := []struct {
+		desc     string
+		uri      string
+		expected []gitcmd.ConfigPair
+	}{
+		{
+			desc: "should return all config",
+			uri:  "https://example.com/bundle.git",
+			expected: []gitcmd.ConfigPair{
 				{
 					Key:   "bundle.version",
 					Value: "1",
 				},
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
 				{
 					Key:   "bundle.mode",
 					Value: "all",
@@ -124,7 +88,7 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 				},
 				{
 					Key:   "bundle.default.uri",
-					Value: "https://example.com/bundle.git?signed=ok",
+					Value: "https://example.com/bundle.git",
 				},
 				{
 					Key:   "bundle.default.creationToken",
@@ -132,34 +96,29 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 				},
 			},
 		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			t.Parallel()
-
-			data := tc.setup(t)
-			require.NoError(t, err)
-
-			actual, err := UploadPackGitConfig(ctx, data.manager, repoProto)
-
+		{
+			desc: "empty uri should disable advertising bundle",
+			uri:  "",
+			expected: []gitcmd.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "false",
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		featureFlagSet.Run(t, func(t *testing.T, ctx context.Context) {
+			got := UploadPackGitConfig(ctx, tt.uri)
 			if featureflag.BundleURI.IsEnabled(ctx) {
-				require.Equal(t, tc.expectedErr, err)
-
-				if tc.expectedConfig != nil {
-					require.Equal(t, len(tc.expectedConfig), len(actual))
-
-					for i, c := range tc.expectedConfig {
-						if strings.HasSuffix(c.Key, ".uri") {
-							// We cannot predict the exact signed URL Value,
-							// so only check the Keys.
-							require.Equal(t, c.Key, actual[i].Key)
-						} else {
-							require.Equal(t, c, actual[i])
-						}
-					}
-				}
+				require.ElementsMatch(t, tt.expected, got)
 			} else {
-				require.NoError(t, err)
-				require.Empty(t, actual)
+				require.ElementsMatch(t, []gitcmd.ConfigPair{
+					{
+						Key:   "uploadpack.advertiseBundleURIs",
+						Value: "false",
+					},
+				}, got)
 			}
 		})
 	}

internal/bundleuri/manager.go

@@ -13,11 +13,14 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/backup"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 var bundleGenerationLatency = prometheus.NewHistogram(
@@ -210,6 +213,20 @@ func (g *GenerationManager) SignedURL(ctx context.Context, repo storage.Reposito
 	return g.sink.signedURL(ctx, relativePath)
 }
 
+// UploadPackGitConfig is a helper function to provide all required, and computed, configurations
+// to inject into the `git-upload-pack` command in order to advertise `bundle-uri` and provide
+// the URI for the bundle for the given repository.
+func (g *GenerationManager) UploadPackGitConfig(ctx context.Context, repo storage.Repository) []gitcmd.ConfigPair {
+	uri, err := g.SignedURL(ctx, repo)
+	if err != nil {
+		if st, ok := status.FromError(err); !ok || st.Code() != codes.NotFound {
+			g.logger.WithField("bundle_uri_error", err)
+		}
+		return CapabilitiesGitConfig(ctx, false)
+	}
+	return UploadPackGitConfig(ctx, uri)
+}
+
 // bundleRelativePath returns a relative path of the bundle-URI bundle inside the bucket.
 func bundleRelativePath(repo storage.Repository, name string) string {
 	repoPath := filepath.Join(repo.GetStorageName(), repo.GetRelativePath())

internal/gitaly/service/smarthttp/inforefs.go

@@ -72,7 +72,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, r
 	if err != nil {
 		return err
 	}
-	gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx)...)
+	gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx, true)...)
 
 	cmdOpts = append(cmdOpts, gitcmd.WithConfig(gitConfig...))
 

internal/gitaly/service/smarthttp/upload_pack.go

@@ -114,16 +114,14 @@ func (s *server) runUploadPack(ctx context.Context, req *gitalypb.PostUploadPack
 		}
 	}()
 
-	gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx)...)
+	repo := s.localRepoFactory.Build(req.GetRepository())
 
-	uploadPackConfig, err := bundleuri.UploadPackGitConfig(ctx, s.bundleManager, req.GetRepository())
-	if err != nil {
+	if s.bundleManager != nil {
+		gitConfig = append(gitConfig, s.bundleManager.UploadPackGitConfig(ctx, req.GetRepository())...)
 	} else {
-		gitConfig = append(gitConfig, uploadPackConfig...)
+		gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx, false)...)
 	}
 
-	repo := s.localRepoFactory.Build(req.GetRepository())
-
 	objectHash, err := repo.ObjectHash(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("detecting object hash: %w", err)

internal/gitaly/service/smarthttp/upload_pack_test.go

@@ -417,6 +417,7 @@ func TestServer_PostUploadPackWithBundleURI(t *testing.T) {
 	ctx := testhelper.Context(t)
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.BundleURI, true)
 
+	// Create secret key for bundle-uri sink
 	tempDir := testhelper.TempDir(t)
 	keyFile, err := os.Create(filepath.Join(tempDir, "secret.key"))
 	require.NoError(t, err)
@@ -433,16 +434,8 @@ func TestServer_PostUploadPackWithBundleURI(t *testing.T) {
 		expectBundleURI bool
 	}{
 		{
-			desc:    "no manager",
-			sinkURI: "",
-		},
-		{
-			desc:    "no bundle",
-			sinkURI: "mem://bundleuri",
-		},
-		{
-			desc:    "broken URL signing",
-			sinkURI: "mem://bundleuri",
+			desc:    "bundle exists",
+			sinkURI: "file://" + testhelper.TempDir(t) + "?base_url=" + baseURL + "&no_tmp_dir=true&secret_key_path=" + keyFile.Name(),
 			setup: func(t *testing.T, ctx context.Context, cfg config.Cfg, manager *bundleuri.GenerationManager, repoProto *gitalypb.Repository, repoPath string) {
 				gittest.WriteCommit(t, cfg, repoPath,
 					gittest.WithTreeEntries(gittest.TreeEntry{Mode: "100644", Path: "README", Content: "much"}),
@@ -451,19 +444,21 @@ func TestServer_PostUploadPackWithBundleURI(t *testing.T) {
 				repo := localrepo.NewTestRepo(t, cfg, repoProto)
 				require.NoError(t, manager.Generate(ctx, repo))
 			},
+			expectBundleURI: true,
 		},
 		{
-			desc:    "valid bundle",
+			desc:    "bundle does not exist",
 			sinkURI: "file://" + testhelper.TempDir(t) + "?base_url=" + baseURL + "&no_tmp_dir=true&secret_key_path=" + keyFile.Name(),
 			setup: func(t *testing.T, ctx context.Context, cfg config.Cfg, manager *bundleuri.GenerationManager, repoProto *gitalypb.Repository, repoPath string) {
 				gittest.WriteCommit(t, cfg, repoPath,
 					gittest.WithTreeEntries(gittest.TreeEntry{Mode: "100644", Path: "README", Content: "much"}),
 					gittest.WithBranch("main"))
-
-				repo := localrepo.NewTestRepo(t, cfg, repoProto)
-				require.NoError(t, manager.Generate(ctx, repo))
 			},
-			expectBundleURI: true,
+			expectBundleURI: false,
+		},
+		{
+			desc:            "manager is nil",
+			expectBundleURI: false,
 		},
 	}
 
@@ -482,6 +477,7 @@ func TestServer_PostUploadPackWithBundleURI(t *testing.T) {
 			hook := testhelper.AddLoggerHook(logger)
 			require.NoError(t, err)
 
+			// create bundle manager if sinkURI is defined
 			var bundleManager *bundleuri.GenerationManager
 			if tc.sinkURI != "" {
 				sink, err := bundleuri.NewSink(ctx, tc.sinkURI)
@@ -491,29 +487,41 @@ func TestServer_PostUploadPackWithBundleURI(t *testing.T) {
 				require.NoError(t, err)
 			}
 
+			// create server
 			server := startSmartHTTPServerWithOptions(t, cfg, nil, []testserver.GitalyServerOpt{
 				testserver.WithBundleGenerationManager(bundleManager),
 				testserver.WithLogger(logger),
 			})
-
 			cfg.SocketPath = server.Address()
+
+			// create repo
 			repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 			if tc.setup != nil {
 				tc.setup(t, ctx, cfg, bundleManager, repoProto, repoPath)
 			}
 
+			// prepare request
 			requestBody := &bytes.Buffer{}
 			gittest.WritePktlineString(t, requestBody, "command=bundle-uri\n")
 			gittest.WritePktlineString(t, requestBody, fmt.Sprintf("object-format=%s\n", gittest.DefaultObjectHash.Format))
 			gittest.WritePktlineFlush(t, requestBody)
 
 			hook.Reset()
+
+			// make actual request
 			req := &gitalypb.PostUploadPackWithSidechannelRequest{Repository: repoProto, GitProtocol: gitcmd.ProtocolV2}
 			responseBuffer, err := makePostUploadPackWithSidechannelRequest(t, ctx, cfg.SocketPath, cfg.Auth.Token, req, requestBody)
-			require.NoError(t, err)
 
 			server.Shutdown()
 
+			// Git should not advertise bundle-uri when no bundle exist
+			// Hence, here we are making sure that if no bundle exist sending
+			// a `command=bundle-uri` causes an error on the Git server
+			if !tc.expectBundleURI {
+				require.Contains(t, err.Error(), "rpc error: code = FailedPrecondition desc = running upload-pack: waiting for upload-pack: exit status 128")
+				return
+			}
+
 			var logEntry *logrus.Entry
 			for _, e := range hook.AllEntries() {
 				if strings.HasPrefix(e.Message, "finished unary call") {
@@ -523,17 +531,7 @@ func TestServer_PostUploadPackWithBundleURI(t *testing.T) {
 			}
 			require.NotNil(t, logEntry)
 			require.Equal(t, "finished unary call with code OK", logEntry.Message)
-
-			bundleURI, ok := logEntry.Data["bundle_uri"]
-
-			if tc.expectBundleURI {
-				require.Contains(t, responseBuffer.String(), "bundle.default.uri="+baseURL)
-				require.True(t, ok)
-				require.True(t, bundleURI.(bool))
-			} else {
-				require.False(t, ok)
-				require.NotContains(t, responseBuffer.String(), "bundle.default.uri")
-			}
+			require.Contains(t, responseBuffer.String(), "bundle.default.uri="+baseURL)
 		})
 	}
 }

internal/gitaly/service/ssh/upload_pack.go

@@ -12,7 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/sidechannel"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/stream"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
@@ -59,13 +58,10 @@ func (s *server) sshUploadPack(ctx context.Context, req *gitalypb.SSHUploadPackW
 		stats.UpdateMetrics(s.packfileNegotiationMetrics)
 	}()
 
-	config = append(config, bundleuri.CapabilitiesGitConfig(ctx)...)
-
-	uploadPackConfig, err := bundleuri.UploadPackGitConfig(ctx, s.bundleURIManager, req.GetRepository())
-	if err != nil {
-		log.AddFields(ctx, log.Fields{"bundle_uri_error": err})
+	if s.bundleURIManager != nil {
+		config = append(config, s.bundleURIManager.UploadPackGitConfig(ctx, req.GetRepository())...)
 	} else {
-		config = append(config, uploadPackConfig...)
+		config = append(config, bundleuri.CapabilitiesGitConfig(ctx, false)...)
 	}
 
 	objectHash, err := repo.ObjectHash(ctx)

internal/gitaly/service/ssh/upload_pack_test.go

@@ -530,7 +530,6 @@ func testUploadPackValidation(t *testing.T, ctx context.Context) {
 
 func TestUploadPack_successful(t *testing.T) {
 	t.Parallel()
-
 	runTestWithAndWithoutConfigOptions(t, func(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
 		testUploadPackSuccessful(t, ctx, opts...)
 	})

[Toon Claes]

Can I give some suggestions about the commit message? I recently also got some commentary on how I wrote a commit message, and I think the advice I've got is really good:

The usual way to compose a log message of this project is to

• Give an observation on how the current system work in the present tense (so no need to say "Currently X is Y", just "X is Y"), and discuss what you perceive as a problem in it.

• Propose a solution (optional---often, problem description trivially leads to an obvious solution in reader's minds).

• Give commands to the codebase to "become like so".

At the moment you're starting with what is changing, it easier to understand why you're doing that, if you set the scene first. If that makes sense?

I like that you split up the SSH vs HTTPs scenario though.

[Olivier Campeau]

Thanks @toon for sharing this! It makes perfect sense yeah! I have updated my commit message to reflect this. Let me know what you think!

[Toon Claes]

This is not fully according coding guidelines: https://gitlab.com/gitlab-org/gitaly/-/blob/3f33592a35304837e020da8d3cdc61f585ab4692/STYLE.md#L313

func TestCapabilitiesGitConfig(t *testing.T) {

[Toon Claes]

Is this the new way of using feature flags in test?

As far as I know we use the following pattern commonly:

func TestUploadPackGitConfig(t *testing.T) {
	testhelper.NewFeatureSets(featureflag.BundleURI).
		Run(t, testUploadPackGitConfig)
}

func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
	// test body
}

By doing it this way, the code changes are very minimal when the feature flag is removed:

func TestUploadPackGitConfig(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.BundleURI).
-		Run(t, testUploadPackGitConfig)
-}
-
-func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)

 	// test body ...
 }

So this has the benefit the indent level doesn't changes for the test body.

[Toon Claes]

I suggest to use the common style here as well:

func TestUploadPackGitConfig(t *testing.T) {

[Olivier Campeau]

Thanks for the feedback.

During my iterations on this MR, I had made those methods private at one point and I forgot to change the test name afterwards.

And I am used to the pattern of prefixing my test name with Test_<lowercase>* when the method under test is private. It makes it more obvious which tests are testing contract/client-facing methods and which one are not.

I have updated the code to reflect the coding guidelines: https://gitlab.com/gitlab-org/gitaly/-/blob/3f33592a35304837e020da8d3cdc61f585ab4692/STYLE.md#L313

Thanks for educating me on the coding guidelines to use here. Much appreciated!