Merge branch 'pks-git-updateref-sha256' into 'master'

updateref: Support repositories with SHA256 object hashes

See merge request gitlab-org/gitaly!4778

9 files changed, 284 insertions, 175 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 8dcdfdee2..0b399dc13 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -259,8 +259,13 @@ func (o *ObjectPool) rescueDanglingObjects(ctx context.Context) error {
 			continue
 		}
 
+		danglingObjectID, err := git.ObjectHashSHA1.FromHex(split[2])
+		if err != nil {
+			return fmt.Errorf("parsing object ID %q: %w", split[2], err)
+		}
+
 		ref := git.ReferenceName(danglingObjectNamespace + split[2])
-		if err := updater.Create(ref, split[2]); 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..f7d82a3d5 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -146,7 +146,7 @@ func NewUpdaterWithHooks(
 // repository.
 func (u *UpdaterWithHooks) UpdateReference(
 	ctx context.Context,
-	repo *gitalypb.Repository,
+	repoProto *gitalypb.Repository,
 	user *gitalypb.User,
 	quarantineDir *quarantine.Dir,
 	reference git.ReferenceName,
@@ -160,13 +160,20 @@ func (u *UpdaterWithHooks) UpdateReference(
 		return fmt.Errorf("getting transaction: %w", err)
 	}
 
+	repo := u.localrepo(repoProto)
+
+	objectHash, err := repo.ObjectHash(ctx)
+	if err != nil {
+		return fmt.Errorf("detecting object hash: %w", err)
+	}
+
 	if reference == "" {
 		return fmt.Errorf("reference cannot be empty")
 	}
-	if err := git.ObjectHashSHA1.ValidateHex(oldrev.String()); err != nil {
+	if err := objectHash.ValidateHex(oldrev.String()); err != nil {
 		return fmt.Errorf("validating old value: %w", err)
 	}
-	if err := git.ObjectHashSHA1.ValidateHex(newrev.String()); err != nil {
+	if err := objectHash.ValidateHex(newrev.String()); err != nil {
 		return fmt.Errorf("validating new value: %w", err)
 	}
 
@@ -183,7 +190,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 	// repository, which carries information about the quarantined object directory. This is
 	// then subsequently passed to Rails, which can use the quarantine directory to more
 	// efficiently query which objects are new.
-	quarantinedRepo := repo
+	quarantinedRepo := repoProto
 	if quarantineDir != nil {
 		quarantinedRepo = quarantineDir.QuarantinedRepo()
 	}
@@ -210,7 +217,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 		// We only need to update the hooks payload to the unquarantined repo in case we
 		// had a quarantine environment. Otherwise, the initial hooks payload is for the
 		// real repository anyway.
-		hooksPayload, err = git.NewHooksPayload(u.cfg, repo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env()
+		hooksPayload, err = git.NewHooksPayload(u.cfg, repoProto, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env()
 		if err != nil {
 			return fmt.Errorf("constructing quarantined hooks payload: %w", err)
 		}
@@ -230,12 +237,12 @@ func (u *UpdaterWithHooks) UpdateReference(
 	// is packed, which is obviously a bad thing as Gitaly nodes may be differently packed. We
 	// thus continue to manually drive the reference-transaction hook here, which doesn't have
 	// this problem.
-	updater, err := New(ctx, u.localrepo(repo), WithDisabledTransactions())
+	updater, err := New(ctx, repo, WithDisabledTransactions())
 	if err != nil {
 		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)
 	}
 
@@ -268,7 +275,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 		return fmt.Errorf("executing committing reference-transaction hook: %w", err)
 	}
 
-	if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
+	if err := u.hookManager.PostReceiveHook(ctx, repoProto, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
 		// CustomHook errors are returned in case a custom hook has returned an error code.
 		// The post-receive hook has special semantics though. Quoting githooks(5):
 		//
diff --git a/internal/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go
index 7bd5a2e2b..808b06454 100644
--- a/internal/git/updateref/update_with_hooks_test.go
+++ b/internal/git/updateref/update_with_hooks_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package updateref_test
 
 import (
@@ -29,20 +27,17 @@ import (
 )
 
 func TestUpdaterWithHooks_UpdateReference_invalidParameters(t *testing.T) {
-	ctx := testhelper.Context(t)
+	t.Parallel()
 
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
-
-	user := &gitalypb.User{
-		GlId:       "1234",
-		GlUsername: "Username",
-		Name:       []byte("Name"),
-		Email:      []byte("mail@example.com"),
-	}
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
+	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
+	repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
 
-	revA, revB := git.ObjectID(strings.Repeat("a", 40)), git.ObjectID(strings.Repeat("b", 40))
+	revA := git.ObjectID(strings.Repeat("a", gittest.DefaultObjectHash.EncodedLen()))
+	revB := git.ObjectID(strings.Repeat("b", gittest.DefaultObjectHash.EncodedLen()))
 
-	updater := updateref.NewUpdaterWithHooks(cfg, nil, &hook.MockManager{}, nil, nil)
+	updater := updateref.NewUpdaterWithHooks(cfg, nil, &hook.MockManager{}, gitCmdFactory, nil)
 
 	testCases := []struct {
 		desc           string
@@ -86,16 +81,17 @@ func TestUpdaterWithHooks_UpdateReference_invalidParameters(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			err := updater.UpdateReference(ctx, repo, user, nil, tc.ref, tc.newRev, tc.oldRev)
+			err := updater.UpdateReference(ctx, repo, gittest.TestUser, nil, tc.ref, tc.newRev, tc.oldRev)
 			require.Equal(t, tc.expectedErr, err)
 		})
 	}
 }
 
 func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
-	ctx := testhelper.Context(t)
+	t.Parallel()
 
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
 
 	// We need to set up a separate "real" hook service here, as it will be used in
 	// git-update-ref(1) spawned by `updateRefWithHooks()`
@@ -103,21 +99,15 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 		gitalypb.RegisterHookServiceServer(srv, hookservice.NewServer(deps.GetHookManager(), deps.GetGitCmdFactory(), deps.GetPackObjectsCache(), deps.GetPackObjectsConcurrencyTracker()))
 	})
 
-	user := &gitalypb.User{
-		GlId:       "1234",
-		GlUsername: "Username",
-		Name:       []byte("Name"),
-		Email:      []byte("mail@example.com"),
-	}
-
-	oldRev := "1e292f8fedd741b75372e19097c76d327140c312"
+	repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
 	requirePayload := func(t *testing.T, env []string) {
 		require.Len(t, env, 1)
 
 		expectedPayload := git.NewHooksPayload(cfg, repo, nil, &git.UserDetails{
-			UserID:   "1234",
-			Username: "Username",
+			UserID:   gittest.TestUser.GlId,
+			Username: gittest.TestUser.GlUsername,
 			Protocol: "web",
 		}, git.ReceivePackHooks, featureflag.FromContext(ctx))
 
@@ -147,22 +137,22 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 				changes, err := io.ReadAll(stdin)
 				require.NoError(t, err)
-				require.Equal(t, fmt.Sprintf("%s %s refs/heads/master\n", oldRev, git.ObjectHashSHA1.ZeroOID.String()), string(changes))
+				require.Equal(t, fmt.Sprintf("%s %s refs/heads/main\n", commitID, gittest.DefaultObjectHash.ZeroOID.String()), string(changes))
 				require.Empty(t, pushOptions)
 				requirePayload(t, env)
 				return nil
 			},
 			update: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, ref, oldValue, newValue string, env []string, stdout, stderr io.Writer) error {
-				require.Equal(t, "refs/heads/master", ref)
-				require.Equal(t, oldRev, oldValue)
-				require.Equal(t, newValue, git.ObjectHashSHA1.ZeroOID.String())
+				require.Equal(t, "refs/heads/main", ref)
+				require.Equal(t, commitID.String(), oldValue)
+				require.Equal(t, newValue, gittest.DefaultObjectHash.ZeroOID.String())
 				requirePayload(t, env)
 				return nil
 			},
 			postReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 				changes, err := io.ReadAll(stdin)
 				require.NoError(t, err)
-				require.Equal(t, fmt.Sprintf("%s %s refs/heads/master\n", oldRev, git.ObjectHashSHA1.ZeroOID.String()), string(changes))
+				require.Equal(t, fmt.Sprintf("%s %s refs/heads/main\n", commitID.String(), gittest.DefaultObjectHash.ZeroOID.String()), string(changes))
 				requirePayload(t, env)
 				require.Empty(t, pushOptions)
 				return nil
@@ -170,7 +160,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			referenceTransaction: func(t *testing.T, ctx context.Context, state hook.ReferenceTransactionState, env []string, stdin io.Reader) error {
 				changes, err := io.ReadAll(stdin)
 				require.NoError(t, err)
-				require.Equal(t, fmt.Sprintf("%s %s refs/heads/master\n", oldRev, git.ObjectHashSHA1.ZeroOID.String()), string(changes))
+				require.Equal(t, fmt.Sprintf("%s %s refs/heads/main\n", commitID.String(), gittest.DefaultObjectHash.ZeroOID.String()), string(changes))
 
 				require.Less(t, referenceTransactionCalls, 2)
 				if referenceTransactionCalls == 0 {
@@ -257,7 +247,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			gitCmdFactory := gittest.NewCommandFactory(t, cfg)
 			updater := updateref.NewUpdaterWithHooks(cfg, config.NewLocator(cfg), hookManager, gitCmdFactory, nil)
 
-			err := updater.UpdateReference(ctx, repo, user, nil, git.ReferenceName("refs/heads/master"), git.ObjectHashSHA1.ZeroOID, git.ObjectID(oldRev))
+			err := updater.UpdateReference(ctx, repo, gittest.TestUser, nil, git.ReferenceName("refs/heads/main"), gittest.DefaultObjectHash.ZeroOID, commitID)
 			if tc.expectedErr == "" {
 				require.NoError(t, err)
 			} else {
@@ -265,24 +255,29 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			}
 
 			if tc.expectedRefDeletion {
-				contained, err := localrepo.NewTestRepo(t, cfg, repo).HasRevision(ctx, git.Revision("refs/heads/master"))
+				contained, err := localrepo.NewTestRepo(t, cfg, repo).HasRevision(ctx, git.Revision("refs/heads/main"))
 				require.NoError(t, err)
 				require.False(t, contained, "branch should have been deleted")
-				gittest.Exec(t, cfg, "-C", repoPath, "branch", "master", oldRev)
+				gittest.Exec(t, cfg, "-C", repoPath, "branch", "main", commitID.String())
 			} else {
-				ref, err := localrepo.NewTestRepo(t, cfg, repo).GetReference(ctx, "refs/heads/master")
+				ref, err := localrepo.NewTestRepo(t, cfg, repo).GetReference(ctx, "refs/heads/main")
 				require.NoError(t, err)
-				require.Equal(t, ref.Target, oldRev)
+				require.Equal(t, ref.Target, commitID.String())
 			}
 		})
 	}
 }
 
 func TestUpdaterWithHooks_quarantine(t *testing.T) {
-	cfg, repoProto, _ := testcfg.BuildWithRepo(t)
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
 	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
 	locator := config.NewLocator(cfg)
-	ctx := testhelper.Context(t)
+
+	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
 	unquarantinedRepo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -366,9 +361,9 @@ func TestUpdaterWithHooks_quarantine(t *testing.T) {
 			Email:      []byte("mail@example.com"),
 		},
 		quarantine,
-		git.ReferenceName("refs/heads/master"),
-		git.ObjectHashSHA1.ZeroOID,
-		git.ObjectID("1e292f8fedd741b75372e19097c76d327140c312"),
+		git.ReferenceName("refs/heads/main"),
+		gittest.DefaultObjectHash.ZeroOID,
+		commitID,
 	))
 
 	require.Equal(t, map[string]int{
@@ -379,7 +374,7 @@ func TestUpdaterWithHooks_quarantine(t *testing.T) {
 		"commit":      1,
 	}, hookExecutions)
 
-	contained, err := unquarantinedRepo.HasRevision(ctx, git.Revision("refs/heads/master"))
+	contained, err := unquarantinedRepo.HasRevision(ctx, git.Revision("refs/heads/main"))
 	require.NoError(t, err)
 	require.False(t, contained, "branch should have been deleted")
 }
diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 54461b5fb..d99b49750 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -25,10 +25,11 @@ func (e *ErrAlreadyLocked) Error() string {
 // that allows references to be easily updated in bulk. It is not suitable for
 // concurrent use.
 type Updater struct {
-	repo   git.RepositoryExecutor
-	cmd    *command.Command
-	stdout *bufio.Reader
-	stderr *bytes.Buffer
+	repo       git.RepositoryExecutor
+	cmd        *command.Command
+	stdout     *bufio.Reader
+	stderr     *bytes.Buffer
+	objectHash git.ObjectHash
 
 	// withStatusFlushing determines whether the Git version used supports proper flushing of
 	// status messages.
@@ -86,11 +87,17 @@ func New(ctx context.Context, repo git.RepositoryExecutor, opts ...UpdaterOpt) (
 		return nil, fmt.Errorf("determining git version: %w", err)
 	}
 
+	objectHash, err := repo.ObjectHash(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("detecting object hash: %w", err)
+	}
+
 	updater := &Updater{
 		repo:               repo,
 		cmd:                cmd,
 		stderr:             &stderr,
 		stdout:             bufio.NewReader(cmd),
+		objectHash:         objectHash,
 		withStatusFlushing: gitVersion.FlushesUpdaterefStatus(),
 	}
 
@@ -105,24 +112,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, u.objectHash.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, u.objectHash.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 008d6f889..96cad19b2 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package updateref
 
 import (
@@ -21,101 +19,103 @@ func TestMain(m *testing.M) {
 	testhelper.Run(m)
 }
 
-func setupUpdater(t *testing.T, ctx context.Context) (config.Cfg, *localrepo.Repo, *Updater) {
+func setupUpdater(t *testing.T, ctx context.Context) (config.Cfg, *localrepo.Repo, string, *Updater) {
 	t.Helper()
 
-	cfg, protoRepo, _ := testcfg.BuildWithRepo(t)
+	cfg := testcfg.Build(t)
 
-	repo := localrepo.NewTestRepo(t, cfg, protoRepo, git.WithSkipHooks())
+	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	repo := localrepo.NewTestRepo(t, cfg, repoProto, git.WithSkipHooks())
 
 	updater, err := New(ctx, repo)
 	require.NoError(t, err)
 
-	return cfg, repo, updater
+	return cfg, repo, repoPath, updater
 }
 
-func TestCreate(t *testing.T) {
-	ctx := testhelper.Context(t)
+func TestUpdater_create(t *testing.T) {
+	t.Parallel()
 
-	_, repo, updater := setupUpdater(t, ctx)
+	ctx := testhelper.Context(t)
 
-	headCommit, err := repo.ReadCommit(ctx, "HEAD")
-	require.NoError(t, err)
+	cfg, _, repoPath, updater := setupUpdater(t, ctx)
 
-	ref := git.ReferenceName("refs/heads/_create")
-	sha := headCommit.Id
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	require.NoError(t, updater.Create(ref, sha))
+	require.NoError(t, updater.Create("refs/heads/_create", commitID))
 	require.NoError(t, updater.Commit())
 
-	// check the ref was created
-	commit, logErr := repo.ReadCommit(ctx, ref.Revision())
-	require.NoError(t, logErr)
-	require.Equal(t, commit.Id, sha, "reference was created with the wrong SHA")
+	// Verify that the reference was created as expected and that it points to the correct
+	// commit.
+	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/_create"), commitID)
 }
 
-func TestUpdate(t *testing.T) {
-	ctx := testhelper.Context(t)
-
-	_, repo, updater := setupUpdater(t, ctx)
+func TestUpdater_update(t *testing.T) {
+	t.Parallel()
 
-	headCommit, err := repo.ReadCommit(ctx, "HEAD")
-	require.NoError(t, err)
+	ctx := testhelper.Context(t)
 
-	ref := git.ReferenceName("refs/heads/feature")
-	sha := headCommit.Id
+	cfg, repo, repoPath, _ := setupUpdater(t, ctx)
 
-	// Sanity check: ensure the ref exists before we start
-	commit, logErr := repo.ReadCommit(ctx, ref.Revision())
-	require.NoError(t, logErr)
-	require.NotEqual(t, commit.Id, sha, "%s points to HEAD: %s in the test repository", ref.String(), sha)
+	oldCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+	newCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(oldCommitID))
+	otherCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("other"))
 
-	require.NoError(t, updater.Update(ref, sha, ""))
-	require.NoError(t, updater.Prepare())
+	// 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, ""))
 	require.NoError(t, updater.Commit())
+	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), newCommitID)
 
-	// check the ref was updated
-	commit, logErr = repo.ReadCommit(ctx, ref.Revision())
-	require.NoError(t, logErr)
-	require.Equal(t, commit.Id, sha, "reference was not updated")
+	// 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, newCommitID))
+	require.NoError(t, updater.Commit())
+	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), oldCommitID)
 
-	// since ref has been updated to HEAD, we know that it does not point to HEAD^. So, HEAD^ is an invalid "old value" for updating ref
-	parentCommit, err := repo.ReadCommit(ctx, "HEAD^")
+	// And finally assert that we fail to update the reference in case we're trying to update
+	// when the old commit ID doesn't match.
+	updater, err = New(ctx, repo)
 	require.NoError(t, err)
-	require.Error(t, updater.Update(ref, parentCommit.Id, parentCommit.Id))
+	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))
 
