Merge branch 'avar/user-delete-branch-GetBranch-vs-GetReference-bugfix' into 'master'

UserDeleteBranch: unify API responses between Go & Ruby response paths

See merge request gitlab-org/gitaly!2864

3 files changed, 130 insertions, 29 deletions

diff --git a/changelogs/unreleased/avar-user-delete-branch-GetBranch-vs-GetReference-bugfix.yml b/changelogs/unreleased/avar-user-delete-branch-GetBranch-vs-GetReference-bugfix.yml
new file mode 100644
index 000000000..45826e0be
--- /dev/null
+++ b/changelogs/unreleased/avar-user-delete-branch-GetBranch-vs-GetReference-bugfix.yml
@@ -0,0 +1,5 @@
+---
+title: 'UserDeleteBranch: unify API responses between Go & Ruby response paths'
+merge_request: 2864
+author:
+type: fixed
diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index cfc0579c2..9d2bdf8ab 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -4,11 +4,11 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"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"
@@ -102,6 +102,9 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB
 }
 
 func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteBranchRequest) (*gitalypb.UserDeleteBranchResponse, error) {
+	// That we do the branch name & user check here first only in
+	// UserDelete but not UserCreate is "intentional", i.e. it's
+	// always been that way.
 	if len(req.BranchName) == 0 {
 		return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty branch name)")
 	}
@@ -110,20 +113,25 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB
 		return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty user)")
 	}
 
+	// Implement UserDeleteBranch in Go
 	if featureflag.IsDisabled(ctx, featureflag.GoUserDeleteBranch) {
 		return s.UserDeleteBranchRuby(ctx, req)
 	}
 
-	// Implement UserDeleteBranch in Go
-
-	revision, err := git.NewRepository(req.Repository).GetBranch(ctx, string(req.BranchName))
+	referenceFmt := "refs/heads/%s"
+	if strings.HasPrefix(string(req.BranchName), "refs/") {
+		// Not the same behavior as UserCreateBranch. This is
+		// Ruby bug emulation. See
+		// https://gitlab.com/gitlab-org/gitaly/-/issues/3218
+		referenceFmt = "%s"
+	}
+	referenceName := fmt.Sprintf(referenceFmt, req.BranchName)
+	referenceValue, err := git.NewRepository(req.Repository).GetReference(ctx, referenceName)
 	if err != nil {
-		return nil, helper.ErrPreconditionFailed(err)
+		return nil, status.Errorf(codes.FailedPrecondition, "branch not found: %s", req.BranchName)
 	}
 
