From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 23 Feb 2026 15:59:09 GMT
Subject: Re: [PATCH 3/4] packfile: expose function to read object stream for an offset
Message-ID: <aZx5Tc-rgih0S4gS@pks.im>
In-Reply-To: <20260223131201.GC215671@coredump.intra.peff.net>

On Mon, Feb 23, 2026 at 08:12:01AM -0500, Jeff King wrote:
> 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.

Yeah, I agree it's a bit ugly. But the function is not likely to gain a
lot of additional callers anyway, as it is an implementation detail of
the packfile store. So overall I think it's okayish.

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

We could of course have it return the `enum object_type` directly, which
gives us enough context to do this. But on the other hand it'd mean that
we might now miss adding calls to `mark_bad_packed_object()` in the
future, and it causes a bit of repetition across callsites.

So I think I lean towards my proposed patch.

Thanks!

Patrick