-	// check the ref was not updated
-	commit, logErr = repo.ReadCommit(ctx, ref.Revision())
-	require.NoError(t, logErr)
-	require.NotEqual(t, commit.Id, parentCommit.Id, "reference was updated when it shouldn't have been")
+	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), oldCommitID)
 }
 
-func TestDelete(t *testing.T) {
+func TestUpdater_delete(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	_, repo, updater := setupUpdater(t, ctx)
+	cfg, repo, repoPath, updater := setupUpdater(t, ctx)
 
-	ref := git.ReferenceName("refs/heads/feature")
+	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
-	require.NoError(t, updater.Delete(ref))
+	require.NoError(t, updater.Delete("refs/heads/main"))
 	require.NoError(t, updater.Commit())
 
-	// check the ref was removed
-	_, err := repo.ReadCommit(ctx, ref.Revision())
-	require.Equal(t, localrepo.ErrObjectNotFound, err, "expected 'not found' error got %v", err)
+	// Check that the reference was removed.
+	_, err := repo.ReadCommit(ctx, "refs/heads/main")
+	require.Equal(t, localrepo.ErrObjectNotFound, err)
 }
 
 func TestUpdater_prepareLocksTransaction(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	_, repo, updater := setupUpdater(t, ctx)
+	cfg, _, repoPath, updater := setupUpdater(t, ctx)
 
-	commit, logErr := repo.ReadCommit(ctx, "refs/heads/master")
-	require.NoError(t, logErr)
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	require.NoError(t, updater.Update("refs/heads/feature", commit.Id, ""))
+	require.NoError(t, updater.Update("refs/heads/feature", commitID, ""))
 	require.NoError(t, updater.Prepare())
-	require.NoError(t, updater.Update("refs/heads/feature", commit.Id, ""))
+	require.NoError(t, updater.Update("refs/heads/feature", commitID, ""))
 
 	err := updater.Commit()
 	require.Error(t, err, "cannot update after prepare")
@@ -125,27 +125,33 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 func TestUpdater_concurrentLocking(t *testing.T) {
 	t.Parallel()
 
-	cfg, protoRepo, _ := testcfg.BuildWithRepo(t)
 	ctx := testhelper.Context(t)
 
+	cfg := testcfg.Build(t)
+
 	if !gittest.GitSupportsStatusFlushing(t, ctx, cfg) {
 		t.Skip("git does not support flushing yet, which is known to be flaky")
 	}
 
-	repo := localrepo.NewTestRepo(t, cfg, protoRepo, git.WithSkipHooks())
+	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	repo := localrepo.NewTestRepo(t, cfg, repoProto, git.WithSkipHooks())
 
-	commit, logErr := repo.ReadCommit(ctx, "refs/heads/master")
-	require.NoError(t, logErr)
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
+	// Create the first updater that prepares the reference transaction so that the reference
+	// we're about to update is locked.
 	firstUpdater, err := New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, firstUpdater.Update("refs/heads/master", "", commit.Id))
+	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", "", commit.Id))
+	require.NoError(t, secondUpdater.Update("refs/heads/master", commitID, ""))
 
+	// Preparing this second updater should fail though because we notice that the reference is
+	// locked.
 	err = secondUpdater.Prepare()
 	var errAlreadyLocked *ErrAlreadyLocked
 	require.ErrorAs(t, err, &errAlreadyLocked)
@@ -153,74 +159,89 @@ func TestUpdater_concurrentLocking(t *testing.T) {
 		Ref: "refs/heads/master",
 	})
 
+	// Whereas committing the first transaction should succeed.
 	require.NoError(t, firstUpdater.Commit())
 }
 
