Merge branch 'pks-lfs-pointers-object-type-filter' into 'master'

blob: Speed up LFS pointer search via object type filters

See merge request gitlab-org/gitaly!3590

6 files changed, 179 insertions, 6 deletions

diff --git a/internal/git/version.go b/internal/git/version.go
index db31701ea..cf5ca632d 100644
--- a/internal/git/version.go
+++ b/internal/git/version.go
@@ -74,6 +74,12 @@ func (v Version) IsSupported() bool {
 	return !v.LessThan(minimumVersion)
 }
 
+// SupportsObjectTypeFilter checks if a version corresponds to a Git version which supports object
+// type filters.
+func (v Version) SupportsObjectTypeFilter() bool {
+	return !v.LessThan(Version{major: 2, minor: 32, patch: 0})
+}
+
 // LessThan determines whether the version is older than another version.
 func (v Version) LessThan(other Version) bool {
 	switch {
diff --git a/internal/git/version_test.go b/internal/git/version_test.go
index e0c705d2f..17b832ca1 100644
--- a/internal/git/version_test.go
+++ b/internal/git/version_test.go
@@ -123,8 +123,30 @@ func TestVersion_IsSupported(t *testing.T) {
 		{"2.31.1", true},
 		{"3.0.0", true},
 	} {
-		version, err := parseVersion(tc.version)
-		require.NoError(t, err)
-		require.Equal(t, tc.expect, version.IsSupported())
+		t.Run(tc.version, func(t *testing.T) {
+			version, err := parseVersion(tc.version)
+			require.NoError(t, err)
+			require.Equal(t, tc.expect, version.IsSupported())
+		})
+	}
+}
+
+func TestVersion_SupportsObjectTypeFilter(t *testing.T) {
+	for _, tc := range []struct {
+		version string
+		expect  bool
+	}{
+		{"2.31.0", false},
+		{"2.31.0-rc0", false},
+		{"2.31.1", false},
+		{"2.32.0", true},
+		{"2.32.1", true},
+		{"3.0.0", true},
+	} {
+		t.Run(tc.version, func(t *testing.T) {
+			version, err := parseVersion(tc.version)
+			require.NoError(t, err)
+			require.Equal(t, tc.expect, version.SupportsObjectTypeFilter())
+		})
 	}
 }
diff --git a/internal/gitaly/service/blob/blobs.go b/internal/gitaly/service/blob/blobs.go
index abe6926b5..ce6e1dca9 100644
--- a/internal/gitaly/service/blob/blobs.go
+++ b/internal/gitaly/service/blob/blobs.go
@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/golang/protobuf/proto"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/chunk"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -52,7 +53,17 @@ func (s *server) ListBlobs(req *gitalypb.ListBlobsRequest, stream gitalypb.BlobS
 		},
 	})
 
