Merge branch 'pks-ff-drop-tx-file-locking' into 'master'

repository: Always lock gitconfig and gitattributes on write

Closes #3764

See merge request gitlab-org/gitaly!3891

10 files changed, 34 insertions, 257 deletions

diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/housekeeping_test.go
index 09b46599d..e62c5de6f 100644
--- a/internal/git/housekeeping/housekeeping_test.go
+++ b/internal/git/housekeeping/housekeeping_test.go
@@ -666,7 +666,7 @@ func TestPerformRepoDoesNotExist(t *testing.T) {
 }
 
 func TestPerform_UnsetConfiguration(t *testing.T) {
-	cfg, repoProto, _ := testcfg.BuildWithRepo(t)
+	cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	ctx, cancel := testhelper.Context()
@@ -679,7 +679,7 @@ func TestPerform_UnsetConfiguration(t *testing.T) {
 		"http.something.else":     "untouched",
 		"totally.unrelated":       "untouched",
 	} {
-		require.NoError(t, repo.Config().Set(ctx, key, value))
+		gittest.Exec(t, cfg, "-C", repoPath, "config", key, value)
 	}
 
 	opts, err := repo.Config().GetRegexp(ctx, ".*", git.ConfigGetRegexpOpts{})
@@ -711,7 +711,7 @@ func testPerformUnsetConfigurationTransactional(t *testing.T, ctx context.Contex
 	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-	require.NoError(t, repo.Config().Set(ctx, "http.some.extraHeader", "value"))
+	gittest.Exec(t, cfg, "-C", repoPath, "config", "http.some.extraHeader", "value")
 
 	votes := 0
 	txManager := &transaction.MockManager{
diff --git a/internal/git/localrepo/config.go b/internal/git/localrepo/config.go
index 97f4de2e4..49faa6cda 100644
--- a/internal/git/localrepo/config.go
+++ b/internal/git/localrepo/config.go
@@ -21,37 +21,6 @@ type Config struct {
 	repo *Repo
 }
 
-// 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: []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.
-		switch {
-		case isExitWithCode(err, 1):
-			// section or key is invalid
-			return fmt.Errorf("%w: bad section or name", git.ErrInvalidArg)
-		case isExitWithCode(err, 2):
-			// no section or name was provided
-			return fmt.Errorf("%w: missing section or name", git.ErrInvalidArg)
-		}
-
-		return err
-	}
-
-	return nil
-}
-
 // 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 f09e525a5..15c440524 100644
--- a/internal/git/localrepo/config_test.go
+++ b/internal/git/localrepo/config_test.go
@@ -36,66 +36,6 @@ func setupRepoConfig(t *testing.T) (Config, string) {
 	return repo.Config(), repoPath
 }
 
-func TestConfig_Set(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	repoConfig, repoPath := setupRepoConfig(t)
-
-	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("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, "3", actual)
-	})
-
-	t.Run("invalid argument", func(t *testing.T) {
-		for _, tc := range []struct {
-			desc   string
-			name   string
-			expErr error
-			expMsg string
-		}{
-			{
-				desc:   "empty name",
-				name:   "",
-				expErr: git.ErrInvalidArg,
-				expMsg: `"name" is blank or empty`,
-			},
-			{
-				desc:   "invalid name",
-				name:   "`.\n",
-				expErr: git.ErrInvalidArg,
-				expMsg: "bad section or name",
-			},
-			{
-				desc:   "no section or name",
-				name:   "missing",
-				expErr: git.ErrInvalidArg,
-				expMsg: "missing section or name",
-			},
-		} {
-			t.Run(tc.desc, func(t *testing.T) {
-				ctx, cancel := testhelper.Context()
-				defer cancel()
-
-				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)
-			})
-		}
-	})
-}
-
 func TestBuildConfigGetRegexpOptsFlags(t *testing.T) {
 	for _, tc := range []struct {
 		desc string
diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index f80be863b..8dc1a3cc5 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -60,7 +60,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-		require.NoError(t, repo.Config().Set(ctx, "fetch.writeCommitGraph", "true"))
+		gittest.Exec(t, cfg, "-C", repoPath, "config", "fetch.writeCommitGraph", "true")
 
 		require.NoError(t, repo.FetchInternal(
 			ctx, remoteRepoProto, []string{"refs/heads/master:refs/heads/master"},
diff --git a/internal/gitaly/service/repository/apply_gitattributes.go b/internal/gitaly/service/repository/apply_gitattributes.go
index 38b517837..00d3c2bf7 100644
--- a/internal/gitaly/service/repository/apply_gitattributes.go
+++ b/internal/gitaly/service/repository/apply_gitattributes.go
@@ -8,12 +8,10 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
@@ -65,57 +63,23 @@ func (s *server) applyGitattributes(ctx context.Context, c catfile.Batch, repoPa
 		return err
 	}
 
-	if featureflag.TxFileLocking.IsEnabled(ctx) {
-		writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
-			FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
-		})
-		if err != nil {
-			return fmt.Errorf("creating gitattributes writer: %w", err)
-		}
-		defer writer.Close()
-
-		if _, err := io.CopyN(writer, blobObj.Reader, blobInfo.Size); err != nil {
-			return err
-		}
-
-		if err := transaction.CommitLockedFile(ctx, s.txManager, writer); err != nil {
-			return fmt.Errorf("committing gitattributes: %w", err)
-		}
-
-		return nil
-	}
-
-	tempFile, err := os.CreateTemp(infoPath, "attributes")
+	writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
+		FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
+	})
 	if err != nil {
-		return helper.ErrInternalf("creating temporary gitattributes file: %w", err)
+		return fmt.Errorf("creating gitattributes writer: %w", err)
 	}
