git: Convert UpdateRef to accept properly typed arguments

The `UpdateRef()` function acceepts reference as well as its old
respectively new values as plain strings. This makes it easy to miss
what the function is expected to get as input.

Let's convert it to accept a `ReferenceName` as well as two `ObjectID`s
and adjust all callers to pass those instead.

7 files changed, 67 insertions, 58 deletions

diff --git a/internal/git/repository.go b/internal/git/repository.go
index bdc1470db..fe852abac 100644
--- a/internal/git/repository.go
+++ b/internal/git/repository.go
@@ -411,18 +411,17 @@ func (repo *LocalRepository) GetBranches(ctx context.Context) ([]Reference, erro
 	return repo.GetReferences(ctx, "refs/heads/")
 }
 
-// UpdateRef updates reference from oldrev to newrev. If oldrev is a
-// non-empty string, the update will fail it the reference is not
-// currently at that revision. If newrev is the zero OID, the reference
-// will be deleted. If oldrev is the zero OID, the reference will
-// created.
-func (repo *LocalRepository) UpdateRef(ctx context.Context, reference ReferenceName, newrev, oldrev string) error {
+// UpdateRef updates reference from oldValue to newValue. If oldValue is a
+// non-empty string, the update will fail it the reference is not currently at
+// that revision. If newValue is the ZeroOID, the reference will be deleted.
+// If oldValue is the ZeroOID, the reference will created.
+func (repo *LocalRepository) UpdateRef(ctx context.Context, reference ReferenceName, newValue, oldValue ObjectID) error {
 	cmd, err := repo.command(ctx, nil,
 		SubCmd{
 			Name:  "update-ref",
 			Flags: []Option{Flag{Name: "-z"}, Flag{Name: "--stdin"}},
 		},
-		WithStdin(strings.NewReader(fmt.Sprintf("update %s\x00%s\x00%s\x00", reference, newrev, oldrev))),
+		WithStdin(strings.NewReader(fmt.Sprintf("update %s\x00%s\x00%s\x00", reference, newValue.String(), oldValue.String()))),
 		WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config),
 	)
 	if err != nil {
@@ -430,7 +429,7 @@ func (repo *LocalRepository) UpdateRef(ctx context.Context, reference ReferenceN
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("UpdateRef: failed updating reference %q from %q to %q: %v", reference, newrev, oldrev, err)
+		return fmt.Errorf("UpdateRef: failed updating reference %q from %q to %q: %w", reference, newValue, oldValue, err)
 	}
 
 	return nil
diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go
index 341c2362c..348b19d0f 100644
--- a/internal/git/repository_test.go
+++ b/internal/git/repository_test.go
@@ -21,8 +21,8 @@ import (
 )
 
 const (
-	MasterID      = "1e292f8fedd741b75372e19097c76d327140c312"
-	NonexistentID = "ba4f184e126b751d1bffad5897f263108befc780"
+	masterOID      = ObjectID("1e292f8fedd741b75372e19097c76d327140c312")
+	nonexistentOID = ObjectID("ba4f184e126b751d1bffad5897f263108befc780")
 )
 
 func TestLocalRepository(t *testing.T) {
@@ -89,7 +89,7 @@ func TestLocalRepository_GetReference(t *testing.T) {
 		{
 			desc:     "fully qualified master branch",
 			ref:      "refs/heads/master",
-			expected: NewReference("refs/heads/master", MasterID),
+			expected: NewReference("refs/heads/master", masterOID.String()),
 		},
 		{
 			desc:     "unqualified master branch fails",
@@ -140,7 +140,7 @@ func TestLocalRepository_GetReferences(t *testing.T) {
 			pattern: "refs/heads/master",
 			match: func(t *testing.T, refs []Reference) {
 				require.Equal(t, []Reference{
-					NewReference("refs/heads/master", MasterID),
+					NewReference("refs/heads/master", masterOID.String()),
 				}, refs)
 			},
 		},
@@ -404,17 +404,17 @@ func TestLocalRepository_UpdateRef(t *testing.T) {
 	require.NoError(t, err)
 
 	testcases := []struct {
-		desc   string
-		ref    string
-		newrev string
-		oldrev string
-		verify func(t *testing.T, repo *LocalRepository, err error)
+		desc     string
+		ref      string
+		newValue ObjectID
+		oldValue ObjectID
+		verify   func(t *testing.T, repo *LocalRepository, err error)
 	}{
 		{
-			desc:   "successfully update master",
-			ref:    "refs/heads/master",
-			newrev: otherRef.Target,
-			oldrev: MasterID,
+			desc:     "successfully update master",
+			ref:      "refs/heads/master",
+			newValue: ObjectID(otherRef.Target),
+			oldValue: masterOID,
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.NoError(t, err)
 				ref, err := repo.GetReference(ctx, "refs/heads/master")
@@ -423,34 +423,34 @@ func TestLocalRepository_UpdateRef(t *testing.T) {
 			},
 		},
 		{
-			desc:   "update fails with stale oldrev",
-			ref:    "refs/heads/master",
-			newrev: otherRef.Target,
-			oldrev: NonexistentID,
+			desc:     "update fails with stale oldValue",
+			ref:      "refs/heads/master",
+			newValue: ObjectID(otherRef.Target),
+			oldValue: nonexistentOID,
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.Error(t, err)
 				ref, err := repo.GetReference(ctx, "refs/heads/master")
 				require.NoError(t, err)
-				require.Equal(t, ref.Target, MasterID)
+				require.Equal(t, ref.Target, masterOID.String())
 			},
 		},
 		{
-			desc:   "update fails with invalid newrev",
-			ref:    "refs/heads/master",
-			newrev: NonexistentID,
-			oldrev: MasterID,
+			desc:     "update fails with invalid newValue",
+			ref:      "refs/heads/master",
+			newValue: nonexistentOID,
+			oldValue: masterOID,
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.Error(t, err)
 				ref, err := repo.GetReference(ctx, "refs/heads/master")
 				require.NoError(t, err)
-				require.Equal(t, ref.Target, MasterID)
+				require.Equal(t, ref.Target, masterOID.String())
 			},
 		},
 		{
-			desc:   "successfully update master with empty oldrev",
-			ref:    "refs/heads/master",
-			newrev: otherRef.Target,
-			oldrev: "",
+			desc:     "successfully update master with empty oldValue",
+			ref:      "refs/heads/master",
+			newValue: ObjectID(otherRef.Target),
+			oldValue: "",
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.NoError(t, err)
 				ref, err := repo.GetReference(ctx, "refs/heads/master")
@@ -459,22 +459,22 @@ func TestLocalRepository_UpdateRef(t *testing.T) {
 			},
 		},
 		{
-			desc:   "updating unqualified branch fails",
-			ref:    "master",
-			newrev: otherRef.Target,
-			oldrev: MasterID,
+			desc:     "updating unqualified branch fails",
+			ref:      "master",
+			newValue: ObjectID(otherRef.Target),
+			oldValue: masterOID,
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.Error(t, err)
 				ref, err := repo.GetReference(ctx, "refs/heads/master")
 				require.NoError(t, err)
-				require.Equal(t, ref.Target, MasterID)
+				require.Equal(t, ref.Target, masterOID.String())
 			},
 		},
 		{
-			desc:   "deleting master succeeds",
-			ref:    "refs/heads/master",
-			newrev: strings.Repeat("0", 40),
-			oldrev: MasterID,
+			desc:     "deleting master succeeds",
+			ref:      "refs/heads/master",
+			newValue: ZeroOID,
+			oldValue: masterOID,
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.NoError(t, err)
 				_, err = repo.GetReference(ctx, "refs/heads/master")
@@ -482,15 +482,15 @@ func TestLocalRepository_UpdateRef(t *testing.T) {
 			},
 		},
 		{
-			desc:   "creating new branch succeeds",
-			ref:    "refs/heads/new",
-			newrev: MasterID,
-			oldrev: strings.Repeat("0", 40),
+			desc:     "creating new branch succeeds",
+			ref:      "refs/heads/new",
+			newValue: masterOID,
+			oldValue: ZeroOID,
 			verify: func(t *testing.T, repo *LocalRepository, err error) {
 				require.NoError(t, err)
 				ref, err := repo.GetReference(ctx, "refs/heads/new")
 				require.NoError(t, err)
-				require.Equal(t, ref.Target, MasterID)
+				require.Equal(t, ref.Target, masterOID.String())
 			},
 		},
 	}
@@ -502,7 +502,7 @@ func TestLocalRepository_UpdateRef(t *testing.T) {
 			defer cleanup()
 
 			repo := NewRepository(testRepo)
-			err := repo.UpdateRef(ctx, ReferenceName(tc.ref), tc.newrev, tc.oldrev)
+			err := repo.UpdateRef(ctx, ReferenceName(tc.ref), tc.newValue, tc.oldValue)
 
 			tc.verify(t, repo, err)
 		})
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go
index ce026e194..2553a8ef1 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts.go
@@ -209,10 +209,15 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader
 		return err
 	}
 
+	commitOID, err := git.NewObjectIDFromHex(result.CommitID)
+	if err != nil {
+		return err
+	}
+
 	if err := git.NewRepository(header.GetRepository()).UpdateRef(
 		stream.Context(),
 		git.ReferenceName("refs/heads/"+string(header.GetSourceBranch())),
-		result.CommitID,
+		commitOID,
 		"",
 	); err != nil {
 		return err
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index de440a181..dda38314c 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -305,7 +305,7 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 	}
 
 	// First, overwrite the reference with the target reference.
-	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid.String(), ""); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid, ""); err != nil {
 		return nil, updateRefError{reference: string(request.TargetRef)}
 	}
 
