updateref: Accept object IDs instead of bare strings

Change the updateref package to accept object IDs as old and new
target of the reference that is to be updated. This increases type
safety and better documents the intent of provided functions.

7 files changed, 27 insertions, 27 deletions

diff --git a/cmd/gitaly-ssh/upload_pack_test.go b/cmd/gitaly-ssh/upload_pack_test.go
index a95e0c8a1..92f562960 100644
--- a/cmd/gitaly-ssh/upload_pack_test.go
+++ b/cmd/gitaly-ssh/upload_pack_test.go
@@ -37,7 +37,7 @@ func TestVisibilityOfHiddenRefs(t *testing.T) {
 	defer clean()
 
 	// Create a keep-around ref
-	existingSha := "1e292f8fedd741b75372e19097c76d327140c312"
+	existingSha := git.ObjectID("1e292f8fedd741b75372e19097c76d327140c312")
 	keepAroundRef := fmt.Sprintf("%s/%s", keepAroundNamespace, existingSha)
 
 	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index cb7516e5d..f99dd8e5d 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -256,7 +256,7 @@ func (o *ObjectPool) rescueDanglingObjects(ctx context.Context) error {
 		}
 
 		ref := git.ReferenceName(danglingObjectNamespace + split[2])
-		if err := updater.Create(ref, danglingObjectID.String()); err != nil {
+		if err := updater.Create(ref, danglingObjectID); err != nil {
 			return err
 		}
 	}
diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index 787711cc8..4fc24de9f 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -235,7 +235,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 		return fmt.Errorf("creating updater: %w", err)
 	}
 
-	if err := updater.Update(reference, newrev.String(), oldrev.String()); err != nil {
+	if err := updater.Update(reference, newrev, oldrev); err != nil {
 		return fmt.Errorf("queueing ref update: %w", err)
 	}
 
diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 54461b5fb..315ba0c3f 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -105,24 +105,24 @@ func New(ctx context.Context, repo git.RepositoryExecutor, opts ...UpdaterOpt) (
 	return updater, nil
 }
 
-// Update commands the reference to be updated to point at the object ID specified in newvalue. If
-// newvalue is the zero OID, then the branch will be deleted. If oldvalue is a non-empty string,
-// then the reference will only be updated if its current value matches the old value. If the old
-// value is the zero OID, then the branch must not exist.
-func (u *Updater) Update(reference git.ReferenceName, newvalue, oldvalue string) error {
-	_, err := fmt.Fprintf(u.cmd, "update %s\x00%s\x00%s\x00", reference.String(), newvalue, oldvalue)
+// Update commands the reference to be updated to point at the object ID specified in newOID. If
+// newOID is the zero OID, then the branch will be deleted. If oldOID is a non-empty string, then
+// the reference will only be updated if its current value matches the old value. If the old value
+// is the zero OID, then the branch must not exist.
+func (u *Updater) Update(reference git.ReferenceName, newOID, oldOID git.ObjectID) error {
+	_, err := fmt.Fprintf(u.cmd, "update %s\x00%s\x00%s\x00", reference.String(), newOID, oldOID)
 	return err
 }
 
 // Create commands the reference to be created with the given object ID. The ref must not exist.
-func (u *Updater) Create(reference git.ReferenceName, value string) error {
-	return u.Update(reference, value, git.ObjectHashSHA1.ZeroOID.String())
+func (u *Updater) Create(reference git.ReferenceName, oid git.ObjectID) error {
+	return u.Update(reference, oid, git.ObjectHashSHA1.ZeroOID)
 }
 
 // Delete commands the reference to be removed from the repository. This command will ignore any old
 // state of the reference and just force-remove it.
 func (u *Updater) Delete(reference git.ReferenceName) error {
-	return u.Update(reference, git.ObjectHashSHA1.ZeroOID.String(), "")
+	return u.Update(reference, git.ObjectHashSHA1.ZeroOID, "")
 }
 
 var refLockedRegex = regexp.MustCompile("cannot lock ref '(.+?)'")
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index 1e2185c45..90a3216e6 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -44,7 +44,7 @@ func TestUpdater_create(t *testing.T) {
 
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	require.NoError(t, updater.Create("refs/heads/_create", commitID.String()))
+	require.NoError(t, updater.Create("refs/heads/_create", commitID))
 	require.NoError(t, updater.Commit())
 
 	// Verify that the reference was created as expected and that it points to the correct
@@ -66,14 +66,14 @@ func TestUpdater_update(t *testing.T) {
 	// Check that we can force-update the reference when we don't give an old object ID.
 	updater, err := New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, updater.Update("refs/heads/main", newCommitID.String(), ""))
+	require.NoError(t, updater.Update("refs/heads/main", newCommitID, ""))
 	require.NoError(t, updater.Commit())
 	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), newCommitID)
 
 	// Check that we can update with safety guards when giving an old commit ID.
 	updater, err = New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, updater.Update("refs/heads/main", oldCommitID.String(), newCommitID.String()))
+	require.NoError(t, updater.Update("refs/heads/main", oldCommitID, newCommitID))
 	require.NoError(t, updater.Commit())
 	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), oldCommitID)
 
