operations: Split out creation of tags

The `UserCreateTag()` RPC first creates a tag object and then updates
its target reference to point to that new object with hooks. This is two
logical steps, but both are currently implemented in the same function.

Split out creation of the tag into its own function. This will make it
easier in a subsequent commit to pass in a quarantined repository and
see that the tag can only be written into the quarantine object
directory.

1 files changed, 100 insertions, 84 deletions

diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index 1b8ec270b..506731ee5 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -91,19 +91,103 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 		return nil, err
 	}
 
-	// Setup
+	targetRevision := git.Revision(req.TargetRevision)
+
+	committerTime := time.Now()
+	if req.Timestamp != nil {
+		var err error
+		committerTime, err = ptypes.Timestamp(req.Timestamp)
+		if err != nil {
+			return nil, helper.ErrInvalidArgument(err)
+		}
+	}
+
 	repo := s.localrepo(req.GetRepository())
+	tag, tagID, err := s.createTag(ctx, repo, targetRevision, req.TagName, req.Message, req.User, committerTime)
+	if err != nil {
+		return nil, err
+	}
+
+	referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName))
+	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, nil, referenceName, tagID, git.ZeroOID); err != nil {
+		var preReceiveError updateref.PreReceiveError
+		if errors.As(err, &preReceiveError) {
+			return &gitalypb.UserCreateTagResponse{
+				PreReceiveError: preReceiveError.Message,
+			}, nil
+		}
+
+		var updateRefError updateref.Error
+		if errors.As(err, &updateRefError) {
+			refNameOK, err := git.CheckRefFormat(ctx, s.gitCmdFactory, referenceName.String())
+			if refNameOK {
+				// The tag might not actually exist,
+				// perhaps update-ref died for some
+				// other reason. But saying that it
+				// does is what the Ruby code used to
+				// do, so let's follow it off that
+				// cliff.
+				return &gitalypb.UserCreateTagResponse{
+					Tag:    nil,
+					Exists: true,
+				}, nil
+			}
+
+			var CheckRefFormatError git.CheckRefFormatError
+			if errors.As(err, &CheckRefFormatError) {
+				// It doesn't make sense either to
+				// tell the user to retry with an
+				// invalid ref name, but ditto on the
+				// Ruby bug-for-bug emulation.
+				return &gitalypb.UserCreateTagResponse{
+					Tag:    nil,
+					Exists: true,
+				}, status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", req.TagName)
+			}
+
+			// Should only be reachable if "git
+			// check-ref-format"'s invocation is
+			// incorrect, or if it segfaults on startup
+			// etc.
+			return nil, status.Error(codes.Internal, err.Error())
+		}
+
+		// The Ruby code did not return this, but always an
+		// "Exists: true" on update-ref failure without any
+		// meaningful error. This is our "PANIC" response, if
+		// we've got an unknown error (it should all be
+		// updateRefError above) let's relay it to the
+		// caller. This should not happen.
+		return &gitalypb.UserCreateTagResponse{
+			Tag:    nil,
+			Exists: true,
+		}, status.Error(codes.Internal, err.Error())
+	}
+
+	return &gitalypb.UserCreateTagResponse{
+		Tag: tag,
+	}, nil
+}
+
+func (s *Server) createTag(
+	ctx context.Context,
+	repo *localrepo.Repo,
+	targetRevision git.Revision,
+	tagName []byte,
+	message []byte,
+	committer *gitalypb.User,
+	committerTime time.Time,
+) (*gitalypb.Tag, git.ObjectID, error) {
 	catFile, err := s.catfileCache.BatchProcess(ctx, repo)
 	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, "", status.Error(codes.Internal, err.Error())
 	}
 
 	// We allow all ways to name a revision that cat-file
 	// supports, not just OID. Resolve it.
-	targetRevision := git.Revision(req.TargetRevision)
 	targetInfo, err := catFile.Info(ctx, targetRevision)
 	if err != nil {
-		return nil, status.Errorf(codes.FailedPrecondition, "revspec '%s' not found", targetRevision)
+		return nil, "", status.Errorf(codes.FailedPrecondition, "revspec '%s' not found", targetRevision)
 	}
 	targetObjectID, targetObjectType := targetInfo.Oid, targetInfo.Type
 
@@ -112,7 +196,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 	// use strings.TrimSpace() eventually, but as the tests show
 	// the Ruby idea of Unicode whitespace is different than its
 	// idea.
-	makingTag := regexp.MustCompile(`\S`).Match(req.Message)
+	makingTag := regexp.MustCompile(`\S`).Match(message)
 
 	// If we're creating a tag to another "tag" we'll need to peel
 	// (possibly recursively) all the way down to the
