Stop using nil internally to signal "commit not found"

changelogs/unreleased/log-get-commit-not-found.yml

+---
+title: Stop using nil internally to signal "commit not found"
+merge_request: 1050
+author:
+type: other

internal/git/log/commit.go

@@ -31,10 +31,6 @@ func GetCommit(ctx context.Context, repo *gitalypb.Repository, revision string)
 func GetCommitCatfile(c *catfile.Batch, revision string) (*gitalypb.GitCommit, error) {
 	info, err := c.Info(revision + "^{commit}")
 	if err != nil {
-		if catfile.IsNotFound(err) {
-			return nil, nil
-		}
-
 		return nil, err
 	}
 

internal/git/log/log.go

@@ -10,6 +10,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 )
 
+// IsNotFound tests if an error is a "not found" error.
+func IsNotFound(err error) bool { return catfile.IsNotFound(err) }
+
 // Parser holds necessary state for parsing a git log stream
 type Parser struct {
 	scanner       *bufio.Scanner

internal/git/updateref/updateref_test.go

@@ -43,7 +43,6 @@ func TestCreate(t *testing.T) {
 	// check the ref was created
 	commit, logErr := log.GetCommit(ctx, testRepo, ref)
 	require.NoError(t, logErr)
-	require.NotNil(t, commit, "reference was not created")
 	require.Equal(t, commit.Id, sha, "reference was created with the wrong SHA")
 }
 
@@ -63,7 +62,6 @@ func TestUpdate(t *testing.T) {
 	// Sanity check: ensure the ref exists before we start
 	commit, logErr := log.GetCommit(ctx, testRepo, ref)
 	require.NoError(t, logErr)
-	require.NotNil(t, commit, "%s does not exist in the test repository", ref)
 	require.NotEqual(t, commit.Id, sha, "%s points to HEAD: %s in the test repository", ref, sha)
 
 	require.NoError(t, updater.Update(ref, sha, ""))
@@ -72,7 +70,6 @@ func TestUpdate(t *testing.T) {
 	// check the ref was updated
 	commit, logErr = log.GetCommit(ctx, testRepo, ref)
 	require.NoError(t, logErr)
-	require.NotNil(t, commit)
 	require.Equal(t, commit.Id, sha, "reference was not updated")
 
 	// 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
@@ -83,7 +80,6 @@ func TestUpdate(t *testing.T) {
 	// check the ref was not updated
 	commit, logErr = log.GetCommit(ctx, testRepo, ref)
 	require.NoError(t, logErr)
-	require.NotNil(t, commit)
 	require.NotEqual(t, commit.Id, parentCommit.Id, "reference was updated when it shouldn't have been")
 }
 
@@ -100,9 +96,8 @@ func TestDelete(t *testing.T) {
 	require.NoError(t, updater.Wait())
 
 	// check the ref was removed
-	commit, logErr := log.GetCommit(ctx, testRepo, ref)
-	require.NoError(t, logErr)
-	require.Nil(t, commit, "reference was not removed")
+	_, err = log.GetCommit(ctx, testRepo, ref)
+	require.True(t, log.IsNotFound(err), "expected 'not found' error got %v", err)
 }
 
 func TestBulkOperation(t *testing.T) {
@@ -147,7 +142,6 @@ func TestContextCancelAbortsRefChanges(t *testing.T) {
 	require.Error(t, updater.Wait())
 
 	// check the ref doesn't exist
-	commit, logErr := log.GetCommit(ctx, testRepo, ref)
-	require.NoError(t, logErr)
-	require.Nil(t, commit, "Reference was created even though the process was aborted")
+	_, err = log.GetCommit(ctx, testRepo, ref)
+	require.True(t, log.IsNotFound(err), "expected 'not found' error got %v", err)
 }

internal/service/commit/find_commit.go

@@ -19,5 +19,9 @@ func (s *server) FindCommit(ctx context.Context, in *gitalypb.FindCommitRequest)
 	repo := in.GetRepository()
 
 	commit, err := log.GetCommit(ctx, repo, string(revision))
+	if log.IsNotFound(err) {
+		return &gitalypb.FindCommitResponse{}, nil
+	}
+
 	return &gitalypb.FindCommitResponse{Commit: commit}, err
 }

internal/service/commit/last_commit_for_path.go

@@ -21,11 +21,11 @@ func (s *server) LastCommitForPath(ctx context.Context, in *gitalypb.LastCommitF
 	}
 
 	commit, err := log.LastCommitForPath(ctx, in.GetRepository(), string(in.GetRevision()), path)
-	if err != nil {
-		return nil, err
+	if log.IsNotFound(err) {
+		return &gitalypb.LastCommitForPathResponse{}, nil
 	}
 
-	return &gitalypb.LastCommitForPathResponse{Commit: commit}, nil
+	return &gitalypb.LastCommitForPathResponse{Commit: commit}, err
 }
 
 func validateLastCommitForPathRequest(in *gitalypb.LastCommitForPathRequest) error {

internal/service/commit/list_commits_by_oid.go

@@ -19,14 +19,13 @@ func (s *server) ListCommitsByOid(in *gitalypb.ListCommitsByOidRequest, stream g
 
 	for _, oid := range in.Oid {
 		commit, err := gitlog.GetCommitCatfile(c, oid)
+		if catfile.IsNotFound(err) {
+			continue
+		}
 		if err != nil {
 			return err
 		}
 
-		if commit == nil {
-			continue
-		}
-
 		if err := sender.Send(commit); err != nil {
 			return err
 		}

internal/service/objectpool/link_test.go

@@ -159,9 +159,8 @@ func TestUnlink(t *testing.T) {
 			require.Equal(t, tc.code, helper.GrpcCode(err))
 
 			if tc.code == codes.OK {
-				commit, err := log.GetCommit(ctx, testRepo, poolCommitID)
-				require.NoError(t, err)
-				require.Nil(t, commit)
+				_, err = log.GetCommit(ctx, testRepo, poolCommitID)
+				require.True(t, log.IsNotFound(err), "expected 'not found' error got %v", err)
 
 				remotes := testhelper.MustRunCommand(t, nil, "git", "-C", pool.FullPath(), "remote")
 				require.Len(t, remotes, 0)