diff options
author | Jacob Vosmaer <jacob@gitlab.com> | 2019-03-29 14:29:23 +0300 |
---|---|---|
committer | Jacob Vosmaer <jacob@gitlab.com> | 2019-03-29 14:29:23 +0300 |
commit | 6a3aaf9cbc17e56f4dd93dfd3f8a7b1c40ecebab (patch) | |
tree | 4a0ca4cf13a45cfb7491adde8e0eff1e4b71f486 | |
parent | b88c19f98b575f1ac85df01b902b4d4e8dd9dc17 (diff) | |
parent | c02b9ad129f348d080fc477172376400d58f19a8 (diff) |
Merge branch 'objectpool-explicit-not-found' into 'master'
UnlinkRepositoryFromObjectPool: stop removing objects/info/alternates
See merge request gitlab-org/gitaly!1151
-rw-r--r-- | changelogs/unreleased/objectpool-explicit-not-found.yml | 5 | ||||
-rw-r--r-- | internal/git/objectpool/link.go | 14 | ||||
-rw-r--r-- | internal/git/objectpool/link_test.go | 24 | ||||
-rw-r--r-- | internal/git/proto.go | 4 | ||||
-rw-r--r-- | internal/helper/error.go | 3 | ||||
-rw-r--r-- | internal/service/objectpool/link.go | 12 | ||||
-rw-r--r-- | internal/service/objectpool/link_test.go | 73 | ||||
-rw-r--r-- | internal/service/objectpool/reduplicate_test.go | 7 | ||||
-rw-r--r-- | internal/service/repository/snapshot.go | 2 | ||||
-rw-r--r-- | internal/service/repository/snapshot_test.go | 4 | ||||
-rw-r--r-- | internal/testhelper/remote.go | 22 |
11 files changed, 126 insertions, 44 deletions
diff --git a/changelogs/unreleased/objectpool-explicit-not-found.yml b/changelogs/unreleased/objectpool-explicit-not-found.yml new file mode 100644 index 000000000..30cb6fb96 --- /dev/null +++ b/changelogs/unreleased/objectpool-explicit-not-found.yml @@ -0,0 +1,5 @@ +--- +title: 'UnlinkRepositoryFromObjectPool: stop removing objects/info/alternates' +merge_request: 1151 +author: +type: changed diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go index 389b24534..7c5160ded 100644 --- a/internal/git/objectpool/link.go +++ b/internal/git/objectpool/link.go @@ -3,6 +3,7 @@ package objectpool import ( "bufio" "context" + "errors" "fmt" "io" "io/ioutil" @@ -21,7 +22,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 := git.AlternatesPath(repo) + altPath, err := git.InfoAlternatesPath(repo) if err != nil { return err } @@ -90,7 +91,7 @@ func (o *ObjectPool) getRelativeObjectPath(repo *gitalypb.Repository) (string, e // LinkedToRepository tests if a repository is linked to an object pool func (o *ObjectPool) LinkedToRepository(repo *gitalypb.Repository) (bool, error) { - altPath, err := git.AlternatesPath(repo) + altPath, err := git.InfoAlternatesPath(repo) if err != nil { return false, err } @@ -124,7 +125,7 @@ func (o *ObjectPool) LinkedToRepository(repo *gitalypb.Repository) (bool, error) // It removes the remote from the object pool too, func (o *ObjectPool) Unlink(ctx context.Context, repo *gitalypb.Repository) error { if !o.Exists() { - return nil + return errors.New("pool does not exist") } // We need to use removeRemote, and can't leverage `git config --remove-section` @@ -136,12 +137,7 @@ func (o *ObjectPool) Unlink(ctx context.Context, repo *gitalypb.Repository) erro } } - altPath, err := git.AlternatesPath(repo) - if err != nil { - return err - } - - return os.RemoveAll(altPath) + return nil } // Config options setting will leak the key value pairs in the logs. This makes diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go index f9d2934ff..c409aca0e 100644 --- a/internal/git/objectpool/link_test.go +++ b/internal/git/objectpool/link_test.go @@ -6,7 +6,6 @@ import ( "strings" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/testhelper" @@ -25,7 +24,7 @@ func TestLink(t *testing.T) { require.NoError(t, pool.Remove(ctx), "make sure pool does not exist prior to creation") require.NoError(t, pool.Create(ctx, testRepo), "create pool") - altPath, err := git.AlternatesPath(testRepo) + altPath, err := git.InfoAlternatesPath(testRepo) require.NoError(t, err) _, err = os.Stat(altPath) require.True(t, os.IsNotExist(err)) @@ -46,9 +45,7 @@ func TestLink(t *testing.T) { require.Equal(t, content, newContent) - // Test if the remote is set - remotes := strings.Split(string(testhelper.MustRunCommand(t, nil, "git", "-C", pool.FullPath(), "remote")), "\n") - assert.Contains(t, remotes, testRepo.GetGlRepository()) + require.True(t, testhelper.RemoteExists(t, pool.FullPath(), testRepo.GetGlRepository()), "pool remotes should include %v", testRepo) } func TestUnlink(t *testing.T) { @@ -62,18 +59,13 @@ func TestUnlink(t *testing.T) { require.NoError(t, err) defer pool.Remove(ctx) - // Without a pool on disk, this doesn't return an error - require.NoError(t, pool.Unlink(ctx, testRepo)) + require.Error(t, pool.Unlink(ctx, testRepo), "removing a non-existing pool should be an error") - altPath, err := git.AlternatesPath(testRepo) - require.NoError(t, err) + require.NoError(t, pool.Create(ctx, testRepo), "create pool") + require.NoError(t, pool.Link(ctx, testRepo), "link test repo to pool") - require.NoError(t, pool.Create(ctx, testRepo)) - require.NoError(t, pool.Link(ctx, testRepo)) + require.True(t, testhelper.RemoteExists(t, pool.FullPath(), testRepo.GetGlRepository()), "pool remotes should include %v", testRepo) - require.FileExists(t, altPath, "alternates file must exist after Link") - - require.NoError(t, pool.Unlink(ctx, testRepo)) - _, err = os.Stat(altPath) - require.True(t, os.IsNotExist(err)) + require.NoError(t, pool.Unlink(ctx, testRepo), "unlink repo") + require.False(t, testhelper.RemoteExists(t, pool.FullPath(), testRepo.GetGlRepository()), "pool remotes should no longer include %v", testRepo) } diff --git a/internal/git/proto.go b/internal/git/proto.go index 9f2a43f24..bfbbcfcc2 100644 --- a/internal/git/proto.go +++ b/internal/git/proto.go @@ -96,8 +96,8 @@ 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) { +// InfoAlternatesPath finds the fully qualified path for the alternates file. +func InfoAlternatesPath(repo repository.GitRepo) (string, error) { repoPath, err := helper.GetRepoPath(repo) if err != nil { return "", err diff --git a/internal/helper/error.go b/internal/helper/error.go index c20a1690a..81975315d 100644 --- a/internal/helper/error.go +++ b/internal/helper/error.go @@ -26,6 +26,9 @@ func ErrInvalidArgument(err error) error { return DecorateError(codes.InvalidArg // ErrPreconditionFailed wraps error with codes.FailedPrecondition, unless err is already a grpc error func ErrPreconditionFailed(err error) error { return DecorateError(codes.FailedPrecondition, err) } +// ErrNotFound wraps error with codes.NotFound, unless err is already a grpc error +func ErrNotFound(err error) error { return DecorateError(codes.NotFound, err) } + // GrpcCode emulates the old grpc.Code function: it translates errors into codes.Code values. func GrpcCode(err error) codes.Code { if err == nil { diff --git a/internal/service/objectpool/link.go b/internal/service/objectpool/link.go index 234a33a87..37e64ad07 100644 --- a/internal/service/objectpool/link.go +++ b/internal/service/objectpool/link.go @@ -2,6 +2,8 @@ package objectpool import ( "context" + "errors" + "fmt" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -28,16 +30,20 @@ func (s *server) LinkRepositoryToObjectPool(ctx context.Context, req *gitalypb.L func (s *server) UnlinkRepositoryFromObjectPool(ctx context.Context, req *gitalypb.UnlinkRepositoryFromObjectPoolRequest) (*gitalypb.UnlinkRepositoryFromObjectPoolResponse, error) { if req.GetRepository() == nil { - return nil, status.Error(codes.InvalidArgument, "no repository") + return nil, helper.ErrInvalidArgument(errors.New("no repository")) } pool, err := poolForRequest(req) if err != nil { - return nil, err + return nil, helper.ErrInternal(err) + } + + if !pool.Exists() { + return nil, helper.ErrNotFound(fmt.Errorf("pool repository not found: %s", pool.FullPath())) } if err := pool.Unlink(ctx, req.GetRepository()); err != nil { - return nil, err + return nil, helper.ErrInternal(err) } return &gitalypb.UnlinkRepositoryFromObjectPoolResponse{}, nil diff --git a/internal/service/objectpool/link_test.go b/internal/service/objectpool/link_test.go index 33404c9cd..870fd27fe 100644 --- a/internal/service/objectpool/link_test.go +++ b/internal/service/objectpool/link_test.go @@ -9,7 +9,6 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" - "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/codes" ) @@ -169,15 +168,27 @@ func TestUnlink(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() + deletedRepo, deletedRepoPath, removeDeletedRepo := testhelper.NewTestRepo(t) + defer removeDeletedRepo() + pool, err := objectpool.NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) + defer pool.Remove(ctx) - require.NoError(t, pool.Remove(ctx), "make sure pool does not exist prior to creation") require.NoError(t, pool.Create(ctx, testRepo), "create pool") - require.NoError(t, pool.Link(ctx, testRepo)) + require.NoError(t, pool.Link(ctx, deletedRepo)) - poolCommitID := testhelper.CreateCommit(t, pool.FullPath(), "pool-test-branch", nil) + removeDeletedRepo() + testhelper.AssertFileNotExists(t, deletedRepoPath) + + pool2, err := objectpool.NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + require.NoError(t, pool2.Create(ctx, testRepo), "create pool 2") + defer pool2.Remove(ctx) + + require.True(t, testhelper.RemoteExists(t, pool.FullPath(), testRepo.GlRepository), "sanity check: remote exists in pool") + require.True(t, testhelper.RemoteExists(t, pool.FullPath(), deletedRepo.GlRepository), "sanity check: remote exists in pool") testCases := []struct { desc string @@ -185,31 +196,73 @@ func TestUnlink(t *testing.T) { code codes.Code }{ { - desc: "Repository does not exist", + desc: "Successful request", + req: &gitalypb.UnlinkRepositoryFromObjectPoolRequest{ + Repository: testRepo, + ObjectPool: pool.ToProto(), + }, + code: codes.OK, + }, + { + desc: "Not linked in the first place", + req: &gitalypb.UnlinkRepositoryFromObjectPoolRequest{ + Repository: testRepo, + ObjectPool: pool2.ToProto(), + }, + code: codes.OK, + }, + { + desc: "No Repository", req: &gitalypb.UnlinkRepositoryFromObjectPoolRequest{ Repository: nil, + ObjectPool: pool.ToProto(), }, code: codes.InvalidArgument, }, { - desc: "Successful request", + desc: "No ObjectPool", req: &gitalypb.UnlinkRepositoryFromObjectPoolRequest{ Repository: testRepo, + ObjectPool: nil, + }, + code: codes.InvalidArgument, + }, + { + desc: "Repo not found", + req: &gitalypb.UnlinkRepositoryFromObjectPoolRequest{ + Repository: deletedRepo, ObjectPool: pool.ToProto(), }, code: codes.OK, }, + { + desc: "Pool not found", + req: &gitalypb.UnlinkRepositoryFromObjectPoolRequest{ + Repository: testRepo, + ObjectPool: &gitalypb.ObjectPool{ + Repository: &gitalypb.Repository{ + StorageName: testRepo.GetStorageName(), + RelativePath: "pool/does/not/exist", + }, + }, + }, + code: codes.NotFound, + }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { _, err := client.UnlinkRepositoryFromObjectPool(ctx, tc.req) - require.Equal(t, tc.code, helper.GrpcCode(err)) - if tc.code == codes.OK { - _, err = log.GetCommit(ctx, testRepo, poolCommitID) - require.True(t, log.IsNotFound(err), "expected 'not found' error got %v", err) + if tc.code != codes.OK { + testhelper.RequireGrpcError(t, err, tc.code) + return } + + require.NoError(t, err, "call UnlinkRepositoryFromObjectPool") + + remoteName := tc.req.Repository.GlRepository + require.False(t, testhelper.RemoteExists(t, pool.FullPath(), remoteName), "remote should no longer exist in pool") }) } } diff --git a/internal/service/objectpool/reduplicate_test.go b/internal/service/objectpool/reduplicate_test.go index e5119bc11..579c687d7 100644 --- a/internal/service/objectpool/reduplicate_test.go +++ b/internal/service/objectpool/reduplicate_test.go @@ -1,6 +1,7 @@ package objectpool import ( + "os" "testing" "github.com/stretchr/testify/require" @@ -31,8 +32,12 @@ func TestReduplicate(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "gc") existingObjectID := "55bc176024cfa3baaceb71db584c7e5df900ea65" + // Corrupt the repository to check if the object can't be found - require.NoError(t, pool.Unlink(ctx, testRepo)) + altPath, err := git.InfoAlternatesPath(testRepo) + require.NoError(t, err, "find info/alternates") + require.NoError(t, os.RemoveAll(altPath)) + cmd, err := git.Command(ctx, testRepo, "cat-file", "-e", existingObjectID) require.NoError(t, err) require.Error(t, cmd.Wait()) diff --git a/internal/service/repository/snapshot.go b/internal/service/repository/snapshot.go index 69a57984e..32ebb8edc 100644 --- a/internal/service/repository/snapshot.go +++ b/internal/service/repository/snapshot.go @@ -81,7 +81,7 @@ func (s *server) GetSnapshot(in *gitalypb.GetSnapshotRequest, stream gitalypb.Re } func addAlternateFiles(ctx context.Context, repository *gitalypb.Repository, builder *archive.TarBuilder) error { - alternateFilePath, err := git.AlternatesPath(repository) + alternateFilePath, err := git.InfoAlternatesPath(repository) if err != nil { return fmt.Errorf("error when getting alternates file path: %v", err) } diff --git a/internal/service/repository/snapshot_test.go b/internal/service/repository/snapshot_test.go index d5aa5e73d..1fcb17ab0 100644 --- a/internal/service/repository/snapshot_test.go +++ b/internal/service/repository/snapshot_test.go @@ -115,7 +115,7 @@ func TestGetSnapshotWithDedupe(t *testing.T) { require.True(t, catfile.IsNotFound(err)) // write alternates file to point to alt objects folder - alternatesPath, err := git.AlternatesPath(testRepo) + alternatesPath, err := git.InfoAlternatesPath(testRepo) require.NoError(t, err) require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(path.Join(repoPath, ".git", fmt.Sprintf("%s\n", alternateObjDir))), 0644)) @@ -146,7 +146,7 @@ func TestGetSnapshotWithDedupeSoftFailures(t *testing.T) { // 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) + alternatesPath, err := git.InfoAlternatesPath(testRepo) require.NoError(t, err) require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(fmt.Sprintf("%s\n", alternateObjPath)), 0644)) diff --git a/internal/testhelper/remote.go b/internal/testhelper/remote.go new file mode 100644 index 000000000..ada387494 --- /dev/null +++ b/internal/testhelper/remote.go @@ -0,0 +1,22 @@ +package testhelper + +import ( + "strings" + "testing" +) + +// RemoteExists tests if the repository at repoPath has a Git remote named remoteName. +func RemoteExists(t *testing.T, repoPath string, remoteName string) bool { + if remoteName == "" { + t.Fatal("empty remote name") + } + + remotes := MustRunCommand(t, nil, "git", "-C", repoPath, "remote") + for _, r := range strings.Split(string(remotes), "\n") { + if r == remoteName { + return true + } + } + + return false +} |