Merge branch 'toon-bundle-uri-fix' into 'master'

smarthttp: Fix upload-pack using bundle-URI with missing backup

Closes #5740

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6591

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: James Liu <jliu@gitlab.com>
Reviewed-by: James Liu <jliu@gitlab.com>

3 files changed, 53 insertions, 11 deletions

diff --git a/internal/bundleuri/git_config.go b/internal/bundleuri/git_config.go
index 747a99bf2..5eb809480 100644
--- a/internal/bundleuri/git_config.go
+++ b/internal/bundleuri/git_config.go
@@ -11,10 +11,12 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 )
 
-// InfoRefsGitConfig return a slice of git.ConfigPairs you can inject into the
-// call to git-upload-pack(1) --advertise-refs, to advertise the use of
-// bundle-URI to the client who clones/fetches from the repository.
-func InfoRefsGitConfig(ctx context.Context) []git.ConfigPair {
+// CapabilitiesGitConfig returns a slice of git.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
+// response to the GET /info/refs request.
+func CapabilitiesGitConfig(ctx context.Context) []git.ConfigPair {
 	if featureflag.BundleURI.IsDisabled(ctx) {
 		return []git.ConfigPair{}
 	}
@@ -30,8 +32,6 @@ func InfoRefsGitConfig(ctx context.Context) []git.ConfigPair {
 // UploadPackGitConfig return a slice of git.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.
-// In case no backups could be found or something else goes wrong, an empty
-// slice is returned without error.
 func UploadPackGitConfig(
 	ctx context.Context,
 	backupLocator backup.Locator,
@@ -39,12 +39,12 @@ func UploadPackGitConfig(
 	repo storage.Repository,
 ) []git.ConfigPair {
 	if backupLocator == nil || backupSink == nil || featureflag.BundleURI.IsDisabled(ctx) {
-		return []git.ConfigPair{}
+		return CapabilitiesGitConfig(ctx)
 	}
 
 	theBackup, err := backupLocator.FindLatest(ctx, repo)
 	if err != nil {
-		return []git.ConfigPair{}
+		return CapabilitiesGitConfig(ctx)
 	}
 
 	for _, step := range theBackup.Steps {
@@ -55,7 +55,7 @@ func UploadPackGitConfig(
 
 		uri, err := backupSink.SignedURL(ctx, step.BundlePath, 10*time.Minute)
 		if err != nil {
-			return []git.ConfigPair{}
+			return CapabilitiesGitConfig(ctx)
 		}
 
 		log.AddFields(ctx, log.Fields{"bundle_uri": true})
@@ -80,5 +80,5 @@ func UploadPackGitConfig(
 		}
 	}
 
-	return []git.ConfigPair{}
+	return CapabilitiesGitConfig(ctx)
 }
diff --git a/internal/bundleuri/git_config_test.go b/internal/bundleuri/git_config_test.go
index c14c3625b..57427fe41 100644
--- a/internal/bundleuri/git_config_test.go
+++ b/internal/bundleuri/git_config_test.go
@@ -40,18 +40,36 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 	}{
 		{
 			desc: "no backup locator nor sink",
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "no backup locator",
 			signedURLFunc: func(context.Context, string, time.Duration) (string, error) {
 				return "", structerr.NewNotFound("not signed")
 			},
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "no backup sink",
 			findLatestFunc: func(context.Context, storage.Repository) (*backup.Backup, error) {
 				return nil, structerr.NewNotFound("no backup found")
 			},
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "no backup found",
@@ -61,6 +79,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 			signedURLFunc: func(context.Context, string, time.Duration) (string, error) {
 				return "", structerr.NewNotFound("not signed")
 			},
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "backup has no steps",
@@ -70,6 +94,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 			signedURLFunc: func(context.Context, string, time.Duration) (string, error) {
 				return "", structerr.NewNotFound("not signed")
 			},
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "backup step is incremental",
@@ -81,6 +111,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 			signedURLFunc: func(context.Context, string, time.Duration) (string, error) {
 				return "", structerr.NewNotFound("not signed")
 			},
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "sign failed",
@@ -92,6 +128,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) {
 			signedURLFunc: func(context.Context, string, time.Duration) (string, error) {
 				return "", structerr.NewNotFound("not signed")
 			},
+			expectedConfig: []git.ConfigPair{
+				{
+					Key:   "uploadpack.advertiseBundleURIs",
+					Value: "true",
+				},
+			},
 		},
 		{
 			desc: "success",
diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index 63be73f0f..2818741de 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -67,7 +67,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, r
 	if err != nil {
 		return err
 	}
-	gitConfig = append(gitConfig, bundleuri.InfoRefsGitConfig(ctx)...)
+	gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx)...)
 
 	cmdOpts = append(cmdOpts, git.WithConfig(gitConfig...))