smarthttp: Verify repo existence before dealing with cache

InfoRefsUploadPack and InfoRefsReceivePack doesn't verify
if repository exist before the caching get involved into the
process. It requires additional operations to run that does
nothing at the end because of missed repository. It also
produces error log entries that may confuse some time.
In order to optimise this case we first get path to the
repository and if it doesn't exist the error is returned
immediately without need to bother caching (and no extra
logs).

2 files changed, 55 insertions, 48 deletions

diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index 318a70590..dcd338daf 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -21,32 +21,36 @@ const (
 )
 
 func (s *server) InfoRefsUploadPack(in *gitalypb.InfoRefsRequest, stream gitalypb.SmartHTTPService_InfoRefsUploadPackServer) error {
+	repoPath, err := s.locator.GetRepoPath(in.GetRepository())
+	if err != nil {
+		return err
+	}
+
 	w := streamio.NewWriter(func(p []byte) error {
 		return stream.Send(&gitalypb.InfoRefsResponse{Data: p})
 	})
 
 	return s.infoRefCache.tryCache(stream.Context(), in, w, func(w io.Writer) error {
-		return s.handleInfoRefs(stream.Context(), uploadPackSvc, in, w)
+		return s.handleInfoRefs(stream.Context(), uploadPackSvc, repoPath, in, w)
 	})
 }
 
 func (s *server) InfoRefsReceivePack(in *gitalypb.InfoRefsRequest, stream gitalypb.SmartHTTPService_InfoRefsReceivePackServer) error {
+	repoPath, err := s.locator.GetRepoPath(in.GetRepository())
+	if err != nil {
+		return err
+	}
 	w := streamio.NewWriter(func(p []byte) error {
 		return stream.Send(&gitalypb.InfoRefsResponse{Data: p})
 	})
-	return s.handleInfoRefs(stream.Context(), receivePackSvc, in, w)
+	return s.handleInfoRefs(stream.Context(), receivePackSvc, repoPath, in, w)
 }
 
-func (s *server) handleInfoRefs(ctx context.Context, service string, req *gitalypb.InfoRefsRequest, w io.Writer) error {
+func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, req *gitalypb.InfoRefsRequest, w io.Writer) error {
 	ctxlogrus.Extract(ctx).WithFields(log.Fields{
 		"service": service,
 	}).Debug("handleInfoRefs")
 
-	repoPath, err := s.locator.GetRepoPath(req.Repository)
-	if err != nil {
-		return err
-	}
-
 	cmdOpts := []git.CmdOpt{git.WithGitProtocol(ctx, req)}
 	if service == "receive-pack" {
 		cmdOpts = append(cmdOpts, git.WithRefTxHook(ctx, req.Repository, s.cfg))
diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go
index 40c5d1a1d..844cb9d7d 100644
--- a/internal/gitaly/service/smarthttp/inforefs_test.go
+++ b/internal/gitaly/service/smarthttp/inforefs_test.go
@@ -20,7 +20,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/objectpool"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v14/streamio"
@@ -46,6 +48,23 @@ func TestSuccessfulInfoRefsUploadPack(t *testing.T) {
 	})
 }
 
