Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options

From: Jakub Narębski <jnareb@gmail.com>
Date: Aug 30, 2016, 14:54 UTC
Message-ID: <9797cb9e-115c-bbea-51d2-a5d686aca514@gmail.com>
In-Reply-To: <e4b48a9a73ff33179556c0137caae85bfb4a48d9.1472457609.git.johannes.schindelin@gmx.de>

W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:

Show 21 quoted lines

> The sequencer reads options from disk and stores them in its struct
> for use during sequencer's operations.
> 
> With this patch, the memory is released afterwards, plugging a
> memory leak.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index b5be0f9..8d79091 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct replay_opts *opts)
>  		free(opts->owned[i]);
>  	free(opts->owned);
>  
> +	free(opts->xopts);
> +

This looks like independent change, not related to using the sequencer_entrust() to store options read from disk in replay_opts struct to be able to free memory afterwards.

I guess you wanted to avoid one line changes...

>  	strbuf_addf(&dir, "%s", get_dir(opts));
>  	remove_dir_recursively(&dir, 0);
>  	strbuf_release(&dir);
> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)

Sidenote: this patch would be easier to read if lines were reordered
as below, but I don't think any slider heuristics could help achieve
that automatically.  Also, the patch might be invalid...

Show 7 quoted lines

>  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
>  	else if (!strcmp(key, "options.mainline"))
>  		opts->mainline = git_config_int(key, value);
> -	else if (!strcmp(key, "options.strategy"))
> +	else if (!strcmp(key, "options.strategy")) {
>  		git_config_string(&opts->strategy, key, value);
> +		sequencer_entrust(opts, (char *) opts->strategy);

I wonder if the ability to free strings dup-ed by git_config_string() be something that is part of replay_opts, or rather remove_sequencer_state(), that is a list of

	free(opts->strategy);
	free(opts->gpg_sign);

And of course

	for (i = 0; i < opts->xopts_nr; i++)
		free(opts->xopts[i]);
	free(opts->xopts);

Though... free(NULL) is nop as per standard, but can we rely on it? If it is a problem, we can create xfree(ptr) being if(ptr)free(ptr);

The *_entrust() mechanism is more generic, but do we use this general-ness? Well, it could be xstrdup or git_config_string doing entrust'ing...

Show 11 quoted lines

> +	}
> -	else if (!strcmp(key, "options.gpg-sign"))
> +	else if (!strcmp(key, "options.gpg-sign")) {
>  		git_config_string(&opts->gpg_sign, key, value);
> +		sequencer_entrust(opts, (char *) opts->gpg_sign);
> +	}
>  	else if (!strcmp(key, "options.strategy-option")) {
>  		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> -		opts->xopts[opts->xopts_nr++] = xstrdup(value);
> +		opts->xopts[opts->xopts_nr++] =
> +			sequencer_entrust(opts, xstrdup(value));

Nice.

>  	} else
>  		return error(_("Invalid key: %s"), key);
>  
>