gittest: Refactor object existence helpers

The object existence helpers exposed by the gittest package have various
problems:

    - They stutter due to their "GitObject" prefix.
    - They accept the Git binary path instead of a configuration.
    - The object ID is an untyped string.

Refactor them to better match our current coding best practices. They're
renamed to `RequireObject{,Not}Exists()`, accept the Gitaly config as
parameter and the object ID is now of type `git.ObjectID`.

4 files changed, 25 insertions, 22 deletions

diff --git a/internal/git/gittest/objects.go b/internal/git/gittest/objects.go
index ea9b9f1e4..c879adb51 100644
--- a/internal/git/gittest/objects.go
+++ b/internal/git/gittest/objects.go
@@ -15,18 +15,20 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 )
 
-// GitObjectMustExist is a test assertion that fails unless the git repo in repoPath contains sha
-func GitObjectMustExist(t testing.TB, gitBin, repoPath, sha string) {
-	gitObjectExists(t, gitBin, repoPath, sha, true)
+// RequireObjectExists asserts that the given repository does contain an object with the specified
+// object ID.
+func RequireObjectExists(t testing.TB, cfg config.Cfg, repoPath string, objectID git.ObjectID) {
+	requireObjectExists(t, cfg, repoPath, objectID, true)
 }
 
-// GitObjectMustNotExist is a test assertion that fails unless the git repo in repoPath contains sha
-func GitObjectMustNotExist(t testing.TB, gitBin, repoPath, sha string) {
-	gitObjectExists(t, gitBin, repoPath, sha, false)
+// RequireObjectNotExists asserts that the given repository does not contain an object with the
+// specified object ID.
+func RequireObjectNotExists(t testing.TB, cfg config.Cfg, repoPath string, objectID git.ObjectID) {
+	requireObjectExists(t, cfg, repoPath, objectID, false)
 }
 
-func gitObjectExists(t testing.TB, gitBin, repoPath, sha string, exists bool) {
-	cmd := exec.Command(gitBin, "-C", repoPath, "cat-file", "-e", sha)
+func requireObjectExists(t testing.TB, cfg config.Cfg, repoPath string, objectID git.ObjectID, exists bool) {
+	cmd := exec.Command(cfg.Git.BinPath, "-C", repoPath, "cat-file", "-e", objectID.String())
 	cmd.Env = []string{
 		"GIT_ALLOW_PROTOCOL=", // To prevent partial clone reaching remote repo over SSH
 	}
diff --git a/internal/gitaly/service/repository/rename_test.go b/internal/gitaly/service/repository/rename_test.go
index 17c20606f..768f766dd 100644
--- a/internal/gitaly/service/repository/rename_test.go
+++ b/internal/gitaly/service/repository/rename_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
@@ -38,7 +39,7 @@ func TestRenameRepository_success(t *testing.T) {
 	require.True(t, storage.IsGitDirectory(newDirectory), "moved Git repository has been corrupted")
 
 	// ensure the git directory that got renamed contains a sha in the seed repo
-	gittest.GitObjectMustExist(t, cfg.Git.BinPath, newDirectory, "913c66a37b4a45b9769037c55c2d238bd0942d2e")
+	gittest.RequireObjectExists(t, cfg, newDirectory, git.ObjectID("913c66a37b4a45b9769037c55c2d238bd0942d2e"))
 }
 
 func TestRenameRepository_DestinationExists(t *testing.T) {
@@ -69,7 +70,7 @@ func TestRenameRepository_DestinationExists(t *testing.T) {
 	testhelper.RequireGrpcCode(t, err, codes.AlreadyExists)
 
 	// ensure the git directory that already existed didn't get overwritten
-	gittest.GitObjectMustExist(t, cfg.Git.BinPath, destinationRepoPath, commitID.String())
+	gittest.RequireObjectExists(t, cfg, destinationRepoPath, commitID)
 }
 
 func TestRenameRepository_invalidRequest(t *testing.T) {
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 4f141ebea..7c165dbf1 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -92,7 +92,7 @@ func testServerPostUpload(t *testing.T, ctx context.Context, makeRequest request
 		"-C", localRepoPath, "unpack-objects", fmt.Sprintf("--pack_header=%d,%d", version, entries),
 	)
 
-	gittest.GitObjectMustExist(t, cfg.Git.BinPath, localRepoPath, newCommit.String())
+	gittest.RequireObjectExists(t, cfg, localRepoPath, newCommit)
 
 	metric, err := negotiationMetrics.GetMetricWithLabelValues("have")
 	require.NoError(t, err)
@@ -385,14 +385,14 @@ func testServerPostUploadPackPartialClone(t *testing.T, ctx context.Context, mak
 	)
 
 	// a4a132b1b0d6720ca9254440a7ba8a6b9bbd69ec is README.md, which is a small file
-	blobLessThanLimit := "a4a132b1b0d6720ca9254440a7ba8a6b9bbd69ec"
+	blobLessThanLimit := git.ObjectID("a4a132b1b0d6720ca9254440a7ba8a6b9bbd69ec")
 
 	// c1788657b95998a2f177a4f86d68a60f2a80117f is CONTRIBUTING.md, which is > 200 bytese
-	blobGreaterThanLimit := "c1788657b95998a2f177a4f86d68a60f2a80117f"
+	blobGreaterThanLimit := git.ObjectID("c1788657b95998a2f177a4f86d68a60f2a80117f")
 
-	gittest.GitObjectMustExist(t, cfg.Git.BinPath, localRepoPath, blobLessThanLimit)
-	gittest.GitObjectMustExist(t, cfg.Git.BinPath, repoPath, blobGreaterThanLimit)
-	gittest.GitObjectMustNotExist(t, cfg.Git.BinPath, localRepoPath, blobGreaterThanLimit)
+	gittest.RequireObjectExists(t, cfg, localRepoPath, blobLessThanLimit)
+	gittest.RequireObjectExists(t, cfg, repoPath, blobGreaterThanLimit)
+	gittest.RequireObjectNotExists(t, cfg, localRepoPath, blobGreaterThanLimit)
 
 	metric, err := negotiationMetrics.GetMetricWithLabelValues("filter")
 	require.NoError(t, err)
@@ -438,7 +438,7 @@ func testServerPostUploadPackAllowAnySHA1InWant(t *testing.T, ctx context.Contex
 		"-C", localRepoPath, "unpack-objects", fmt.Sprintf("--pack_header=%d,%d", version, entries),
 	)
 
-	gittest.GitObjectMustExist(t, cfg.Git.BinPath, localRepoPath, newCommit.String())
+	gittest.RequireObjectExists(t, cfg, localRepoPath, newCommit)
 }
 
 func makePostUploadPackRequest(ctx context.Context, t *testing.T, serverSocketPath, token string, in *gitalypb.PostUploadPackRequest, body io.Reader) (*bytes.Buffer, error) {
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 599f85c13..eb25a7e74 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -373,9 +373,9 @@ func testUploadPackCloneWithPartialCloneFilter(t *testing.T, opts ...testcfg.Opt
 	serverSocketPath := runSSHServer(t, cfg)
 
 	// Ruby file which is ~1kB in size and not present in HEAD
-	blobLessThanLimit := "6ee41e85cc9bf33c10b690df09ca735b22f3790f"
+	blobLessThanLimit := git.ObjectID("6ee41e85cc9bf33c10b690df09ca735b22f3790f")
 	// Image which is ~100kB in size and not present in HEAD
-	blobGreaterThanLimit := "18079e308ff9b3a5e304941020747e5c39b46c88"
+	blobGreaterThanLimit := git.ObjectID("18079e308ff9b3a5e304941020747e5c39b46c88")
 
 	tests := []struct {
 		desc     string
@@ -385,7 +385,7 @@ func testUploadPackCloneWithPartialCloneFilter(t *testing.T, opts ...testcfg.Opt
 		{
 			desc: "full_clone",
 			repoTest: func(t *testing.T, repoPath string) {
-				gittest.GitObjectMustExist(t, cfg.Git.BinPath, repoPath, blobGreaterThanLimit)
+				gittest.RequireObjectExists(t, cfg, repoPath, blobGreaterThanLimit)
 			},
 			cmd: git.SubCmd{
 				Name: "clone",
@@ -394,7 +394,7 @@ func testUploadPackCloneWithPartialCloneFilter(t *testing.T, opts ...testcfg.Opt
 		{
 			desc: "partial_clone",
 			repoTest: func(t *testing.T, repoPath string) {
-				gittest.GitObjectMustNotExist(t, cfg.Git.BinPath, repoPath, blobGreaterThanLimit)
+				gittest.RequireObjectNotExists(t, cfg, repoPath, blobGreaterThanLimit)
 			},
 			cmd: git.SubCmd{
 				Name: "clone",
@@ -424,7 +424,7 @@ func testUploadPackCloneWithPartialCloneFilter(t *testing.T, opts ...testcfg.Opt
 			defer func() { require.NoError(t, os.RemoveAll(localPath)) }()
 			require.NoError(t, err, "clone failed")
 
-			gittest.GitObjectMustExist(t, cfg.Git.BinPath, localPath, blobLessThanLimit)
+			gittest.RequireObjectExists(t, cfg, localPath, blobLessThanLimit)
 			tc.repoTest(t, localPath)
 		})
 	}