W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 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@xxxxxx>
> ---
> 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...
> 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...
> + }
> - 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);
>
>