repository: Resolve revisions passed to the WriteRef RPC

The WriteRef RPC gets as input a reference name that is to be updated as
well as the old and new revisions that the reference should be updated
from respectively to. It's not entirely clear what these revisions
contain: while one would typically expect it to only ever contain object
IDs, that may or may not be the case.

Resolve the revision to an object ID before invoking git-update-ref(1)
with them. This will ease the transition when we migrate the updateref
package to only ever accept object IDs as input, but it will also give
us better indicators when the revision does not exist in the repository.

2 files changed, 78 insertions, 3 deletions

diff --git a/internal/gitaly/service/repository/write_ref.go b/internal/gitaly/service/repository/write_ref.go
index 5079ebc03..3997b5462 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.String(), oldObjectID.String()); 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)
+		})
+	}
+}