Merge branch '4452-feature-flag-roll-out-simplify_find_local_branches_response' into 'master'

featureflag: Remove SimplifyFindLocalBranchesResponse

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4946

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>

4 files changed, 72 insertions, 258 deletions

diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go
index 4825363fe..afd88b05e 100644
--- a/internal/gitaly/service/ref/refs_test.go
+++ b/internal/gitaly/service/ref/refs_test.go
@@ -5,7 +5,6 @@ package ref
 import (
 	"bufio"
 	"bytes"
-	"context"
 	"fmt"
 	"io"
 	"strings"
@@ -17,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -255,76 +253,38 @@ func TestInvalidRepoFindDefaultBranchNameRequest(t *testing.T) {
 
 func TestSuccessfulFindLocalBranches(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testSuccessfulFindLocalBranches)
-}
+	ctx := testhelper.Context(t)
 
-func testSuccessfulFindLocalBranches(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setupRefService(t, ctx)
 
 	rpcRequest := &gitalypb.FindLocalBranchesRequest{Repository: repo}
 	c, err := client.FindLocalBranches(ctx, rpcRequest)
 	require.NoError(t, err)
 
-	if featureflag.SimplifyFindLocalBranchesResponse.IsEnabled(ctx) {
-		var branches []*gitalypb.Branch
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			branches = append(branches, r.GetLocalBranches()...)
-		}
-
-		for name, target := range localBranches {
-			localBranch := &gitalypb.Branch{
-				Name:         []byte(name),
-				TargetCommit: target,
-			}
-
-			assertContainsBranch(t, branches, localBranch)
+	var branches []*gitalypb.Branch
+	for {
+		r, err := c.Recv()
+		if err == io.EOF {
+			break
 		}
+		require.NoError(t, err)
+		branches = append(branches, r.GetLocalBranches()...)
+	}
 
-	} else {
-		var branches []*gitalypb.FindLocalBranchResponse
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			branches = append(branches, r.GetBranches()...)
+	for name, target := range localBranches {
+		localBranch := &gitalypb.Branch{
+			Name:         []byte(name),
+			TargetCommit: target,
 		}
 
-		for name, target := range localBranches {
-			localBranch := &gitalypb.FindLocalBranchResponse{
-				Name:          []byte(name),
-				CommitId:      target.Id,
-				CommitSubject: target.Subject,
-				CommitAuthor: &gitalypb.FindLocalBranchCommitAuthor{
-					Name:  target.Author.Name,
-					Email: target.Author.Email,
-					Date:  target.Author.Date,
-				},
-				CommitCommitter: &gitalypb.FindLocalBranchCommitAuthor{
-					Name:  target.Committer.Name,
-					Email: target.Committer.Email,
-					Date:  target.Committer.Date,
-				},
-				Commit: target,
-			}
-
-			assertContainsLocalBranch(t, branches, localBranch)
-		}
+		assertContainsBranch(t, branches, localBranch)
 	}
 }
 
 func TestFindLocalBranchesHugeCommitter(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testFindLocalBranchesHugeCommitter)
-}
+	ctx := testhelper.Context(t)
 
-func testFindLocalBranchesHugeCommitter(t *testing.T, ctx context.Context) {
 	cfg, repo, repoPath, client := setupRefService(t, ctx)
 
 	gittest.WriteCommit(t, cfg, repoPath,
@@ -348,10 +308,8 @@ func testFindLocalBranchesHugeCommitter(t *testing.T, ctx context.Context) {
 
 func TestFindLocalBranchesPagination(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testFindLocalBranchesPagination)
-}
+	ctx := testhelper.Context(t)
 
-func testFindLocalBranchesPagination(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setupRefService(t, ctx)
 
 	limit := 1
@@ -368,63 +326,29 @@ func testFindLocalBranchesPagination(t *testing.T, ctx context.Context) {
 	expectedBranch := "refs/heads/improve/awesome"
 	target := localBranches[expectedBranch]
 
-	if featureflag.SimplifyFindLocalBranchesResponse.IsEnabled(ctx) {
-		var branches []*gitalypb.Branch
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			branches = append(branches, r.GetLocalBranches()...)
-		}
-
-		require.Len(t, branches, limit)
-
-		branch := &gitalypb.Branch{
-			Name:         []byte(expectedBranch),
-			TargetCommit: target,
-		}
-		assertContainsBranch(t, branches, branch)
-	} else {
-		var branches []*gitalypb.FindLocalBranchResponse
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			branches = append(branches, r.GetBranches()...)
+	var branches []*gitalypb.Branch
+	for {
+		r, err := c.Recv()
+		if err == io.EOF {
+			break
 		}
+		require.NoError(t, err)
+		branches = append(branches, r.GetLocalBranches()...)
+	}
 
-		require.Len(t, branches, limit)
+	require.Len(t, branches, limit)
 
-		branch := &gitalypb.FindLocalBranchResponse{
-			Name:          []byte(expectedBranch),
-			CommitId:      target.Id,
-			CommitSubject: target.Subject,
-			CommitAuthor: &gitalypb.FindLocalBranchCommitAuthor{
-				Name:  target.Author.Name,
-				Email: target.Author.Email,
-				Date:  target.Author.Date,
-			},
-			CommitCommitter: &gitalypb.FindLocalBranchCommitAuthor{
-				Name:  target.Committer.Name,
-				Email: target.Committer.Email,
-				Date:  target.Committer.Date,
-			},
-			Commit: target,
-		}
-		assertContainsLocalBranch(t, branches, branch)
+	branch := &gitalypb.Branch{
+		Name:         []byte(expectedBranch),
+		TargetCommit: target,
 	}
+	assertContainsBranch(t, branches, branch)
 }
 
 func TestFindLocalBranchesPaginationSequence(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testFindLocalBranchesPaginationSequence)
-}
+	ctx := testhelper.Context(t)
 
-func testFindLocalBranchesPaginationSequence(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setupRefService(t, ctx)
 
 	limit := 2
@@ -437,85 +361,46 @@ func testFindLocalBranchesPaginationSequence(t *testing.T, ctx context.Context)
 	c, err := client.FindLocalBranches(ctx, firstRPCRequest)
 	require.NoError(t, err)
 
-	if featureflag.SimplifyFindLocalBranchesResponse.IsEnabled(ctx) {
-		var firstResponseBranches []*gitalypb.Branch
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			firstResponseBranches = append(firstResponseBranches, r.GetLocalBranches()...)
-		}
-
-		require.Len(t, firstResponseBranches, limit)
-
-		secondRPCRequest := &gitalypb.FindLocalBranchesRequest{
-			Repository: repo,
-			PaginationParams: &gitalypb.PaginationParameter{
-				Limit:     1,
-				PageToken: string(firstResponseBranches[0].Name),
-			},
+	var firstResponseBranches []*gitalypb.Branch
+	for {
+		r, err := c.Recv()
+		if err == io.EOF {
+			break
 		}
-		c, err = client.FindLocalBranches(ctx, secondRPCRequest)
 		require.NoError(t, err)
+		firstResponseBranches = append(firstResponseBranches, r.GetLocalBranches()...)
+	}
 
-		var secondResponseBranches []*gitalypb.Branch
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			secondResponseBranches = append(secondResponseBranches, r.GetLocalBranches()...)
-		}
-
-		require.Len(t, secondResponseBranches, 1)
-		require.Equal(t, firstResponseBranches[1], secondResponseBranches[0])
-	} else {
-		var firstResponseBranches []*gitalypb.FindLocalBranchResponse
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			firstResponseBranches = append(firstResponseBranches, r.GetBranches()...)
-		}
+	require.Len(t, firstResponseBranches, limit)
 
-		require.Len(t, firstResponseBranches, limit)
+	secondRPCRequest := &gitalypb.FindLocalBranchesRequest{
+		Repository: repo,
+		PaginationParams: &gitalypb.PaginationParameter{
+			Limit:     1,
+			PageToken: string(firstResponseBranches[0].Name),
+		},
+	}
+	c, err = client.FindLocalBranches(ctx, secondRPCRequest)
+	require.NoError(t, err)
 
-		secondRPCRequest := &gitalypb.FindLocalBranchesRequest{
-			Repository: repo,
-			PaginationParams: &gitalypb.PaginationParameter{
-				Limit:     1,
-				PageToken: string(firstResponseBranches[0].Name),
-			},
+	var secondResponseBranches []*gitalypb.Branch
+	for {
+		r, err := c.Recv()
+		if err == io.EOF {
+			break
 		}
-		c, err = client.FindLocalBranches(ctx, secondRPCRequest)
 		require.NoError(t, err)
-
-		var secondResponseBranches []*gitalypb.FindLocalBranchResponse
-		for {
-			r, err := c.Recv()
-			if err == io.EOF {
-				break
-			}
-			require.NoError(t, err)
-			secondResponseBranches = append(secondResponseBranches, r.GetBranches()...)
-		}
-
-		require.Len(t, secondResponseBranches, 1)
-		require.Equal(t, firstResponseBranches[1], secondResponseBranches[0])
+		secondResponseBranches = append(secondResponseBranches, r.GetLocalBranches()...)
 	}
+
+	require.Len(t, secondResponseBranches, 1)
+	require.Equal(t, firstResponseBranches[1], secondResponseBranches[0])
 }
 
 func TestFindLocalBranchesPaginationWithIncorrectToken(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testFindLocalBranchesPaginationWithIncorrectToken)
-}
+	ctx := testhelper.Context(t)
 
-func testFindLocalBranchesPaginationWithIncorrectToken(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setupRefService(t, ctx)
 
 	limit := 1
@@ -554,10 +439,8 @@ func isOrderedSubset(subset, set []string) bool {
 
 func TestFindLocalBranchesSort(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testFindLocalBranchesSort)
-}
+	ctx := testhelper.Context(t)
 
-func testFindLocalBranchesSort(t *testing.T, ctx context.Context) {
 	testCases := []struct {
 		desc          string
 		relativeOrder []string
@@ -597,16 +480,9 @@ func testFindLocalBranchesSort(t *testing.T, ctx context.Context) {
 				}
 				require.NoError(t, err)
 
-				if featureflag.SimplifyFindLocalBranchesResponse.IsEnabled(ctx) {
-					for _, branch := range r.GetLocalBranches() {
-						branches = append(branches, string(branch.Name))
-					}
-				} else {
-					for _, branch := range r.GetBranches() {
-						branches = append(branches, string(branch.Name))
-					}
+				for _, branch := range r.GetLocalBranches() {
+					branches = append(branches, string(branch.Name))
 				}
-
 			}
 
 			if !isOrderedSubset(testCase.relativeOrder, branches) {
@@ -618,10 +494,8 @@ func testFindLocalBranchesSort(t *testing.T, ctx context.Context) {
 
 func TestEmptyFindLocalBranchesRequest(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SimplifyFindLocalBranchesResponse).Run(t, testEmptyFindLocalBranchesRequest)
-}
+	ctx := testhelper.Context(t)
 
-func testEmptyFindLocalBranchesRequest(t *testing.T, ctx context.Context) {
 	_, client := setupRefServiceWithoutRepo(t)
 
 	rpcRequest := &gitalypb.FindLocalBranchesRequest{}
diff --git a/internal/gitaly/service/ref/testhelper_test.go b/internal/gitaly/service/ref/testhelper_test.go
index 2f66789cf..dda2730ba 100644
--- a/internal/gitaly/service/ref/testhelper_test.go
+++ b/internal/gitaly/service/ref/testhelper_test.go
@@ -92,35 +92,6 @@ func newRefServiceClient(tb testing.TB, serverSocketPath string) (gitalypb.RefSe
 	return gitalypb.NewRefServiceClient(conn), conn
 }
 
-func assertContainsLocalBranch(t *testing.T, branches []*gitalypb.FindLocalBranchResponse, branch *gitalypb.FindLocalBranchResponse) {
-	t.Helper()
-
-	for _, b := range branches {
-		if bytes.Equal(branch.Name, b.Name) {
-			if !findLocalBranchResponsesEqual(branch, b) {
-				t.Errorf("Expected branch\n%v\ngot\n%v", branch, b)
-			}
-
-			testhelper.ProtoEqual(t, branch.Commit, b.Commit)
-			return // Found the branch and it matches. Success!
-		}
-	}
-	t.Errorf("Expected to find branch %q in local branches", branch.Name)
-}
-
-func findLocalBranchCommitAuthorsEqual(a *gitalypb.FindLocalBranchCommitAuthor, b *gitalypb.FindLocalBranchCommitAuthor) bool {
-	return bytes.Equal(a.Name, b.Name) &&
-		bytes.Equal(a.Email, b.Email) &&
-		a.Date.Seconds == b.Date.Seconds
-}
-
-func findLocalBranchResponsesEqual(a *gitalypb.FindLocalBranchResponse, b *gitalypb.FindLocalBranchResponse) bool {
-	return a.CommitId == b.CommitId &&
-		bytes.Equal(a.CommitSubject, b.CommitSubject) &&
-		findLocalBranchCommitAuthorsEqual(a.CommitAuthor, b.CommitAuthor) &&
-		findLocalBranchCommitAuthorsEqual(a.CommitCommitter, b.CommitCommitter)
-}
-
 func assertContainsAllBranchesResponseBranch(t *testing.T, branches []*gitalypb.FindAllBranchesResponse_Branch, branch *gitalypb.FindAllBranchesResponse_Branch) {
 	t.Helper()
 
diff --git a/internal/gitaly/service/ref/util.go b/internal/gitaly/service/ref/util.go
index 309a5ad77..ebeb48a68 100644
--- a/internal/gitaly/service/ref/util.go
+++ b/internal/gitaly/service/ref/util.go
@@ -8,7 +8,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/lines"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
@@ -88,44 +87,24 @@ func newFindLocalBranchesWriter(stream gitalypb.RefService_FindLocalBranchesServ
 		ctx := stream.Context()
 		var response *gitalypb.FindLocalBranchesResponse
 
-		if featureflag.SimplifyFindLocalBranchesResponse.IsEnabled(ctx) {
-			var branches []*gitalypb.Branch
-
-			for _, ref := range refs {
-				elements, err := parseRef(ref)
-				if err != nil {
-					return err
-				}
-
-				branch, err := buildBranch(ctx, objectReader, elements)
-				if err != nil {
-					return err
-				}
+		var branches []*gitalypb.Branch
 
-				branches = append(branches, branch)
+		for _, ref := range refs {
+			elements, err := parseRef(ref)
+			if err != nil {
+				return err
 			}
 
-			response = &gitalypb.FindLocalBranchesResponse{LocalBranches: branches}
-		} else {
-			var branches []*gitalypb.FindLocalBranchResponse
-
-			for _, ref := range refs {
-				elements, err := parseRef(ref)
-				if err != nil {
-					return err
-				}
-
-				target, err := catfile.GetCommit(ctx, objectReader, git.Revision(elements[1]))
-				if err != nil {
-					return err
-				}
-
-				branches = append(branches, buildLocalBranch(elements[0], target))
+			branch, err := buildBranch(ctx, objectReader, elements)
+			if err != nil {
+				return err
 			}
 
-			response = &gitalypb.FindLocalBranchesResponse{Branches: branches}
+			branches = append(branches, branch)
 		}
 
+		response = &gitalypb.FindLocalBranchesResponse{LocalBranches: branches}
+
 		return stream.Send(response)
 	}
 }
diff --git a/internal/metadata/featureflag/ff_simplify_find_local_branches_response.go b/internal/metadata/featureflag/ff_simplify_find_local_branches_response.go
deleted file mode 100644
index 04dfc9f3b..000000000
--- a/internal/metadata/featureflag/ff_simplify_find_local_branches_response.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// SimplifyFindLocalBranchesResponse enables the simplification of FindLocalBranchesRespnose
-// by adding the generic Branch type in the response
-var SimplifyFindLocalBranchesResponse = NewFeatureFlag(
-	"simplify_find_local_branches_response",
-	"v15.4.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/1294",
-	false,
-)