On 6/25/2014 9:29 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html