Re: [PATCH 3/4] packfile: expose function to read object stream for an offset

From: Jeff King <peff@peff.net>
Date: Feb 23, 2026, 13:12 UTC
Message-ID: <20260223131201.GC215671@coredump.intra.peff.net>
In-Reply-To: <aZxGMrGkVNeAdC1N@pks.im>

On Mon, Feb 23, 2026 at 01:21:06PM +0100, Patrick Steinhardt wrote:

Show 8 quoted lines

> > So your patch here might be making the problem a tiny bit worse, but not
> > in a material way. I think we can ignore it for now.
> 
> I guess the "tiny bit worse" part is that we don't handle the case
> anymore where `unpack_object_header()` returns `OBJ_BAD`. As you say, we
> previously didn't fully parse the object anyway, so we couldn't have
> detected all kinds of corruptions. But we definitely handled the case
> where `unpack_object_header()` failed.

Yeah, I think that would cover it. Technically packed_object_info() could error on more cases (e.g., errors chasing delta bases for type/size info). But we would bail on trying to stream those anyway, so presumably any errors would be found via the non-streaming code paths in those cases.

Show 12 quoted lines

> So maybe we should do something like the below patch?
> [...]
> @@ -2571,6 +2572,9 @@ int packfile_read_object_stream(struct odb_read_stream **out,
>  	switch (in_pack_type) {
>  	default:
>  		return -1; /* we do not do deltas for now */
> +	case OBJ_BAD:
> +		mark_bad_packed_object(pack, oid);
> +		return -1;
>  	case OBJ_COMMIT:
>  	case OBJ_TREE:
>  	case OBJ_BLOB:

I think that restores the original behavior. But I'm not sure it's even worth it. We are still missing the much more likely case of a bit error in the actual zlib stream, which would not be caught until much later.

So yeah, if you want to feel better about making sure your patch keeps the behavior as identical as possible, I don't mind adding this. But it feels like the tip of the iceberg, and I'd be OK leaving it for later (or never).

My biggest objection is not the two lines above (which I actually think clarify what is going on) but rather this interface change:

>  int packfile_read_object_stream(struct odb_read_stream **out,
> +				const struct object_id *oid,
>  				struct packed_git *pack,
>  				off_t offset);

Now we are back to taking an oid, except we don't ever use it to look up the object! So it's a little misleading that it's there at all. It may be the best we can do, though.

The only other way I could think of is for packfile_read_object_stream() to return a more detailed error: one of "success", "chose not to stream", or "broken object". And then the caller can call mark_bad_packed_object() as appropriate. In this case, I think packfile_store_read_object_stream() would do so, but verify_pack() probably would not choose to (it is not interested in fallbacks at all but is going through an individual pack).

-Peff