-	defer func() {
-		if err := os.Remove(tempFile.Name()); err != nil && !errors.Is(err, os.ErrNotExist) {
-			ctxlogrus.Extract(ctx).WithError(err).Errorf("failed to remove tmp file %q", tempFile.Name())
-		}
-	}()
+	defer writer.Close()
 
-	// Write attributes to temp file
-	if _, err := io.CopyN(tempFile, blobObj.Reader, blobInfo.Size); err != nil {
+	if _, err := io.CopyN(writer, blobObj.Reader, blobInfo.Size); err != nil {
 		return err
 	}
 
-	if err := tempFile.Close(); err != nil {
-		return err
+	if err := transaction.CommitLockedFile(ctx, s.txManager, writer); err != nil {
+		return fmt.Errorf("committing gitattributes: %w", err)
 	}
 
-	// Change the permission of tempFile as the permission of file attributesPath
-	if err := os.Chmod(tempFile.Name(), attributesFileMode); err != nil {
-		return err
-	}
-
-	// Vote on the contents of the newly written gitattributes file.
-	if err := s.vote(ctx, blobInfo.Oid); err != nil {
-		return fmt.Errorf("could not commit gitattributes: %w", err)
-	}
-
-	// Rename temp file and return the result
-	return os.Rename(tempFile.Name(), attributesPath)
+	return nil
 }
 
 func (s *server) vote(ctx context.Context, oid git.ObjectID) error {
diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go
index b83c2fd09..f85b6f456 100644
--- a/internal/gitaly/service/repository/apply_gitattributes_test.go
+++ b/internal/gitaly/service/repository/apply_gitattributes_test.go
@@ -12,10 +12,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backchannel"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
@@ -29,13 +27,11 @@ import (
 )
 
 func TestApplyGitattributesSuccess(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testApplyGitattributesSuccess)
-}
-
-func testApplyGitattributesSuccess(t *testing.T, ctx context.Context) {
 	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	cfg, repo, _, client := setupRepositoryService(t)
 
 	infoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath(), "info")
@@ -93,13 +89,11 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp
 }
 
 func TestApplyGitattributesWithTransaction(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testApplyGitattributesWithTransaction)
-}
-
-func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
 	transactionServer := &testTransactionServer{}
@@ -138,16 +132,8 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 			desc:     "successful vote writes gitattributes",
 			revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"),
 			voteFn: func(t *testing.T, request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
-				var expectedHash []byte
-				if featureflag.TxFileLocking.IsEnabled(ctx) {
-					vote := voting.VoteFromData([]byte("/custom-highlighting/*.gitlab-custom gitlab-language=ruby\n"))
-					expectedHash = vote.Bytes()
-				} else {
-					oid, err := git.NewObjectIDFromHex("36814a3da051159a1683479e7a1487120309db8f")
-					require.NoError(t, err)
-					expectedHash, err = oid.Bytes()
-					require.NoError(t, err)
-				}
+				vote := voting.VoteFromData([]byte("/custom-highlighting/*.gitlab-custom gitlab-language=ruby\n"))
+				expectedHash := vote.Bytes()
 
 				require.Equal(t, expectedHash, request.ReferenceUpdatesHash)
 				return &gitalypb.VoteTransactionResponse{
@@ -166,11 +152,7 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 			},
 			shouldExist: false,
 			expectedErr: func() error {
-				if featureflag.TxFileLocking.IsEnabled(ctx) {
-					return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted")
-				}
-
-				return status.Error(codes.Unknown, "could not commit gitattributes: vote failed: transaction was aborted")
+				return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted")
 			}(),
 		},
 		{
@@ -180,13 +162,8 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 				return nil, errors.New("foobar")
 			},
 			shouldExist: false,
-
 			expectedErr: func() error {
-				if featureflag.TxFileLocking.IsEnabled(ctx) {
-					return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar")
-				}
-
-				return status.Error(codes.Unknown, "could not commit gitattributes: vote failed: rpc error: code = Unknown desc = foobar")
+				return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar")
 			}(),
 		},
 		{
@@ -232,13 +209,11 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 }
 
 func TestApplyGitattributesFailure(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testApplyGitattributesFailure)