@@ -81,7 +81,7 @@ func TestUpdater_update(t *testing.T) {
 	// when the old commit ID doesn't match.
 	updater, err = New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, updater.Update("refs/heads/main", newCommitID.String(), otherCommitID.String()))
+	require.NoError(t, updater.Update("refs/heads/main", newCommitID, otherCommitID))
 	err = updater.Commit()
 	require.Error(t, err)
 	require.Contains(t, err.Error(), fmt.Sprintf("fatal: commit: cannot lock ref 'refs/heads/main': is at %s but expected %s", oldCommitID, otherCommitID))
@@ -115,9 +115,9 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	require.NoError(t, updater.Update("refs/heads/feature", commitID.String(), ""))
+	require.NoError(t, updater.Update("refs/heads/feature", commitID, ""))
 	require.NoError(t, updater.Prepare())
-	require.NoError(t, updater.Update("refs/heads/feature", commitID.String(), ""))
+	require.NoError(t, updater.Update("refs/heads/feature", commitID, ""))
 
 	err := updater.Commit()
 	require.Error(t, err, "cannot update after prepare")
@@ -144,13 +144,13 @@ func TestUpdater_concurrentLocking(t *testing.T) {
 	// we're about to update is locked.
 	firstUpdater, err := New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, firstUpdater.Update("refs/heads/master", commitID.String(), ""))
+	require.NoError(t, firstUpdater.Update("refs/heads/master", commitID, ""))
 	require.NoError(t, firstUpdater.Prepare())
 
 	// Now we create a second updater that tries to update the same reference.
 	secondUpdater, err := New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, secondUpdater.Update("refs/heads/master", commitID.String(), ""))
+	require.NoError(t, secondUpdater.Update("refs/heads/master", commitID, ""))
 
 	// Preparing this second updater should fail though because we notice that the reference is
 	// locked.
@@ -181,7 +181,7 @@ func TestUpdater_bulkOperation(t *testing.T) {
 			Target: commitID.String(),
 		}
 
-		require.NoError(t, updater.Create(reference.Name, commitID.String()))
+		require.NoError(t, updater.Create(reference.Name, commitID))
 		expectedRefs = append(expectedRefs, reference)
 	}
 
@@ -206,7 +206,7 @@ func TestUpdater_contextCancellation(t *testing.T) {
 	updater, err := New(childCtx, repo)
 	require.NoError(t, err)
 
-	require.NoError(t, updater.Create("refs/heads/main", commitID.String()))
+	require.NoError(t, updater.Create("refs/heads/main", commitID))
 
 	// Force the update-ref process to terminate early by cancelling the context.
 	childCancel()
@@ -264,7 +264,7 @@ func TestUpdater_closingStdinAbortsChanges(t *testing.T) {
 
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	require.NoError(t, updater.Create("refs/heads/main", commitID.String()))
+	require.NoError(t, updater.Create("refs/heads/main", commitID))
 
 	// Note that we call `Wait()` on the command, not on the updater. This
 	// circumvents our usual semantics of sending "commit" and thus
@@ -286,8 +286,8 @@ func TestUpdater_capturesStderr(t *testing.T) {
 
 	cfg, _, _, updater := setupUpdater(t, ctx)
 
-	newValue := strings.Repeat("1", gittest.DefaultObjectHash.EncodedLen())
-	oldValue := gittest.DefaultObjectHash.ZeroOID.String()
+	newValue := git.ObjectID(strings.Repeat("1", gittest.DefaultObjectHash.EncodedLen()))
+	oldValue := gittest.DefaultObjectHash.ZeroOID
 
 	require.NoError(t, updater.Update("refs/heads/main", newValue, oldValue))
 
diff --git a/internal/gitaly/service/ref/delete_refs_test.go b/internal/gitaly/service/ref/delete_refs_test.go
index 4210bae9d..a821999f7 100644
--- a/internal/gitaly/service/ref/delete_refs_test.go
+++ b/internal/gitaly/service/ref/delete_refs_test.go
@@ -241,7 +241,7 @@ func testDeleteRefsRefLocked(t *testing.T, ctx context.Context) {
 	require.NoError(t, updater.Update(
 		git.ReferenceName("refs/heads/master"),
 		"0b4bc9a49b562e85de7cc9e834518ea6828729b9",
-		oldValue.String(),
+		oldValue,
 	))
 	require.NoError(t, updater.Prepare())
 
diff --git a/internal/gitaly/service/repository/write_ref.go b/internal/gitaly/service/repository/write_ref.go
index 3997b5462..32271acfa 100644
--- a/internal/gitaly/service/repository/write_ref.go
+++ b/internal/gitaly/service/repository/write_ref.go
@@ -60,7 +60,7 @@ func updateRef(ctx context.Context, repo *localrepo.Repo, req *gitalypb.WriteRef
 		return fmt.Errorf("error when running creating new updater: %v", err)
 	}
 
-	if err = u.Update(git.ReferenceName(req.GetRef()), newObjectID.String(), oldObjectID.String()); err != nil {
+	if err = u.Update(git.ReferenceName(req.GetRef()), newObjectID, oldObjectID); err != nil {
 		return fmt.Errorf("error when creating update-ref command: %v", err)
 	}