On Thu, Feb 27, 2025 at 07:32:25PM -0500, Taylor Blau wrote:
> On Tue, Feb 25, 2025 at 01:30:56AM -0500, Jeff King wrote:
> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> > index e493600aff..86a2825473 100755
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -951,6 +951,8 @@ test_expect_success 'object reading handles zlib dictionary' - <<\EOT
> > printf '\170\273\017\112\003\143' >$objpath &&
> >
> > test_must_fail git cat-file blob $blob 2>err &&
> > + test_grep ! 'too long' err &&
> > + test_grep 'error: unable to unpack' err &&
> > test_grep 'error: inflate: needs dictionary' err
> > EOT
>
> All looking good here, too.
>
> I think the test_grep is hiding what is a fairly unpleasant error
> message that says the same thing a few times from different points in
> the call-stack. But that isn't anything new from this series, and I'm
> content to let it be a problem for another day ;-).
Yeah, we get one set of errors when we ask for the type to find out if
we need to peel a tag (it's not a tag, it's OBJ_ERR ;) ). And then
again we ask if it's a blob to try streaming. It's still OBJ_ERR. And
then we fall back to the non-streaming case.
We should probably check for errors earlier. And also avoid asking for
the type twice when we didn't peel a tag, which is just stupidly
inefficient.
Perhaps something like this (untested):
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 9de1016acd..e1dbbfeb43 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -236,7 +236,13 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
if (exp_type_id == OBJ_BLOB) {
struct object_id blob_oid;
- if (oid_object_info(the_repository, &oid, NULL) == OBJ_TAG) {
+ enum object_type found_type = oid_object_info(the_repository,
+ &oid, NULL);
+
+ if (found_type < 0)
+ die(_("unable to read %s"), oid_to_hex(&oid));
+
+ if (found_type == OBJ_TAG) {
char *buffer = repo_read_object_file(the_repository,
&oid,
&type,
@@ -251,10 +257,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
&hash_algos[oid.algo]))
die("%s not a valid tag", oid_to_hex(&oid));
free(buffer);
+ found_type = type;
} else
oidcpy(&blob_oid, &oid);
- if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) {
+ if (found_type == OBJ_BLOB) {
ret = stream_blob(&blob_oid);
goto cleanup;
}
But yes, this is all way out of scope for this series, and is true of
any corruption.
-Peff
