Merge branch 'pks-user-merge-to-ref-drop-precursor-ff' into 'master'

operations: Drop code supporting UserMergeToRef's precursory ref update

See merge request gitlab-org/gitaly!3614

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,
 }