diff options
author | John Cai <jcai@gitlab.com> | 2019-02-19 20:22:15 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2019-02-19 20:22:15 +0300 |
commit | 5e94dc966ac1900c11794b107a77496552591f9b (patch) | |
tree | 7b05805c05550ba5e7a9112dfbcc31ae9dfcefce | |
parent | 08cf43fbc9617048aabdbf13a155afa867c3e90e (diff) | |
parent | f1ee36279e56f74c146a7e0705e1cb8494a80f5f (diff) |
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
-rw-r--r-- | changelogs/unreleased/jc-make-snapshots-work-with-dedupe.yml | 5 | ||||
-rw-r--r-- | internal/archive/match_walker.go | 15 | ||||
-rw-r--r-- | internal/archive/tar_builder.go | 12 | ||||
-rw-r--r-- | internal/git/helper.go | 14 | ||||
-rw-r--r-- | internal/git/objectpool/link.go | 4 | ||||
-rw-r--r-- | internal/git/objectpool/link_test.go | 5 | ||||
-rw-r--r-- | internal/git/objectpool/path.go | 12 | ||||
-rw-r--r-- | internal/service/blob/lfs_pointers_test.go | 3 | ||||
-rw-r--r-- | internal/service/commit/find_commits_test.go | 3 | ||||
-rw-r--r-- | internal/service/commit/isancestor_test.go | 3 | ||||
-rw-r--r-- | internal/service/repository/create_from_snapshot_test.go | 13 | ||||
-rw-r--r-- | internal/service/repository/repack_test.go | 3 | ||||
-rw-r--r-- | internal/service/repository/snapshot.go | 89 | ||||
-rw-r--r-- | internal/service/repository/snapshot_test.go | 128 | ||||
-rw-r--r-- | internal/testhelper/commit.go | 15 |
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] } |