-}
-
-func testApplyGitattributesFailure(t *testing.T, ctx context.Context) {
 	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	_, repo, _, client := setupRepositoryService(t)
 
 	tests := []struct {
diff --git a/internal/gitaly/service/repository/config.go b/internal/gitaly/service/repository/config.go
index c650bc65b..a0a7f420c 100644
--- a/internal/gitaly/service/repository/config.go
+++ b/internal/gitaly/service/repository/config.go
@@ -1,16 +1,11 @@
 package repository
 
 import (
-	"context"
-	"fmt"
 	"io"
 	"os"
 	"path/filepath"
 
-	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v14/streamio"
 	"google.golang.org/grpc/codes"
@@ -49,38 +44,3 @@ func (s *server) GetConfig(
 
 	return nil
 }
-
-func (s *server) voteOnConfig(ctx context.Context, repo *gitalypb.Repository) error {
-	return transaction.RunOnContext(ctx, func(tx txinfo.Transaction) error {
-		repoPath, err := s.locator.GetPath(repo)
-		if err != nil {
-			return fmt.Errorf("get repo path: %w", err)
-		}
-
-		var vote voting.Vote
-
-		config, err := os.Open(filepath.Join(repoPath, "config"))
-		switch {
-		case err == nil:
-			hash := voting.NewVoteHash()
-			if _, err := io.Copy(hash, config); err != nil {
-				return fmt.Errorf("seeding vote: %w", err)
-			}
-
-			vote, err = hash.Vote()
-			if err != nil {
-				return fmt.Errorf("computing vote: %w", err)
-			}
-		case os.IsNotExist(err):
-			vote = voting.VoteFromData([]byte("notfound"))
-		default:
-			return fmt.Errorf("open repo config: %w", err)
-		}
-
-		if err := s.txManager.Vote(ctx, tx, vote); err != nil {
-			return fmt.Errorf("casting vote: %w", err)
-		}
-
-		return nil
-	})
-}
diff --git a/internal/gitaly/service/repository/fullpath.go b/internal/gitaly/service/repository/fullpath.go
index 0fbe27878..d2fdbe86d 100644
--- a/internal/gitaly/service/repository/fullpath.go
+++ b/internal/gitaly/service/repository/fullpath.go
@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -26,24 +25,8 @@ func (s *server) SetFullPath(
 
 	repo := s.localrepo(request.GetRepository())
 
-	if featureflag.TxFileLocking.IsEnabled(ctx) {
-		if err := repo.SetConfig(ctx, fullPathKey, request.GetPath(), s.txManager); err != nil {
-			return nil, helper.ErrInternalf("setting config: %w", err)
-		}
-
-		return &gitalypb.SetFullPathResponse{}, nil
-	}
-
-	if err := s.voteOnConfig(ctx, request.GetRepository()); err != nil {
-		return nil, helper.ErrInternalf("preimage vote on config: %w", err)
-	}
-
-	if err := repo.Config().Set(ctx, fullPathKey, request.GetPath()); err != nil {
-		return nil, helper.ErrInternalf("writing config: %w", err)
-	}
-
-	if err := s.voteOnConfig(ctx, request.GetRepository()); err != nil {
-		return nil, helper.ErrInternalf("postimage vote on config: %w", err)
+	if err := repo.SetConfig(ctx, fullPathKey, request.GetPath(), s.txManager); err != nil {
+		return nil, helper.ErrInternalf("setting config: %w", err)
 	}
 
 	return &gitalypb.SetFullPathResponse{}, nil
diff --git a/internal/gitaly/service/repository/fullpath_test.go b/internal/gitaly/service/repository/fullpath_test.go
index 1224e30fd..dea6d131e 100644
--- a/internal/gitaly/service/repository/fullpath_test.go
+++ b/internal/gitaly/service/repository/fullpath_test.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -12,7 +11,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -21,12 +19,9 @@ import (
 func TestSetFullPath(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testSetFullPath)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSetFullPath(t *testing.T, ctx context.Context) {
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
 	t.Run("missing repository", func(t *testing.T) {
@@ -79,12 +74,8 @@ func testSetFullPath(t *testing.T, ctx context.Context) {
 
 		require.Nil(t, response)
 
-		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = writing config: rpc "+
+		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+
 			"error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath)
-		if featureflag.TxFileLocking.IsEnabled(ctx) {
-			expectedErr = fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+
-				"error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath)
-		}
 		require.EqualError(t, err, expectedErr)
 	})
 
diff --git a/internal/metadata/featureflag/ff_tx_file_locking.go b/internal/metadata/featureflag/ff_tx_file_locking.go
deleted file mode 100644
index 08f1c9f6c..000000000
--- a/internal/metadata/featureflag/ff_tx_file_locking.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// TxFileLocking enables two-phase voting on files with proper locking semantics such that no races
-// can exist anymore.
-var TxFileLocking = NewFeatureFlag("tx_file_locking", false)