-func TestBulkOperation(t *testing.T) {
+func TestUpdater_bulkOperation(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	_, repo, updater := setupUpdater(t, ctx)
+	cfg, repo, repoPath, updater := setupUpdater(t, ctx)
 
-	headCommit, err := repo.ReadCommit(ctx, "HEAD")
-	require.NoError(t, err)
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
+	var expectedRefs []git.Reference
 	for i := 0; i < 1000; i++ {
-		ref := fmt.Sprintf("refs/head/_test_%d", i)
-		require.NoError(t, updater.Create(git.ReferenceName(ref), headCommit.Id), "Failed to create ref %d", i)
+		reference := git.Reference{
+			Name:   git.ReferenceName(fmt.Sprintf("refs/head/test_%d", i)),
+			Target: commitID.String(),
+		}
+
+		require.NoError(t, updater.Create(reference.Name, commitID))
+		expectedRefs = append(expectedRefs, reference)
 	}
 
 	require.NoError(t, updater.Commit())
 
 	refs, err := repo.GetReferences(ctx, "refs/")
 	require.NoError(t, err)
-	require.Greater(t, len(refs), 1000, "At least 1000 refs should be present")
+	require.ElementsMatch(t, expectedRefs, refs)
 }
 
-func TestContextCancelAbortsRefChanges(t *testing.T) {
+func TestUpdater_contextCancellation(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
+	childCtx, childCancel := context.WithCancel(ctx)
 
-	cfg, repo, _ := setupUpdater(t, ctx)
+	cfg, repoProto, repoPath, _ := setupUpdater(t, ctx)
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-	headCommit, err := repo.ReadCommit(ctx, "HEAD")
-	require.NoError(t, err)
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	childCtx, childCancel := context.WithCancel(ctx)
-	localRepo := localrepo.NewTestRepo(t, cfg, repo)
-	updater, err := New(childCtx, localRepo)
+	updater, err := New(childCtx, repo)
 	require.NoError(t, err)
 
-	ref := git.ReferenceName("refs/heads/_shouldnotexist")
+	require.NoError(t, updater.Create("refs/heads/main", commitID))
 
-	require.NoError(t, updater.Create(ref, headCommit.Id))
-
-	// Force the update-ref process to terminate early
+	// Force the update-ref process to terminate early by cancelling the context.
 	childCancel()
+
+	// We should see that committing the update fails now.
 	require.Error(t, updater.Commit())
 
-	// check the ref doesn't exist
-	_, err = repo.ReadCommit(ctx, ref.Revision())
-	require.Equal(t, localrepo.ErrObjectNotFound, err, "expected 'not found' error got %v", err)
+	// And the reference should not have been created.
+	_, err = repo.ReadCommit(ctx, "refs/heads/main")
+	require.Equal(t, localrepo.ErrObjectNotFound, err)
 }
 
 func TestUpdater_cancel(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	cfg, repo, updater := setupUpdater(t, ctx)
+	cfg, repo, repoPath, updater := setupUpdater(t, ctx)
 
 	if !gittest.GitSupportsStatusFlushing(t, ctx, cfg) {
 		t.Skip("git does not support flushing yet, which is known to be flaky")
 	}
 
-	require.NoError(t, updater.Delete(git.ReferenceName("refs/heads/master")))
+	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+
+	// Queue the branch for deletion and lock it.
+	require.NoError(t, updater.Delete(git.ReferenceName("refs/heads/main")))
 	require.NoError(t, updater.Prepare())
 
-	// A concurrent update shouldn't be allowed.
+	// Try to delete the same reference via a concurrent updater. This should not be allowed
+	// because the reference is locked already.
 	concurrentUpdater, err := New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, concurrentUpdater.Delete(git.ReferenceName("refs/heads/master")))
+	require.NoError(t, concurrentUpdater.Delete(git.ReferenceName("refs/heads/main")))
 	err = concurrentUpdater.Commit()
-	require.NotNil(t, err)
-	require.Contains(t, err.Error(), "fatal: commit: cannot lock ref 'refs/heads/master'")
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "fatal: commit: cannot lock ref 'refs/heads/main'")
 
 	// We now cancel the initial updater. Afterwards, it should be possible again to update the
 	// ref because locks should have been released.
@@ -228,21 +249,20 @@ func TestUpdater_cancel(t *testing.T) {
 
 	concurrentUpdater, err = New(ctx, repo)
 	require.NoError(t, err)
-	require.NoError(t, concurrentUpdater.Delete(git.ReferenceName("refs/heads/master")))
+	require.NoError(t, concurrentUpdater.Delete(git.ReferenceName("refs/heads/main")))
 	require.NoError(t, concurrentUpdater.Commit())
 }
 
 func TestUpdater_closingStdinAbortsChanges(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	_, repo, updater := setupUpdater(t, ctx)
+	cfg, repo, repoPath, updater := setupUpdater(t, ctx)
 
-	headCommit, err := repo.ReadCommit(ctx, "HEAD")
-	require.NoError(t, err)
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	ref := git.ReferenceName("refs/heads/shouldnotexist")
-
-	require.NoError(t, updater.Create(ref, headCommit.Id))
+	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
@@ -253,30 +273,30 @@ func TestUpdater_closingStdinAbortsChanges(t *testing.T) {
 
 	// ... but as we now use explicit transactional behaviour, this is no
 	// longer the case.
-	_, err = repo.ReadCommit(ctx, ref.Revision())
+	_, err := repo.ReadCommit(ctx, "refs/heads/main")
 	require.Equal(t, localrepo.ErrObjectNotFound, err, "expected 'not found' error got %v", err)
 }
 
 func TestUpdater_capturesStderr(t *testing.T) {
 	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	cfg, _, updater := setupUpdater(t, ctx)
+	cfg, _, _, updater := setupUpdater(t, ctx)
 
-	ref := "refs/heads/a"
-	newValue := strings.Repeat("1", 40)
-	oldValue := git.ObjectHashSHA1.ZeroOID.String()
+	newValue := git.ObjectID(strings.Repeat("1", gittest.DefaultObjectHash.EncodedLen()))
+	oldValue := gittest.DefaultObjectHash.ZeroOID
 
-	require.NoError(t, updater.Update(git.ReferenceName(ref), newValue, oldValue))
+	require.NoError(t, updater.Update("refs/heads/main", newValue, oldValue))
 
 	var expectedErr string
 	if gittest.GitSupportsStatusFlushing(t, ctx, cfg) {
-		expectedErr = fmt.Sprintf("state update to \"commit\" failed: EOF, stderr: \"fatal: commit: cannot update ref '%s': "+
-			"trying to write ref '%s' with nonexistent object %s\\n\"", ref, ref, newValue)
+		expectedErr = fmt.Sprintf("state update to \"commit\" failed: EOF, stderr: \"fatal: commit: cannot update ref '%[1]s': "+
+			"trying to write ref '%[1]s' with nonexistent object %[2]s\\n\"", "refs/heads/main", newValue)
 	} else {
 		expectedErr = fmt.Sprintf("git update-ref: exit status 128, stderr: "+
-			"\"fatal: commit: cannot update ref '%s': "+
-			"trying to write ref '%s' with nonexistent object %s\\n\"", ref, ref, newValue)
+			"\"fatal: commit: cannot update ref '%[1]s': "+
+			"trying to write ref '%[1]s' with nonexistent object %[2]s\\n\"", "refs/heads/main", newValue)
 	}
 
 	err := updater.Commit()
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 5079ebc03..32271acfa 100644
--- a/internal/gitaly/service/repository/write_ref.go
+++ b/internal/gitaly/service/repository/write_ref.go
@@ -38,16 +38,36 @@ func (s *server) writeRef(ctx context.Context, req *gitalypb.WriteRefRequest) er
 }
 
 func updateRef(ctx context.Context, repo *localrepo.Repo, req *gitalypb.WriteRefRequest) error {
+	// We need to resolve the new revision in order to make sure that we're actually passing an
+	// object ID to git-update-ref(1), but more importantly this will also ensure that the
+	// object ID we're updating to actually exists. Note that we also verify that the object
+	// actually exists in the repository by adding "^{object}".
+	newObjectID, err := repo.ResolveRevision(ctx, git.Revision(req.GetRevision())+"^{object}")
+	if err != nil {
+		return fmt.Errorf("resolving new revision: %w", err)
+	}
+
+	var oldObjectID git.ObjectID
+	if len(req.GetOldRevision()) > 0 {
+		oldObjectID, err = repo.ResolveRevision(ctx, git.Revision(req.GetOldRevision())+"^{object}")
+		if err != nil {
+			return fmt.Errorf("resolving old revision: %w", err)
+		}
+	}
+
 	u, err := updateref.New(ctx, repo)
 	if err != nil {
 		return fmt.Errorf("error when running creating new updater: %v", err)
 	}
-	if err = u.Update(git.ReferenceName(req.GetRef()), string(req.GetRevision()), string(req.GetOldRevision())); 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)
 	}
