Re: [PATCH 2/4] object-file: adapt `stream_object_signature()` to take a stream

> On Mon, Feb 23, 2026 at 10:50:41AM +0100, Patrick Steinhardt wrote:
> 
> > The function `stream_object_signature()` is responsible for verifying
> > whether the given object ID matches the actual hash of the object's
> > contents. In contrast to `check_object_signature()` it does so in a
> > streaming fashion so that we don't have to load the full object into
> > memory.
> 
> Makes sense, and is the first step I expected to see.
> 
> The code looks OK, though a few small comments below.
> 
> > -int stream_object_signature(struct repository *r, const struct object_id *oid)
> > +int stream_object_signature(struct repository *r,
> > +			    struct odb_read_stream *st,
> > +			    const struct object_id *oid)
> >  {
> >  	struct object_id real_oid;
> > -	struct odb_read_stream *st;
> >  	struct git_hash_ctx c;
> >  	char hdr[MAX_HEADER_LEN];
> >  	int hdrlen;
> >  
> > -	st = odb_read_stream_open(r->objects, oid, NULL);
> > -	if (!st)
> > -		return -1;
> > -
> >  	/* Generate the header */
> >  	hdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);
> >  
> > @@ -160,7 +157,6 @@ int stream_object_signature(struct repository *r, const struct object_id *oid)
> >  		git_hash_update(&c, buf, readlen);
> >  	}
> >  	git_hash_final_oid(&real_oid, &c);
> > -	odb_read_stream_close(st);
> >  	return !oideq(oid, &real_oid) ? -1 : 0;
> >  }
> 
> The minimal change for callers would be to give a wrapper like
> stream_object_signature_from_oid() or similar, that included the
> open/close. But it's not too many lines, and of the two callers, one is
> the caller we are trying to split apart anyway. So inlining these bits
> in the callers makes sense.

> > @@ -330,9 +331,17 @@ struct object *parse_object_with_flags(struct repository *r,
> >  
> >  	if ((!obj || obj->type == OBJ_NONE || obj->type == OBJ_BLOB) &&
> >  	    odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {
> > -		if (!skip_hash && stream_object_signature(r, repl) < 0) {
> > -			error(_("hash mismatch %s"), oid_to_hex(oid));
> > -			return NULL;
> > +		if (!skip_hash) {
> > +			struct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL);
> > +			if (!stream || stream_object_signature(r, stream, repl) < 0) {
> > +				error(_("hash mismatch %s"), oid_to_hex(oid));
> > +				if (stream)
> > +					odb_read_stream_close(stream);
> > +				return NULL;
> > +			}
> > +
> > +			if (stream)
> > +				odb_read_stream_close(stream);
> >  		}
> 
> This final "if (stream)" is a noop; we'd have exited the function
> already if "!stream".
> 
> In the earlier conditional:
> 
>   if (!stream || stream_object_signature(r, stream, repl) < 0)
> 
> we're combining two error checks: opening the stream and actually
> hashing it.

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

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