operations: Always enable structured errors in UserDeleteBranch

With 30d4fd427 (operations: Use structured errors in UserDeleteBranch,
2022-06-07), we have introduced structured errors in UserDeletBranch.
The intent is to both fix silent errors in case we failed to delete the
branch, and to fix replication in case no transactional votes were cast
in the error case.

The change has been rolled out on June 14th and is part of v15.1. So
let's remove the feature flag guarding it.

Changelog: fixed

3 files changed, 91 insertions, 174 deletions

diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index 5adf2d990..02a25649c 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -8,7 +8,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -165,77 +164,61 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, nil, referenceName, git.ZeroOID, referenceValue); err != nil {
-		if featureflag.UserDeleteBranchStructuredErrors.IsEnabled(ctx) {
-			var notAllowedError hook.NotAllowedError
-			var customHookErr updateref.CustomHookError
-			var updateRefError updateref.Error
-
-			if errors.As(err, &notAllowedError) {
-				detailedErr, err := helper.ErrWithDetails(
-					helper.ErrPermissionDeniedf("deletion denied by access checks: %w", err),
-					&gitalypb.UserDeleteBranchError{
-						Error: &gitalypb.UserDeleteBranchError_AccessCheck{
-							AccessCheck: &gitalypb.AccessCheckError{
-								ErrorMessage: notAllowedError.Message,
-								UserId:       notAllowedError.UserID,
-								Protocol:     notAllowedError.Protocol,
-								Changes:      notAllowedError.Changes,
-							},
+		var notAllowedError hook.NotAllowedError
+		var customHookErr updateref.CustomHookError
+		var updateRefError updateref.Error
+
+		if errors.As(err, &notAllowedError) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrPermissionDeniedf("deletion denied by access checks: %w", err),
+				&gitalypb.UserDeleteBranchError{
+					Error: &gitalypb.UserDeleteBranchError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: notAllowedError.Message,
+							UserId:       notAllowedError.UserID,
+							Protocol:     notAllowedError.Protocol,
+							Changes:      notAllowedError.Changes,
 						},
 					},
-				)
-				if err != nil {
-					return nil, helper.ErrInternalf("error details: %w", err)
-				}
-
-				return nil, detailedErr
-			} else if errors.As(err, &customHookErr) {
-				detailedErr, err := helper.ErrWithDetails(
-					helper.ErrPermissionDeniedf("deletion denied by custom hooks: %w", err),
-					&gitalypb.UserDeleteBranchError{
-						Error: &gitalypb.UserDeleteBranchError_CustomHook{
-							CustomHook: customHookErr.Proto(),
-						},
+				},
+			)
+			if err != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
+		} else if errors.As(err, &customHookErr) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrPermissionDeniedf("deletion denied by custom hooks: %w", err),
+				&gitalypb.UserDeleteBranchError{
+					Error: &gitalypb.UserDeleteBranchError_CustomHook{
+						CustomHook: customHookErr.Proto(),
 					},
-				)
-				if err != nil {
-					return nil, helper.ErrInternalf("error details: %w", err)
-				}
-
-				return nil, detailedErr
-			} else if errors.As(err, &updateRefError) {
-				detailedErr, err := helper.ErrWithDetails(
-					helper.ErrFailedPreconditionf("reference update failed: %w", updateRefError),
-					&gitalypb.UserDeleteBranchError{
-						Error: &gitalypb.UserDeleteBranchError_ReferenceUpdate{
-							ReferenceUpdate: &gitalypb.ReferenceUpdateError{
-								ReferenceName: []byte(updateRefError.Reference.String()),
-								OldOid:        updateRefError.OldOID.String(),
-								NewOid:        updateRefError.NewOID.String(),
-							},
+				},
+			)
+			if err != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
+		} else if errors.As(err, &updateRefError) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrFailedPreconditionf("reference update failed: %w", updateRefError),
+				&gitalypb.UserDeleteBranchError{
+					Error: &gitalypb.UserDeleteBranchError_ReferenceUpdate{
+						ReferenceUpdate: &gitalypb.ReferenceUpdateError{
+							ReferenceName: []byte(updateRefError.Reference.String()),
+							OldOid:        updateRefError.OldOID.String(),
+							NewOid:        updateRefError.NewOID.String(),
 						},
 					},
-				)
-				if err != nil {
-					return nil, helper.ErrInternalf("error details: %w", err)
-				}
-
-				return nil, detailedErr
+				},
+			)
+			if err != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
 			}
 
-			return nil, helper.ErrInternalf("deleting reference: %w", err)
-		}
-
-		var customHookErr updateref.CustomHookError
-		if errors.As(err, &customHookErr) {
-			return &gitalypb.UserDeleteBranchResponse{
-				PreReceiveError: customHookErr.Error(),
-			}, nil
-		}
-
-		var updateRefError updateref.Error
-		if errors.As(err, &updateRefError) {
-			return nil, helper.ErrFailedPrecondition(err)
+			return nil, detailedErr
 		}
 
 		return nil, helper.ErrInternalf("deleting reference: %w", err)
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index da0262bcb..6973f389f 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -380,12 +379,8 @@ func TestUserCreateBranch_failure(t *testing.T) {
 
 func TestUserDeleteBranch_success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchSuccess)
