Merge branch 'go_user_ff_branch' into 'master'

Port UserFFBranch to go

See merge request gitlab-org/gitaly!2584

3 files changed, 147 insertions, 52 deletions

diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index c6a94f3c1..466cbf5e0 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -8,6 +8,7 @@ import (
 	"io"
 	"strings"
 
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/internal/git2go"
@@ -281,6 +282,64 @@ func (s *server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return nil, helper.ErrInvalidArgument(err)
 	}
 
+	if featureflag.IsDisabled(ctx, featureflag.GoUserFFBranch) {
+		return s.userFFBranchRuby(ctx, in)
+	}
+
+	revision, err := parseRevision(ctx, in.Repository, string(in.Branch))
+	if err != nil {
+		return nil, helper.ErrInvalidArgument(err)
+	}
+
+	cmd, err := git.SafeCmd(ctx, in.Repository, nil, git.SubCmd{
+		Name:  "merge-base",
+		Flags: []git.Option{git.Flag{Name: "--is-ancestor"}},
+		Args:  []string{revision, in.CommitId},
+	})
+	if err != nil {
+		return nil, helper.ErrInternal(err)
+	}
+	if err := cmd.Wait(); err != nil {
+		status, ok := command.ExitStatus(err)
+		if !ok {
+			return nil, helper.ErrInternal(err)
+		}
+		// --is-ancestor errors are signaled by a non-zero status that is not 1.
+		// https://git-scm.com/docs/git-merge-base#Documentation/git-merge-base.txt---is-ancestor
+		if status != 1 {
+			return nil, helper.ErrInvalidArgument(err)
+		}
+		return nil, helper.ErrPreconditionFailedf("not fast forward")
+	}
+
+	branch := fmt.Sprintf("refs/heads/%s", in.Branch)
+	if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, branch, in.CommitId, revision); err != nil {
+		var preReceiveError preReceiveError
+		if errors.As(err, &preReceiveError) {
+			return &gitalypb.UserFFBranchResponse{
+				PreReceiveError: preReceiveError.message,
+			}, nil
+		}
+
+		var updateRefError updateRefError
+		if errors.As(err, &updateRefError) {
+			// When an error happens updating the reference, e.g. because of a race
+			// with another update, then Ruby code didn't send an error but just an
+			// empty response.
+			return &gitalypb.UserFFBranchResponse{}, nil
+		}
+
+		return nil, err
+	}
+
+	return &gitalypb.UserFFBranchResponse{
+		BranchUpdate: &gitalypb.OperationBranchUpdate{
+			CommitId: in.CommitId,
+		},
+	}, nil
+}
+
+func (s *server) userFFBranchRuby(ctx context.Context, in *gitalypb.UserFFBranchRequest) (*gitalypb.UserFFBranchResponse, error) {
 	client, err := s.ruby.OperationServiceClient(ctx)
 	if err != nil {
 		return nil, err
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 3b4ef9e12..feeb9a408 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -301,42 +301,53 @@ func testFailedMergeDueToHooks(t *testing.T, ctx context.Context) {
 }
 
 func TestSuccessfulUserFFBranchRequest(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserFFBranch,
+	})
+	require.NoError(t, err)
 
-	serverSocketPath, stop := runOperationServiceServer(t)
-	defer stop()
+	for _, featureSet := range featureSets {
+		t.Run("disabled "+featureSet.String(), func(t *testing.T) {
+			ctx, cancel := testhelper.Context()
+			defer cancel()
 
-	client, conn := newOperationClient(t, serverSocketPath)
-	defer conn.Close()
+			ctx = featureSet.Disable(ctx)
 
-	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
-	defer cleanupFn()
+			serverSocketPath, stop := runOperationServiceServer(t)
+			defer stop()
 
-	commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
-	branchName := "test-ff-target-branch"
-	request := &gitalypb.UserFFBranchRequest{
-		Repository: testRepo,
-		CommitId:   commitID,
-		Branch:     []byte(branchName),
-		User:       testhelper.TestUser,
-	}
-	expectedResponse := &gitalypb.UserFFBranchResponse{
-		BranchUpdate: &gitalypb.OperationBranchUpdate{
-			RepoCreated:   false,
-			BranchCreated: false,
-			CommitId:      commitID,
-		},
-	}
+			client, conn := newOperationClient(t, serverSocketPath)
+			defer conn.Close()
 
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchName).Run()
+			testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+			defer cleanupFn()
 
-	resp, err := client.UserFFBranch(ctx, request)
-	require.NoError(t, err)
-	testhelper.ProtoEqual(t, expectedResponse, resp)
-	newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", branchName)
-	require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
+			commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
+			branchName := "test-ff-target-branch"
+			request := &gitalypb.UserFFBranchRequest{
+				Repository: testRepo,
+				CommitId:   commitID,
+				Branch:     []byte(branchName),
+				User:       testhelper.TestUser,
+			}
+			expectedResponse := &gitalypb.UserFFBranchResponse{
+				BranchUpdate: &gitalypb.OperationBranchUpdate{
+					RepoCreated:   false,
+					BranchCreated: false,
+					CommitId:      commitID,
+				},
+			}
+
+			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
+			defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchName).Run()
+
+			resp, err := client.UserFFBranch(ctx, request)
+			require.NoError(t, err)
+			testhelper.ProtoEqual(t, expectedResponse, resp)
+			newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", branchName)
+			require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
+		})
+	}
 }
 
 func TestFailedUserFFBranchRequest(t *testing.T) {
@@ -355,6 +366,11 @@ func TestFailedUserFFBranchRequest(t *testing.T) {
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
 	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchName).Run()
 
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserFFBranch,
+	})
+	require.NoError(t, err)
+
 	testCases := []struct {
 		desc     string
 		user     *gitalypb.User
@@ -417,19 +433,25 @@ func TestFailedUserFFBranchRequest(t *testing.T) {
 		},
 	}
 
