From: Jeff King <peff@peff.net>
Date: Tue, 04 Nov 2025 23:54:46 GMT
Subject: Re: [PATCH v4 14/14] ref-filter: parse objects on demand
Message-ID: <20251104235446.GA3667@coredump.intra.peff.net>
In-Reply-To: <xmqqcy5xnz7e.fsf@gitster.g>

On Tue, Nov 04, 2025 at 03:40:53PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Oct 23, 2025 at 09:16:23AM +0200, Patrick Steinhardt wrote:
> >
> >> -static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
> >> +static int get_object(struct ref_array_item *ref, int deref,
> >>  		      struct expand_data *oi, struct strbuf *err)
> >>  {
> >> -	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
> >> -	int eaten = 1;
> >> +	/* parse_object_buffer() will set eaten to 1 if free() will be needed */
> >> +	int eaten = 0;
> >
> > This comment is surely wrong now, isn't it? It will be set to 1 if
> > free() is _not_ needed:
> >
> >> +out:
> >>  	if (!eaten)
> >>  		free(oi->content);
> >
> > -Peff
> 
> Wow.  Is it just the comment or the updated logic is upside down,
> too?

I think the logic is fine. The meaning of "eaten" did not change. It's
just that some code paths will not bother calling parse_object_buffer()
now (if no atoms need it). So we need to default to "0" (the buffer must
be freed) for those cases. And if parse_object_buffer() is called, it
will always correctly set the value to 1.

The new code does mean that if contentp is NULL, we will always call
free(oi->content), even though nobody would ever have set it. But
presumably it was initialized to NULL in that case and the free is a
noop.

-Peff