+
 	if err = u.Commit(); err != nil {
 		return fmt.Errorf("error when running update-ref command: %v", err)
 	}
+
 	return nil
 }
 
diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go
index b61586603..cb6262c27 100644
--- a/internal/gitaly/service/repository/write_ref_test.go
+++ b/internal/gitaly/service/repository/write_ref_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -18,7 +19,9 @@ import (
 	"google.golang.org/grpc/codes"
 )
 
-func TestWriteRefSuccessful(t *testing.T) {
+func TestWriteRef_successful(t *testing.T) {
+	t.Parallel()
+
 	txManager := transaction.NewTrackingManager()
 	cfg, repo, repoPath, client := setupRepositoryService(testhelper.Context(t), t, testserver.WithTransactionManager(txManager))
 
@@ -86,7 +89,9 @@ func TestWriteRefSuccessful(t *testing.T) {
 	}
 }
 
-func TestWriteRefValidationError(t *testing.T) {
+func TestWriteRef_validation(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 	_, repo, _, client := setupRepositoryService(ctx, t)
 
@@ -158,3 +163,53 @@ func TestWriteRefValidationError(t *testing.T) {
 		})
 	}
 }
+
+func TestWriteRef_missingRevisions(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	cfg, client := setupRepositoryServiceWithoutRepo(t)
+
+	repo, repoPath := gittest.CreateRepository(ctx, t, cfg)
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
+
+	for _, tc := range []struct {
+		desc        string
+		request     *gitalypb.WriteRefRequest
+		expectedErr error
+	}{
+		{
+			desc: "revision refers to missing reference",
+			request: &gitalypb.WriteRefRequest{
+				Repository: repo,
+				Ref:        []byte("refs/heads/main"),
+				Revision:   []byte("refs/heads/missing"),
+			},
+			expectedErr: helper.ErrInternalf("resolving new revision: reference not found"),
+		},
+		{
+			desc: "revision refers to missing object",
+			request: &gitalypb.WriteRefRequest{
+				Repository: repo,
+				Ref:        []byte("refs/heads/main"),
+				Revision:   bytes.Repeat([]byte("1"), gittest.DefaultObjectHash.EncodedLen()),
+			},
+			expectedErr: helper.ErrInternalf("resolving new revision: reference not found"),
+		},
+		{
+			desc: "old revision refers to missing reference",
+			request: &gitalypb.WriteRefRequest{
+				Repository:  repo,
+				Ref:         []byte("refs/heads/main"),
+				Revision:    []byte(commitID),
+				OldRevision: bytes.Repeat([]byte("1"), gittest.DefaultObjectHash.EncodedLen()),
+			},
+			expectedErr: helper.ErrInternalf("resolving old revision: reference not found"),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, err := client.WriteRef(ctx, tc.request)
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
+		})
+	}
+}