Re: [PATCH 06/10] unpack_loose_header(): avoid numeric comparison of zlib status

From: Jeff King <peff@peff.net>
Date: Mar 4, 2025, 06:55 UTC
Message-ID: <20250304065501.GB1283901@coredump.intra.peff.net>
In-Reply-To: <Z8EEGd4Q+Py3Ym0l@nand.local>

On Thu, Feb 27, 2025 at 07:32:25PM -0500, Taylor Blau wrote:

Show 20 quoted lines

> 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