Re: [PATCH 10/13] sparse-checkout: free sparse_filename after use

From: Jeff King <peff@peff.net>
Date: Jun 4, 2024, 10:01 UTC
Message-ID: <20240604100142.GC1298378@coredump.intra.peff.net>
In-Reply-To: <Zl7FgVPHGiXJYSah@framework>

On Tue, Jun 04, 2024 at 09:42:57AM +0200, Patrick Steinhardt wrote:

Show 13 quoted lines

> > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> > index 356b7349f9..3af9fec1fb 100644
> > --- a/builtin/sparse-checkout.c
> > +++ b/builtin/sparse-checkout.c
> > @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)
> >  		return 0;
> >  	}
> >  
> > +	free(sparse_filename);
> > +
> 
> I wonder whether it would make sense to merge this patch and patch 4
> and then refactor the code to have a common exit path.

I thought about that, too, but it doesn't quite work. In the non-error exit path we _don't_ clean up the pattern_list, because we tail-call into write_patterns_and_update(), which frees it itself.

If we refactored that function to _not_ free, and then switched here to a "ret" variable, like:

	...
	ret = write_patterns_and_update(&pl);
  out:
	clear_pattern_list(&pl);
	free(sparse_filename);
	return ret;

it could work. I mostly tried to err on the side of minimizing refactoring, though.

-Peff