operations: Use `ExpectedOldOid` in `UserDeleteTag`4407-usermergebranch-lacks-ability-to-let-clients-update-a-reference-without-races

If the clients provide the `ExpectedOldOid` in the request of
`UserDeleteTag`, we need to use this as the ref's current OID for
updating the reference.

This is directly fed to git-update-ref(1) where it'll exit with an error
code if the current OID of the ref doesn't match the OID provided by us.
This allows us to avoid any race conditions wherein the branch was
updated concurrently by another process.

2 files changed, 87 insertions, 6 deletions

diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index eb6d99817..91791043b 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -33,16 +33,27 @@ func validateUserDeleteTagRequest(in *gitalypb.UserDeleteTagRequest) error {
 
 //nolint:revive // This is unintentionally missing documentation.
 func (s *Server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) {
-	if err := validateUserDeleteTagRequest(req); err != nil {
+	var err error
+
+	if err = validateUserDeleteTagRequest(req); err != nil {
 		return nil, helper.ErrInvalidArgument(err)
 	}
 	referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName))
-	revision, err := s.localrepo(req.GetRepository()).ResolveRevision(ctx, referenceName.Revision())
-	if err != nil {
-		if errors.Is(err, git.ErrReferenceNotFound) {
-			return nil, helper.ErrFailedPreconditionf("tag not found: %s", req.TagName)
+
+	var revision git.ObjectID
+	if expectedOldOID := req.GetExpectedOldOid(); expectedOldOID != "" {
+		revision, err = s.localrepo(req.GetRepository()).ResolveRevision(ctx, git.Revision(expectedOldOID))
+		if err != nil {
+			return nil, helper.ErrFailedPreconditionf("resolve object ID: %s: %w", expectedOldOID, err)
+		}
+	} else {
+		revision, err = s.localrepo(req.GetRepository()).ResolveRevision(ctx, referenceName.Revision())
+		if err != nil {
+			if errors.Is(err, git.ErrReferenceNotFound) {
+				return nil, helper.ErrFailedPreconditionf("tag not found: %s", req.TagName)
+			}
+			return nil, helper.ErrInternalf("resolving revision %q: %w", referenceName, err)
 		}
-		return nil, helper.ErrInternalf("resolving revision %q: %w", referenceName, err)
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, nil, referenceName, git.ObjectHashSHA1.ZeroOID, revision); err != nil {
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 9ecf3c82b..4c192350c 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -51,6 +51,76 @@ func TestUserDeleteTag_successful(t *testing.T) {
 	require.NotContains(t, string(tags), tagNameInput, "tag name still exists in tags list")
 }
 
+func TestUserDeleteTag_successfulWithExpectedOldOID(t *testing.T) {
+	t.Parallel()
+	ctx := testhelper.Context(t)
+
+	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
+
+	tagNameInput := "to-be-deleted-soon-tag"
+
+	gittest.Exec(t, cfg, "-C", repoPath, "tag", tagNameInput)
+
+	request := &gitalypb.UserDeleteTagRequest{
+		Repository:     repo,
+		TagName:        []byte(tagNameInput),
+		User:           gittest.TestUser,
+		ExpectedOldOid: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", tagNameInput)),
+	}
+
+	response, err := client.UserDeleteTag(ctx, request)
+	require.NoError(t, err)
+	require.Empty(t, response.PreReceiveError)
+
+	tags := gittest.Exec(t, cfg, "-C", repoPath, "tag")
+	require.NotContains(t, string(tags), tagNameInput, "tag name still exists in tags list")
+}
+
+func TestUserDeleteTag_failureWithExpectedOldOID(t *testing.T) {
+	t.Parallel()
+	ctx := testhelper.Context(t)
+
+	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
+
+	tagNameInput := "to-be-deleted-soon-tag"
+
+	gittest.Exec(t, cfg, "-C", repoPath, "tag", tagNameInput)
+
+	testCases := []struct {
+		desc        string
+		request     *gitalypb.UserDeleteTagRequest
+		expectedErr error
+	}{
+		{
+			desc: "invalid expectedOldOID",
+			request: &gitalypb.UserDeleteTagRequest{
+				Repository:     repo,
+				TagName:        []byte(tagNameInput),
+				User:           gittest.TestUser,
+				ExpectedOldOid: "foobar",
+			},
+			expectedErr: helper.ErrFailedPreconditionf(`resolve object ID: foobar: reference not found`),
+		},
+		{
+			desc: "incorrect expectedOldOID",
+			request: &gitalypb.UserDeleteTagRequest{
+				Repository:     repo,
+				TagName:        []byte(tagNameInput),
+				User:           gittest.TestUser,
+				ExpectedOldOid: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", tagNameInput+"~1")),
+			},
+			expectedErr: helper.ErrFailedPreconditionf(`Could not update refs/tags/to-be-deleted-soon-tag. Please refresh and try again.`),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, err := client.UserDeleteTag(ctx, tc.request)
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
+		})
+	}
+}
+
 func TestUserDeleteTag_hooks(t *testing.T) {
 	t.Parallel()
 	ctx := testhelper.Context(t)