Merge branch 'pks-set-full-path-fix' into 'master'

repository: Fix SetFullPath writing multiple entries

See merge request gitlab-org/gitaly!3712

6 files changed, 56 insertions, 64 deletions

diff --git a/internal/git/config.go b/internal/git/config.go
index e824fc8fd..7e0226966 100644
--- a/internal/git/config.go
+++ b/internal/git/config.go
@@ -7,10 +7,9 @@ import (
 // Config represents 'config' sub-command.
 // https://git-scm.com/docs/git-config
 type Config interface {
-	// Add adds a new configuration value.
-	// WARNING: you can't ever use it for anything that contains secrets.
-	// https://git-scm.com/docs/git-config#Documentation/git-config.txt---add
-	Add(ctx context.Context, name, value string, opts ConfigAddOpts) error
+	// Set will set a configuration value. Any preexisting values will be overwritten with the
+	// new value.
+	Set(ctx context.Context, name, value string) error
 
 	// GetRegexp returns configurations matched to nameRegexp regular expression.
 	// https://git-scm.com/docs/git-config#Documentation/git-config.txt---get-regexp
@@ -47,12 +46,6 @@ var (
 	ConfigTypePath = ConfigType("--path")
 )
 
-// ConfigAddOpts is used to configure invocation of the 'git config --add' command.
-type ConfigAddOpts struct {
-	// Type controls rules used to check the value.
-	Type ConfigType
-}
-
 // ConfigGetRegexpOpts is used to configure invocation of the 'git config --get-regexp' command.
 type ConfigGetRegexpOpts struct {
 	// Type allows to specify an expected type for the configuration.
diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/housekeeping_test.go
index 5cb10d7b9..4f85aee6b 100644
--- a/internal/git/housekeeping/housekeeping_test.go
+++ b/internal/git/housekeeping/housekeeping_test.go
@@ -672,7 +672,7 @@ func TestPerform_UnsetConfiguration(t *testing.T) {
 		"http.something.else":     "untouched",
 		"totally.unrelated":       "untouched",
 	} {
-		require.NoError(t, repo.Config().Add(ctx, key, value, git.ConfigAddOpts{}))
+		require.NoError(t, repo.Config().Set(ctx, key, value))
 	}
 
 	opts, err := repo.Config().GetRegexp(ctx, ".*", git.ConfigGetRegexpOpts{})
diff --git a/internal/git/localrepo/config.go b/internal/git/localrepo/config.go
index c78344033..607f7dbdd 100644
--- a/internal/git/localrepo/config.go
+++ b/internal/git/localrepo/config.go
@@ -17,16 +17,19 @@ type Config struct {
 	repo *Repo
 }
 
-// Add adds a new entry to the repository's configuration.
-func (cfg Config) Add(ctx context.Context, name, value string, opts git.ConfigAddOpts) error {
+// Set will set a configuration value. Any preexisting values will be overwritten with the new
+// value.
+func (cfg Config) Set(ctx context.Context, name, value string) error {
 	if err := validateNotBlank(name, "name"); err != nil {
 		return err
 	}
 
 	if err := cfg.repo.ExecAndWait(ctx, git.SubCmd{
-		Name:  "config",
-		Flags: append(buildConfigAddOptsFlags(opts), git.Flag{Name: "--add"}),
-		Args:  []string{name, value},
+		Name: "config",
+		Flags: []git.Option{
+			git.Flag{Name: "--replace-all"},
+		},
+		Args: []string{name, value},
 	}); err != nil {
 		// Please refer to https://git-scm.com/docs/git-config#_description
 		// on return codes.
@@ -45,15 +48,6 @@ func (cfg Config) Add(ctx context.Context, name, value string, opts git.ConfigAd
 	return nil
 }
 
-func buildConfigAddOptsFlags(opts git.ConfigAddOpts) []git.Option {
-	var flags []git.Option
-	if opts.Type != git.ConfigTypeDefault {
-		flags = append(flags, git.Flag{Name: opts.Type.String()})
-	}
-
-	return flags
-}
-
 // GetRegexp gets all config entries which whose keys match the given regexp.
 func (cfg Config) GetRegexp(ctx context.Context, nameRegexp string, opts git.ConfigGetRegexpOpts) ([]git.ConfigPair, error) {
 	if err := validateNotBlank(nameRegexp, "nameRegexp"); err != nil {
diff --git a/internal/git/localrepo/config_test.go b/internal/git/localrepo/config_test.go
index b5c2892a4..2ef7caac9 100644
--- a/internal/git/localrepo/config_test.go
+++ b/internal/git/localrepo/config_test.go
@@ -29,54 +29,24 @@ func setupRepoConfig(t *testing.T) (Config, string) {
 	return repo.Config(), repoPath
 }
 
-func TestBuildConfigAddOptsFlags(t *testing.T) {
-	for _, tc := range []struct {
-		desc string
-		opts git.ConfigAddOpts
-		exp  []git.Option
-	}{
-		{
-			desc: "none",
-			opts: git.ConfigAddOpts{},
-			exp:  nil,
-		},
-		{
-			desc: "all set",
-			opts: git.ConfigAddOpts{Type: git.ConfigTypeBoolOrInt},
-			exp:  []git.Option{git.Flag{Name: "--bool-or-int"}},
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			require.Equal(t, tc.exp, buildConfigAddOptsFlags(tc.opts))
-		})
-	}
-}
-
-func TestConfig_Add(t *testing.T) {
+func TestConfig_Set(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
 	repoConfig, repoPath := setupRepoConfig(t)
 
-	t.Run("ok", func(t *testing.T) {
-		require.NoError(t, repoConfig.Add(ctx, "key.one", "1", git.ConfigAddOpts{}))
+	t.Run("setting a new value", func(t *testing.T) {
+		require.NoError(t, repoConfig.Set(ctx, "key.one", "1"))
 
 		actual := text.ChompBytes(gittest.Exec(t, repoConfig.repo.cfg, "-C", repoPath, "config", "key.one"))
 		require.Equal(t, "1", actual)
 	})
 
-	t.Run("appends to an old value", func(t *testing.T) {
-		require.NoError(t, repoConfig.Add(ctx, "key.two", "2", git.ConfigAddOpts{}))
-		require.NoError(t, repoConfig.Add(ctx, "key.two", "3", git.ConfigAddOpts{}))
+	t.Run("overwriting an old value", func(t *testing.T) {
+		require.NoError(t, repoConfig.Set(ctx, "key.two", "2"))
+		require.NoError(t, repoConfig.Set(ctx, "key.two", "3"))
 
 		actual := text.ChompBytes(gittest.Exec(t, repoConfig.repo.cfg, "-C", repoPath, "config", "--get-all", "key.two"))
-		require.Equal(t, "2\n3", actual)
-	})
-
-	t.Run("options are passed", func(t *testing.T) {
-		require.NoError(t, repoConfig.Add(ctx, "key.three", "3", git.ConfigAddOpts{Type: git.ConfigTypeInt}))
-
-		actual := text.ChompBytes(gittest.Exec(t, repoConfig.repo.cfg, "-C", repoPath, "config", "--int", "key.three"))
 		require.Equal(t, "3", actual)
 	})
 
@@ -110,7 +80,7 @@ func TestConfig_Add(t *testing.T) {
 				ctx, cancel := testhelper.Context()
 				defer cancel()
 
-				err := repoConfig.Add(ctx, tc.name, "some", git.ConfigAddOpts{})
+				err := repoConfig.Set(ctx, tc.name, "some")
 				require.Error(t, err)
 				require.True(t, errors.Is(err, tc.expErr), err.Error())
 				require.Contains(t, err.Error(), tc.expMsg)
diff --git a/internal/gitaly/service/repository/fullpath.go b/internal/gitaly/service/repository/fullpath.go
index 9fdf70c99..1c22f0815 100644
--- a/internal/gitaly/service/repository/fullpath.go
+++ b/internal/gitaly/service/repository/fullpath.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
@@ -31,7 +30,7 @@ func (s *server) SetFullPath(
 		return nil, helper.ErrInternal(fmt.Errorf("preimage vote on config: %w", err))
 	}
 
-	if err := repo.Config().Add(ctx, fullPathKey, request.GetPath(), git.ConfigAddOpts{}); err != nil {
+	if err := repo.Config().Set(ctx, fullPathKey, request.GetPath()); err != nil {
 		return nil, helper.ErrInternal(fmt.Errorf("writing config: %w", err))
 	}
 
diff --git a/internal/gitaly/service/repository/fullpath_test.go b/internal/gitaly/service/repository/fullpath_test.go
index 92078d7df..606e37c78 100644
--- a/internal/gitaly/service/repository/fullpath_test.go
+++ b/internal/gitaly/service/repository/fullpath_test.go
@@ -113,4 +113,40 @@ func TestSetFullPath(t *testing.T) {
 		fullPath := gittest.Exec(t, cfg, "-C", repoPath, "config", fullPathKey)
 		require.Equal(t, "foo/bar", text.ChompBytes(fullPath))
 	})
+
+	t.Run("multiple times", func(t *testing.T) {
+		repo, repoPath, cleanup := gittest.InitBareRepoAt(t, cfg, cfg.Storages[0])
+		defer cleanup()
+
+		for i := 0; i < 5; i++ {
+			response, err := client.SetFullPath(ctx, &gitalypb.SetFullPathRequest{
+				Repository: repo,
+				Path:       fmt.Sprintf("foo/%d", i),
+			})
+			require.NoError(t, err)
+			testassert.ProtoEqual(t, &gitalypb.SetFullPathResponse{}, response)
+		}
+
+		fullPath := gittest.Exec(t, cfg, "-C", repoPath, "config", "--get-all", fullPathKey)
+		require.Equal(t, "foo/4", text.ChompBytes(fullPath))
+	})
+
+	t.Run("multiple preexisting paths", func(t *testing.T) {
+		repo, repoPath, cleanup := gittest.InitBareRepoAt(t, cfg, cfg.Storages[0])
+		defer cleanup()
+
+		for i := 0; i < 5; i++ {
+			gittest.Exec(t, cfg, "-C", repoPath, "config", "--add", fullPathKey, fmt.Sprintf("foo/%d", i))
+		}
+
+		response, err := client.SetFullPath(ctx, &gitalypb.SetFullPathRequest{
+			Repository: repo,
+			Path:       "replace",
+		})
+		require.NoError(t, err)
+		testassert.ProtoEqual(t, &gitalypb.SetFullPathResponse{}, response)
+
+		fullPath := gittest.Exec(t, cfg, "-C", repoPath, "config", "--get-all", fullPathKey)
+		require.Equal(t, "replace", text.ChompBytes(fullPath))
+	})
 }