@@ -125,7 +209,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 	if targetObjectType == "tag" {
 		peeledTargetObjectInfo, err := catFile.Info(ctx, targetRevision+"^{}")
 		if err != nil {
-			return nil, status.Error(codes.Internal, err.Error())
+			return nil, "", status.Error(codes.Internal, err.Error())
 		}
 		peeledTargetObjectID, peeledTargetObjectType = peeledTargetObjectInfo.Oid, peeledTargetObjectInfo.Type
 
@@ -144,37 +228,27 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 	refObjectID := targetObjectID
 	var tagObject *gitalypb.Tag
 	if makingTag {
-		localRepo := s.localrepo(repo)
-
-		committerTime := time.Now()
-		if req.Timestamp != nil {
-			committerTime, err = ptypes.Timestamp(req.Timestamp)
-			if err != nil {
-				return nil, helper.ErrInvalidArgument(err)
-			}
-		}
-
-		tagObjectID, err := localRepo.WriteTag(ctx, targetObjectID, targetObjectType, req.TagName, req.User.Name, req.User.Email, req.Message, committerTime)
+		tagObjectID, err := repo.WriteTag(ctx, targetObjectID, targetObjectType, tagName, committer.Name, committer.Email, message, committerTime)
 		if err != nil {
 			var FormatTagError localrepo.FormatTagError
 			if errors.As(err, &FormatTagError) {
-				return nil, status.Errorf(codes.Unknown, "Rugged::InvalidError: failed to parse signature - expected prefix doesn't match actual")
+				return nil, "", status.Errorf(codes.Unknown, "Rugged::InvalidError: failed to parse signature - expected prefix doesn't match actual")
 			}
 
 			var MktagError localrepo.MktagError
 			if errors.As(err, &MktagError) {
-				return nil, status.Errorf(codes.NotFound, "Gitlab::Git::CommitError: %s", err.Error())
+				return nil, "", status.Errorf(codes.NotFound, "Gitlab::Git::CommitError: %s", err.Error())
 			}
-			return nil, status.Error(codes.Internal, err.Error())
+			return nil, "", status.Error(codes.Internal, err.Error())
 		}
 
-		createdTag, err := catfile.GetTag(ctx, catFile, tagObjectID.Revision(), string(req.TagName), false, false)
+		createdTag, err := catfile.GetTag(ctx, catFile, tagObjectID.Revision(), string(tagName), false, false)
 		if err != nil {
-			return nil, status.Error(codes.Internal, err.Error())
+			return nil, "", status.Error(codes.Internal, err.Error())
 		}
 
 		tagObject = &gitalypb.Tag{
-			Name:        req.TagName,
+			Name:        tagName,
 			Id:          tagObjectID.String(),
 			Message:     createdTag.Message,
 			MessageSize: createdTag.MessageSize,
@@ -184,79 +258,21 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 		refObjectID = tagObjectID
 	} else {
 		tagObject = &gitalypb.Tag{
-			Name: req.TagName,
+			Name: tagName,
 			Id:   peeledTargetObjectID.String(),
 			//TargetCommit: is filled in below if needed
 		}
 	}
 
-	referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName))
-	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, nil, referenceName, refObjectID, git.ZeroOID); err != nil {
-		var preReceiveError updateref.PreReceiveError
-		if errors.As(err, &preReceiveError) {
-			return &gitalypb.UserCreateTagResponse{
-				PreReceiveError: preReceiveError.Message,
-			}, nil
-		}
-
-		var updateRefError updateref.Error
-		if errors.As(err, &updateRefError) {
-			refNameOK, err := git.CheckRefFormat(ctx, s.gitCmdFactory, referenceName.String())
-			if refNameOK {
-				// The tag might not actually exist,
-				// perhaps update-ref died for some
-				// other reason. But saying that it
-				// does is what the Ruby code used to
-				// do, so let's follow it off that
-				// cliff.
-				return &gitalypb.UserCreateTagResponse{
-					Tag:    nil,
-					Exists: true,
-				}, nil
-			}
-
-			var CheckRefFormatError git.CheckRefFormatError
-			if errors.As(err, &CheckRefFormatError) {
-				// It doesn't make sense either to
-				// tell the user to retry with an
-				// invalid ref name, but ditto on the
-				// Ruby bug-for-bug emulation.
-				return &gitalypb.UserCreateTagResponse{
-					Tag:    nil,
-					Exists: true,
-				}, status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", req.TagName)
-			}
-
-			// Should only be reachable if "git
-			// check-ref-format"'s invocation is
-			// incorrect, or if it segfaults on startup
-			// etc.
-			return nil, status.Error(codes.Internal, err.Error())
-		}
-
-		// The Ruby code did not return this, but always an
-		// "Exists: true" on update-ref failure without any
-		// meaningful error. This is our "PANIC" response, if
-		// we've got an unknown error (it should all be
-		// updateRefError above) let's relay it to the
-		// caller. This should not happen.
-		return &gitalypb.UserCreateTagResponse{
-			Tag:    nil,
-			Exists: true,
-		}, status.Error(codes.Internal, err.Error())
-	}
-
 	// Save ourselves looking this up earlier in case update-ref
 	// died
 	if peeledTargetObjectType == "commit" {
 		peeledTargetCommit, err := catfile.GetCommit(ctx, catFile, peeledTargetObjectID.Revision())
 		if err != nil {
-			return nil, status.Error(codes.Internal, err.Error())
+			return nil, "", status.Error(codes.Internal, err.Error())
 		}
 		tagObject.TargetCommit = peeledTargetCommit
 	}
 
-	return &gitalypb.UserCreateTagResponse{
-		Tag: tagObject,
-	}, nil
+	return tagObject, refObjectID, nil
 }