-	revlistChan := revlist(ctx, repo, req.GetRevisions())
+	gitVersion, err := git.CurrentVersion(ctx, s.gitCmdFactory)
+	if err != nil {
+		return helper.ErrInternalf("cannot determine Git version: %v", err)
+	}
+
+	var revlistOptions []revlistOption
+	if gitVersion.SupportsObjectTypeFilter() {
+		revlistOptions = append(revlistOptions, withObjectTypeFilter(objectTypeBlob))
+	}
+
+	revlistChan := revlist(ctx, repo, req.GetRevisions(), revlistOptions...)
 	catfileInfoChan := catfileInfo(ctx, catfileProcess, revlistChan)
 	catfileInfoChan = catfileInfoFilter(ctx, catfileInfoChan, func(r catfileInfoResult) bool {
 		return r.objectInfo.Type == "blob"
diff --git a/internal/gitaly/service/blob/lfs_pointers.go b/internal/gitaly/service/blob/lfs_pointers.go
index f1a5c625b..200377d77 100644
--- a/internal/gitaly/service/blob/lfs_pointers.go
+++ b/internal/gitaly/service/blob/lfs_pointers.go
@@ -73,7 +73,17 @@ func (s *server) ListLFSPointers(in *gitalypb.ListLFSPointersRequest, stream git
 			return helper.ErrInternal(fmt.Errorf("creating catfile process: %w", err))
 		}
 
-		revlistChan := revlist(ctx, repo, in.GetRevisions(), withBlobLimit(lfsPointerMaxSize))
+		gitVersion, err := git.CurrentVersion(ctx, s.gitCmdFactory)
+		if err != nil {
+			return helper.ErrInternalf("cannot determine Git version: %v", err)
+		}
+
+		revlistOptions := []revlistOption{withBlobLimit(lfsPointerMaxSize)}
+		if gitVersion.SupportsObjectTypeFilter() {
+			revlistOptions = append(revlistOptions, withObjectTypeFilter(objectTypeBlob))
+		}
+
+		revlistChan := revlist(ctx, repo, in.GetRevisions(), revlistOptions...)
 		catfileInfoChan := catfileInfo(ctx, catfileProcess, revlistChan)
 		catfileInfoChan = catfileInfoFilter(ctx, catfileInfoChan, func(r catfileInfoResult) bool {
 			return r.objectInfo.Type == "blob" && r.objectInfo.Size <= lfsPointerMaxSize
diff --git a/internal/gitaly/service/blob/pipeline.go b/internal/gitaly/service/blob/pipeline.go
index 1ccc251f6..52500b401 100644
--- a/internal/gitaly/service/blob/pipeline.go
+++ b/internal/gitaly/service/blob/pipeline.go
@@ -28,9 +28,19 @@ type revlistResult struct {
 	objectName []byte
 }
 
+type objectType string
+
+const (
+	objectTypeCommit = objectType("commit")
+	objectTypeBlob   = objectType("blob")
+	objectTypeTree   = objectType("tree")
+	objectTypeTag    = objectType("tag")
+)
+
 // revlistConfig is configuration for the revlist pipeline step.
 type revlistConfig struct {
-	blobLimit int
+	blobLimit  int
+	objectType objectType
 }
 
 // revlistOption is an option for the revlist pipeline step.
@@ -44,6 +54,16 @@ func withBlobLimit(limit int) revlistOption {
 	}
 }
 
+// withObjectTypeFilter will set up a `--filter=object:type=` filter for git-rev-list(1). This will
+// cause it to filter out any objects which do not match the given type. Because git-rev-list(1) by
+// default never filters provided arguments, this option also sets up the `--filter-provided` flag.
+// Note that this option is only supported starting with Git v2.32.0 or later.
+func withObjectTypeFilter(t objectType) revlistOption {
+	return func(cfg *revlistConfig) {
+		cfg.objectType = t
+	}
+}
+
 // revlist runs git-rev-list(1) with objects and object names enabled. The returned channel will
 // contain all object IDs listed by this command. Cancelling the context will cause the pipeline to
 // be cancelled, too.
@@ -81,6 +101,12 @@ func revlist(
 				Name: fmt.Sprintf("--filter=blob:limit=%d", cfg.blobLimit),
 			})
 		}
+		if cfg.objectType != "" {
+			flags = append(flags,
+				git.Flag{Name: fmt.Sprintf("--filter=object:type=%s", cfg.objectType)},
+				git.Flag{Name: "--filter-provided-objects"},
+			)
+		}
 
 		revlist, err := repo.Exec(ctx, git.SubCmd{
 			Name:  "rev-list",
diff --git a/internal/gitaly/service/blob/pipeline_test.go b/internal/gitaly/service/blob/pipeline_test.go
index 92469b0c8..96c071d13 100644
--- a/internal/gitaly/service/blob/pipeline_test.go
+++ b/internal/gitaly/service/blob/pipeline_test.go
@@ -9,6 +9,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"
@@ -23,8 +24,21 @@ func TestRevlist(t *testing.T) {
 	defer cleanup()
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
+	needsObjectTypeFilters := func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
+		gitVersion, err := git.CurrentVersion(ctx, git.NewExecCommandFactory(cfg))
+		require.NoError(t, err)
+
+		if !gitVersion.SupportsObjectTypeFilter() {
+			t.Skip("Git does not support object type filters")
+		}
+	}
+
 	for _, tc := range []struct {
 		desc            string
+		precondition    func(t *testing.T)
 		revisions       []string
 		options         []revlistOption
 		expectedResults []revlistResult
@@ -131,6 +145,86 @@ func TestRevlist(t *testing.T) {
 			},
 		},
 		{
+			desc:         "tree with blob object type filter",
+			precondition: needsObjectTypeFilters,
+			revisions: []string{
+				"79d5f98270ad677c86a7e1ab2baa922958565135",
+			},
+			options: []revlistOption{
+				withObjectTypeFilter(objectTypeBlob),
+			},
+			expectedResults: []revlistResult{
+				{oid: "8af7f880ce38649fc49f66e3f38857bfbec3f0b7", objectName: []byte("feature-1.txt")},
+				{oid: "16ca0b267f82cd2f5ca1157dd162dae98745eab8", objectName: []byte("feature-2.txt")},
+				{oid: "0fb47f093f769008049a0b0976ac3fa6d6125033", objectName: []byte("hotfix-1.txt")},
+				{oid: "4ae6c5e14452a35d04156277ae63e8356eb17cae", objectName: []byte("hotfix-2.txt")},
+				{oid: "b988ffed90cb6a9b7f98a3686a933edb3c5d70c0", objectName: []byte("iso8859.txt")},
+				{oid: "570f8e1dfe8149c1d17002712310d43dfeb43159", objectName: []byte("russian.rb")},
+				{oid: "7a17968582c21c9153ec24c6a9d5f33592ad9103", objectName: []byte("test.txt")},
+				{oid: "f3064a3aa9c14277483f690250072e987e2c8356", objectName: []byte("\xe3\x83\x86\xe3\x82\xb9\xe3\x83\x88.txt")},
+				{oid: "3a26c18b02e843b459732e7ade7ab9a154a1002b", objectName: []byte("\xe3\x83\x86\xe3\x82\xb9\xe3\x83\x88.xls")},
+			},
+		},
+		{
+			desc:         "tree with tag object type filter",
+			precondition: needsObjectTypeFilters,
+			revisions: []string{
+				"--all",
+			},
+			options: []revlistOption{
+				withObjectTypeFilter(objectTypeTag),
+			},
+			expectedResults: []revlistResult{
+				{oid: "f4e6814c3e4e7a0de82a9e7cd20c626cc963a2f8", objectName: []byte("v1.0.0")},
+				{oid: "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b", objectName: []byte("v1.1.0")},
+				{oid: "8f03acbcd11c53d9c9468078f32a2622005a4841", objectName: []byte("v1.1.1")},
+			},
+		},
+		{
+			desc:         "tree with commit object type filter",
+			precondition: needsObjectTypeFilters,
+			revisions: []string{
+				"79d5f98270ad677c86a7e1ab2baa922958565135",
+			},
+			options: []revlistOption{
+				withObjectTypeFilter(objectTypeTree),
+			},
+			expectedResults: []revlistResult{
+				{oid: "79d5f98270ad677c86a7e1ab2baa922958565135"},
+			},
+		},
+		{
+			desc:         "tree with commit object type filter",
+			precondition: needsObjectTypeFilters,
+			revisions: []string{
+				"^refs/heads/master~",
+				"refs/heads/master",
+			},
+			options: []revlistOption{
+				withObjectTypeFilter(objectTypeCommit),
+			},
+			expectedResults: []revlistResult{
+				{oid: "1e292f8fedd741b75372e19097c76d327140c312"},
+				{oid: "c1c67abbaf91f624347bb3ae96eabe3a1b742478"},
+			},
+		},
+		{
+			desc:         "tree with object type and blob size filter",
+			precondition: needsObjectTypeFilters,
+			revisions: []string{
+				"79d5f98270ad677c86a7e1ab2baa922958565135",
+			},
+			options: []revlistOption{
+				withBlobLimit(10),
+				withObjectTypeFilter(objectTypeBlob),
+			},
+			expectedResults: []revlistResult{
+				{oid: "0fb47f093f769008049a0b0976ac3fa6d6125033", objectName: []byte("hotfix-1.txt")},
+				{oid: "4ae6c5e14452a35d04156277ae63e8356eb17cae", objectName: []byte("hotfix-2.txt")},
+				{oid: "b988ffed90cb6a9b7f98a3686a933edb3c5d70c0", objectName: []byte("iso8859.txt")},
+			},
+		},
+		{
 			desc: "invalid revision",
 			revisions: []string{
 				"refs/heads/does-not-exist",
@@ -151,6 +245,10 @@ func TestRevlist(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
+			if tc.precondition != nil {
+				tc.precondition(t)
+			}
+
 			ctx, cancel := testhelper.Context()
 			defer cancel()