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.schinde...@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... > 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); > >