Re: [PATCH 0/4] fix a leak with excludes_file

From: Rubén Justo <rjusto@gmail.com>
Date: Apr 7, 2024, 17:48 UTC
Message-ID: <1323321b-554e-4b16-a814-03710c50db99@gmail.com>
In-Reply-To: <xmqq7chajs3e.fsf@gitster.g>

On Sat, Apr 06, 2024 at 10:53:09AM -0700, Junio C Hamano wrote:

> Rubén Justo <rjusto@gmail.com> writes:
> 
> > We call twice to git_config(): first to get the main git-branch(1) options,
> > and second to get the ones related to the sequencer.

Obviously I meant git-rebase(1) -- or any other command that uses the sequencer under the hood.

Show 5 quoted lines

> Adding a parallel API
> next to the existing git_config_pathname() and interpolate_path()
> and convert only these callers that touch excludes_file would not
> help other callers that hold the pointer git_config_pathname()
> returned.

It does not have to be like that. We may no longer need the current and problematic git_config_pathname(). However I did not want to go that far in this series.

Show 15 quoted lines

>  config.c         | 4 +++-
>  t/t7300-clean.sh | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git c/config.c w/config.c
> index eebce8c7e0..ae3652b08f 100644
> --- c/config.c
> +++ w/config.c
> @@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value,
>  	if (!strcmp(var, "core.askpass"))
>  		return git_config_string(&askpass_program, var, value);
>  
> -	if (!strcmp(var, "core.excludesfile"))
> +	if (!strcmp(var, "core.excludesfile")) {
> +		free((char *)excludes_file);

Aaah, you prefer this :-( ... this free is what I was referring to, in the message you are replying to, as the simpler fix ...

It obviously plugs the leak and so my itch is gone; therefore OK by me to the series I have seen that you have already sent with this.

Still, I find the approach in this series more interesting than the simple free. The strbuf saves us from subsequent free+alloc's, for exactly the same "core.excludesFile". The strbuf states clearly, IMHO, who owns the pointer. And other niceties.