-}
-
-func testUserDeleteBranchSuccess(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
@@ -439,25 +434,8 @@ func testUserDeleteBranchSuccess(t *testing.T, ctx context.Context) {
 
 func TestUserDeleteBranch_allowed(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchAllowed)
-}
-
-func testUserDeleteBranchAllowed(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
-	responseIfUnstructured := func(response *gitalypb.UserDeleteBranchResponse) *gitalypb.UserDeleteBranchResponse {
-		if featureflag.UserDeleteBranchStructuredErrors.IsEnabled(ctx) {
-			return nil
-		}
-		return response
-	}
-
-	errIfStructured := func(err error) error {
-		if featureflag.UserDeleteBranchStructuredErrors.IsEnabled(ctx) {
-			return err
-		}
-		return nil
-	}
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc             string
@@ -477,7 +455,7 @@ func testUserDeleteBranchAllowed(t *testing.T, ctx context.Context) {
 			allowed: func(context.Context, gitlab.AllowedParams) (bool, string, error) {
 				return false, "something something", nil
 			},
-			expectedErr: errIfStructured(errWithDetails(t,
+			expectedErr: errWithDetails(t,
 				helper.ErrPermissionDeniedf("deletion denied by access checks: GitLab: something something"),
 				&gitalypb.UserDeleteBranchError{
 					Error: &gitalypb.UserDeleteBranchError_AccessCheck{
@@ -491,17 +469,14 @@ func testUserDeleteBranchAllowed(t *testing.T, ctx context.Context) {
 						},
 					},
 				},
-			)),
-			expectedResponse: responseIfUnstructured(&gitalypb.UserDeleteBranchResponse{
-				PreReceiveError: "GitLab: something something",
-			}),
+			),
 		},
 		{
 			desc: "error",
 			allowed: func(context.Context, gitlab.AllowedParams) (bool, string, error) {
 				return false, "something something", fmt.Errorf("something else")
 			},
-			expectedErr: errIfStructured(errWithDetails(t,
+			expectedErr: errWithDetails(t,
 				helper.ErrPermissionDeniedf("deletion denied by access checks: GitLab: something else"),
 				&gitalypb.UserDeleteBranchError{
 					Error: &gitalypb.UserDeleteBranchError_AccessCheck{
@@ -515,10 +490,7 @@ func testUserDeleteBranchAllowed(t *testing.T, ctx context.Context) {
 						},
 					},
 				},
-			)),
-			expectedResponse: responseIfUnstructured(&gitalypb.UserDeleteBranchResponse{
-				PreReceiveError: "GitLab: something else",
-			}),
+			),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -542,11 +514,8 @@ func testUserDeleteBranchAllowed(t *testing.T, ctx context.Context) {
 
 func TestUserDeleteBranch_concurrentUpdate(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchConcurrentUpdate)
-}
 
-func testUserDeleteBranchConcurrentUpdate(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -568,32 +537,25 @@ func testUserDeleteBranchConcurrentUpdate(t *testing.T, ctx context.Context) {
 		BranchName: []byte("concurrent-update"),
 		User:       gittest.TestUser,
 	})
-	if featureflag.UserDeleteBranchStructuredErrors.IsEnabled(ctx) {
-		testhelper.RequireGrpcError(t, errWithDetails(t,
-			helper.ErrFailedPreconditionf("reference update failed: Could not update refs/heads/concurrent-update. Please refresh and try again."),
-			&gitalypb.UserDeleteBranchError{
-				Error: &gitalypb.UserDeleteBranchError_ReferenceUpdate{
-					ReferenceUpdate: &gitalypb.ReferenceUpdateError{
-						OldOid:        commitID.String(),
-						NewOid:        git.ZeroOID.String(),
-						ReferenceName: []byte("refs/heads/concurrent-update"),
-					},
+	testhelper.RequireGrpcError(t, errWithDetails(t,
+		helper.ErrFailedPreconditionf("reference update failed: Could not update refs/heads/concurrent-update. Please refresh and try again."),
+		&gitalypb.UserDeleteBranchError{
+			Error: &gitalypb.UserDeleteBranchError_ReferenceUpdate{
+				ReferenceUpdate: &gitalypb.ReferenceUpdateError{
+					OldOid:        commitID.String(),
+					NewOid:        git.ZeroOID.String(),
+					ReferenceName: []byte("refs/heads/concurrent-update"),
 				},
 			},
-		), err)
-	} else {
-		testhelper.RequireGrpcError(t, helper.ErrFailedPreconditionf("Could not update refs/heads/concurrent-update. Please refresh and try again."), err)
-	}
+		},
+	), err)
 	require.Nil(t, response)
 }
 
 func TestUserDeleteBranch_hooks(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchHooks)
-}
 
-func testUserDeleteBranchHooks(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -622,11 +584,8 @@ func testUserDeleteBranchHooks(t *testing.T, ctx context.Context) {
 
 func TestUserDeleteBranch_transaction(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchTransaction)
-}
 
-func testUserDeleteBranchTransaction(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
 	// This creates a new branch "delete-me" which exists both in the packed-refs file and as a
@@ -679,11 +638,8 @@ func testUserDeleteBranchTransaction(t *testing.T, ctx context.Context) {
 
 func TestUserDeleteBranch_invalidArgument(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchInvalidArgument)
-}
 
-func testUserDeleteBranchInvalidArgument(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
@@ -734,11 +690,8 @@ func testUserDeleteBranchInvalidArgument(t *testing.T, ctx context.Context) {
 
 func TestUserDeleteBranch_hookFailure(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testUserDeleteBranchHookFailure)
-}
 
-func testUserDeleteBranchHookFailure(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -770,24 +723,19 @@ func testUserDeleteBranchHookFailure(t *testing.T, ctx context.Context) {
 			gittest.WriteCustomHook(t, repoPath, tc.hookName, hookContent)
 
 			response, err := client.UserDeleteBranch(ctx, request)
-			if featureflag.UserDeleteBranchStructuredErrors.IsEnabled(ctx) {
-				testhelper.RequireGrpcError(t, errWithDetails(t,
-					helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s\n", "GL_ID=user-123"),
-					&gitalypb.UserDeleteBranchError{
-						Error: &gitalypb.UserDeleteBranchError_CustomHook{
-							CustomHook: &gitalypb.CustomHookError{
-								HookType: tc.hookType,
-								Stdout:   []byte("GL_ID=user-123\n"),
-							},
+			testhelper.RequireGrpcError(t, errWithDetails(t,
+				helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s\n", "GL_ID=user-123"),
+				&gitalypb.UserDeleteBranchError{
+					Error: &gitalypb.UserDeleteBranchError_CustomHook{
+						CustomHook: &gitalypb.CustomHookError{
+							HookType: tc.hookType,
+							Stdout:   []byte("GL_ID=user-123\n"),
 						},
 					},
-				), err)
+				},
+			), err)
 
-				require.Nil(t, response)
-			} else {
-				require.NoError(t, err)
-				require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId)
-			}
+			require.Nil(t, response)
 
 			branches := gittest.Exec(t, cfg, "-C", repoPath, "for-each-ref", "--", "refs/heads/"+branchNameInput)
 			require.Contains(t, string(branches), branchNameInput, "branch name does not exist in branches list")
@@ -797,11 +745,8 @@ func testUserDeleteBranchHookFailure(t *testing.T, ctx context.Context) {
 
 func TestBranchHookOutput(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserDeleteBranchStructuredErrors).Run(t, testBranchHookOutput)
-}
 
-func testBranchHookOutput(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -893,25 +838,19 @@ func testBranchHookOutput(t *testing.T, ctx context.Context) {
 				defer gittest.Exec(t, cfg, "-C", repoPath, "branch", "-d", branchNameInput)
 
 				deleteResponse, err := client.UserDeleteBranch(ctx, deleteRequest)
-				if featureflag.UserDeleteBranchStructuredErrors.IsEnabled(ctx) {
-					testhelper.RequireGrpcError(t, errWithDetails(t,
-						helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s", expectedError),
-						&gitalypb.UserDeleteBranchError{
-							Error: &gitalypb.UserDeleteBranchError_CustomHook{
-								CustomHook: &gitalypb.CustomHookError{
-									HookType: hookTestCase.hookType,
-									Stdout:   []byte(testCase.expectedStdout),
-									Stderr:   []byte(testCase.expectedStderr),
-								},
+				testhelper.RequireGrpcError(t, errWithDetails(t,
+					helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s", expectedError),
+					&gitalypb.UserDeleteBranchError{
+						Error: &gitalypb.UserDeleteBranchError_CustomHook{
+							CustomHook: &gitalypb.CustomHookError{
+								HookType: hookTestCase.hookType,
+								Stdout:   []byte(testCase.expectedStdout),
+								Stderr:   []byte(testCase.expectedStderr),
 							},
 						},
-					), err)
-
-					require.Nil(t, deleteResponse)
-				} else {
-					require.NoError(t, err)
-					require.Equal(t, expectedError, deleteResponse.PreReceiveError)
-				}
+					},
+				), err)
+				require.Nil(t, deleteResponse)
 			})
 		}
 	}
diff --git a/internal/metadata/featureflag/ff_user_delete_branch_structured_errors.go b/internal/metadata/featureflag/ff_user_delete_branch_structured_errors.go
deleted file mode 100644
index 75d527112..000000000
--- a/internal/metadata/featureflag/ff_user_delete_branch_structured_errors.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// UserDeleteBranchStructuredErrors enables the use of structured errors for the UserDeleteBranch
-// RPC.
-var UserDeleteBranchStructuredErrors = NewFeatureFlag("user_delete_branch_structured_errors", false)