W dniu 30.08.2016 o 19:52, Johannes Schindelin pisze:
> 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).
I guess from cursory browsing through the Git code that _currently_
they are only read from the config file, where git_config_string()
strdup's them, isn't it? And we want to prepare for the future, where
the caller would prepare replay_opts, and the caller would be responsible
for freeing data if necessary?
Would there be any sane situation where some of data should be owned
by caller (and freed by caller), and some of data should be owned by
sequencer library API (and freed in remove_sequencer_state())? If
not, perhaps *_entrust() mechanism is overthinking it, and we simply
need 'is_strdup' boolean flag or something like that...
>> 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().
Maybe inelegant, but it might be easier than inventing and implementing
*_entrust() mechanism, like Hannes wrote.
>
> 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.
Yeah, that's my take on it too.
--
Jakub Narębski