repository: Avoid injecting smudge filter configuration if not needed

We're currently always injecting configuration required for the LFS
smudge filter, even if the caller of `GetArchive()` didn't ask us to
convert LFS pointers.

Refactor the code to only inject the configuration when required.

3 files changed, 68 insertions, 33 deletions

diff --git a/internal/git/smudge/config.go b/internal/git/smudge/config.go
index 79ca4b1ba..aa3d10a5b 100644
--- a/internal/git/smudge/config.go
+++ b/internal/git/smudge/config.go
@@ -4,7 +4,9 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"path/filepath"
 
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/env"
 )
@@ -77,3 +79,11 @@ func (c Config) Environment() (string, error) {
 
 	return fmt.Sprintf("%s=%s", ConfigEnvironmentKey, marshalled), nil
 }
+
+// GitConfiguration returns the Git configuration required to run the smudge filter.
+func (c Config) GitConfiguration(cfg config.Cfg) (git.ConfigPair, error) {
+	return git.ConfigPair{
+		Key:   "filter.lfs.smudge",
+		Value: filepath.Join(cfg.BinDir, "gitaly-lfs-smudge"),
+	}, nil
+}
diff --git a/internal/gitaly/service/repository/archive.go b/internal/gitaly/service/repository/archive.go
index f08ee71a7..d7ccbb95c 100644
--- a/internal/gitaly/service/repository/archive.go
+++ b/internal/gitaly/service/repository/archive.go
@@ -8,7 +8,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
@@ -33,7 +32,6 @@ type archiveParams struct {
 	format      string
 	archivePath string
 	exclude     []string
-	binDir      string
 	loggingDir  string
 }
 
@@ -94,7 +92,6 @@ func (s *server) GetArchive(in *gitalypb.GetArchiveRequest, stream gitalypb.Repo
 		format:      format,
 		archivePath: path,
 		exclude:     exclude,
-		binDir:      s.binDir,
 		loggingDir:  s.loggingCfg.Dir,
 	})
 }
@@ -196,28 +193,33 @@ func (s *server) handleArchive(p archiveParams) error {
 		pathspecs = append(pathspecs, ":(exclude)"+exclude)
 	}
 
-	smudgeCfg := smudge.Config{
-		GlRepository: p.in.GetRepository().GetGlRepository(),
-		Gitlab:       s.cfg.Gitlab,
-		TLS:          s.cfg.TLS,
-	}
+	var env []string
+	var config []git.ConfigPair
 
-	smudgeEnv, err := smudgeCfg.Environment()
-	if err != nil {
-		return fmt.Errorf("setting up smudge environment: %w", err)
-	}
+	if p.in.GetIncludeLfsBlobs() {
+		smudgeCfg := smudge.Config{
+			GlRepository: p.in.GetRepository().GetGlRepository(),
+			Gitlab:       s.cfg.Gitlab,
+			TLS:          s.cfg.TLS,
+		}
 
-	env := []string{
-		smudgeEnv,
-		fmt.Sprintf("CORRELATION_ID=%s", correlation.ExtractFromContext(p.ctx)),
-		fmt.Sprintf("%s=%s", log.GitalyLogDirEnvKey, p.loggingDir),
-	}
+		smudgeEnv, err := smudgeCfg.Environment()
+		if err != nil {
+			return fmt.Errorf("setting up smudge environment: %w", err)
+		}
 
-	var config []git.ConfigPair
+		smudgeGitConfig, err := smudgeCfg.GitConfiguration(s.cfg)
+		if err != nil {
+			return fmt.Errorf("setting up smudge gitconfig: %w", err)
+		}
 
-	if p.in.GetIncludeLfsBlobs() {
-		binary := filepath.Join(p.binDir, "gitaly-lfs-smudge")
-		config = append(config, git.ConfigPair{Key: "filter.lfs.smudge", Value: binary})
+		env = append(
+			env,
+			smudgeEnv,
+			fmt.Sprintf("CORRELATION_ID=%s", correlation.ExtractFromContext(p.ctx)),
+			fmt.Sprintf("%s=%s", log.GitalyLogDirEnvKey, p.loggingDir),
+		)
+		config = append(config, smudgeGitConfig)
 	}
 
 	archiveCommand, err := s.gitCmdFactory.New(p.ctx, p.in.GetRepository(), git.SubCmd{
diff --git a/internal/gitaly/service/repository/archive_test.go b/internal/gitaly/service/repository/archive_test.go
index 995189aaf..3f95bcf73 100644
--- a/internal/gitaly/service/repository/archive_test.go
+++ b/internal/gitaly/service/repository/archive_test.go
@@ -16,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/smudge"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -497,11 +498,6 @@ func TestGetArchiveEnv(t *testing.T) {
 	correlationID := correlation.SafeRandomID()
 	ctx = correlation.ContextWithCorrelation(ctx, correlationID)
 
-	req := &gitalypb.GetArchiveRequest{
-		Repository: repo,
-		CommitId:   commitID,
-	}
-
 	smudgeCfg := smudge.Config{
 		GlRepository: gittest.GlRepository,
 		Gitlab:       cfg.Gitlab,
@@ -511,14 +507,41 @@ func TestGetArchiveEnv(t *testing.T) {
 	smudgeEnv, err := smudgeCfg.Environment()
 	require.NoError(t, err)
 
-	stream, err := client.GetArchive(ctx, req)
-	require.NoError(t, err)
+	for _, tc := range []struct {
+		desc            string
+		includeLFSBlobs bool
+		expectedEnv     []string
+	}{
+		{
+			desc:            "without LFS blobs",
+			includeLFSBlobs: false,
+			expectedEnv: []string{
+				"CORRELATION_ID=" + correlationID,
+			},
+		},
+		{
+			desc:            "with LFS blobs",
+			includeLFSBlobs: true,
+			expectedEnv: []string{
+				"CORRELATION_ID=" + correlationID,
+				smudgeEnv,
+				"GITALY_LOG_DIR=" + cfg.Logging.Dir,
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			stream, err := client.GetArchive(ctx, &gitalypb.GetArchiveRequest{
+				Repository:      repo,
+				CommitId:        commitID,
+				IncludeLfsBlobs: tc.includeLFSBlobs,
+			})
+			require.NoError(t, err)
 
-	data, err := consumeArchive(stream)
-	require.NoError(t, err)
-	require.Contains(t, string(data), "CORRELATION_ID="+correlationID)
-	require.Contains(t, string(data), "GITALY_LOG_DIR="+cfg.Logging.Dir)
-	require.Contains(t, string(data), smudgeEnv)
+			data, err := consumeArchive(stream)
+			require.NoError(t, err)
+			require.ElementsMatch(t, tc.expectedEnv, strings.Split(text.ChompBytes(data), "\n"))
+		})
+	}
 }
 
 func compressedFileContents(t *testing.T, format gitalypb.GetArchiveRequest_Format, name string) []byte {