Merge branch 'master' into rewrite-fetch-remote

9 files changed, 107 insertions, 47 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2a5235dc0..abb198d16 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,25 @@
 # Gitaly changelog
 
+## v1.19.0
+
+#### Fixed
+- Return blank checksum for git repositories with only invalid refs
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/1065
+
+#### Other
+- Use chunker in GetRawChanges
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/1043
+
+## v1.18.0
+
+#### Other
+- Make clear there is no []byte reuse bug in SearchFilesByContent
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/1055
+- Use chunker in FindCommits
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/1059
+- Statically link jaeger into Gitaly by default
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/1063
+
 ## v1.17.0
 
 #### Other
diff --git a/VERSION b/VERSION
index 092afa15d..815d5ca06 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-1.17.0
+1.19.0
diff --git a/_support/makegen.go b/_support/makegen.go
index 3200f3d25..c381b149b 100644
--- a/_support/makegen.go
+++ b/_support/makegen.go
@@ -232,6 +232,7 @@ PREFIX ?= /usr/local
 INSTALL_DEST_DIR := $(DESTDIR)$(PREFIX)/bin/
 BUNDLE_FLAGS ?= --deployment
 ASSEMBLY_ROOT ?= {{ .BuildDir }}/assembly
+BUILD_TAGS := tracer_static tracer_static_jaeger
 
 unexport GOROOT
 unexport GOBIN
diff --git a/internal/service/commit/find_commits.go b/internal/service/commit/find_commits.go
index bd7a178f5..8d4967873 100644
--- a/internal/service/commit/find_commits.go
+++ b/internal/service/commit/find_commits.go
@@ -13,6 +13,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/chunk"
 )
 
 const commitsPerPage int = 20
@@ -115,40 +116,37 @@ func (g *GetCommits) Commit() (*gitalypb.GitCommit, error) {
 	return commit, nil
 }
 