+func TestInfoRefsUploadPack_repositoryDoesntExist(t *testing.T) {
+	cfg := testcfg.Build(t)
+
+	serverSocketPath := runSmartHTTPServer(t, cfg)
+
+	rpcRequest := &gitalypb.InfoRefsRequest{Repository: &gitalypb.Repository{
+		StorageName:  cfg.Storages[0].Name,
+		RelativePath: "doesnt/exist",
+	}}
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	_, err := makeInfoRefsUploadPackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest)
+	testassert.GrpcEqualErr(t, helper.ErrNotFoundf(`GetRepoPath: not a git repository: "`+cfg.Storages[0].Path+`/doesnt/exist"`), err)
+}
+
 func TestSuccessfulInfoRefsUploadWithPartialClone(t *testing.T) {
 	cfg, repo, _ := testcfg.BuildWithRepo(t)
 
@@ -143,20 +162,12 @@ func TestSuccessfulInfoRefsReceivePack(t *testing.T) {
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	client, conn := newSmartHTTPClient(t, serverSocketPath, cfg.Auth.Token)
-	defer conn.Close()
-
 	rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo}
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
-	c, err := client.InfoRefsReceivePack(ctx, rpcRequest)
-	require.NoError(t, err)
 
-	response, err := ioutil.ReadAll(streamio.NewReader(func() ([]byte, error) {
-		resp, err := c.Recv()
-		return resp.GetData(), err
-	}))
+	response, err := makeInfoRefsReceivePackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest)
 	require.NoError(t, err)
 
 	assertGitRefAdvertisement(t, "InfoRefsReceivePack", string(response), "001f# service=git-receive-pack", "0000", []string{
@@ -172,9 +183,6 @@ func TestObjectPoolRefAdvertisementHiding(t *testing.T) {
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	client, conn := newSmartHTTPClient(t, serverSocketPath, cfg.Auth.Token)
-	defer conn.Close()
-
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -199,14 +207,7 @@ func TestObjectPoolRefAdvertisementHiding(t *testing.T) {
 
 	rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo}
 
-	c, err := client.InfoRefsReceivePack(ctx, rpcRequest)
-	require.NoError(t, err)
-
-	response, err := ioutil.ReadAll(streamio.NewReader(func() ([]byte, error) {
-		resp, err := c.Recv()
-		return resp.GetData(), err
-	}))
-
+	response, err := makeInfoRefsReceivePackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest)
 	require.NoError(t, err)
 	require.NotContains(t, string(response), commitID+" .have")
 }
@@ -216,22 +217,14 @@ func TestFailureRepoNotFoundInfoRefsReceivePack(t *testing.T) {
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	client, conn := newSmartHTTPClient(t, serverSocketPath, cfg.Auth.Token)
-	defer conn.Close()
 	repo := &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "testdata/scratch/another_repo"}
 	rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo}
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
-	c, err := client.InfoRefsReceivePack(ctx, rpcRequest)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	for err == nil {
-		_, err = c.Recv()
-	}
-	testhelper.RequireGrpcError(t, err, codes.NotFound)
+	_, err := makeInfoRefsReceivePackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest)
+	msg := `GetRepoPath: not a git repository: "` + cfg.Storages[0].Path + "/" + repo.RelativePath + `"`
+	testassert.GrpcEqualErr(t, helper.ErrNotFoundf(msg), err)
 }
 
 func TestFailureRepoNotSetInfoRefsReceivePack(t *testing.T) {
@@ -239,21 +232,31 @@ func TestFailureRepoNotSetInfoRefsReceivePack(t *testing.T) {
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	client, conn := newSmartHTTPClient(t, serverSocketPath, cfg.Auth.Token)
-	defer conn.Close()
 	rpcRequest := &gitalypb.InfoRefsRequest{}
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
+	_, err := makeInfoRefsReceivePackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest)
+	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+}
+
+func makeInfoRefsReceivePackRequest(ctx context.Context, t *testing.T, serverSocketPath, token string, rpcRequest *gitalypb.InfoRefsRequest) ([]byte, error) {
+	t.Helper()
+
+	client, conn := newSmartHTTPClient(t, serverSocketPath, token)
+	defer conn.Close()
+
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
 	c, err := client.InfoRefsReceivePack(ctx, rpcRequest)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
-	for err == nil {
-		_, err = c.Recv()
-	}
-	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+	response, err := ioutil.ReadAll(streamio.NewReader(func() ([]byte, error) {
+		resp, err := c.Recv()
+		return resp.GetData(), err
+	}))
+
+	return response, err
 }
 
 func assertGitRefAdvertisement(t *testing.T, rpc, responseBody string, firstLine, lastLine string, middleLines []string) {