Merge branch 'pks-replicate-repository-fix-umask-test-dependency' into 'master'

repository: Fix ReplicateRepository test depending on host umask

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

Merged-by: James Fargher <jfargher@gitlab.com>
Approved-by: James Fargher <jfargher@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

2 files changed, 48 insertions, 20 deletions

diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index 2762fd3ef..b5b542da5 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -3,8 +3,7 @@ package repository
 import (
 	"bytes"
 	"context"
-	"crypto/sha1"
-	"encoding/hex"
+	"encoding/binary"
 	"fmt"
 	"io"
 	"io/fs"
@@ -35,6 +34,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/transaction/txinfo"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/status"
@@ -686,6 +686,8 @@ func TestReplicateRepository_transactional(t *testing.T) {
 }
 
 func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	cfgBuilder := testcfg.NewGitalyCfgBuilder(testcfg.WithStorages("default", "replica"))
 	cfg := cfgBuilder.Build(t)
 
@@ -702,10 +704,13 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 	targetRepo := proto.Clone(sourceRepo).(*gitalypb.Repository)
 	targetRepo.StorageName = cfg.Storages[1].Name
 
-	var votes []string
+	var votes []voting.Vote
 	txServer := testTransactionServer{
 		vote: func(request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
-			votes = append(votes, hex.EncodeToString(request.ReferenceUpdatesHash))
+			vote, err := voting.VoteFromHash(request.ReferenceUpdatesHash)
+			require.NoError(t, err)
+			votes = append(votes, vote)
+
 			return &gitalypb.VoteTransactionResponse{
 				State: gitalypb.VoteTransactionResponse_COMMIT,
 			}, nil
@@ -737,23 +742,29 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 	require.NoError(t, err)
 
 	// There is no gitattributes file, so we vote on the empty contents of that file.
-	gitattributesVote := sha1.Sum([]byte{})
+	gitattributesVote := voting.VoteFromData([]byte{})
 	// There is a gitconfig though, so the vote should reflect its contents.
-	gitconfigVote := sha1.Sum(testhelper.MustReadFile(t, filepath.Join(sourceRepoPath, "config")))
+	gitconfigVote := voting.VoteFromData(testhelper.MustReadFile(t, filepath.Join(sourceRepoPath, "config")))
 
-	noHooksVote := "fd69c38637bf443296edc19e2b2c649d0502f7c0"
+	// The custom hooks directory is empty, so we will only write a single empty directory. Thus, we only
+	// write the relative path of that directory (".") as well as its mode bits to the vote hash. Note that
+	// we use a temporary file here to figure out the expected permissions as they would in fact be subject
+	// to change depending on the current umask.
+	noHooksVoteData := [5]byte{'.', 0, 0, 0, 0}
+	binary.BigEndian.PutUint32(noHooksVoteData[1:], uint32(fs.ModeDir|testhelper.MaskedPerms(t, perm.PublicDir)))
+	noHooksVote := voting.VoteFromData(noHooksVoteData[:])
 
-	expectedVotes := []string{
+	expectedVotes := []voting.Vote{
 		// We cannot easily derive these first two votes: they are based on the complete
 		// hashed contents of the unpacked repository. We thus just only assert that they
 		// are always the first two entries and that they are the same by simply taking the
 		// first vote twice here.
 		votes[0],
 		votes[0],
-		hex.EncodeToString(gitconfigVote[:]),
-		hex.EncodeToString(gitconfigVote[:]),
-		hex.EncodeToString(gitattributesVote[:]),
-		hex.EncodeToString(gitattributesVote[:]),
+		gitconfigVote,
+		gitconfigVote,
+		gitattributesVote,
+		gitattributesVote,
 		noHooksVote,
 		noHooksVote,
 	}
@@ -762,7 +773,7 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 
 	// We're about to change refs/heads/master, and thus the mirror-fetch will update it. The
 	// vote should reflect that.
-	replicationVote := sha1.Sum([]byte(fmt.Sprintf("%[1]s %[2]s refs/heads/master\n", commitID2, commitID1)))
+	replicationVote := voting.VoteFromData([]byte(fmt.Sprintf("%[1]s %[2]s refs/heads/master\n", commitID2, commitID1)))
 
 	// We're now changing a reference in the source repository such that we can observe changes
 	// in the target repo.
@@ -777,13 +788,13 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 	})
 	require.NoError(t, err)
 
-	expectedVotes = []string{
-		hex.EncodeToString(gitconfigVote[:]),
-		hex.EncodeToString(gitconfigVote[:]),
-		hex.EncodeToString(gitattributesVote[:]),
-		hex.EncodeToString(gitattributesVote[:]),
-		hex.EncodeToString(replicationVote[:]),
-		hex.EncodeToString(replicationVote[:]),
+	expectedVotes = []voting.Vote{
+		gitconfigVote,
+		gitconfigVote,
+		gitattributesVote,
+		gitattributesVote,
+		replicationVote,
+		replicationVote,
 		noHooksVote,
 		noHooksVote,
 	}
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 03869008d..24481581c 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"math/rand"
 	"net"
 	"net/http"
@@ -328,6 +329,22 @@ func WriteExecutable(tb testing.TB, path string, content []byte) string {
 	return path
 }
 
+// MaskedPerms determines permission bits with the current process umask applied. This is preferable over calls to
+// `perm.GetUmask()` because it can be executed in parallel. Note that this function does not include non-permission
+// mode bits like `fs.ModeDir` or `fs.ModeSocket`. You can OR these manually as required.
+func MaskedPerms(tb testing.TB, mode fs.FileMode) fs.FileMode {
+	tb.Helper()
+
+	f, err := os.OpenFile(filepath.Join(TempDir(tb), "file"), os.O_CREATE|os.O_EXCL, mode)
+	require.NoError(tb, err)
+	defer MustClose(tb, f)
+
+	stat, err := f.Stat()
+	require.NoError(tb, err)
+
+	return stat.Mode().Perm()
+}
+
 // Unsetenv unsets an environment variable. The variable will be restored after the test has
 // finished.
 func Unsetenv(tb testing.TB, key string) {