Hi Kuba,
On Tue, 30 Aug 2016, Jakub Narębski wrote:
> 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...
Actually, it is not an independent change, but it free()s memory that has
been allocated while reading the options, as the commit message says ;-)
> > @@ -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);
That is not necessarily possible because the way sequencer works, the
options may have not actually be read from the file, but may be populated
by the caller (in which case we do not necessarily want to require
strdup()ing the strings just so that the sequencer can clean stuff up
afterwards).
> Though... free(NULL) is nop as per standard, but can we rely on it?
We can, and we do.
> 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...
Right, but that is exactly what I wanted to avoid, because it is rather
inelegant to strdup() strings just so that we do not have to record what
to free() and what not to free().
BTW I have no objection at all to generalize this sequencer_entrust()
mechanism further (read: to other, similar use cases), should it withstand
the test of time.
Ciao,
Johannes