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

[Tanay Abhra <tanayabh@gmail.com>]

On 6/25/2014 9:29 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  pager.c | 44 +++++++++++++++-----------------------------
>>  1 file changed, 15 insertions(+), 29 deletions(-)
>>
>> diff --git a/pager.c b/pager.c
>> index 8b5cbc5..96abe6d 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -6,12 +6,6 @@
>>  #define DEFAULT_PAGER "less"
>>  #endif
>>
>> -struct pager_config {
>> -       const char *cmd;
>> -       int want;
>> -       char *value;
>> -};
>> -
>>  /*
>>   * This is split up from the rest of git so that we can do
>>   * something different on Windows.
>> @@ -155,30 +149,22 @@ int decimal_width(int number)
>>         return width;
>>  }
>>
>> -static int pager_command_config(const char *var, const char *value, void *data)
>> -{
>> -       struct pager_config *c = data;
>> -       if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
>> -               int b = git_config_maybe_bool(var, value);
>> -               if (b >= 0)
>> -                       c->want = b;
>> -               else {
>> -                       c->want = 1;
>> -                       c->value = xstrdup(value);
>> -               }
>> -       }
>> -       return 0;
>> -}
>> -
>>  /* 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;
> 
> Two issues:
> 
> First, why is 'if(value)' standing by itself? Although this works, it
> seems to imply that 'value' might be able to become non-NULL by some
> mechanism other than the get_config_maybe_bool() call, which means
> that people reading this code have to spend extra time trying to
> understand the overall logic. If you follow the example of the
> original code, where 'value' is only ever set when 'b < 0', then it is
> obvious even to the most casual reader that 'pager_program' is
> assigned only for that one condition.
> 

Noted.

> 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.
> 

Noted. Thanks.

>> +       strbuf_release(&key);
>> +       return want;
>>  }
>> --
>> 1.9.0.GIT
>