-	for _, testCase := range testCases {
-		t.Run(testCase.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
-			request := &gitalypb.UserFFBranchRequest{
-				Repository: testCase.repo,
-				User:       testCase.user,
-				Branch:     testCase.branch,
-				CommitId:   testCase.commitID,
+	for _, featureSet := range featureSets {
+		t.Run("disabled "+featureSet.String(), func(t *testing.T) {
+			for _, testCase := range testCases {
+				t.Run(testCase.desc, func(t *testing.T) {
+					ctx, cancel := testhelper.Context()
+					defer cancel()
+
+					ctx = featureSet.Disable(ctx)
+
+					request := &gitalypb.UserFFBranchRequest{
+						Repository: testCase.repo,
+						User:       testCase.user,
+						Branch:     testCase.branch,
+						CommitId:   testCase.commitID,
+					}
+					_, err := client.UserFFBranch(ctx, request)
+					testhelper.RequireGrpcError(t, err, testCase.code)
+				})
 			}
-			_, err := client.UserFFBranch(ctx, request)
-			testhelper.RequireGrpcError(t, err, testCase.code)
 		})
 	}
 }
@@ -458,18 +480,29 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) {
 
 	hookContent := []byte("#!/bin/sh\necho 'failure'\nexit 1")
 
-	for _, hookName := range gitlabPreHooks {
-		t.Run(hookName, func(t *testing.T) {
-			remove, err := testhelper.WriteCustomHook(testRepoPath, hookName, hookContent)
-			require.NoError(t, err)
-			defer remove()
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserFFBranch,
+	})
+	require.NoError(t, err)
 
-			ctx, cancel := testhelper.Context()
-			defer cancel()
+	for _, featureSet := range featureSets {
+		t.Run("disabled "+featureSet.String(), func(t *testing.T) {
+			for _, hookName := range gitlabPreHooks {
+				t.Run(hookName, func(t *testing.T) {
+					remove, err := testhelper.WriteCustomHook(testRepoPath, hookName, hookContent)
+					require.NoError(t, err)
+					defer remove()
 
-			resp, err := client.UserFFBranch(ctx, request)
-			require.Nil(t, err)
-			require.Contains(t, resp.PreReceiveError, "failure")
+					ctx, cancel := testhelper.Context()
+					defer cancel()
+
+					ctx = featureSet.Disable(ctx)
+
+					resp, err := client.UserFFBranch(ctx, request)
+					require.Nil(t, err)
+					require.Contains(t, resp.PreReceiveError, "failure")
+				})
+			}
 		})
 	}
 }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 43b34c8a4..19c32ca43 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -38,6 +38,8 @@ var (
 	GoUserMergeBranch = FeatureFlag{Name: "go_user_merge_branch", OnByDefault: false}
 	// GoUserMergeToRef enable the Go implementation of UserMergeToRef
 	GoUserMergeToRef = FeatureFlag{Name: "go_user_merge_to_ref", OnByDefault: false}
+	// GoUserFFBranch enables the Go implementation of GoUserFFBranch
+	GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: false}
 )
 
 // All includes all feature flags.
@@ -54,6 +56,7 @@ var All = []FeatureFlag{
 	RubyReferenceTransactionHook,
 	GoUserMergeBranch,
 	GoUserMergeToRef,
+	GoUserFFBranch,
 }
 
 const (