Merge branch 'pks-get-tree-entries-invalid-revisions' into 'master'

commit: Do not raise error when listing tree entries for nonexistent ref

See merge request gitlab-org/gitaly!4097

6 files changed, 97 insertions, 88 deletions

diff --git a/internal/gitaly/service/commit/tree_entries.go b/internal/gitaly/service/commit/tree_entries.go
index 98eb0b7f9..897d93f05 100644
--- a/internal/gitaly/service/commit/tree_entries.go
+++ b/internal/gitaly/service/commit/tree_entries.go
@@ -108,6 +108,11 @@ func (s *server) sendTreeEntries(
 				return nil
 			}
 
+			// Same if we try to list tree entries of a revision which doesn't exist.
+			if errors.Is(err, lstree.ErrNotExist) {
+				return nil
+			}
+
 			return fmt.Errorf("listing tree entries: %w", err)
 		}
 
diff --git a/internal/gitaly/service/commit/tree_entries_test.go b/internal/gitaly/service/commit/tree_entries_test.go
index 1c952d68c..b5371dbeb 100644
--- a/internal/gitaly/service/commit/tree_entries_test.go
+++ b/internal/gitaly/service/commit/tree_entries_test.go
@@ -19,7 +19,11 @@ import (
 	"google.golang.org/grpc/codes"
 )
 