+type findCommitsSender struct {
+	stream  gitalypb.CommitService_FindCommitsServer
+	commits []*gitalypb.GitCommit
+}
+
+func (s *findCommitsSender) Reset() { s.commits = nil }
+func (s *findCommitsSender) Append(it chunk.Item) {
+	s.commits = append(s.commits, it.(*gitalypb.GitCommit))
+}
+func (s *findCommitsSender) Send() error {
+	return s.stream.Send(&gitalypb.FindCommitsResponse{Commits: s.commits})
+}
+
 func streamPaginatedCommits(getCommits *GetCommits, commitsPerPage int, stream gitalypb.CommitService_FindCommitsServer) error {
-	var commitPage []*gitalypb.GitCommit
+	chunker := chunk.New(&findCommitsSender{stream: stream})
 
 	for getCommits.Scan() {
 		commit, err := getCommits.Commit()
 		if err != nil {
 			return err
 		}
-		commitPage = append(commitPage, commit)
-		if len(commitPage) == commitsPerPage {
-			if err := stream.Send(
-				&gitalypb.FindCommitsResponse{
-					Commits: commitPage,
-				},
-			); err != nil {
-				return fmt.Errorf("error when sending stream response: %v", err)
-			}
-			commitPage = nil
+
+		if err := chunker.Send(commit); err != nil {
+			return err
 		}
 	}
 	if getCommits.Err() != nil {
 		return fmt.Errorf("get commits: %v", getCommits.Err())
 	}
-	// send the last page
-	if len(commitPage) > 0 {
-		if err := stream.Send(
-			&gitalypb.FindCommitsResponse{
-				Commits: commitPage,
-			},
-		); err != nil {
-			return fmt.Errorf("error when sending stream response: %v", err)
-		}
-	}
-	return nil
+
+	return chunker.Flush()
 }
 
 func getLogCommandFlags(req *gitalypb.FindCommitsRequest) []string {
diff --git a/internal/service/repository/calculate_checksum.go b/internal/service/repository/calculate_checksum.go
index b0804d036..85fafbdfe 100644
--- a/internal/service/repository/calculate_checksum.go
+++ b/internal/service/repository/calculate_checksum.go
@@ -71,7 +71,7 @@ func (s *server) CalculateChecksum(ctx context.Context, in *gitalypb.CalculateCh
 		return nil, status.Errorf(codes.Internal, err.Error())
 	}
 
-	if err := cmd.Wait(); err != nil {
+	if err := cmd.Wait(); checksum == nil || err != nil {
 		if isValidRepo(ctx, repo) {
 			return &gitalypb.CalculateChecksumResponse{Checksum: blankChecksum}, nil
 		}
diff --git a/internal/service/repository/calculate_checksum_test.go b/internal/service/repository/calculate_checksum_test.go
index 76f99902d..5a949da19 100644
--- a/internal/service/repository/calculate_checksum_test.go
+++ b/internal/service/repository/calculate_checksum_test.go
@@ -143,3 +143,29 @@ func TestFailedCalculateChecksum(t *testing.T) {
 		testhelper.RequireGrpcError(t, err, testCase.code)
 	}
 }
+
+func TestInvalidRefsCalculateChecksum(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	// Force the refs database of testRepo into a known state
+	require.NoError(t, os.RemoveAll(path.Join(testRepoPath, "refs")))
+	for _, d := range []string{"refs/heads", "refs/tags", "refs/notes"} {
+		require.NoError(t, os.MkdirAll(path.Join(testRepoPath, d), 0755))
+	}
+	require.NoError(t, exec.Command("cp", "testdata/checksum-test-invalid-refs", path.Join(testRepoPath, "packed-refs")).Run())
+
+	request := &gitalypb.CalculateChecksumRequest{Repository: testRepo}
+	testCtx, cancelCtx := testhelper.Context()
+	defer cancelCtx()
+
+	response, err := client.CalculateChecksum(testCtx, request)
+	require.NoError(t, err)
+	require.Equal(t, "0000000000000000000000000000000000000000", response.Checksum)
+}
diff --git a/internal/service/repository/raw_changes.go b/internal/service/repository/raw_changes.go
index 1a8b243c0..41b2faa05 100644
--- a/internal/service/repository/raw_changes.go
+++ b/internal/service/repository/raw_changes.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/rawdiff"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/chunk"
 )
 
 func (s *server) GetRawChanges(req *gitalypb.GetRawChangesRequest, stream gitalypb.RepositoryService_GetRawChangesServer) error {
@@ -62,17 +63,16 @@ func getRawChanges(stream gitalypb.RepositoryService_GetRawChangesServer, repo *
 	if err != nil {
 		return fmt.Errorf("start git diff: %v", err)
 	}
-	p := rawdiff.NewParser(diffCmd)
 
-	var chunk []*gitalypb.GetRawChangesResponse_RawChange
+	p := rawdiff.NewParser(diffCmd)
+	chunker := chunk.New(&rawChangesSender{stream: stream})
 
 	for {
 		d, err := p.NextDiff()
+		if err == io.EOF {
+			break // happy path
+		}
 		if err != nil {
-			if err == io.EOF {
-				break // happy path
-			}
-
 			return fmt.Errorf("read diff: %v", err)
 		}
 
@@ -80,21 +80,8 @@ func getRawChanges(stream gitalypb.RepositoryService_GetRawChangesServer, repo *
 		if err != nil {
 			return fmt.Errorf("build change from diff line: %v", err)
 		}
-		chunk = append(chunk, change)
-
-		const chunkSize = 50
-		if len(chunk) >= chunkSize {
-			resp := &gitalypb.GetRawChangesResponse{RawChanges: chunk}
-			if err := stream.Send(resp); err != nil {
-				return fmt.Errorf("send response: %v", err)
-			}
-			chunk = nil
-		}
-	}
 
-	if len(chunk) > 0 {
-		resp := &gitalypb.GetRawChangesResponse{RawChanges: chunk}
-		if err := stream.Send(resp); err != nil {
+		if err := chunker.Send(change); err != nil {
 			return fmt.Errorf("send response: %v", err)
 		}
 	}
@@ -103,7 +90,21 @@ func getRawChanges(stream gitalypb.RepositoryService_GetRawChangesServer, repo *
 		return fmt.Errorf("wait git diff: %v", err)
 	}
 
-	return nil
+	return chunker.Flush()
+}
+
+type rawChangesSender struct {
+	stream  gitalypb.RepositoryService_GetRawChangesServer
+	changes []*gitalypb.GetRawChangesResponse_RawChange
+}
+
+func (s *rawChangesSender) Reset() { s.changes = nil }
+func (s *rawChangesSender) Append(it chunk.Item) {
+	s.changes = append(s.changes, it.(*gitalypb.GetRawChangesResponse_RawChange))
+}
+func (s *rawChangesSender) Send() error {
+	response := &gitalypb.GetRawChangesResponse{RawChanges: s.changes}
+	return s.stream.Send(response)
 }
 
 // Ordinarily, Git uses 0000000000000000000000000000000000000000, the
diff --git a/internal/service/repository/search_files.go b/internal/service/repository/search_files.go
index 936a97798..a69ce913b 100644
--- a/internal/service/repository/search_files.go
+++ b/internal/service/repository/search_files.go
@@ -122,8 +122,13 @@ func sendSearchFilesResultChunked(cmd *command.Command, stream gitalypb.Reposito
 	scanner := bufio.NewScanner(cmd)
 
 	for scanner.Scan() {
-		line := append(scanner.Bytes(), '\n')
-		if bytes.Equal(line, contentDelimiter) {
+		// Intentionally avoid scanner.Bytes() because that returns a []byte that
+		// becomes invalid on the next loop iteration, and we want to hold on to
+		// the contents of the current line for a while. Scanner.Text() is a
+		// string and hence immutable.
+		line := scanner.Text() + "\n"
+
+		if line == string(contentDelimiter) {
 			if err := sendMatchInChunks(buf, stream); err != nil {
 				return err
 			}
diff --git a/internal/service/repository/testdata/checksum-test-invalid-refs b/internal/service/repository/testdata/checksum-test-invalid-refs
new file mode 100644
index 000000000..04e8ae6e5
--- /dev/null
+++ b/internal/service/repository/testdata/checksum-test-invalid-refs
@@ -0,0 +1,9 @@
+# pack-refs with: peeled fully-peeled sorted
+1450cd639e0bc6721eb02800169e464f212cde06 foo/bar
+259a6fba859cc91c54cd86a2cbd4c2f720e3a19d foo/bar:baz
+d0a293c0ac821fadfdc086fe528f79423004229d refs/foo/bar:baz
+21751bf5cb2b556543a11018c1f13b35e44a99d7 tags/v0.0.1
+498214de67004b1da3d820901307bed2a68a8ef6 keep-around/498214de67004b1da3d820901307bed2a68a8ef6
+38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e merge-requests/11/head
+c347ca2e140aa667b968e51ed0ffe055501fe4f4 environments/3/head
+4ed78158b5b018c43005cec917129861541e25bc notes/commits
\ No newline at end of file