@@ -326,15 +326,20 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 		return nil, helper.ErrPreconditionFailedf("Failed to create merge commit for source_sha %s and target_sha %s at %s", sourceRef, oid, string(request.TargetRef))
 	}
 
+	mergeOID, err := git.NewObjectIDFromHex(merge.CommitID)
+	if err != nil {
+		return nil, err
+	}
+
 	// ... and move branch from target ref to the merge commit. The Ruby
 	// implementation doesn't invoke hooks, so we don't either.
-	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), merge.CommitID, oid.String()); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), mergeOID, oid); err != nil {
 		//nolint:stylecheck
 		return nil, helper.ErrPreconditionFailed(fmt.Errorf("Could not update %s. Please refresh and try again", string(request.TargetRef)))
 	}
 
 	return &gitalypb.UserMergeToRefResponse{
-		CommitId: merge.CommitID,
+		CommitId: mergeOID.String(),
 	}, nil
 }
 
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 0f7752f7a..96d748ef4 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -271,7 +271,7 @@ func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) {
 
 	repo := git.NewRepository(testRepo)
 
-	master, err := repo.GetReference(ctx, "refs/heads/master")
+	masterOID, err := repo.ResolveRevision(ctx, "refs/heads/master")
 	require.NoError(t, err)
 
 	// We're now creating all kinds of potentially ambiguous references in
@@ -284,7 +284,7 @@ func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) {
 		"refs/tags/heads/" + mergeBranchName,
 		"refs/tags/refs/heads/" + mergeBranchName,
 	} {
-		require.NoError(t, repo.UpdateRef(ctx, git.ReferenceName(reference), master.Target, git.ZeroOID.String()))
+		require.NoError(t, repo.UpdateRef(ctx, git.ReferenceName(reference), masterOID, git.ZeroOID))
 	}
 
 	mergeCommitMessage := "Merged by Gitaly"
