operations: Drop code supporting UserMergeToRef's precursory ref update

Previous to commit 907c03bf9 (operations: Skip precursory update of
target ref in UserMergeToRef, 2021-06-09), UserMergeToRef used to always
create the target reference even if the merge failed. This behaviour was
unexpected and caused issues for transactions because we have a vote
before an expected failure, which would as a result cause replication
jobs to be created if the merge fails.

This code was replaced by code which simply asserts that the target
reference didn't change while we were computing the merge commit. The
old code continued to exist behind a feature flag though in case this
semantic change were to cause issues.

No issues have been observed, so let's remove the feature flag.

3 files changed, 15 insertions, 58 deletions

diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 424dba8ec..f227f0efd 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -11,7 +11,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -266,39 +265,25 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 		}
 	}
 
+	// Resolve the current state of the target reference. We do not care whether it
+	// exists or not, but what we do want to assert is that the target reference doesn't
+	// change while we compute the merge commit as a small protection against races.
 	var oldTargetOID git.ObjectID
-	if featureflag.IsEnabled(ctx, featureflag.UserMergeToRefSkipPrecursorRefUpdate) {
-		// Resolve the current state of the target reference. We do not care whether it
-		// exists or not, but what we do want to assert is that the target reference doesn't
-		// change while we compute the merge commit as a small protection against races.
-		targetRef, err := repo.GetReference(ctx, git.ReferenceName(request.TargetRef))
-		if err == nil {
-			if targetRef.IsSymbolic {
-				return nil, helper.ErrPreconditionFailedf("target reference is symbolic: %q", request.TargetRef)
-			}
-
-			oid, err := git.NewObjectIDFromHex(targetRef.Target)
-			if err != nil {
-				return nil, helper.ErrInternalf("invalid target revision: %v", err)
-			}
-
-			oldTargetOID = oid
-		} else if errors.Is(err, git.ErrReferenceNotFound) {
-			oldTargetOID = git.ZeroOID
-		} else {
-			return nil, helper.ErrInternalf("could not read target reference: %v", err)
+	if targetRef, err := repo.GetReference(ctx, git.ReferenceName(request.TargetRef)); err == nil {
+		if targetRef.IsSymbolic {
+			return nil, helper.ErrPreconditionFailedf("target reference is symbolic: %q", request.TargetRef)
 		}
-	} else {
-		// This is the old code path which always force-updated the target reference before
-		// computing the merge. As a result, even if the merge failed, we'd have updated the
-		// reference to point to the first parent ref. It does feel unexpected that the
-		// target reference changes even if the merge itself fails. Furthermore, this is
-		// causing problems with transactions: if the merge fails, we have already voted
-		// once to update the target reference and will thus trigger a replication job.
-		if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid, ""); err != nil {
-			return nil, updateRefError{reference: string(request.TargetRef)}
+
+		oid, err := git.NewObjectIDFromHex(targetRef.Target)
+		if err != nil {
+			return nil, helper.ErrInternalf("invalid target revision: %v", err)
 		}
+
 		oldTargetOID = oid
+	} else if errors.Is(err, git.ErrReferenceNotFound) {
+		oldTargetOID = git.ZeroOID
+	} else {
+		return nil, helper.ErrInternalf("could not read target reference: %v", err)
 	}
 
 	// Now, we create the merge commit...
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index c91aabdc7..d3abca08a 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -17,7 +17,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -806,8 +805,6 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	targetRef := git.Revision("refs/merge-requests/foo")
 
 	t.Run("failing merge does not update target reference if skipping precursor update-ref", func(t *testing.T) {
-		ctx := featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.UserMergeToRefSkipPrecursorRefUpdate)
-
 		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", t.Name())
 		request.TargetRef = []byte(targetRef)
 
@@ -818,27 +815,6 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 		require.NoError(t, err)
 		require.False(t, hasRevision, "branch should not have been created")
 	})
-
-	t.Run("failing merge does update target reference if not skipping precursor update-ref", func(t *testing.T) {
-		ctx := featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.UserMergeToRefSkipPrecursorRefUpdate)
-
-		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath,
-			"1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", t.Name())
-		request.TargetRef = []byte(targetRef)
-
-		_, err := client.UserMergeToRef(ctx, request)
-		testhelper.RequireGrpcError(t, err, codes.FailedPrecondition)
-
-		firstParentRevision, err := repo.ResolveRevision(ctx, git.Revision(request.FirstParentRef))
-		require.NoError(t, err)
-
-		// The previous implementation of UserMergeToRef would've written the first parent
-		// ref into the target reference before computing the merge. As a result, it exists
-		// even if the merge itself failed.
-		targetRevision, err := repo.ResolveRevision(ctx, targetRef)
-		require.NoError(t, err)
-		require.Equal(t, firstParentRevision, targetRevision)
-	})
 }
 
 func buildUserMergeToRefRequest(t testing.TB, cfg config.Cfg, repo *gitalypb.Repository, repoPath string, sourceSha string, targetSha string, mergeBranchName string) *gitalypb.UserMergeToRefRequest {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 0ec6a989c..50bb19293 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -13,9 +13,6 @@ var (
 	GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false}
 	// FetchInternalRemoteErrors makes FetchInternalRemote return actual errors instead of a boolean
 	FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: false}
-	// UserMergeToRefSkipPrecursorRefUpdate causes UserMergeToRef to not update the
-	// target reference in case computing the merge fails.
-	UserMergeToRefSkipPrecursorRefUpdate = FeatureFlag{Name: "user_merge_to_ref_skip_precursor_ref_update", OnByDefault: true}
 	// LFSPointersPipeline enables the alternative pipeline implementation of LFS-pointer
 	// related RPCs.
 	LFSPointersPipeline = FeatureFlag{Name: "lfs_pointers_pipeline", OnByDefault: false}
@@ -25,6 +22,5 @@ var (
 var All = []FeatureFlag{
 	GoUpdateRemoteMirror,
 	FetchInternalRemoteErrors,
-	UserMergeToRefSkipPrecursorRefUpdate,
 	LFSPointersPipeline,
 }