git: Rename ResolveRefish() and have it accept a Revision

The `ResolveRefish()` function is misnamed, as it does not act on
refishes but on revisions. This commit thus renames the function to
`ResolveRevision()` and converts it to accept a Revision instead.

11 files changed, 56 insertions, 51 deletions

diff --git a/internal/git/remoterepo/repository.go b/internal/git/remoterepo/repository.go
index 738083ce0..08342b191 100644
--- a/internal/git/remoterepo/repository.go
+++ b/internal/git/remoterepo/repository.go
@@ -32,13 +32,13 @@ func New(ctx context.Context, repo *gitalypb.Repository, pool *client.Pool) (Rep
 	return Repository{repo: repo, conn: cc}, nil
 }
 
-// ResolveRefish will dial to the remote repository and attempt to resolve the
-// refish string via the gRPC interface
-func (rr Repository) ResolveRefish(ctx context.Context, ref string) (string, error) {
+// 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) {
 	cli := gitalypb.NewCommitServiceClient(rr.conn)
 	resp, err := cli.FindCommit(ctx, &gitalypb.FindCommitRequest{
 		Repository: rr.repo,
-		Revision:   []byte(ref),
+		Revision:   []byte(revision.String()),
 	})
 	if err != nil {
 		return "", err
diff --git a/internal/git/repository.go b/internal/git/repository.go
index 64a25a476..6df6ead3b 100644
--- a/internal/git/repository.go
+++ b/internal/git/repository.go
@@ -102,12 +102,12 @@ func (opts FetchOpts) buildFlags() []Option {
 
 // Repository is the common interface of different repository implementations.
 type Repository interface {
-	// ResolveRef resolves the given refish to its object ID. This uses the
-	// typical DWIM mechanism of Git to resolve the reference. See
-	// gitrevisions(1) 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}`.
-	ResolveRefish(ctx context.Context, ref string) (string, error)
+	// ResolveRevision tries to resolve the given revision to its object
+	// ID. This uses the typical DWIM mechanism of git, see gitrevisions(1)
+	// 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)
 	// HasBranches returns whether the repository has branches.
 	HasBranches(ctx context.Context) (bool, error)
 }
@@ -284,15 +284,20 @@ func (repo *LocalRepository) ReadObject(ctx context.Context, oid string) ([]byte
 	return stdout.Bytes(), nil
 }
 
-func (repo *LocalRepository) ResolveRefish(ctx context.Context, refish string) (string, error) {
-	if refish == "" {
+// ResolveRevision resolves the given revision to its object ID. This will not
+// verify whether the target object exists. To do so, you can peel the
+// 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) {
+	if revision.String() == "" {
 		return "", errors.New("repository cannot contain empty reference name")
 	}
 
 	cmd, err := repo.command(ctx, nil, SubCmd{
 		Name:  "rev-parse",
 		Flags: []Option{Flag{Name: "--verify"}},
-		Args:  []string{refish},
+		Args:  []string{revision.String()},
 	}, WithStderr(ioutil.Discard))
 	if err != nil {
 		return "", err
@@ -321,7 +326,7 @@ func (repo *LocalRepository) ResolveRefish(ctx context.Context, refish string) (
 // reference to a given object type, e.g. by passing
 // `refs/heads/master^{commit}`.
 func (repo *LocalRepository) ContainsRef(ctx context.Context, ref string) (bool, error) {
-	if _, err := repo.ResolveRefish(ctx, ref); err != nil {
+	if _, err := repo.ResolveRevision(ctx, Revision(ref)); err != nil {
 		if errors.Is(err, ErrReferenceNotFound) {
 			return false, nil
 		}
diff --git a/internal/git/repository_suite.go b/internal/git/repository_suite.go
index 7139444e7..9aa2e1d89 100644
--- a/internal/git/repository_suite.go
+++ b/internal/git/repository_suite.go
@@ -16,8 +16,8 @@ func TestRepository(t *testing.T, getRepository func(testing.TB, *gitalypb.Repos
 		test func(*testing.T, func(testing.TB, *gitalypb.Repository) Repository)
 	}{
 		{
-			desc: "ResolveRefish",
-			test: testRepositoryResolveRefish,
+			desc: "ResolveRevision",
+			test: testRepositoryResolveRevision,
 		},
 		{
 			desc: "HasBranches",
@@ -30,7 +30,7 @@ func TestRepository(t *testing.T, getRepository func(testing.TB, *gitalypb.Repos
 	}
 }
 
-func testRepositoryResolveRefish(t *testing.T, getRepository func(testing.TB, *gitalypb.Repository) Repository) {
+func testRepositoryResolveRevision(t *testing.T, getRepository func(testing.TB, *gitalypb.Repository) Repository) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -39,40 +39,40 @@ func testRepositoryResolveRefish(t *testing.T, getRepository func(testing.TB, *g
 
 	for _, tc := range []struct {
 		desc     string
-		refish   string
+		revision string
 		expected string
 	}{
 		{
 			desc:     "unqualified master branch",
-			refish:   "master",
+			revision: "master",
 			expected: "1e292f8fedd741b75372e19097c76d327140c312",
 		},
 		{
 			desc:     "fully qualified master branch",
-			refish:   "refs/heads/master",
+			revision: "refs/heads/master",
 			expected: "1e292f8fedd741b75372e19097c76d327140c312",
 		},
 		{
 			desc:     "typed commit",
-			refish:   "refs/heads/master^{commit}",
+			revision: "refs/heads/master^{commit}",
 			expected: "1e292f8fedd741b75372e19097c76d327140c312",
 		},
 		{
 			desc:     "extended SHA notation",
-			refish:   "refs/heads/master^2",
+			revision: "refs/heads/master^2",
 			expected: "c1c67abbaf91f624347bb3ae96eabe3a1b742478",
 		},
 		{
-			desc:   "nonexistent branch",
-			refish: "refs/heads/foobar",
+			desc:     "nonexistent branch",
+			revision: "refs/heads/foobar",
 		},
 		{
-			desc:   "SHA notation gone wrong",
-			refish: "refs/heads/master^3",
+			desc:     "SHA notation gone wrong",
+			revision: "refs/heads/master^3",
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			oid, err := getRepository(t, pbRepo).ResolveRefish(ctx, tc.refish)
+			oid, err := getRepository(t, pbRepo).ResolveRevision(ctx, Revision(tc.revision))
 			if tc.expected == "" {
 				require.Equal(t, err, ErrReferenceNotFound)
 				return
diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go
index f9ed67fe2..df13ef926 100644
--- a/internal/git/repository_test.go
+++ b/internal/git/repository_test.go
@@ -621,7 +621,7 @@ func TestLocalRepository_FetchRemote(t *testing.T) {
 		require.Contains(t, fetchHead, "e56497bb5f03a90a51293fc6d516788730953899	not-for-merge	branch ''test''")
 		require.Contains(t, fetchHead, "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b	not-for-merge	tag 'v1.1.0'")
 
-		sha, err := repo.ResolveRefish(ctx, "refs/remotes/origin/master^{commit}")
+		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)
 	})
diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go
index 34c8d0fd2..996399912 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files.go
@@ -23,12 +23,12 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s
 
 	repo := git.NewRepository(request.Repository)
 
-	ours, err := repo.ResolveRefish(ctx, request.OurCommitOid+"^{commit}")
+	ours, err := repo.ResolveRevision(ctx, git.Revision(request.OurCommitOid+"^{commit}"))
 	if err != nil {
 		return helper.ErrPreconditionFailedf("could not lookup 'our' OID: %s", err)
 	}
 
-	theirs, err := repo.ResolveRefish(ctx, request.TheirCommitOid+"^{commit}")
+	theirs, err := repo.ResolveRevision(ctx, git.Revision(request.TheirCommitOid+"^{commit}"))
 	if err != nil {
 		return helper.ErrPreconditionFailedf("could not lookup 'their' OID: %s", err)
 	}
diff --git a/internal/gitaly/service/conflicts/list_conflict_files_test.go b/internal/gitaly/service/conflicts/list_conflict_files_test.go
index dd10dd0e8..c5c52c811 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files_test.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files_test.go
@@ -208,7 +208,7 @@ func buildCommit(t *testing.T, ctx context.Context, repo *gitalypb.Repository, r
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "commit", "-m", "message")
 
-	oid, err := git.NewRepository(repo).ResolveRefish(ctx, "HEAD")
+	oid, err := git.NewRepository(repo).ResolveRevision(ctx, git.Revision("HEAD"))
 	require.NoError(t, err)
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "reset", "--hard", "HEAD~")
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go
index 4a5966a78..32e392bdb 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts.go
@@ -255,7 +255,7 @@ func (s *server) repoWithBranchCommit(ctx context.Context, srcRepo, targetRepo *
 
 	src := git.NewRepository(srcRepo)
 	if sameRepo(srcRepo, targetRepo) {
-		_, err := src.ResolveRefish(ctx, string(targetBranch)+peelCommit)
+		_, err := src.ResolveRevision(ctx, git.Revision(string(targetBranch)+peelCommit))
 		return err
 	}
 
@@ -264,7 +264,7 @@ func (s *server) repoWithBranchCommit(ctx context.Context, srcRepo, targetRepo *
 		return err
 	}
 
-	oid, err := target.ResolveRefish(ctx, string(targetBranch)+peelCommit)
+	oid, err := target.ResolveRevision(ctx, git.Revision(string(targetBranch)+peelCommit))
 	if err != nil {
 		return err
 	}
diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 4ef4d2509..ee9922272 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -173,7 +173,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 	localRepo := git.NewRepository(header.Repository)
 
 	targetBranchName := "refs/heads/" + string(header.BranchName)
-	targetBranchCommit, err := localRepo.ResolveRefish(ctx, targetBranchName+"^{commit}")
+	targetBranchCommit, err := localRepo.ResolveRevision(ctx, git.Revision(targetBranchName+"^{commit}"))
 	if err != nil {
 		if !errors.Is(err, git.ErrReferenceNotFound) {
 			return fmt.Errorf("resolve target branch commit: %w", err)
@@ -406,7 +406,7 @@ func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository,
 	}
 	refish := branch + "^{commit}"
 
-	commit, err := repo.ResolveRefish(ctx, refish)
+	commit, err := repo.ResolveRevision(ctx, git.Revision(refish))
 	if err != nil {
 		return "", fmt.Errorf("resolving refish %q in %T: %w", refish, repo, err)
 	}
@@ -415,7 +415,7 @@ func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository,
 }
 
 func (s *Server) fetchMissingCommit(ctx context.Context, local, remote *gitalypb.Repository, commitID string) error {
-	if _, err := git.NewRepository(local).ResolveRefish(ctx, commitID+"^{commit}"); err != nil {
+	if _, err := git.NewRepository(local).ResolveRevision(ctx, git.Revision(commitID+"^{commit}")); err != nil {
 		if !errors.Is(err, git.ErrReferenceNotFound) || remote == nil {
 			return fmt.Errorf("lookup parent commit: %w", err)
 		}
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 4feac68c7..947274a58 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -108,7 +108,7 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 	}
 
 	branch := "refs/heads/" + text.ChompBytes(firstRequest.Branch)
-	revision, err := git.NewRepository(repo).ResolveRefish(ctx, branch)
+	revision, err := git.NewRepository(repo).ResolveRevision(ctx, git.Revision(branch))
 	if err != nil {
 		return err
 	}
@@ -199,7 +199,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 	}
 
 	branch := fmt.Sprintf("refs/heads/%s", in.Branch)
-	revision, err := git.NewRepository(in.Repository).ResolveRefish(ctx, branch)
+	revision, err := git.NewRepository(in.Repository).ResolveRevision(ctx, git.Revision(branch))
 	if err != nil {
 		return nil, helper.ErrInvalidArgument(err)
 	}
@@ -287,25 +287,25 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 
 	repo := git.NewRepository(request.Repository)
 
-	refName := string(request.Branch)
+	revision := git.Revision(request.Branch)
 	if request.FirstParentRef != nil {
-		refName = string(request.FirstParentRef)
+		revision = git.Revision(request.FirstParentRef)
 	}
 
-	ref, err := repo.ResolveRefish(ctx, refName)
+	oid, err := repo.ResolveRevision(ctx, revision)
 	if err != nil {
 		//nolint:stylecheck
 		return nil, helper.ErrInvalidArgument(errors.New("Invalid merge source"))
 	}
 
-	sourceRef, err := repo.ResolveRefish(ctx, request.SourceSha)
+	sourceRef, err := repo.ResolveRevision(ctx, git.Revision(request.SourceSha))
 	if err != nil {
 		//nolint:stylecheck
 		return nil, helper.ErrInvalidArgument(errors.New("Invalid merge source"))
 	}
 
 	// First, overwrite the reference with the target reference.
-	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), ref, ""); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid, ""); err != nil {
 		return nil, updateRefError{reference: string(request.TargetRef)}
 	}
 
@@ -315,7 +315,7 @@ 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:       ref,
+		Ours:       oid,
 		Theirs:     sourceRef,
 	}.Run(ctx, s.cfg)
 	if err != nil {
@@ -323,12 +323,12 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 			return nil, helper.ErrInvalidArgument(err)
 		}
 		//nolint:stylecheck
-		return nil, helper.ErrPreconditionFailedf("Failed to create merge commit for source_sha %s and target_sha %s at %s", sourceRef, ref, string(request.TargetRef))
+		return nil, helper.ErrPreconditionFailedf("Failed to create merge commit for source_sha %s and target_sha %s at %s", sourceRef, oid, string(request.TargetRef))
 	}
 
 	// ... 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, ref); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), merge.CommitID, oid); 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 4dfce39e5..7195061f1 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -68,7 +68,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	branch := fmt.Sprintf("refs/heads/%s", req.BranchName)
 
 	branchCreated := false
-	oldrev, err := localRepo.ResolveRefish(ctx, fmt.Sprintf("%s^{commit}", branch))
+	oldrev, err := localRepo.ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", branch)))
 	if errors.Is(err, git.ErrReferenceNotFound) {
 		branchCreated = true
 		oldrev = git.NullSHA
@@ -141,7 +141,7 @@ func (s *Server) fetchStartRevision(ctx context.Context, req *gitalypb.UserRever
 	if err != nil {
 		return "", helper.ErrInternal(err)
 	}
-	startRevision, err := remote.ResolveRefish(ctx, fmt.Sprintf("%s^{commit}", startBranchName))
+	startRevision, err := remote.ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", startBranchName)))
 	if err != nil {
 		return "", helper.ErrInvalidArgumentf("resolve start ref: %w", err)
 	}
@@ -150,7 +150,7 @@ func (s *Server) fetchStartRevision(ctx context.Context, req *gitalypb.UserRever
 		return startRevision, nil
 	}
 
-	_, err = git.NewRepository(req.Repository).ResolveRefish(ctx, fmt.Sprintf("%s^{commit}", startRevision))
+	_, err = git.NewRepository(req.Repository).ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", startRevision)))
 	if errors.Is(err, git.ErrReferenceNotFound) {
 		if err := s.fetchRemoteObject(ctx, req.Repository, req.StartRepository, startRevision); err != nil {
 			return "", helper.ErrInternalf("fetch start: %w", err)
diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go
index 5843cc32f..296653c53 100644
--- a/internal/gitaly/service/repository/fetch.go
+++ b/internal/gitaly/service/repository/fetch.go
@@ -44,7 +44,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 	if helper.RepoPathEqual(req.GetRepository(), req.GetSourceRepository()) {
 		var err error
 
-		sourceOid, err = targetRepo.ResolveRefish(ctx, string(req.GetSourceBranch()))
+		sourceOid, err = targetRepo.ResolveRevision(ctx, git.Revision(req.GetSourceBranch()))
 		if err != nil {
 			if errors.Is(err, git.ErrReferenceNotFound) {
 				return &gitalypb.FetchSourceBranchResponse{Result: false}, nil
@@ -56,7 +56,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 	} else {
 		var err error
 
-		sourceOid, err = sourceRepo.ResolveRefish(ctx, string(req.GetSourceBranch()))
+		sourceOid, err = sourceRepo.ResolveRevision(ctx, git.Revision(req.GetSourceBranch()))
 		if err != nil {
 			if errors.Is(err, git.ErrReferenceNotFound) {
 				return &gitalypb.FetchSourceBranchResponse{Result: false}, nil