-	branch := fmt.Sprintf("refs/heads/%s", req.BranchName)
-
-	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, git.NullSHA, revision.Target); err != nil {
+	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, git.NullSHA, referenceValue.Target); err != nil {
 		var preReceiveError preReceiveError
 		if errors.As(err, &preReceiveError) {
 			return &gitalypb.UserDeleteBranchResponse{
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index a0dcead6a..d2c899cc9 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -469,23 +469,51 @@ func testSuccessfulUserDeleteBranchRequest(t *testing.T, ctx context.Context) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
-	branchNameInput := "to-be-deleted-soon-branch"
-
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
+	testCases := []struct {
+		desc            string
+		branchNameInput string
+		branchCommit    string
+		user            *gitalypb.User
+		response        *gitalypb.UserDeleteBranchResponse
+		err             error
+	}{
+		{
+			desc:            "simple successful deletion",
+			branchNameInput: "to-attempt-to-delete-soon-branch",
+			branchCommit:    "c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd",
+			user:            testhelper.TestUser,
+			response:        &gitalypb.UserDeleteBranchResponse{},
+			err:             nil,
+		},
+		{
+			desc:            "partially prefixed successful deletion",
+			branchNameInput: "heads/to-attempt-to-delete-soon-branch",
+			branchCommit:    "9a944d90955aaf45f6d0c88f30e27f8d2c41cec0",
+			user:            testhelper.TestUser,
+			response:        &gitalypb.UserDeleteBranchResponse{},
+			err:             nil,
+		},
+	}
 
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput)
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", testCase.branchNameInput, testCase.branchCommit)
+			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", testCase.branchNameInput).Run()
 
-	request := &gitalypb.UserDeleteBranchRequest{
-		Repository: testRepo,
-		BranchName: []byte(branchNameInput),
-		User:       testhelper.TestUser,
-	}
+			request := &gitalypb.UserDeleteBranchRequest{
+				Repository: testRepo,
+				BranchName: []byte(testCase.branchNameInput),
+				User:       testCase.user,
+			}
 
-	_, err := client.UserDeleteBranch(ctx, request)
-	require.NoError(t, err)
+			response, err := client.UserDeleteBranch(ctx, request)
+			require.Equal(t, testCase.err, err)
+			testhelper.ProtoEqual(t, testCase.response, response)
 
-	branches := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/heads/"+branchNameInput)
-	require.NotContains(t, string(branches), branchNameInput, "branch name still exists in branches list")
+			refs := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/heads/"+testCase.branchNameInput)
+			require.NotContains(t, string(refs), testCase.branchCommit, "branch deleted from refs")
+		})
+	}
 }
 
 func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) {
@@ -541,9 +569,10 @@ func testFailedUserDeleteBranchDueToValidation(t *testing.T, ctx context.Context
 	defer cleanupFn()
 
 	testCases := []struct {
-		desc    string
-		request *gitalypb.UserDeleteBranchRequest
-		code    codes.Code
+		desc     string
+		request  *gitalypb.UserDeleteBranchRequest
+		response *gitalypb.UserDeleteBranchResponse
+		err      error
 	}{
 		{
 			desc: "empty user",
@@ -551,7 +580,8 @@ func testFailedUserDeleteBranchDueToValidation(t *testing.T, ctx context.Context
 				Repository: testRepo,
 				BranchName: []byte("does-matter-the-name-if-user-is-empty"),
 			},
-			code: codes.InvalidArgument,
+			response: nil,
+			err:      status.Error(codes.InvalidArgument, "Bad Request (empty user)"),
 		},
 		{
 			desc: "empty branch name",
@@ -559,7 +589,8 @@ func testFailedUserDeleteBranchDueToValidation(t *testing.T, ctx context.Context
 				Repository: testRepo,
 				User:       testhelper.TestUser,
 			},
-			code: codes.InvalidArgument,
+			response: nil,
+			err:      status.Error(codes.InvalidArgument, "Bad Request (empty branch name)"),
 		},
 		{
 			desc: "non-existent branch name",
@@ -568,14 +599,71 @@ func testFailedUserDeleteBranchDueToValidation(t *testing.T, ctx context.Context
 				User:       testhelper.TestUser,
 				BranchName: []byte("i-do-not-exist"),
 			},
-			code: codes.FailedPrecondition,
+			response: nil,
+			err:      status.Errorf(codes.FailedPrecondition, "branch not found: %s", "i-do-not-exist"),
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
-			_, err := client.UserDeleteBranch(ctx, testCase.request)
-			testhelper.RequireGrpcError(t, err, testCase.code)
+			response, err := client.UserDeleteBranch(ctx, testCase.request)
+			require.Equal(t, testCase.err, err)
+			testhelper.ProtoEqual(t, testCase.response, response)
+		})
+	}
+}
+
+func TestUserDeleteBranch_failedDueToRefsHeadsPrefix(t *testing.T) {
+	testWithFeature(t, featureflag.GoUserDeleteBranch, testUserDeleteBranchFailedDueToRefsHeadsPrefix)
+}
+
+func testUserDeleteBranchFailedDueToRefsHeadsPrefix(t *testing.T, ctx context.Context) {
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	serverSocketPath, stop := runOperationServiceServer(t)
+	defer stop()
+
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testCases := []struct {
+		desc            string
+		branchNameInput string
+		branchCommit    string
+		user            *gitalypb.User
+		response        *gitalypb.UserDeleteBranchResponse
+		err             error
+	}{
+		{
+			// TODO: Make this possible. See
+			// https://gitlab.com/gitlab-org/gitaly/-/issues/3343
+			desc:            "not possible to delete a branch called refs/heads/something",
+			branchNameInput: "refs/heads/can-not-find-this",
+			branchCommit:    "c642fe9b8b9f28f9225d7ea953fe14e74748d53b",
+			user:            testhelper.TestUser,
+			response:        nil,
+			err:             status.Errorf(codes.FailedPrecondition, "branch not found: %s", "refs/heads/can-not-find-this"),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", testCase.branchNameInput, testCase.branchCommit)
+			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", testCase.branchNameInput).Run()
+
+			request := &gitalypb.UserDeleteBranchRequest{
+				Repository: testRepo,
+				BranchName: []byte(testCase.branchNameInput),
+				User:       testCase.user,
+			}
+
+			response, err := client.UserDeleteBranch(ctx, request)
+			require.Equal(t, testCase.err, err)
+			testhelper.ProtoEqual(t, testCase.response, response)
+
+			refs := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/heads/"+testCase.branchNameInput)
+			require.Contains(t, string(refs), testCase.branchCommit, "branch kept because we stripped off refs/heads/*")
 		})
 	}
 }