git: Convert ResolveRevision to return an ObjectID

The `ResolveRevision()` function currently returns the resolved object
ID as a string, which makes it hard to see what it actually is. This
commit converts it to return an ObjectID instead and adjusts all
callers.

11 files changed, 44 insertions, 38 deletions

diff --git a/internal/git/remoterepo/repository.go b/internal/git/remoterepo/repository.go
index 08342b191..ce3230109 100644
--- a/internal/git/remoterepo/repository.go
+++ b/internal/git/remoterepo/repository.go
@@ -34,7 +34,7 @@ func New(ctx context.Context, repo *gitalypb.Repository, pool *client.Pool) (Rep
 
 // ResolveRevision will dial to the remote repository and attempt to resolve the
 // revision string via the gRPC interface.
-func (rr Repository) ResolveRevision(ctx context.Context, revision git.Revision) (string, error) {
+func (rr Repository) ResolveRevision(ctx context.Context, revision git.Revision) (git.ObjectID, error) {
 	cli := gitalypb.NewCommitServiceClient(rr.conn)
 	resp, err := cli.FindCommit(ctx, &gitalypb.FindCommitRequest{
 		Repository: rr.repo,
@@ -44,11 +44,16 @@ func (rr Repository) ResolveRevision(ctx context.Context, revision git.Revision)
 		return "", err
 	}
 
-	oid := resp.GetCommit().GetId()
-	if oid == "" {
+	oidHex := resp.GetCommit().GetId()
+	if oidHex == "" {
 		return "", git.ErrReferenceNotFound
 	}
 
+	oid, err := git.NewObjectIDFromHex(oidHex)
+	if err != nil {
+		return "", err
+	}
+
 	return oid, nil
 }
 
diff --git a/internal/git/repository.go b/internal/git/repository.go
index 3d8941627..bdc1470db 100644
--- a/internal/git/repository.go
+++ b/internal/git/repository.go
@@ -107,7 +107,7 @@ type Repository interface {
 	// for accepted syntax. This will not verify whether the object ID
 	// exists. To do so, you can peel the reference to a given object type,
 	// e.g. by passing `refs/heads/master^{commit}`.
-	ResolveRevision(ctx context.Context, revision Revision) (string, error)
+	ResolveRevision(ctx context.Context, revision Revision) (ObjectID, error)
 	// HasBranches returns whether the repository has branches.
 	HasBranches(ctx context.Context) (bool, error)
 }
@@ -289,7 +289,7 @@ func (repo *LocalRepository) ReadObject(ctx context.Context, oid string) ([]byte
 // reference to a given object type, e.g. by passing
 // `refs/heads/master^{commit}`. Returns an ErrReferenceNotFound error in case
 // the revision does not exist.
-func (repo *LocalRepository) ResolveRevision(ctx context.Context, revision Revision) (string, error) {
+func (repo *LocalRepository) ResolveRevision(ctx context.Context, revision Revision) (ObjectID, error) {
 	if revision.String() == "" {
 		return "", errors.New("repository cannot contain empty reference name")
 	}
@@ -313,9 +313,10 @@ func (repo *LocalRepository) ResolveRevision(ctx context.Context, revision Revis
 		return "", err
 	}
 
-	oid := strings.TrimSpace(stdout.String())
-	if len(oid) != 40 {
-		return "", fmt.Errorf("unsupported object hash %q", oid)
+	hex := strings.TrimSpace(stdout.String())
+	oid, err := NewObjectIDFromHex(hex)
+	if err != nil {
+		return "", fmt.Errorf("unsupported object hash %q: %w", hex, err)
 	}
 
 	return oid, nil
diff --git a/internal/git/repository_suite.go b/internal/git/repository_suite.go
index 539cf63d1..0ed26ea65 100644
--- a/internal/git/repository_suite.go
+++ b/internal/git/repository_suite.go
@@ -40,7 +40,7 @@ func testRepositoryResolveRevision(t *testing.T, getRepository func(testing.TB,
 	for _, tc := range []struct {
 		desc     string
 		revision string
-		expected string
+		expected ObjectID
 	}{
 		{
 			desc:     "unqualified master branch",
diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go
index dce4a559f..341c2362c 100644
--- a/internal/git/repository_test.go
+++ b/internal/git/repository_test.go
@@ -574,7 +574,7 @@ func TestLocalRepository_FetchRemote(t *testing.T) {
 
 		sha, err := repo.ResolveRevision(ctx, Revision("refs/remotes/origin/master^{commit}"))
 		require.NoError(t, err, "the object from remote should exists in local after fetch done")
-		require.Equal(t, "1e292f8fedd741b75372e19097c76d327140c312", sha)
+		require.Equal(t, ObjectID("1e292f8fedd741b75372e19097c76d327140c312"), sha)
 	})
 
 	t.Run("with env", func(t *testing.T) {
diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go
index 996399912..20078ec0e 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files.go
@@ -40,8 +40,8 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s
 
 	conflicts, err := git2go.ConflictsCommand{
 		Repository: repoPath,
-		Ours:       ours,
-		Theirs:     theirs,
+		Ours:       ours.String(),
+		Theirs:     theirs.String(),
 	}.Run(ctx, s.cfg)
 	if err != nil {
 		if errors.Is(err, git2go.ErrInvalidArgument) {
diff --git a/internal/gitaly/service/conflicts/list_conflict_files_test.go b/internal/gitaly/service/conflicts/list_conflict_files_test.go
index c5c52c811..e33e8e1d1 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files_test.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files_test.go
@@ -213,7 +213,7 @@ func buildCommit(t *testing.T, ctx context.Context, repo *gitalypb.Repository, r
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "reset", "--hard", "HEAD~")
 
-	return oid
+	return oid.String()
 }
 
 func TestListConflictFilesFailedPrecondition(t *testing.T) {
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go
index 024e2df1a..ce026e194 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts.go
@@ -288,7 +288,7 @@ func (s *server) repoWithBranchCommit(ctx context.Context, srcRepo, targetRepo *
 		git.SubCmd{
 			Name:  "fetch",
 			Flags: []git.Option{git.Flag{Name: "--no-tags"}},
-			Args:  []string{gitalyssh.GitalyInternalURL, oid},
+			Args:  []string{gitalyssh.GitalyInternalURL, oid.String()},
 		},
 		git.WithEnv(env...),
 	)
diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index e5b68c1af..449176f33 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -189,7 +189,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 			localRepo,
 			remoteRepo,
 			targetBranchName,
-			targetBranchCommit,
+			targetBranchCommit.String(),
 			string(header.StartBranchName),
 		)
 		if err != nil {
@@ -197,7 +197,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 		}
 	}
 
-	if parentCommitOID != targetBranchCommit {
+	if parentCommitOID != targetBranchCommit.String() {
 		if err := s.fetchMissingCommit(ctx, header.Repository, remoteRepo, parentCommitOID); err != nil {
 			return fmt.Errorf("fetch missing commit: %w", err)
 		}
@@ -343,7 +343,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 	if targetBranchCommit == "" {
 		oldRevision = git.ZeroOID.String()
 	} else if header.Force {
-		oldRevision = targetBranchCommit
+		oldRevision = targetBranchCommit.String()
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, header.Repository, header.User, targetBranchName, commitID, oldRevision); err != nil {
@@ -411,7 +411,7 @@ func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository,
 		return "", fmt.Errorf("resolving refish %q in %T: %w", refish, repo, err)
 	}
 
-	return commit, nil
+	return commit.String(), nil
 }
 
 func (s *Server) fetchMissingCommit(ctx context.Context, local, remote *gitalypb.Repository, commitID string) error {
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 947274a58..de440a181 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -118,7 +118,7 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 		AuthorName: string(firstRequest.User.Name),
 		AuthorMail: string(firstRequest.User.Email),
 		Message:    string(firstRequest.Message),
-		Ours:       revision,
+		Ours:       revision.String(),
 		Theirs:     firstRequest.CommitId,
 	}.Run(ctx, s.cfg)
 	if err != nil {
@@ -142,7 +142,7 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 		return helper.ErrPreconditionFailedf("merge aborted by client")
 	}
 
-	if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, merge.CommitID, revision); err != nil {
+	if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, merge.CommitID, revision.String()); err != nil {
 		var preReceiveError preReceiveError
 		var updateRefError updateRefError
 
@@ -204,7 +204,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return nil, helper.ErrInvalidArgument(err)
 	}
 
-	ancestor, err := isAncestor(ctx, in.Repository, revision, in.CommitId)
+	ancestor, err := isAncestor(ctx, in.Repository, revision.String(), in.CommitId)
 	if err != nil {
 		return nil, err
 	}
@@ -212,7 +212,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return nil, helper.ErrPreconditionFailedf("not fast forward")
 	}
 
-	if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, branch, in.CommitId, revision); err != nil {
+	if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, branch, in.CommitId, revision.String()); err != nil {
 		var preReceiveError preReceiveError
 		if errors.As(err, &preReceiveError) {
 			return &gitalypb.UserFFBranchResponse{
@@ -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, ""); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid.String(), ""); err != nil {
 		return nil, updateRefError{reference: string(request.TargetRef)}
 	}
 
@@ -315,8 +315,8 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 		AuthorName: string(request.User.Name),
 		AuthorMail: string(request.User.Email),
 		Message:    string(request.Message),
-		Ours:       oid,
-		Theirs:     sourceRef,
+		Ours:       oid.String(),
+		Theirs:     sourceRef.String(),
 	}.Run(ctx, s.cfg)
 	if err != nil {
 		if errors.Is(err, git2go.ErrInvalidArgument) {
@@ -328,7 +328,7 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 
 	// ... 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); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), merge.CommitID, oid.String()); err != nil {
 		//nolint:stylecheck
 		return nil, helper.ErrPreconditionFailed(fmt.Errorf("Could not update %s. Please refresh and try again", string(request.TargetRef)))
 	}
diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go
index ea9b19ba6..021e75e7b 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -71,7 +71,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	oldrev, err := localRepo.ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", branch)))
 	if errors.Is(err, git.ErrReferenceNotFound) {
 		branchCreated = true
-		oldrev = git.ZeroOID.String()
+		oldrev = git.ZeroOID
 	} else if err != nil {
 		return nil, helper.ErrInvalidArgumentf("resolve ref: %w", err)
 	}
@@ -81,7 +81,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	}
 
 	if !branchCreated {
-		ancestor, err := isAncestor(ctx, req.Repository, oldrev, newrev)
+		ancestor, err := isAncestor(ctx, req.Repository, oldrev.String(), newrev)
 		if err != nil {
 			return nil, err
 		}
@@ -92,7 +92,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 		}
 	}
 
-	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev, oldrev); err != nil {
+	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev, oldrev.String()); err != nil {
 		var preReceiveError preReceiveError
 		if errors.As(err, &preReceiveError) {
 			return &gitalypb.UserRevertResponse{
@@ -147,17 +147,17 @@ func (s *Server) fetchStartRevision(ctx context.Context, req *gitalypb.UserRever
 	}
 
 	if req.StartRepository == nil {
-		return startRevision, nil
+		return startRevision.String(), nil
 	}
 
-	_, err = git.NewRepository(req.Repository).ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", startRevision)))
+	_, err = git.NewRepository(req.Repository).ResolveRevision(ctx, startRevision.Revision()+"^{commit}")
 	if errors.Is(err, git.ErrReferenceNotFound) {
-		if err := s.fetchRemoteObject(ctx, req.Repository, req.StartRepository, startRevision); err != nil {
+		if err := s.fetchRemoteObject(ctx, req.Repository, req.StartRepository, startRevision.String()); err != nil {
 			return "", helper.ErrInternalf("fetch start: %w", err)
 		}
 	} else if err != nil {
 		return "", helper.ErrInvalidArgumentf("resolve start: %w", err)
 	}
 
-	return startRevision, nil
+	return startRevision.String(), nil
 }
diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go
index 3d7d357a7..14c47238c 100644
--- a/internal/gitaly/service/repository/fetch.go
+++ b/internal/gitaly/service/repository/fetch.go
@@ -34,7 +34,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		return nil, helper.ErrInternal(err)
 	}
 
-	var sourceOid string
+	var sourceOid git.ObjectID
 	var containsObject bool
 
 	// If source and target repository are the same, then we know that both
@@ -67,7 +67,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		// Otherwise, if the source is a remote repository, we check
 		// whether the target repo already contains the desired object.
 		// If so, we can skip the fetch.
-		containsObject, err = targetRepo.HasRevision(ctx, git.Revision(sourceOid+"^{commit}"))
+		containsObject, err = targetRepo.HasRevision(ctx, sourceOid.Revision()+"^{commit}")
 		if err != nil {
 			return nil, helper.ErrInternal(err)
 		}
@@ -84,7 +84,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		cmd, err := git.NewCommand(ctx, req.Repository, nil,
 			git.SubCmd{
 				Name:  "fetch",
-				Args:  []string{gitalyssh.GitalyInternalURL, sourceOid},
+				Args:  []string{gitalyssh.GitalyInternalURL, sourceOid.String()},
 				Flags: []git.Option{git.Flag{Name: "--no-tags"}, git.Flag{Name: "--quiet"}},
 			},
 			git.WithEnv(env...),
@@ -96,13 +96,13 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		if err := cmd.Wait(); err != nil {
 			// Design quirk: if the fetch fails, this RPC returns Result: false, but no error.
 			ctxlogrus.Extract(ctx).
-				WithField("oid", sourceOid).
+				WithField("oid", sourceOid.String()).
 				WithError(err).Warn("git fetch failed")
 			return &gitalypb.FetchSourceBranchResponse{Result: false}, nil
 		}
 	}
 
-	if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid, ""); err != nil {
+	if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid.String(), ""); err != nil {
 		return nil, err
 	}