From: Jeff King <peff@peff.net>
Date: Mon, 23 Feb 2026 12:59:55 GMT
Subject: Re: [PATCH 2/4] object-file: adapt `stream_object_signature()` to take a stream
Message-ID: <20260223125955.GB215671@coredump.intra.peff.net>
In-Reply-To: <aZxGLKycnZcVoXPt@pks.im>

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

> > That matches the existing code (since it all happened in a
> > single function), but should we take this opportunity to give more
> > accurate error messages? I.e., to do:
> > 
> >   if (!stream) {
> > 	error(_("unable to open object stream for %s"), oid_to_hex(oid));
> > 	return NULL;
> >   }
> >   if (stream_object_signature(r, stream, repl) < 0) {
> > 	error(_("hash mismatch %s"), oid_to_hex(oid));
> > 	odb_read_stream_close(stream);
> > 	return NULL;
> >   }
> >   odb_read_stream_close(stream);
> > 
> > I dunno. It should be quite uncommon to see either of these messages,
> > but that is sometimes the moment when details are most important.
> 
> Agreed, that feels like a sensible change indeed. Also makes the code
> flow easier to follow in my opinion.

Yeah, the readability was actually what got me thinking on it in the
first place.

> > Also, as an aside, I found it curious that we still need to pass the
> > repository struct to stream_object_signature(). That's because it needs
> > to know the correct hash_algo. I wondered if the stream struct itself
> > might know about that, but it doesn't seem to (it doesn't know anything
> > about where it came from). So it's unavoidable that we'd need to retain
> > it.
> 
> Yeah, agreed. I wondered whether we should eventually extend `struct
> odb_read_stream` to have a pointer to the owning object source, and in
> that case we could've avoided the extra repository parameter. But I
> decided it was out of scope for this patch series, also because I don't
> want to cause conflicts with other stuff I'm working on in this vicinity
> :)

Yes, definitely out of scope for this series.

-Peff