-func TestSuccessfulGetTreeEntriesWithCurlyBraces(t *testing.T) {
+func TestGetTreeEntries_curlyBraces(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Run(t, testGetTreeEntriesCurlyBraces)
+}
+
+func testGetTreeEntriesCurlyBraces(t *testing.T, ctx context.Context) {
 	t.Parallel()
 	cfg, repo, repoPath, client := setupCommitServiceWithRepo(t, false)
 
@@ -67,8 +71,6 @@ func TestSuccessfulGetTreeEntriesWithCurlyBraces(t *testing.T) {
 				Recursive:  testCase.recursive,
 			}
 
-			ctx, cancel := testhelper.Context()
-			defer cancel()
 			c, err := client.GetTreeEntries(ctx, request)
 			require.NoError(t, err)
 
@@ -79,7 +81,11 @@ func TestSuccessfulGetTreeEntriesWithCurlyBraces(t *testing.T) {
 	}
 }
 
-func TestSuccessfulGetTreeEntries(t *testing.T) {
+func TestGetTreeEntries_successful(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Run(t, testGetTreeEntriesSuccessful)
+}
+
+func testGetTreeEntriesSuccessful(t *testing.T, ctx context.Context) {
 	t.Parallel()
 	commitID := "d25b6d94034242f3930dfcfeb6d8d9aac3583992"
 	rootOid := "21bdc8af908562ae485ed46d71dd5426c08b084a"
@@ -372,6 +378,13 @@ func TestSuccessfulGetTreeEntries(t *testing.T) {
 			entries:     nil,
 		},
 		{
+			description: "with a non-existing path",
+			revision:    []byte(commitID),
+			path:        []byte("i-dont/exist"),
+			recursive:   true,
+			entries:     nil,
+		},
+		{
 			description: "with root path and sorted by trees first",
 			revision:    []byte(commitID),
 			path:        []byte("."),
@@ -447,8 +460,6 @@ func TestSuccessfulGetTreeEntries(t *testing.T) {
 				}
 			}
 
-			ctx, cancel := testhelper.Context()
-			defer cancel()
 			c, err := client.GetTreeEntries(ctx, request)
 
 			require.NoError(t, err)
@@ -463,7 +474,11 @@ func TestSuccessfulGetTreeEntries(t *testing.T) {
 	}
 }
 
-func TestUnsuccessfulGetTreeEntries(t *testing.T) {
+func TestGetTreeEntries_unsuccessful(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Run(t, testGetTreeEntriesUnsuccessful)
+}
+
+func testGetTreeEntriesUnsuccessful(t *testing.T, ctx context.Context) {
 	commitID := "d25b6d94034242f3930dfcfeb6d8d9aac3583992"
 
 	_, repo, _, client := setupCommitServiceWithRepo(t, true)
@@ -498,8 +513,6 @@ func TestUnsuccessfulGetTreeEntries(t *testing.T) {
 				}
 			}
 
-			ctx, cancel := testhelper.Context()
-			defer cancel()
 			c, err := client.GetTreeEntries(ctx, request)
 			require.NoError(t, err)
 
@@ -511,38 +524,11 @@ func TestUnsuccessfulGetTreeEntries(t *testing.T) {
 	}
 }
 
-func getTreeEntriesFromTreeEntryClient(t *testing.T, client gitalypb.CommitService_GetTreeEntriesClient, expectedError error) ([]*gitalypb.TreeEntry, *gitalypb.PaginationCursor) {
-	t.Helper()
-
-	var entries []*gitalypb.TreeEntry
-	var cursor *gitalypb.PaginationCursor
-	firstEntryReceived := false
-
-	for {
-		resp, err := client.Recv()
-		if err == io.EOF {
-			break
-		}
-
-		if expectedError == nil {
-			require.NoError(t, err)
-			entries = append(entries, resp.Entries...)
-
-			if !firstEntryReceived {
-				cursor = resp.PaginationCursor
-				firstEntryReceived = true
-			} else {
-				require.Equal(t, nil, resp.PaginationCursor)
-			}
-		} else {
-			require.Error(t, expectedError, err)
-			break
-		}
-	}
-	return entries, cursor
+func TestGetTreeEntries_deepFlatpath(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Run(t, testGetTreeEntriesDeepFlatpath)
 }
 
-func TestSuccessfulGetTreeEntries_FlatPathMaxDeep_SingleFoldersStructure(t *testing.T) {
+func testGetTreeEntriesDeepFlatpath(t *testing.T, ctx context.Context) {
 	t.Parallel()
 	cfg, repo, repoPath, client := setupCommitServiceWithRepo(t, false)
 
@@ -567,9 +553,6 @@ func TestSuccessfulGetTreeEntries_FlatPathMaxDeep_SingleFoldersStructure(t *test
 		Recursive:  false,
 	}
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	// request entries of the tree with single-folder structure on each level
 	entriesClient, err := client.GetTreeEntries(ctx, request)
 	require.NoError(t, err)
@@ -590,13 +573,14 @@ func TestSuccessfulGetTreeEntries_FlatPathMaxDeep_SingleFoldersStructure(t *test
 }
 
 func TestGetTreeEntries_file(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Run(t, testGetTreeEntriesFile)
+}
+
+func testGetTreeEntriesFile(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
 	cfg, repo, repoPath, client := setupCommitServiceWithRepo(t, true)
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	commitID := gittest.WriteCommit(t, cfg, repoPath,
 		gittest.WithTreeEntries(gittest.TreeEntry{
 			Mode:    "100644",
@@ -621,7 +605,11 @@ func TestGetTreeEntries_file(t *testing.T) {
 	require.Empty(t, entries)
 }
 
-func TestFailedGetTreeEntriesRequestDueToValidationError(t *testing.T) {
+func TestGetTreeEntries_validation(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Run(t, testGetTreeEntriesValidation)
+}
+
+func testGetTreeEntriesValidation(t *testing.T, ctx context.Context) {
 	t.Parallel()
 	_, repo, _, client := setupCommitServiceWithRepo(t, true)
 
@@ -638,8 +626,6 @@ func TestFailedGetTreeEntriesRequestDueToValidationError(t *testing.T) {
 
 	for _, rpcRequest := range rpcRequests {
 		t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
 			c, err := client.GetTreeEntries(ctx, rpcRequest)
 			require.NoError(t, err)
 
@@ -649,18 +635,8 @@ func TestFailedGetTreeEntriesRequestDueToValidationError(t *testing.T) {
 	}
 }
 
-func drainTreeEntriesResponse(c gitalypb.CommitService_GetTreeEntriesClient) error {
-	var err error
-	for err == nil {
-		_, err = c.Recv()
-	}
-	return err
-}
-
 func BenchmarkGetTreeEntries(b *testing.B) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TreeEntriesViaLsTree,
-	}).Bench(b, benchmarkGetTreeEntries)
+	testhelper.NewFeatureSets(featureflag.TreeEntriesViaLsTree).Bench(b, benchmarkGetTreeEntries)
 }
 
 func benchmarkGetTreeEntries(b *testing.B, ctx context.Context) {
@@ -730,3 +706,42 @@ func benchmarkGetTreeEntries(b *testing.B, ctx context.Context) {
 		})
 	}
 }
+
+func getTreeEntriesFromTreeEntryClient(t *testing.T, client gitalypb.CommitService_GetTreeEntriesClient, expectedError error) ([]*gitalypb.TreeEntry, *gitalypb.PaginationCursor) {
+	t.Helper()
+
+	var entries []*gitalypb.TreeEntry
+	var cursor *gitalypb.PaginationCursor
+	firstEntryReceived := false
+
+	for {
+		resp, err := client.Recv()
+		if err == io.EOF {
+			break
+		}
+
+		if expectedError == nil {
+			require.NoError(t, err)
+			entries = append(entries, resp.Entries...)
+
+			if !firstEntryReceived {
+				cursor = resp.PaginationCursor
+				firstEntryReceived = true
+			} else {
+				require.Equal(t, nil, resp.PaginationCursor)
+			}
+		} else {
+			require.Error(t, expectedError, err)
+			break
+		}
+	}
+	return entries, cursor
+}
+
+func drainTreeEntriesResponse(c gitalypb.CommitService_GetTreeEntriesClient) error {
+	var err error
+	for err == nil {
+		_, err = c.Recv()
+	}
+	return err
+}
diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go
index 02e1f74ef..001afaddc 100644
--- a/internal/gitaly/service/ref/refs_test.go
+++ b/internal/gitaly/service/ref/refs_test.go
@@ -355,9 +355,7 @@ func TestSuccessfulFindLocalBranches(t *testing.T) {
 }
 
 func TestFindLocalBranches_huge_committer(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ExactPaginationTokenMatch,
-	}).Run(t, testFindLocalBranchesHugeCommitter)
+	testhelper.NewFeatureSets(featureflag.ExactPaginationTokenMatch).Run(t, testFindLocalBranchesHugeCommitter)
 }
 
 func testFindLocalBranchesHugeCommitter(t *testing.T, ctx context.Context) {
@@ -383,9 +381,7 @@ func testFindLocalBranchesHugeCommitter(t *testing.T, ctx context.Context) {
 }
 
 func TestFindLocalBranchesPagination(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ExactPaginationTokenMatch,
-	}).Run(t, testFindLocalBranchesPagination)
+	testhelper.NewFeatureSets(featureflag.ExactPaginationTokenMatch).Run(t, testFindLocalBranchesPagination)
 }
 
 func testFindLocalBranchesPagination(t *testing.T, ctx context.Context) {
@@ -437,9 +433,7 @@ func testFindLocalBranchesPagination(t *testing.T, ctx context.Context) {
 }
 
 func TestFindLocalBranchesPaginationSequence(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ExactPaginationTokenMatch,
-	}).Run(t, testFindLocalBranchesPaginationSequence)
+	testhelper.NewFeatureSets(featureflag.ExactPaginationTokenMatch).Run(t, testFindLocalBranchesPaginationSequence)
 }
 
 func testFindLocalBranchesPaginationSequence(t *testing.T, ctx context.Context) {
@@ -492,9 +486,7 @@ func testFindLocalBranchesPaginationSequence(t *testing.T, ctx context.Context)
 }
 
 func TestFindLocalBranchesPaginationWithIncorrectToken(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ExactPaginationTokenMatch,
-	}).Run(t, testFindLocalBranchesPaginationWithIncorrectToken)
+	testhelper.NewFeatureSets(featureflag.ExactPaginationTokenMatch).Run(t, testFindLocalBranchesPaginationWithIncorrectToken)
 }
 
 func testFindLocalBranchesPaginationWithIncorrectToken(t *testing.T, ctx context.Context) {
@@ -570,9 +562,7 @@ func isOrderedSubset(subset, set []string) bool {
 }
 
 func TestFindLocalBranchesSort(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ExactPaginationTokenMatch,
-	}).Run(t, testFindLocalBranchesSort)
+	testhelper.NewFeatureSets(featureflag.ExactPaginationTokenMatch).Run(t, testFindLocalBranchesSort)
 }
 
 func testFindLocalBranchesSort(t *testing.T, ctx context.Context) {
diff --git a/internal/gitaly/service/repository/license_test.go b/internal/gitaly/service/repository/license_test.go
index e784b9cb5..49dfe1c91 100644
--- a/internal/gitaly/service/repository/license_test.go
+++ b/internal/gitaly/service/repository/license_test.go
@@ -16,9 +16,7 @@ import (
 )
 
 func testSuccessfulFindLicenseRequest(t *testing.T, cfg config.Cfg, client gitalypb.RepositoryServiceClient, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoFindLicense,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
+	testhelper.NewFeatureSets(featureflag.GoFindLicense).Run(t, func(t *testing.T, ctx context.Context) {
 		for _, tc := range []struct {
 			desc                  string
 			nonExistentRepository bool
@@ -108,9 +106,7 @@ SOFTWARE.`,
 }
 
 func testFindLicenseRequestEmptyRepo(t *testing.T, cfg config.Cfg, client gitalypb.RepositoryServiceClient, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoFindLicense,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
+	testhelper.NewFeatureSets(featureflag.GoFindLicense).Run(t, func(t *testing.T, ctx context.Context) {
 		repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
 		require.NoError(t, os.RemoveAll(repoPath))
 
diff --git a/internal/testhelper/featureset.go b/internal/testhelper/featureset.go
index cb89a36af..06b794fa8 100644
--- a/internal/testhelper/featureset.go
+++ b/internal/testhelper/featureset.go
@@ -54,9 +54,14 @@ func (f FeatureSet) Disable(ctx context.Context) context.Context {
 // FeatureSets is a slice containing many FeatureSets
 type FeatureSets []FeatureSet
 
-// NewFeatureSets takes a slice of go feature flags, and an optional variadic set of ruby feature flags
-// and returns a FeatureSets slice
-func NewFeatureSets(goFeatures []featureflag.FeatureFlag, rubyFeatures ...featureflag.FeatureFlag) FeatureSets {
+// NewFeatureSets takes Go feature flags and returns the combination of FeatureSets.
+func NewFeatureSets(features ...featureflag.FeatureFlag) FeatureSets {
+	return NewFeatureSetsWithRubyFlags(features, nil)
+}
+
+// NewFeatureSetsWithRubyFlags takes a Go- and Ruby-specific feature flags and returns a the
+// combination of FeatureSets.
+func NewFeatureSetsWithRubyFlags(goFeatures []featureflag.FeatureFlag, rubyFeatures []featureflag.FeatureFlag) FeatureSets {
 	var sets FeatureSets
 
 	length := len(goFeatures) + len(rubyFeatures)
diff --git a/internal/testhelper/featureset_test.go b/internal/testhelper/featureset_test.go
index 2d1872203..da67acfae 100644
--- a/internal/testhelper/featureset_test.go
+++ b/internal/testhelper/featureset_test.go
@@ -23,7 +23,7 @@ func features(flag ...ff.FeatureFlag) map[ff.FeatureFlag]struct{} {
 	return features
 }
 
-func TestNewFeatureSets(t *testing.T) {
+func TestNewFeatureSetsWithRubyFlags(t *testing.T) {
 	testcases := []struct {
 		desc         string
 		features     []ff.FeatureFlag
@@ -129,7 +129,7 @@ func TestNewFeatureSets(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			featureSets := NewFeatureSets(tc.features, tc.rubyFeatures...)
+			featureSets := NewFeatureSetsWithRubyFlags(tc.features, tc.rubyFeatures)
 			require.Len(t, featureSets, len(tc.expected))
 			for _, expected := range tc.expected {
 				require.Contains(t, featureSets, expected)
@@ -152,9 +152,7 @@ func TestFeatureSets_Run(t *testing.T) {
 	}(ff.All)
 	ff.All = append(ff.All, featureFlagA, featureFlagB)
 
-	NewFeatureSets([]ff.FeatureFlag{
-		featureFlagB, featureFlagA,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
+	NewFeatureSets(featureFlagB, featureFlagA).Run(t, func(t *testing.T, ctx context.Context) {
 		incomingMD, ok := grpc_metadata.FromIncomingContext(ctx)
 		require.True(t, ok)