Re: [PATCH 06/22] read-cache: fix leaking hashfile when writing index fails

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 8, 2024, 05:04 UTC
Message-ID: <ZrRR8crXHBGvMdjM@tanuki>
In-Reply-To: <D39HIJOL7UTK.29BEN5ZXSH4KF@jamesliu.io>

On Wed, Aug 07, 2024 at 05:01:17PM +1000, James Liu wrote:

Show 25 quoted lines

> On Tue Aug 6, 2024 at 7:00 PM AEST, Patrick Steinhardt wrote:
> > Refactor the code to have a common exit path where we can free this and
> > other allocated memory. While at it, refactor our use of `strbuf`s such
> > that we reuse the same buffer to avoid some unneeded allocations.
> >
> > @@ -3105,7 +3117,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
> >  	trace2_data_intmax("index", the_repository, "write/cache_nr",
> >  			   istate->cache_nr);
> >  
> > -	return 0;
> > +	ret = 0;
> > +
> > +out:
> > +	if (f)
> > +		free_hashfile(f);
> > +	strbuf_release(&sb);
> > +	free(ieot);
> > +	return ret;
> >  }
> 
> Is it generally a pattern in Git to use `goto <label>` instead of
> returns when there are multiple return points in a function? We're also
> performing cleanup duties here and in most of those scenarios but there
> are some cases like `reftable_be_pack_refs()` where the goto simply
> collapses multiple return points into a single path.

Yes, that's usually how we avoid repetetive cleanup code for each of the return paths. `reftable_be_pack_refs()` is a bit more on the curious side as the common exit path doesn't really do helpful. That one could have just as well used plain returns.

Patrick