Re: [PATCH 21/21] list-objects-filter-options: work around reported leak on error

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 18, 2024 at 02:04:18PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> > index 00611107d20..fa72e81e4ad 100644
> > --- a/list-objects-filter-options.c
> > +++ b/list-objects-filter-options.c
> > @@ -252,16 +252,14 @@ void parse_list_objects_filter(
> >  	const char *arg)
> >  {
> >  	struct strbuf errbuf = STRBUF_INIT;
> > -	int parse_error;
> >  
> >  	if (!filter_options->filter_spec.buf)
> >  		BUG("filter_options not properly initialized");
> >  
> >  	if (!filter_options->choice) {
> > +		if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
> > +			die("%s", errbuf.buf);
> >  		strbuf_addstr(&filter_options->filter_spec, arg);
> > -
> > -		parse_error = gently_parse_list_objects_filter(
> > -			filter_options, arg, &errbuf);
> >  	} else {
> >  		struct list_objects_filter_options *sub;
> >  
> > @@ -271,18 +269,17 @@ void parse_list_objects_filter(
> >  		 */
> >  		transform_to_combine_type(filter_options);
> >  
> > -		strbuf_addch(&filter_options->filter_spec, '+');
> > -		filter_spec_append_urlencode(filter_options, arg);
> >  		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
> >  			      filter_options->sub_alloc);
> >  		sub = &filter_options->sub[filter_options->sub_nr - 1];
> >  
> >  		list_objects_filter_init(sub);
> > -		parse_error = gently_parse_list_objects_filter(sub, arg,
> > -							       &errbuf);
> > +		if (gently_parse_list_objects_filter(sub, arg, &errbuf))
> > +			die("%s", errbuf.buf);
> 
> Do we actually have a test hitting this code path? I wanted to figure
> out why `filter_options->sub` wasn't leaky (I assume that's what you're
> talking about in your commit message), but I wasn't able to reproduce a
> scenario where we should die here.

You only refer to the second hunk, right? I couldn't find any code paths
hitting this, so this is more of a "Let's massage this code in the same
way" change. I don't want the next person to go through the same rabbit
hole as I had to go through :)

Patrick