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

[Jeff King <peff@peff.net>]

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

> > 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.

> 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