catfile: Refactor NotFoundError to match current best practices

The `catfile.NotFoundError` is not really following our current best
practices around error handling. It embeds a wrapped error that is the
same in all except one case, doesn't provide information about which
revision wasn't found and uses a `IsNotFound()` function instead of the
modern `errors.As()`. Last but not least, it's hard to construct this
error in dependents of this package, which is something we'll need in a
follow-up commit.

Refactor the error type to embed the revision that wasn't found instead
of an error such that we can attach structured error metadata to the
type. Furthermore, we convert all callsites to use `errors.As()` in
order to get rid of `IsNotFound()`.

Note that `catfile.GetTag()` abused this type by returning the error
with an embedded error effectively saying "object is not a tag". This
doesn't mean that the object wasn't found though, but instead indicates
that its type doesn't match our expectations, and thus using this error
is not a good fit. The function is converted to now use a plain error,
which is fine given than none of its callers actually checked whether
the returned error is of type `catfile.NotFoundError`.

10 files changed, 25 insertions, 13 deletions

diff --git a/internal/gitaly/service/blob/get_blob.go b/internal/gitaly/service/blob/get_blob.go
index ea4d294d7..e0a8b3b1a 100644
--- a/internal/gitaly/service/blob/get_blob.go
+++ b/internal/gitaly/service/blob/get_blob.go
@@ -29,7 +29,7 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic
 
 	blob, err := objectReader.Object(ctx, git.Revision(in.Oid))
 	if err != nil {
-		if catfile.IsNotFound(err) {
+		if errors.As(err, &catfile.NotFoundError{}) {
 			if err := stream.Send(&gitalypb.GetBlobResponse{}); err != nil {
 				return structerr.NewAborted("sending empty response: %w", err)
 			}
diff --git a/internal/gitaly/service/commit/check_objects_exist.go b/internal/gitaly/service/commit/check_objects_exist.go
index d612e23d4..43b7d0ffb 100644
--- a/internal/gitaly/service/commit/check_objects_exist.go
+++ b/internal/gitaly/service/commit/check_objects_exist.go
@@ -2,6 +2,7 @@ package commit
 
 import (
 	"context"
+	"errors"
 	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
@@ -108,7 +109,7 @@ func checkObjectsExist(
 		}
 		_, err := objectInfoReader.Info(ctx, git.Revision(revision))
 		if err != nil {
-			if catfile.IsNotFound(err) {
+			if errors.As(err, &(catfile.NotFoundError{})) {
 				revisionExistence.Exists = false
 			} else {
 				return structerr.NewInternal("reading object info: %w", err)
diff --git a/internal/gitaly/service/commit/commit_signatures.go b/internal/gitaly/service/commit/commit_signatures.go
index 2aa9c957d..079d954e4 100644
--- a/internal/gitaly/service/commit/commit_signatures.go
+++ b/internal/gitaly/service/commit/commit_signatures.go
@@ -50,7 +50,7 @@ func (s *server) GetCommitSignatures(request *gitalypb.GetCommitSignaturesReques
 	for _, commitID := range request.CommitIds {
 		commitObj, err := objectReader.Object(ctx, git.Revision(commitID)+"^{commit}")
 		if err != nil {
-			if catfile.IsNotFound(err) {
+			if errors.As(err, &catfile.NotFoundError{}) {
 				continue
 			}
 			return structerr.NewInternal("%w", err)
diff --git a/internal/gitaly/service/commit/filter_shas_with_signatures.go b/internal/gitaly/service/commit/filter_shas_with_signatures.go
index 205391e28..237cc0319 100644
--- a/internal/gitaly/service/commit/filter_shas_with_signatures.go
+++ b/internal/gitaly/service/commit/filter_shas_with_signatures.go
@@ -2,6 +2,7 @@ package commit
 
 import (
 	"context"
+	"errors"
 	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
@@ -67,7 +68,7 @@ func filterCommitShasWithSignatures(ctx context.Context, objectReader catfile.Ob
 	var foundShas [][]byte
 	for _, sha := range shas {
 		commit, err := catfile.GetCommit(ctx, objectReader, git.Revision(sha))
-		if catfile.IsNotFound(err) {
+		if errors.As(err, &catfile.NotFoundError{}) {
 			continue
 		}
 
diff --git a/internal/gitaly/service/commit/last_commit_for_path.go b/internal/gitaly/service/commit/last_commit_for_path.go
index 35e6b0ccd..4bd3a1b2b 100644
--- a/internal/gitaly/service/commit/last_commit_for_path.go
+++ b/internal/gitaly/service/commit/last_commit_for_path.go
@@ -2,8 +2,10 @@ package commit
 
 import (
 	"context"
+	"errors"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
@@ -50,7 +52,7 @@ func (s *server) lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitF
 	}
 
 	commit, err := log.LastCommitForPath(ctx, s.gitCmdFactory, objectReader, repo, git.Revision(in.GetRevision()), path, options)
-	if log.IsNotFound(err) {
+	if errors.As(err, &catfile.NotFoundError{}) {
 		return &gitalypb.LastCommitForPathResponse{}, nil
 	}
 
diff --git a/internal/gitaly/service/commit/list_commits_by_oid.go b/internal/gitaly/service/commit/list_commits_by_oid.go
index b261f798c..3175c4307 100644
--- a/internal/gitaly/service/commit/list_commits_by_oid.go
+++ b/internal/gitaly/service/commit/list_commits_by_oid.go
@@ -1,6 +1,8 @@
 package commit
 
 import (
+	"errors"
+
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
@@ -41,7 +43,7 @@ func (s *server) ListCommitsByOid(in *gitalypb.ListCommitsByOidRequest, stream g
 
 	for _, oid := range in.Oid {
 		commit, err := catfile.GetCommit(ctx, objectReader, git.Revision(oid))
-		if catfile.IsNotFound(err) {
+		if errors.As(err, &catfile.NotFoundError{}) {
 			continue
 		}
 		if err != nil {
diff --git a/internal/gitaly/service/commit/list_commits_by_ref_name.go b/internal/gitaly/service/commit/list_commits_by_ref_name.go
index 387efbbe9..e77c9f916 100644
--- a/internal/gitaly/service/commit/list_commits_by_ref_name.go
+++ b/internal/gitaly/service/commit/list_commits_by_ref_name.go
@@ -1,6 +1,8 @@
 package commit
 
 import (
+	"errors"
+
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/chunk"
@@ -27,7 +29,7 @@ func (s *server) ListCommitsByRefName(in *gitalypb.ListCommitsByRefNameRequest,
 
 	for _, refName := range in.RefNames {
 		commit, err := catfile.GetCommit(ctx, objectReader, git.Revision(refName))
-		if catfile.IsNotFound(err) {
+		if errors.As(err, &catfile.NotFoundError{}) {
 			continue
 		}
 		if err != nil {
diff --git a/internal/gitaly/service/conflicts/list_conflict_files_test.go b/internal/gitaly/service/conflicts/list_conflict_files_test.go
index 367845c96..bfe2018d5 100644
--- a/internal/gitaly/service/conflicts/list_conflict_files_test.go
+++ b/internal/gitaly/service/conflicts/list_conflict_files_test.go
@@ -217,9 +217,12 @@ func TestListConflictFiles(t *testing.T) {
 				}
 
 				return setupData{
-					client:        client,
-					request:       request,
-					expectedError: structerr.NewFailedPrecondition("getting objectreader: object not found"),
+					client:  client,
+					request: request,
+					expectedError: testhelper.WithInterceptedMetadata(
+						structerr.NewFailedPrecondition("getting objectreader: object not found"),
+						"revision", subCommitID.String(),
+					),
 				}
 			},
 		},
diff --git a/internal/gitaly/service/diff/find_changed_paths.go b/internal/gitaly/service/diff/find_changed_paths.go
index 447ad68f7..849073252 100644
--- a/internal/gitaly/service/diff/find_changed_paths.go
+++ b/internal/gitaly/service/diff/find_changed_paths.go
@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"strconv"
@@ -262,7 +263,7 @@ func resolveObjectWithType(
 
 	info, err := objectInfoReader.Info(ctx, git.Revision(fmt.Sprintf("%s^{%s}", revision, expectedType)))
 	if err != nil {
-		if catfile.IsNotFound(err) {
+		if errors.As(err, &catfile.NotFoundError{}) {
 			return "", structerr.NewNotFound("revision can not be found: %q", revision)
 		}
 		return "", err
diff --git a/internal/gitaly/service/repository/apply_gitattributes.go b/internal/gitaly/service/repository/apply_gitattributes.go
index 9d97e9e71..131d961ee 100644
--- a/internal/gitaly/service/repository/apply_gitattributes.go
+++ b/internal/gitaly/service/repository/apply_gitattributes.go
@@ -37,7 +37,7 @@ func (s *server) applyGitattributes(ctx context.Context, repo *localrepo.Repo, o
 	}
 
 	blobObj, err := objectReader.Object(ctx, git.Revision(fmt.Sprintf("%s:.gitattributes", revision)))
-	if err != nil && !catfile.IsNotFound(err) {
+	if err != nil && !errors.As(err, &catfile.NotFoundError{}) {
 		return err
 	}
 
@@ -46,7 +46,7 @@ func (s *server) applyGitattributes(ctx context.Context, repo *localrepo.Repo, o
 		return err
 	}
 
-	if catfile.IsNotFound(err) || blobObj.Type != "blob" {
+	if errors.As(err, &catfile.NotFoundError{}) || blobObj.Type != "blob" {
 		locker, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
 			FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
 		})