Branch: fix API use bug in User(Create|Delete)Branchavar/user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix

Change code added in [1] and [2] to use GetReference() instead of
GetBranch(). This solves a bug that could theoretically occur, but is
mainly a trivial API code cleanup.

The GetBranch() function can take refs/heads/master or heads/master or
whatever to resolve "master", but the underlying
updateReferenceWithHooks() we're calling will only accept a
refs/heads/master.

Since we'd accept a request of "master" which we'd always normalize to
"refs/heads/master" this shouldn't change anything in practice.

It does solve an obscure bug though. If you had both a
refs/heads/master and a refs/heads/heads/master we'd get the old
revision from the latter, but operate on the former.

Thus hooks would get an invalid old revision in such a case, and
furthermore we would unexpectedly allow deletion of
refs/heads/heads/master in the obscure edge case where we raced
between GetReference() and s.updateReferenceWithHooks() while another
process updated refs/heads/heads/master.

1. c3b327226 (Initial go implementation of UserDeleteBranch, 2020-09-29)
2. c1e3ccca6 (Initial go implementation of UserCreateBranch, 2020-09-30)

2 files changed, 11 insertions, 8 deletions

diff --git a/changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml b/changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml
new file mode 100644
index 000000000..de315c9bb
--- /dev/null
+++ b/changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml
@@ -0,0 +1,5 @@
+---
+title: 'Branch: fix API use bug in User(Create|Delete)Branch'
+merge_request: 2838
+author:
+type: fixed
diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index f1cb37c04..b2697451b 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -39,16 +39,15 @@ func (s *server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB
 		return nil, helper.ErrPreconditionFailed(err)
 	}
 
-	_, err = git.NewRepository(req.Repository).GetBranch(ctx, string(req.BranchName))
+	referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName)
+	_, err = git.NewRepository(req.Repository).GetReference(ctx, referenceName)
 	if err == nil {
 		return nil, status.Errorf(codes.FailedPrecondition, "Bad Request (branch exists)")
 	} else if !errors.Is(err, git.ErrReferenceNotFound) {
 		return nil, status.Error(codes.Internal, err.Error())
 	}
 
-	branch := fmt.Sprintf("refs/heads/%s", req.BranchName)
-
-	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, string(req.StartPoint), git.NullSHA); err != nil {
+	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, string(req.StartPoint), git.NullSHA); err != nil {
 		var preReceiveError preReceiveError
 		if errors.As(err, &preReceiveError) {
 			return &gitalypb.UserCreateBranchResponse{
@@ -118,14 +117,13 @@ func (s *server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB
 
 	// Implement UserDeleteBranch in Go
 
-	revision, err := git.NewRepository(req.Repository).GetBranch(ctx, string(req.BranchName))
+	referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName)
+	revision, err := git.NewRepository(req.Repository).GetReference(ctx, referenceName)
 	if err != nil {
 		return nil, helper.ErrPreconditionFailed(err)
 	}
 
-	branch := fmt.Sprintf("refs/heads/%s", req.BranchName)
-
-	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, git.NullSHA, revision.Name); err != nil {
+	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, git.NullSHA, revision.Name); err != nil {
 		var preReceiveError preReceiveError
 		if errors.As(err, &preReceiveError) {
 			return &gitalypb.UserDeleteBranchResponse{