Merge branch 'da-checksum-rpc-invalid-git-directory' into 'master'

Return DataLoss error for non-valid git repositories

See merge request gitlab-org/gitaly!697

3 files changed, 51 insertions, 7 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d0dc54237..1bfb212cd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,8 @@ UNRELEASED
   https://gitlab.com/gitlab-org/gitaly/merge_requests/677
 - Introduce feature flag package based on gRPC metadata
   https://gitlab.com/gitlab-org/gitaly/merge_requests/704
+- Return DataLoss error for non-valid git repositories when calculating the checksum
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/697
 
 v0.98.0
 
diff --git a/internal/service/repository/calculate_checksum.go b/internal/service/repository/calculate_checksum.go
index bbbe50e99..433b43de3 100644
--- a/internal/service/repository/calculate_checksum.go
+++ b/internal/service/repository/calculate_checksum.go
@@ -2,13 +2,17 @@ package repository
 
 import (
 	"bufio"
+	"bytes"
 	"crypto/sha1"
 	"encoding/hex"
 	"math/big"
+	"os/exec"
+	"strings"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 
 	"golang.org/x/net/context"
@@ -21,7 +25,7 @@ const blankChecksum = "0000000000000000000000000000000000000000"
 func (s *server) CalculateChecksum(ctx context.Context, in *pb.CalculateChecksumRequest) (*pb.CalculateChecksumResponse, error) {
 	repo := in.GetRepository()
 
-	_, err := helper.GetRepoPath(repo)
+	repoPath, err := helper.GetRepoPath(repo)
 	if err != nil {
 		return nil, err
 	}
@@ -65,14 +69,32 @@ func (s *server) CalculateChecksum(ctx context.Context, in *pb.CalculateChecksum
 	}
 
 	if err := cmd.Wait(); err != nil {
-		if code, ok := command.ExitStatus(err); ok && code == 1 {
-			// Exit code 1: the repository doesn't have any ref
+		if isValidRepo(ctx, repo) {
 			return &pb.CalculateChecksumResponse{Checksum: blankChecksum}, nil
 		}
 
-		// This will normally occur when exit code > 1
-		return nil, status.Errorf(codes.Internal, err.Error())
+		return nil, status.Errorf(codes.DataLoss, "CalculateChecksum: not a git repository '%s'", repoPath)
 	}
 
 	return &pb.CalculateChecksumResponse{Checksum: hex.EncodeToString(checksum.Bytes())}, nil
 }
+
+func isValidRepo(ctx context.Context, repo *pb.Repository) bool {
+	repoPath, env, err := alternates.PathAndEnv(repo)
+	if err != nil {
+		return false
+	}
+
+	args := []string{"-C", repoPath, "rev-parse", "--is-inside-git-dir"}
+	stdout := &bytes.Buffer{}
+	cmd, err := command.New(ctx, exec.Command(command.GitPath(), args...), nil, stdout, nil, env...)
+	if err != nil {
+		return false
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return false
+	}
+
+	return strings.EqualFold(strings.TrimRight(stdout.String(), "\n"), "true")
+}
diff --git a/internal/service/repository/calculate_checksum_test.go b/internal/service/repository/calculate_checksum_test.go
index e800f0a0d..a5c2f029a 100644
--- a/internal/service/repository/calculate_checksum_test.go
+++ b/internal/service/repository/calculate_checksum_test.go
@@ -6,11 +6,10 @@ import (
 	"path"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
-
-	"github.com/stretchr/testify/require"
 )
 
 func TestSuccessfulCalculateChecksum(t *testing.T) {
@@ -58,6 +57,27 @@ func TestEmptyRepositoryCalculateChecksum(t *testing.T) {
 	require.Equal(t, "0000000000000000000000000000000000000000", response.Checksum)
 }
 
+func TestBrokenRepositoryCalculateChecksum(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	repo, testRepoPath, cleanupFn := testhelper.InitBareRepo(t)
+	defer cleanupFn()
+
+	// Force an empty HEAD file
+	require.NoError(t, os.Truncate(path.Join(testRepoPath, "HEAD"), 0))
+
+	request := &pb.CalculateChecksumRequest{Repository: repo}
+	testCtx, cancelCtx := testhelper.Context()
+	defer cancelCtx()
+
+	_, err := client.CalculateChecksum(testCtx, request)
+	testhelper.AssertGrpcError(t, err, codes.DataLoss, "not a git repository")
+}
+
 func TestFailedCalculateChecksum(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()