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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

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.

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