Merge branch 'jc-make-snapshots-work-with-dedupe' into 'master'

Fix GetSnapshot RPC to work with repos with object pool

See merge request gitlab-org/gitaly!1045

15 files changed, 281 insertions, 43 deletions

diff --git a/changelogs/unreleased/jc-make-snapshots-work-with-dedupe.yml b/changelogs/unreleased/jc-make-snapshots-work-with-dedupe.yml
new file mode 100644
index 000000000..2d6466243
--- /dev/null
+++ b/changelogs/unreleased/jc-make-snapshots-work-with-dedupe.yml
@@ -0,0 +1,5 @@
+---
+title: Fix GetSnapshot RPC to work with repos with object pools
+merge_request: 1045
+author:
+type: fixed
diff --git a/internal/archive/match_walker.go b/internal/archive/match_walker.go
index 327e0a7f0..1d3ae5cf8 100644
--- a/internal/archive/match_walker.go
+++ b/internal/archive/match_walker.go
@@ -6,13 +6,14 @@ import (
 	"regexp"
 )
 
-// Walk a directory tree, only calling the wrapped WalkFunc if the path matches
-type matchWalker struct {
+// MatchWalker walks a directory tree, only calling the wrapped WalkFunc if the path matches
+type MatchWalker struct {
 	wrapped  filepath.WalkFunc
 	patterns []*regexp.Regexp
 }
 
-func (m matchWalker) Walk(path string, info os.FileInfo, err error) error {
+// Walk walks the tree, filtering on regexp patterns
+func (m MatchWalker) Walk(path string, info os.FileInfo, err error) error {
 	for _, pattern := range m.patterns {
 		if pattern.MatchString(path) {
 			return m.wrapped(path, info, err)
@@ -21,3 +22,11 @@ func (m matchWalker) Walk(path string, info os.FileInfo, err error) error {
 
 	return nil
 }
+
+// NewMatchWalker returns a new MatchWalker given a slice of patterns and a filepath.WalkFunc
+func NewMatchWalker(patterns []*regexp.Regexp, walkFunc filepath.WalkFunc) *MatchWalker {
+	return &MatchWalker{
+		wrapped:  walkFunc,
+		patterns: patterns,
+	}
+}
diff --git a/internal/archive/tar_builder.go b/internal/archive/tar_builder.go
index cccd13c23..2ae1a0b32 100644
--- a/internal/archive/tar_builder.go
+++ b/internal/archive/tar_builder.go
@@ -162,7 +162,7 @@ func (t *TarBuilder) RecursiveDir(rel string, mustExist bool, patterns ...*regex
 
 	walker := t.walk
 	if len(patterns) > 0 {
-		walker = matchWalker{wrapped: t.walk, patterns: patterns}.Walk
+		walker = NewMatchWalker(patterns, t.walk).Walk
 	}
 
 	// Walk the root and its children, recursively
@@ -174,6 +174,16 @@ func (t *TarBuilder) FileIfExist(rel string) error {
 	return t.File(rel, false)
 }
 
+// VirtualFileWithContents creates an entry at relPath with contents from the given file.
+// This can be used to build a virtual directory structure inside the tar archive.
+func (t *TarBuilder) VirtualFileWithContents(relPath string, contents *os.File) error {
+	fi, err := contents.Stat()
+	if err != nil {
+		return err
+	}
+	return t.entry(fi, relPath, contents)
+}
+
 // RecursiveDirIfExist is a helper for RecursiveDir that sets `mustExist` to
 // false.
 func (t *TarBuilder) RecursiveDirIfExist(rel string, patterns ...*regexp.Regexp) error {
diff --git a/internal/git/helper.go b/internal/git/helper.go
index 87a7222be..99e4629fe 100644
--- a/internal/git/helper.go
+++ b/internal/git/helper.go
@@ -4,8 +4,12 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"path/filepath"
 	"strings"
 	"time"
+
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
 // FallbackTimeValue is the value returned by `SafeTimeParse` in case it
@@ -69,3 +73,13 @@ func BuildGitOptions(gitOpts []string, otherOpts ...string) []string {
 
 	return append(args, otherOpts...)
 }
+
+// AlternatesPath finds the fully qualified path for the alternates file.
+func AlternatesPath(repo repository.GitRepo) (string, error) {
+	repoPath, err := helper.GetRepoPath(repo)
+	if err != nil {
+		return "", err
+	}
+
+	return filepath.Join(repoPath, "objects", "info", "alternates"), nil
+}
diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go
index c212593b7..7645f7dd9 100644
--- a/internal/git/objectpool/link.go
+++ b/internal/git/objectpool/link.go
@@ -16,7 +16,7 @@ import (
 // is to join the pool. This does not trigger deduplication, which is the
 // responsibility of the caller.
 func (o *ObjectPool) Link(ctx context.Context, repo *gitalypb.Repository) error {
-	altPath, err := alternatesPath(repo)
+	altPath, err := git.AlternatesPath(repo)
 	if err != nil {
 		return err
 	}
@@ -61,7 +61,7 @@ func (o *ObjectPool) Unlink(ctx context.Context, repo *gitalypb.Repository) erro
 		}
 	}
 
-	altPath, err := alternatesPath(repo)
+	altPath, err := git.AlternatesPath(repo)
 	if err != nil {
 		return err
 	}
diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go
index a97bc897c..deaebf8d0 100644
--- a/internal/git/objectpool/link_test.go
+++ b/internal/git/objectpool/link_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
@@ -23,7 +24,7 @@ func TestLink(t *testing.T) {
 
 	require.NoError(t, pool.Create(ctx, testRepo))
 
-	altPath, err := alternatesPath(testRepo)
+	altPath, err := git.AlternatesPath(testRepo)
 	require.NoError(t, err)
 	_, err = os.Stat(altPath)
 	require.True(t, os.IsNotExist(err))
@@ -62,7 +63,7 @@ func TestUnlink(t *testing.T) {
 	// Without a pool on disk, this doesn't return an error
 	require.NoError(t, pool.Unlink(ctx, testRepo))
 
-	altPath, err := alternatesPath(testRepo)
+	altPath, err := git.AlternatesPath(testRepo)
 	require.NoError(t, err)
 
 	require.NoError(t, pool.Create(ctx, testRepo))
diff --git a/internal/git/objectpool/path.go b/internal/git/objectpool/path.go
index d7e2002e6..89ef642f7 100644
--- a/internal/git/objectpool/path.go
+++ b/internal/git/objectpool/path.go
@@ -2,9 +2,6 @@ package objectpool
 
 import (
 	"path/filepath"
-
-	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
-	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
 // GetRelativePath will create the relative path to the ObjectPool from the
@@ -23,12 +20,3 @@ func (o *ObjectPool) GetStorageName() string {
 func (o *ObjectPool) FullPath() string {
 	return filepath.Join(o.storagePath, o.GetRelativePath())
 }
-
-func alternatesPath(repo repository.GitRepo) (string, error) {
-	repoPath, err := helper.GetRepoPath(repo)
-	if err != nil {
-		return "", err
-	}
-
-	return filepath.Join(repoPath, "objects", "info", "alternates"), nil
-}
diff --git a/internal/service/blob/lfs_pointers_test.go b/internal/service/blob/lfs_pointers_test.go
index 86025f816..3f4f7c377 100644
--- a/internal/service/blob/lfs_pointers_test.go
+++ b/internal/service/blob/lfs_pointers_test.go
@@ -139,7 +139,8 @@ func TestSuccessfulGetNewLFSPointersRequest(t *testing.T) {
 	cmd := exec.Command("git", cmdArgs...)
 	// Skip smudge since it doesn't work with file:// remotes and we don't need it
 	cmd.Env = append(cmd.Env, "GIT_LFS_SKIP_SMUDGE=1")
-	altDirsCommit, altDirs := testhelper.CreateCommitInAlternateObjectDirectory(t, testRepoPath, cmd)
+	altDirs := "./alt-objects"
+	altDirsCommit := testhelper.CreateCommitInAlternateObjectDirectory(t, testRepoPath, altDirs, cmd)
 
 	// Create a commit not pointed at by any ref to emulate being in the
 	// pre-receive hook so that `--not --all` returns some objects
diff --git a/internal/service/commit/find_commits_test.go b/internal/service/commit/find_commits_test.go
index 6b9a642e3..f7b7803f1 100644
--- a/internal/service/commit/find_commits_test.go
+++ b/internal/service/commit/find_commits_test.go
@@ -316,7 +316,8 @@ func TestSuccessfulFindCommitsRequestWithAltGitObjectDirs(t *testing.T) {
 		"-c", fmt.Sprintf("user.name=%s", committerName),
 		"-c", fmt.Sprintf("user.email=%s", committerEmail),
 		"commit", "--allow-empty", "-m", "An empty commit")
-	currentHead, altObjectsDir := testhelper.CreateCommitInAlternateObjectDirectory(t, testRepoCopyPath, cmd)
+	altObjectsDir := "./alt-objects"
+	currentHead := testhelper.CreateCommitInAlternateObjectDirectory(t, testRepoCopyPath, altObjectsDir, cmd)
 
 	testCases := []struct {
 		desc          string
diff --git a/internal/service/commit/isancestor_test.go b/internal/service/commit/isancestor_test.go
index d4c7af209..99ff46d47 100644
--- a/internal/service/commit/isancestor_test.go
+++ b/internal/service/commit/isancestor_test.go
@@ -195,7 +195,8 @@ func TestSuccessfulIsAncestorRequestWithAltGitObjectDirs(t *testing.T) {
 		"-c", fmt.Sprintf("user.name=%s", committerName),
 		"-c", fmt.Sprintf("user.email=%s", committerEmail),
 		"commit", "--allow-empty", "-m", "An empty commit")
-	currentHead, altObjectsDir := testhelper.CreateCommitInAlternateObjectDirectory(t, testRepoCopyPath, cmd)
+	altObjectsDir := "./alt-objects"
+	currentHead := testhelper.CreateCommitInAlternateObjectDirectory(t, testRepoCopyPath, altObjectsDir, cmd)
 
 	testCases := []struct {
 		desc    string
diff --git a/internal/service/repository/create_from_snapshot_test.go b/internal/service/repository/create_from_snapshot_test.go
index bfbb2258c..e252ce9e2 100644
--- a/internal/service/repository/create_from_snapshot_test.go
+++ b/internal/service/repository/create_from_snapshot_test.go
@@ -23,12 +23,13 @@ var (
 	tarPath      = "/snapshot.tar"
 )
 
-type testhandler struct {
+type tarTesthandler struct {
 	tarData io.Reader
+	secret  string
 }
 
-func (h *testhandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	if r.Header.Get("Authorization") != secret {
+func (h *tarTesthandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Authorization") != h.secret {
 		http.Error(w, "Unauthorized", http.StatusUnauthorized)
 		return
 	}
@@ -77,7 +78,7 @@ func TestCreateRepositoryFromSnapshotSuccess(t *testing.T) {
 	data, entries := generateTarFile(t, repoPath)
 
 	// Create a HTTP server that serves a given tar file
-	srv := httptest.NewServer(&testhandler{bytes.NewReader(data)})
+	srv := httptest.NewServer(&tarTesthandler{tarData: bytes.NewReader(data), secret: secret})
 	defer srv.Close()
 
 	// Delete the repository so we can re-use the path
@@ -171,7 +172,7 @@ func TestCreateRepositoryFromSnapshotBadRequests(t *testing.T) {
 		},
 	}
 
-	srv := httptest.NewServer(&testhandler{})
+	srv := httptest.NewServer(&tarTesthandler{secret: secret})
 	defer srv.Close()
 
 	for _, tc := range testCases {
@@ -202,7 +203,7 @@ func TestCreateRepositoryFromSnapshotHandlesMalformedResponse(t *testing.T) {
 	// Only serve half of the tar file
 	dataReader := io.LimitReader(bytes.NewReader(data), int64(len(data)/2))
 
-	srv := httptest.NewServer(&testhandler{dataReader})
+	srv := httptest.NewServer(&tarTesthandler{tarData: dataReader, secret: secret})
 	defer srv.Close()
 
 	// Delete the repository so we can re-use the path
diff --git a/internal/service/repository/repack_test.go b/internal/service/repository/repack_test.go
index 0c9861f1f..186095594 100644
--- a/internal/service/repository/repack_test.go
+++ b/internal/service/repository/repack_test.go
@@ -55,7 +55,8 @@ func TestRepackLocal(t *testing.T) {
 	commiterArgs := []string{"-c", "user.name=Scrooge McDuck", "-c", "user.email=scrooge@mcduck.com"}
 	cmdArgs := append(commiterArgs, "-C", repoPath, "commit", "--allow-empty", "-m", "An empty commit")
 	cmd := exec.Command("git", cmdArgs...)
-	altDirsCommit, altObjectsDir := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, cmd)
+	altObjectsDir := "./alt-objects"
+	altDirsCommit := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, altObjectsDir, cmd)
 
 	repoCommit := testhelper.CreateCommit(t, repoPath, t.Name(), &testhelper.CreateCommitOpts{Message: t.Name()})
 
diff --git a/internal/service/repository/snapshot.go b/internal/service/repository/snapshot.go
index 9c829b402..69a57984e 100644
--- a/internal/service/repository/snapshot.go
+++ b/internal/service/repository/snapshot.go
@@ -1,14 +1,21 @@
 package repository
 
 import (
+	"bufio"
+	"context"
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
 	"regexp"
 
+	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
+	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/archive"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 var objectFiles = []*regexp.Regexp{
@@ -62,8 +69,84 @@ func (s *server) GetSnapshot(in *gitalypb.GetSnapshotRequest, stream gitalypb.Re
 	// safe than sorry.
 	builder.FileIfExist("shallow")
 
+	if err := addAlternateFiles(stream.Context(), in.GetRepository(), builder); err != nil {
+		return helper.ErrInternal(err)
+	}
+
 	if err := builder.Close(); err != nil {
-		return status.Errorf(codes.Internal, "Building snapshot failed: %v", err)
+		return helper.ErrInternal(fmt.Errorf("building snapshot failed: %v", err))
+	}
+
+	return nil
+}
+
+func addAlternateFiles(ctx context.Context, repository *gitalypb.Repository, builder *archive.TarBuilder) error {
+	alternateFilePath, err := git.AlternatesPath(repository)
+	if err != nil {
+		return fmt.Errorf("error when getting alternates file path: %v", err)
+	}
+
+	if stat, err := os.Stat(alternateFilePath); err == nil && stat.Size() > 0 {
+		alternatesFile, err := os.Open(alternateFilePath)
+		if err != nil {
+			grpc_logrus.Extract(ctx).WithField("error", err).Warn("error opening alternates file")
+			return nil
+		}
+		defer alternatesFile.Close()
+
+		alternateObjDir, err := bufio.NewReader(alternatesFile).ReadString('\n')
+		if err != nil && err != io.EOF {
+			grpc_logrus.Extract(ctx).WithField("error", err).Warn("error reading alternates file")
+			return nil
+		}
+
+		if err == nil {
+			alternateObjDir = alternateObjDir[:len(alternateObjDir)-1]
+		}
+
+		if stat, err := os.Stat(alternateObjDir); err != nil || !stat.IsDir() {
+			grpc_logrus.Extract(ctx).WithFields(
+				log.Fields{"error": err, "object_dir": alternateObjDir}).Warn("error reading alternate objects directory")
+			return nil
+		}
+
+		if err := walkAndAddToBuilder(alternateObjDir, builder); err != nil {
+			return fmt.Errorf("walking alternates file: %v", err)
+		}
+	}
+	return nil
+}
+
+func walkAndAddToBuilder(alternateObjDir string, builder *archive.TarBuilder) error {
+
+	matchWalker := archive.NewMatchWalker(objectFiles, func(path string, info os.FileInfo, err error) error {
+		fmt.Printf("walking down %v\n", path)
+		if err != nil {
+			return fmt.Errorf("error walking %v: %v", path, err)
+		}
+
+		relPath, err := filepath.Rel(alternateObjDir, path)
+		if err != nil {
+			return err
+		}
+
+		file, err := os.Open(path)
+		if err != nil {
+			return fmt.Errorf("opening file %s: %v", path, err)
+		}
+		defer file.Close()
+
+		objectPath := filepath.Join("objects", relPath)
+
+		if err := builder.VirtualFileWithContents(objectPath, file); err != nil {
+			return fmt.Errorf("expected file %v to exist: %v", path, err)
+		}
+
+		return nil
+	})
+
+	if err := filepath.Walk(alternateObjDir, matchWalker.Walk); err != nil {
+		return fmt.Errorf("error when traversing: %v", err)
 	}
 
 	return nil
diff --git a/internal/service/repository/snapshot_test.go b/internal/service/repository/snapshot_test.go
index d60516a0b..d5aa5e73d 100644
--- a/internal/service/repository/snapshot_test.go
+++ b/internal/service/repository/snapshot_test.go
@@ -5,14 +5,20 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/http/httptest"
 	"os"
+	"os/exec"
+	"path"
 	"path/filepath"
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/archive"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -84,6 +90,126 @@ func TestGetSnapshotSuccess(t *testing.T) {
 	require.NotContains(t, entries, "hooks/")
 }
 
+func TestGetSnapshotWithDedupe(t *testing.T) {
+	testRepo, repoPath, cleanup := testhelper.NewTestRepoWithWorktree(t)
+	defer cleanup()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	committerName := "Scrooge McDuck"
+	committerEmail := "scrooge@mcduck.com"
+
+	cmd := exec.Command("git", "-C", repoPath,
+		"-c", fmt.Sprintf("user.name=%s", committerName),
+		"-c", fmt.Sprintf("user.email=%s", committerEmail),
+		"commit", "--allow-empty", "-m", "An empty commit")
+	alternateObjDir := "./alt-objects"
+	commitSha := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
+	originalAlternatesCommit := string(commitSha)
+
+	// ensure commit cannot be found in current repository
+	c, err := catfile.New(ctx, testRepo)
+	require.NoError(t, err)
+	_, err = c.Info(originalAlternatesCommit)
+	require.True(t, catfile.IsNotFound(err))
+
+	// write alternates file to point to alt objects folder
+	alternatesPath, err := git.AlternatesPath(testRepo)
+	require.NoError(t, err)
+	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(path.Join(repoPath, ".git", fmt.Sprintf("%s\n", alternateObjDir))), 0644))
+
+	// write another commit and ensure we can find it
+	cmd = exec.Command("git", "-C", repoPath,
+		"-c", fmt.Sprintf("user.name=%s", committerName),
+		"-c", fmt.Sprintf("user.email=%s", committerEmail),
+		"commit", "--allow-empty", "-m", "Another empty commit")
+	commitSha = testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
+
+	c, err = catfile.New(ctx, testRepo)
+	require.NoError(t, err)
+	_, err = c.Info(string(commitSha))
+	require.NoError(t, err)
+
+	_, repoCopyPath, cleanupCopy := copyRepoUsingSnapshot(t, testRepo)
+	defer cleanupCopy()
+
+	// ensure the sha committed to the alternates directory can be accessed
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "cat-file", "-p", originalAlternatesCommit)
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "fsck")
+}
+
+func TestGetSnapshotWithDedupeSoftFailures(t *testing.T) {
+	testRepo, repoPath, cleanup := testhelper.NewTestRepoWithWorktree(t)
+	defer cleanup()
+
+	// write alternates file to point to alternates objects folder that doesn't exist
+	alternateObjDir := "./alt-objects"
+	alternateObjPath := path.Join(repoPath, ".git", alternateObjDir)
+	alternatesPath, err := git.AlternatesPath(testRepo)
+	require.NoError(t, err)
+	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(fmt.Sprintf("%s\n", alternateObjPath)), 0644))
+
+	req := &gitalypb.GetSnapshotRequest{Repository: testRepo}
+	_, err = getSnapshot(t, req)
+	assert.NoError(t, err)
+	require.NoError(t, os.Remove(alternatesPath))
+	// write alternates file with bad permissions
+	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(fmt.Sprintf("%s\n", alternateObjPath)), 0000))
+	_, err = getSnapshot(t, req)
+	assert.NoError(t, err)
+	require.NoError(t, os.Remove(alternatesPath))
+
+	// write alternates file without newline
+	committerName := "Scrooge McDuck"
+	committerEmail := "scrooge@mcduck.com"
+
+	cmd := exec.Command("git", "-C", repoPath,
+		"-c", fmt.Sprintf("user.name=%s", committerName),
+		"-c", fmt.Sprintf("user.email=%s", committerEmail),
+		"commit", "--allow-empty", "-m", "An empty commit")
+
+	commitSha := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, alternateObjDir, cmd)
+	originalAlternatesCommit := string(commitSha)
+
+	require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(alternateObjPath), 0644))
+
+	_, repoCopyPath, cleanupCopy := copyRepoUsingSnapshot(t, testRepo)
+	defer cleanupCopy()
+
+	// ensure the sha committed to the alternates directory can be accessed
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "cat-file", "-p", originalAlternatesCommit)
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoCopyPath, "fsck")
+}
+
+// copyRepoUsingSnapshot creates a tarball snapshot, then creates a new repository from that snapshot
+func copyRepoUsingSnapshot(t *testing.T, source *gitalypb.Repository) (*gitalypb.Repository, string, func()) {
+	// create the tar
+	req := &gitalypb.GetSnapshotRequest{Repository: source}
+	data, err := getSnapshot(t, req)
+	require.NoError(t, err)
+
+	secret := "my secret"
+	srv := httptest.NewServer(&tarTesthandler{tarData: bytes.NewBuffer(data), secret: secret})
+	defer srv.Close()
+
+	repoCopy, repoCopyPath, cleanupCopy := testhelper.NewTestRepo(t)
+
+	// Delete the repository so we can re-use the path
+	require.NoError(t, os.RemoveAll(repoCopyPath))
+
+	createRepoReq := &gitalypb.CreateRepositoryFromSnapshotRequest{
+		Repository: repoCopy,
+		HttpUrl:    srv.URL + tarPath,
+		HttpAuth:   secret,
+	}
+
+	rsp, err := createFromSnapshot(t, createRepoReq)
+	require.NoError(t, err)
+	require.Equal(t, rsp, &gitalypb.CreateRepositoryFromSnapshotResponse{})
+	return repoCopy, repoCopyPath, cleanupCopy
+}
+
 func TestGetSnapshotFailsIfRepositoryMissing(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	cleanupFn() // Remove the repo
@@ -106,7 +232,7 @@ func TestGetSnapshotFailsIfRepositoryContainsSymlink(t *testing.T) {
 	req := &gitalypb.GetSnapshotRequest{Repository: testRepo}
 	data, err := getSnapshot(t, req)
 	testhelper.RequireGrpcError(t, err, codes.Internal)
-	require.Contains(t, err.Error(), "Building snapshot failed")
+	require.Contains(t, err.Error(), "building snapshot failed")
 
 	// At least some of the tar file should have been written so far
 	require.NotEmpty(t, data)
diff --git a/internal/testhelper/commit.go b/internal/testhelper/commit.go
index bee4e3e51..fe3b48d1d 100644
--- a/internal/testhelper/commit.go
+++ b/internal/testhelper/commit.go
@@ -8,6 +8,8 @@ import (
 	"path"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 // CreateCommitOpts holds extra options for CreateCommit.
@@ -57,18 +59,15 @@ func CreateCommit(t *testing.T, repoPath, branchName string, opts *CreateCommitO
 // CreateCommitInAlternateObjectDirectory runs a command such that its created
 // objects will live in an alternate objects directory. It returns the current
 // head after the command is run and the alternate objects directory path
-func CreateCommitInAlternateObjectDirectory(t *testing.T, repoPath string, cmd *exec.Cmd) (currentHead []byte, altObjectsDir string) {
+func CreateCommitInAlternateObjectDirectory(t *testing.T, repoPath, altObjectsDir string, cmd *exec.Cmd) (currentHead []byte) {
 	gitPath := path.Join(repoPath, ".git")
-	altObjectsDir = "./alt-objects"
 
 	altObjectsPath := path.Join(gitPath, altObjectsDir)
 	gitObjectEnv := []string{
 		fmt.Sprintf("GIT_OBJECT_DIRECTORY=%s", altObjectsPath),
 		fmt.Sprintf("GIT_ALTERNATE_OBJECT_DIRECTORIES=%s", path.Join(gitPath, "objects")),
 	}
-	if err := os.Mkdir(altObjectsPath, 0777); err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, os.MkdirAll(altObjectsPath, 0755))
 
 	// Because we set 'gitObjectEnv', the new objects created by this command
 	// will go into 'find-commits-alt-test-repo/.git/alt-objects'.
@@ -81,9 +80,7 @@ func CreateCommitInAlternateObjectDirectory(t *testing.T, repoPath string, cmd *
 	cmd = exec.Command("git", "-C", repoPath, "rev-parse", "HEAD")
 	cmd.Env = gitObjectEnv
 	currentHead, err := cmd.Output()
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
-	return currentHead[:len(currentHead)-1], altObjectsDir
+	return currentHead[:len(currentHead)-1]
 }