Merge branch 'cc-port-userdeletebranch-to-go' into 'master'

Port UserDeleteBranch to Go

See merge request gitlab-org/gitaly!2609

3 files changed, 89 insertions, 20 deletions

diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index 981f6075a..f3bed19bb 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -2,8 +2,13 @@ package operations
 
 import (
 	"context"
+	"errors"
+	"fmt"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -46,6 +51,42 @@ func (s *server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB
 		return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty user)")
 	}
 
+	if featureflag.IsDisabled(ctx, featureflag.GoUserDeleteBranch) {
+		return s.UserDeleteBranchRuby(ctx, req)
+	}
+
+	// Implement UserDeleteBranch in Go
+
+	revision, err := parseRevision(ctx, req.Repository, string(req.BranchName))
+	if err != nil {
+		return nil, helper.ErrPreconditionFailed(err)
+	}
+
+	branch := fmt.Sprintf("refs/heads/%s", req.BranchName)
+
+	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, git.NullSHA, revision); err != nil {
+		var preReceiveError preReceiveError
+		if errors.As(err, &preReceiveError) {
+			return &gitalypb.UserDeleteBranchResponse{
+				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.UserDeleteBranchResponse{}, nil
+		}
+
+		return nil, err
+	}
+
+	return &gitalypb.UserDeleteBranchResponse{}, nil
+}
+
+func (s *server) UserDeleteBranchRuby(ctx context.Context, req *gitalypb.UserDeleteBranchRequest) (*gitalypb.UserDeleteBranchResponse, error) {
 	client, err := s.ruby.OperationServiceClient(ctx)
 	if err != nil {
 		return nil, err
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index a3be918b6..bc5e57eed 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -350,7 +350,10 @@ func TestFailedUserCreateBranchRequest(t *testing.T) {
 }
 
 func TestSuccessfulUserDeleteBranchRequest(t *testing.T) {
-	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{featureflag.ReferenceTransactions})
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+		featureflag.GoUserDeleteBranch,
+	})
 	require.NoError(t, err)
 
 	for _, featureSet := range featureSets {
@@ -438,6 +441,11 @@ func TestFailedUserDeleteBranchDueToValidation(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserDeleteBranch,
+	})
+	require.NoError(t, err)
+
 	testCases := []struct {
 		desc    string
 		request *gitalypb.UserDeleteBranchRequest
@@ -470,13 +478,19 @@ func TestFailedUserDeleteBranchDueToValidation(t *testing.T) {
 		},
 	}
 
-	for _, testCase := range testCases {
-		t.Run(testCase.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
+	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()
 
-			_, err := client.UserDeleteBranch(ctx, testCase.request)
-			testhelper.RequireGrpcError(t, err, testCase.code)
+					ctx = featureSet.Disable(ctx)
+
+					_, err := client.UserDeleteBranch(ctx, testCase.request)
+					testhelper.RequireGrpcError(t, err, testCase.code)
+				})
+			}
 		})
 	}
 }
@@ -495,6 +509,11 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) {
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput)
 	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserDeleteBranch,
+	})
+	require.NoError(t, err)
+
 	request := &gitalypb.UserDeleteBranchRequest{
 		Repository: testRepo,
 		BranchName: []byte(branchNameInput),
@@ -503,21 +522,27 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) {
 
 	hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\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()
+	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()
 
-			ctx, cancel := testhelper.Context()
-			defer cancel()
+					ctx, cancel := testhelper.Context()
+					defer cancel()
 
-			response, err := client.UserDeleteBranch(ctx, request)
-			require.Nil(t, err)
-			require.Contains(t, response.PreReceiveError, "GL_ID="+testhelper.TestUser.GlId)
+					ctx = featureSet.Disable(ctx)
 
-			branches := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch")
-			require.Contains(t, string(branches), branchNameInput, "branch name does not exist in branches list")
+					response, err := client.UserDeleteBranch(ctx, request)
+					require.NoError(t, err)
+					require.Contains(t, response.PreReceiveError, "GL_ID="+testhelper.TestUser.GlId)
+
+					branches := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch")
+					require.Contains(t, string(branches), branchNameInput, "branch name does not exist in branches list")
+				})
+			}
 		})
 	}
 }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 812556d55..df2b79ab5 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -30,8 +30,10 @@ 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 enables the Go implementation of UserFFBranch
 	GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: false}
+	// GoUserDeleteBranch enables the Go implementation of UserDeleteBranch
+	GoUserDeleteBranch = FeatureFlag{Name: "go_user_delete_branch", OnByDefault: false}
 )
 
 // All includes all feature flags.
@@ -45,6 +47,7 @@ var All = []FeatureFlag{
 	GoUserMergeBranch,
 	GoUserMergeToRef,
 	GoUserFFBranch,
+	GoUserDeleteBranch,
 }
 
 const (