Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Karsten Blees <karsten.blees@gmail.com>]

Am 25.06.2014 05:59, schrieb Eric Sunshine:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:

[...]

>>  /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
>>  int check_pager_config(const char *cmd)
>>  {
>> -       struct pager_config c;
>> -       c.cmd = cmd;
>> -       c.want = -1;
>> -       c.value = NULL;
>> -       git_config(pager_command_config, &c);
>> -       if (c.value)
>> -               pager_program = c.value;
>> -       return c.want;
>> +       struct strbuf key = STRBUF_INIT;
>> +       int want = -1;
>> +       const char *value = NULL;
>> +       strbuf_addf(&key, "pager.%s", cmd);
>> +       if (!git_config_get_string(key.buf, &value)) {
>> +               int b = git_config_maybe_bool(key.buf, value);
>> +               if (b >= 0)
>> +                       want = b;
>> +               else
>> +                       want = 1;
>> +       }
>> +       if (value)
>> +               pager_program = value;

[...]
> 
> Second, don't you want to xstrdup(value) when assigning to
> 'pager_program'? If you don't, then 'pager_program' will become a
> dangling pointer when config_cache_free() is invoked.
> 

I don't think that values from the global config cache should be xstrdup()ed.
After all, caching the values during the lifetime of the git process is the
entire point of the config cache, isn't it?

The only reason to call config_cache_free() is to load a _different_
configuration. In this case, however, you would also need to call the relevant
config functions again, leaking all xstrdup()ed strings.

If for some reason a config string is accessed after config_cache_free()
(which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
will continue to run with some invalid configuration). This is IMO much worse
than failing with segfault.