Merge branch 'pks-gitpipe-small-tweaks' into 'master'

gitpipe: Smallish tweaks

See merge request gitlab-org/gitaly!3681

7 files changed, 83 insertions, 17 deletions

diff --git a/internal/git/catfile/commit.go b/internal/git/catfile/commit.go
index dc6a4dc3e..76047849d 100644
--- a/internal/git/catfile/commit.go
+++ b/internal/git/catfile/commit.go
@@ -25,7 +25,7 @@ func GetCommit(ctx context.Context, c Batch, revision git.Revision) (*gitalypb.G
 		return nil, err
 	}
 
-	return ParseCommit(obj.Reader, &obj.ObjectInfo)
+	return ParseCommit(obj.Reader, obj.ObjectInfo.Oid)
 }
 
 // GetCommitWithTrailers looks up a commit by revision using an existing Batch instance, and
@@ -82,12 +82,12 @@ func GetCommitMessage(ctx context.Context, c Batch, repo repository.GitRepo, rev
 }
 
 // ParseCommit parses the commit data from the Reader.
-func ParseCommit(r io.Reader, info *ObjectInfo) (*gitalypb.GitCommit, error) {
+func ParseCommit(r io.Reader, oid git.ObjectID) (*gitalypb.GitCommit, error) {
 	header, body, err := splitRawCommit(r)
 	if err != nil {
 		return nil, err
 	}
-	return buildCommit(header, body, info)
+	return buildCommit(header, body, oid)
 }
 
 func splitRawCommit(r io.Reader) ([]byte, []byte, error) {
@@ -107,9 +107,9 @@ func splitRawCommit(r io.Reader) ([]byte, []byte, error) {
 	return header, body, nil
 }
 
-func buildCommit(header, body []byte, info *ObjectInfo) (*gitalypb.GitCommit, error) {
+func buildCommit(header, body []byte, oid git.ObjectID) (*gitalypb.GitCommit, error) {
 	commit := &gitalypb.GitCommit{
-		Id:       info.Oid.String(),
+		Id:       oid.String(),
 		BodySize: int64(len(body)),
 		Body:     body,
 		Subject:  subjectFromBody(body),
diff --git a/internal/git/catfile/commit_test.go b/internal/git/catfile/commit_test.go
index df65bae49..212e0ae97 100644
--- a/internal/git/catfile/commit_test.go
+++ b/internal/git/catfile/commit_test.go
@@ -101,7 +101,7 @@ func TestParseCommit(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			info.Size = int64(len(tc.in))
-			out, err := ParseCommit(bytes.NewBuffer(tc.in), info)
+			out, err := ParseCommit(bytes.NewBuffer(tc.in), info.Oid)
 			require.NoError(t, err, "parse error")
 			require.Equal(t, tc.out, out)
 		})
diff --git a/internal/git/gitpipe/pipeline_test.go b/internal/git/gitpipe/pipeline_test.go
index 97062e6f6..94e174ad2 100644
--- a/internal/git/gitpipe/pipeline_test.go
+++ b/internal/git/gitpipe/pipeline_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
@@ -15,7 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 )
 
-func TestPipeline(t *testing.T) {
+func TestPipeline_revlist(t *testing.T) {
 	cfg := testcfg.Build(t)
 
 	repoProto, _, cleanup := gittest.CloneRepoAtStorage(t, cfg, cfg.Storages[0], t.Name())
@@ -330,3 +331,68 @@ func TestPipeline(t *testing.T) {
 		require.Greater(t, i, 1000)
 	})
 }
+
+func TestPipeline_forEachRef(t *testing.T) {
+	cfg := testcfg.Build(t)
+
+	repoProto, _, cleanup := gittest.CloneRepoAtStorage(t, cfg, cfg.Storages[0], t.Name())
+	defer cleanup()
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	catfileCache := catfile.NewCache(cfg)
+	defer catfileCache.Stop()
+
+	catfileProcess, err := catfileCache.BatchProcess(ctx, repo)
+	require.NoError(t, err)
+
+	forEachRefIter := ForEachRef(ctx, repo, nil)
+	catfileInfoIter := CatfileInfo(ctx, catfileProcess, forEachRefIter)
+	catfileObjectIter := CatfileObject(ctx, catfileProcess, catfileInfoIter)
+
+	type object struct {
+		oid     git.ObjectID
+		content []byte
+	}
+
+	objectsByRef := make(map[git.ReferenceName]object)
+	for catfileObjectIter.Next() {
+		result := catfileObjectIter.Result()
+
+		// While we could also assert object data, let's not do
+		// this: it would just be too annoying.
+		require.NotNil(t, result.ObjectReader)
+
+		objectData, err := ioutil.ReadAll(result.ObjectReader)
+		require.NoError(t, err)
+		require.Len(t, objectData, int(result.ObjectInfo.Size))
+
+		objectsByRef[git.ReferenceName(result.ObjectName)] = object{
+			oid:     result.ObjectInfo.Oid,
+			content: objectData,
+		}
+	}
+	require.NoError(t, catfileObjectIter.Err())
+	require.Greater(t, len(objectsByRef), 90)
+
+	// We certainly don't want to hard-code all the references, so we just cross-check with the
+	// localrepo implementation to verify that both return the same data.
+	refs, err := repo.GetReferences(ctx)
+	require.NoError(t, err)
+	require.Equal(t, len(refs), len(objectsByRef))
+
+	expectedObjectsByRef := make(map[git.ReferenceName]object)
+	for _, ref := range refs {
+		oid := git.ObjectID(ref.Target)
+		content, err := repo.ReadObject(ctx, oid)
+		require.NoError(t, err)
+
+		expectedObjectsByRef[ref.Name] = object{
+			oid:     oid,
+			content: content,
+		}
+	}
+	require.Equal(t, expectedObjectsByRef, objectsByRef)
+}
diff --git a/internal/git/gitpipe/revision.go b/internal/git/gitpipe/revision.go
index 293f0bd07..6cd576f50 100644
--- a/internal/git/gitpipe/revision.go
+++ b/internal/git/gitpipe/revision.go
@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
@@ -317,7 +318,7 @@ func ForEachRef(
 				// us to read the referenced commit's object. It would thus be about
 				// 2-3x slower to use the default format, and instead we move the
 				// burden into the next pipeline step.
-				git.Flag{Name: "--format=%(objectname) %(refname)"},
+				git.ValueFlag{Name: "--format", Value: "%(objectname) %(refname)"},
 			},
 			Args: patterns,
 		})
@@ -328,10 +329,9 @@ func ForEachRef(
 
 		scanner := bufio.NewScanner(forEachRef)
 		for scanner.Scan() {
-			line := make([]byte, len(scanner.Bytes()))
-			copy(line, scanner.Bytes())
+			line := scanner.Text()
 
-			oidAndRef := bytes.SplitN(line, []byte{' '}, 2)
+			oidAndRef := strings.SplitN(line, " ", 2)
 			if len(oidAndRef) != 2 {
 				sendRevisionResult(ctx, resultChan, RevisionResult{
 					err: fmt.Errorf("invalid for-each-ref format: %q", line),
@@ -341,7 +341,7 @@ func ForEachRef(
 
 			if isDone := sendRevisionResult(ctx, resultChan, RevisionResult{
 				OID:        git.ObjectID(oidAndRef[0]),
-				ObjectName: oidAndRef[1],
+				ObjectName: []byte(oidAndRef[1]),
 			}); isDone {
 				return
 			}
diff --git a/internal/gitaly/service/commit/list_all_commits.go b/internal/gitaly/service/commit/list_all_commits.go
index f99e6424d..1ad5179b1 100644
--- a/internal/gitaly/service/commit/list_all_commits.go
+++ b/internal/gitaly/service/commit/list_all_commits.go
@@ -78,7 +78,7 @@ func (s *server) ListAllCommits(
 
 		object := catfileObjectIter.Result()
 
-		commit, err := catfile.ParseCommit(object.ObjectReader, object.ObjectInfo)
+		commit, err := catfile.ParseCommit(object.ObjectReader, object.ObjectInfo.Oid)
 		if err != nil {
 			return helper.ErrInternal(fmt.Errorf("parsing commit: %w", err))
 		}
diff --git a/internal/gitaly/service/commit/list_commits.go b/internal/gitaly/service/commit/list_commits.go
index f38cfe40a..3f14bbabd 100644
--- a/internal/gitaly/service/commit/list_commits.go
+++ b/internal/gitaly/service/commit/list_commits.go
@@ -123,7 +123,7 @@ func (s *server) ListCommits(
 
 		object := catfileObjectIter.Result()
 
-		commit, err := catfile.ParseCommit(object.ObjectReader, object.ObjectInfo)
+		commit, err := catfile.ParseCommit(object.ObjectReader, object.ObjectInfo.Oid)
 		if err != nil {
 			return helper.ErrInternal(fmt.Errorf("parsing commit: %w", err))
 		}
diff --git a/internal/gitaly/service/ref/refs.go b/internal/gitaly/service/ref/refs.go
index f25e7dd0e..6d1df145e 100644
--- a/internal/gitaly/service/ref/refs.go
+++ b/internal/gitaly/service/ref/refs.go
@@ -76,7 +76,7 @@ type tagSender struct {
 }
 
 func (t *tagSender) Reset() {
-	t.tags = nil
+	t.tags = t.tags[:0]
 }
 
 func (t *tagSender) Append(m proto.Message) {
@@ -192,7 +192,7 @@ func (s *server) findAllTags(ctx context.Context, repo *localrepo.Repo, stream g
 				return fmt.Errorf("parsing annotated tag: %w", err)
 			}
 		case "commit":
-			commit, err := catfile.ParseCommit(tag.ObjectReader, tag.ObjectInfo)
+			commit, err := catfile.ParseCommit(tag.ObjectReader, tag.ObjectInfo.Oid)
 			if err != nil {
 				return fmt.Errorf("parsing tagged commit: %w", err)
 			}
@@ -224,7 +224,7 @@ func (s *server) findAllTags(ctx context.Context, repo *localrepo.Repo, stream g
 		// We only need to parse the tagged object in case we have an annotated tag which
 		// refers to a commit object. Otherwise, we discard the object's contents.
 		if tag.ObjectInfo.Type == "tag" && peeledTag.ObjectInfo.Type == "commit" {
-			result.TargetCommit, err = catfile.ParseCommit(peeledTag.ObjectReader, peeledTag.ObjectInfo)
+			result.TargetCommit, err = catfile.ParseCommit(peeledTag.ObjectReader, peeledTag.ObjectInfo.Oid)
 			if err != nil {
 				return fmt.Errorf("parsing tagged commit: %w", err)
 			}