gitserver: Reduce instances of EnsureRevision (sourcegraph#57732)

Attempting to understand when we schedule repo updates, I found a lot of places where we didn't explicitly say that we don't need to ensure the revision, but based on assumptions about data above we can safely do so.

cmd/frontend/backend/repos.go

@@ -18,7 +18,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/errcode"
 	"github.com/sourcegraph/sourcegraph/internal/extsvc"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver"
-	"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
 	"github.com/sourcegraph/sourcegraph/internal/httpcli"
 	"github.com/sourcegraph/sourcegraph/internal/inventory"
 	"github.com/sourcegraph/sourcegraph/internal/repoupdater"
@@ -38,7 +37,6 @@ type ReposService interface {
 	DeleteRepositoryFromDisk(ctx context.Context, repoID api.RepoID) error
 	RequestRepositoryClone(ctx context.Context, repoID api.RepoID) error
 	ResolveRev(ctx context.Context, repo *types.Repo, rev string) (api.CommitID, error)
-	GetCommit(ctx context.Context, repo *types.Repo, commitID api.CommitID) (*gitdomain.Commit, error)
 }
 
 // NewRepos uses the provided `database.DB` to initialize a new RepoService.
@@ -341,19 +339,6 @@ func (s *repos) ResolveRev(ctx context.Context, repo *types.Repo, rev string) (c
 	return s.gitserverClient.ResolveRevision(ctx, repo.Name, rev, gitserver.ResolveRevisionOptions{})
 }
 
-func (s *repos) GetCommit(ctx context.Context, repo *types.Repo, commitID api.CommitID) (res *gitdomain.Commit, err error) {
-	ctx, done := startTrace(ctx, "GetCommit", map[string]any{"repo": repo.Name, "commitID": commitID}, &err)
-	defer done()
-
-	s.logger.Debug("GetCommit", log.String("repo", string(repo.Name)), log.String("commitID", string(commitID)))
-
-	if !gitdomain.IsAbsoluteRevision(string(commitID)) {
-		return nil, errors.Errorf("non-absolute CommitID for Repos.GetCommit: %v", commitID)
-	}
-
-	return s.gitserverClient.GetCommit(ctx, repo.Name, commitID, gitserver.ResolveRevisionOptions{})
-}
-
 // ErrRepoSeeOther indicates that the repo does not exist on this server but might exist on an external Sourcegraph
 // server.
 type ErrRepoSeeOther struct {

cmd/frontend/backend/repos_vcs_test.go

@@ -10,7 +10,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver"
-	"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
 	"github.com/sourcegraph/sourcegraph/internal/repoupdater"
 	"github.com/sourcegraph/sourcegraph/internal/repoupdater/protocol"
 	"github.com/sourcegraph/sourcegraph/internal/types"
@@ -174,42 +173,3 @@ func TestRepos_ResolveRev_commitIDSpecified_failsToResolve(t *testing.T) {
 		t.Error("!calledVCSRepoResolveRevision")
 	}
 }
-
-func TestRepos_GetCommit_repoupdaterError(t *testing.T) {
-	ctx := testContext()
-	logger := logtest.Scoped(t)
-
-	const wantRepo = "a"
-	want := api.CommitID(strings.Repeat("a", 40))
-
-	calledRepoLookup := false
-	repoupdater.MockRepoLookup = func(args protocol.RepoLookupArgs) (*protocol.RepoLookupResult, error) {
-		calledRepoLookup = true
-		if args.Repo != wantRepo {
-			t.Errorf("got %q, want %q", args.Repo, wantRepo)
-		}
-		return &protocol.RepoLookupResult{ErrorNotFound: true}, nil
-	}
-	defer func() { repoupdater.MockRepoLookup = nil }()
-	var calledVCSRepoGetCommit bool
-
-	gsClient := gitserver.NewMockClient()
-	gsClient.GetCommitFunc.SetDefaultHook(func(context.Context, api.RepoName, api.CommitID, gitserver.ResolveRevisionOptions) (*gitdomain.Commit, error) {
-		calledVCSRepoGetCommit = true
-		return &gitdomain.Commit{ID: want}, nil
-	})
-
-	commit, err := NewRepos(logger, dbmocks.NewMockDB(), gsClient).GetCommit(ctx, &types.Repo{Name: "a"}, want)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if calledRepoLookup {
-		t.Error("calledRepoLookup")
-	}
-	if !calledVCSRepoGetCommit {
-		t.Error("!calledVCSRepoGetCommit")
-	}
-	if commit.ID != want {
-		t.Errorf("got commit %q, want %q", commit.ID, want)
-	}
-}

cmd/frontend/graphqlbackend/repository_git_refs.go

@@ -157,7 +157,11 @@ func hydrateBranchCommits(ctx context.Context, gitserverClient gitserver.Client,
 	}
 
 	for _, branch := range branches {
-		branch.Commit, err = gitserverClient.GetCommit(ctx, repo, branch.Head, gitserver.ResolveRevisionOptions{})
+		branch.Commit, err = gitserverClient.GetCommit(ctx, repo, branch.Head, gitserver.ResolveRevisionOptions{
+			// The passed in branches are returned from git a second ago, no reason
+			// to believe the revision doesn't exist.
+			NoEnsureRevision: true,
+		})
 		if err != nil {
 			if parentCtx.Err() == nil && ctx.Err() != nil {
 				// reached interactive timeout

cmd/frontend/internal/httpapi/stream_blame.go

@@ -109,7 +109,12 @@ func handleStreamBlame(logger log.Logger, db database.DB, gitserverClient gitser
 			if p, ok := parentsCache[h.CommitID]; ok {
 				parents = p
 			} else {
-				c, err := gitserverClient.GetCommit(ctx, repo.Name, h.CommitID, gitserver.ResolveRevisionOptions{})
+				c, err := gitserverClient.GetCommit(ctx, repo.Name, h.CommitID, gitserver.ResolveRevisionOptions{
+					// The list of hunks and commit IDs came from gitserver, that
+					// means the commit will exist and we don't need to ensure
+					// the revision exists.
+					NoEnsureRevision: true,
+				})
 				if err != nil {
 					tr.SetError(err)
 					http.Error(w, html.EscapeString(err.Error()), http.StatusInternalServerError)

internal/gitserver/commands.go

@@ -1706,7 +1706,12 @@ func (c *clientImplementor) CommitsUniqueToBranch(ctx context.Context, repo api.
 func (c *clientImplementor) filterCommitsUniqueToBranch(ctx context.Context, repo api.RepoName, commitsMap map[string]time.Time) map[string]time.Time {
 	filtered := make(map[string]time.Time, len(commitsMap))
 	for commitID, timeStamp := range commitsMap {
-		if _, err := c.GetCommit(ctx, repo, api.CommitID(commitID), ResolveRevisionOptions{}); !errors.HasType(err, &gitdomain.RevisionNotFoundError{}) {
+		_, err := c.GetCommit(ctx, repo, api.CommitID(commitID), ResolveRevisionOptions{
+			// The commits are returned from git log, so we are sure they exist and
+			// don't need to ensure the revision.
+			NoEnsureRevision: true,
+		})
+		if !errors.HasType(err, &gitdomain.RevisionNotFoundError{}) {
 			filtered[commitID] = timeStamp
 		}
 	}
@@ -2170,7 +2175,11 @@ func (c *clientImplementor) Head(ctx context.Context, repo api.RepoName) (_ stri
 	}
 	commitID := string(out)
 	if authz.SubRepoEnabled(c.subRepoPermsChecker) {
-		if _, err := c.GetCommit(ctx, repo, api.CommitID(commitID), ResolveRevisionOptions{}); err != nil {
+		if _, err := c.GetCommit(ctx, repo, api.CommitID(commitID), ResolveRevisionOptions{
+			// We expect the HEAD reference to not be broken, let's not ensure the
+			// revision here.
+			NoEnsureRevision: true,
+		}); err != nil {
 			return checkError(err)
 		}
 	}
@@ -2252,7 +2261,11 @@ func parseCommitFromLog(parts [][]byte) (*wrappedCommit, error) {
 func (c *clientImplementor) BranchesContaining(ctx context.Context, repo api.RepoName, commit api.CommitID) ([]string, error) {
 	if authz.SubRepoEnabled(c.subRepoPermsChecker) {
 		// GetCommit to validate that the user has permissions to access it.
-		if _, err := c.GetCommit(ctx, repo, commit, ResolveRevisionOptions{}); err != nil {
+		if _, err := c.GetCommit(ctx, repo, commit, ResolveRevisionOptions{
+			// Don't ensure the revision here, the branch command below will fail
+			// anyways.
+			NoEnsureRevision: true,
+		}); err != nil {
 			return nil, err
 		}
 	}
@@ -2338,7 +2351,11 @@ func (c *clientImplementor) filterRefDescriptions(ctx context.Context,
 ) map[string][]gitdomain.RefDescription {
 	filtered := make(map[string][]gitdomain.RefDescription, len(refDescriptions))
 	for commitID, descriptions := range refDescriptions {
-		if _, err := c.GetCommit(ctx, repo, api.CommitID(commitID), ResolveRevisionOptions{}); !errors.HasType(err, &gitdomain.RevisionNotFoundError{}) {
+		_, err := c.GetCommit(ctx, repo, api.CommitID(commitID), ResolveRevisionOptions{
+			// The commits are returned from git already, so they must exist.
+			NoEnsureRevision: true,
+		})
+		if !errors.HasType(err, &gitdomain.RevisionNotFoundError{}) {
 			filtered[commitID] = descriptions
 		}
 	}
@@ -2429,7 +2446,11 @@ lineLoop:
 func (c *clientImplementor) CommitDate(ctx context.Context, repo api.RepoName, commit api.CommitID) (_ string, _ time.Time, revisionExists bool, err error) {
 	if authz.SubRepoEnabled(c.subRepoPermsChecker) {
 		// GetCommit to validate that the user has permissions to access it.
-		if _, err := c.GetCommit(ctx, repo, commit, ResolveRevisionOptions{}); err != nil {
+		if _, err := c.GetCommit(ctx, repo, commit, ResolveRevisionOptions{
+			// The show command below will fail anyways, so no need to ensure
+			// that the revision exists here.
+			NoEnsureRevision: true,
+		}); err != nil {
 			return "", time.Time{}, false, nil
 		}
 	}
@@ -2727,7 +2748,11 @@ func (c *clientImplementor) ListBranches(ctx context.Context, repo api.RepoName,
 
 		branch := &gitdomain.Branch{Name: name, Head: ref.CommitID}
 		if opt.IncludeCommit {
-			branch.Commit, err = c.GetCommit(ctx, repo, ref.CommitID, ResolveRevisionOptions{})
+			branch.Commit, err = c.GetCommit(ctx, repo, ref.CommitID, ResolveRevisionOptions{
+				// The commitID comes from git, so surely it knows about it and
+				// there's no need to ensure the revision exists.
+				NoEnsureRevision: true,
+			})
 			if err != nil {
 				return nil, err
 			}