Merge branch 'objectpool-explicit-not-found' into 'master'

UnlinkRepositoryFromObjectPool: stop removing objects/info/alternates

See merge request gitlab-org/gitaly!1151

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
+}