diff --git a/internal/gitaly/service/ref/pack_refs_test.go b/internal/gitaly/service/ref/pack_refs_test.go
index 252864afc..ba4f40851 100644
--- a/internal/gitaly/service/ref/pack_refs_test.go
+++ b/internal/gitaly/service/ref/pack_refs_test.go
@@ -36,7 +36,7 @@ func TestPackRefsSuccessfulRequest(t *testing.T) {
 	// creates some new heads
 	newBranches := 10
 	for i := 0; i < newBranches; i++ {
-		require.NoError(t, repo.UpdateRef(ctx, git.ReferenceName(fmt.Sprintf("refs/heads/new-ref-%d", i)), "refs/heads/master", git.ZeroOID.String()))
+		require.NoError(t, repo.UpdateRef(ctx, git.ReferenceName(fmt.Sprintf("refs/heads/new-ref-%d", i)), "refs/heads/master", git.ZeroOID))
 	}
 
 	// pack all refs
diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go
index 14c47238c..a3890ba4a 100644
--- a/internal/gitaly/service/repository/fetch.go
+++ b/internal/gitaly/service/repository/fetch.go
@@ -102,7 +102,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		}
 	}
 
-	if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid.String(), ""); err != nil {
+	if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid, ""); err != nil {
 		return nil, err
 	}