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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 25, 2014, 02:12 UTC
Message-ID: <CAPig+cQ3hzFFRRhbDTddArBC0nB9gZ7yMXuh35GHe73x-1JDng@mail.gmail.com>
In-Reply-To: <1403520105-23250-1-git-send-email-tanayabh@gmail.com>

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:

Show 42 quoted lines

> 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>
> ---
>  alias.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/alias.c b/alias.c
> index 5efc3d6..0fe32bc 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -1,25 +1,17 @@
>  #include "cache.h"
>
> -static const char *alias_key;
> -static char *alias_val;
> -
> -static int alias_lookup_cb(const char *k, const char *v, void *cb)
> -{
> -       if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
> -               if (!v)
> -                       return config_error_nonbool(k);
> -               alias_val = xstrdup(v);
> -               return 0;
> -       }
> -       return 0;
> -}
> -
>  char *alias_lookup(const char *alias)
>  {
> -       alias_key = alias;
> -       alias_val = NULL;
> -       git_config(alias_lookup_cb, NULL);
> -       return alias_val;
> +       const char *v;
> +       char *value;
> +       struct strbuf key = STRBUF_INIT;
> +       strbuf_addf(&key, "alias.%s", alias);
> +       git_config_get_string(key.buf, &v);
> +       if (!v)
> +               config_error_nonbool(key.buf);

If 'v' is NULL, you correctly report an error, but then fall through and invoke xstrdup() with NULL, which invites undefined behavior [1].

[1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html

> +       value = xstrdup(v);
> +       strbuf_release(&key);
> +       return value;

You could release the strbuf earlier, which would allow you to 'return xstrdup(v)' and drop the 'value' variable. Perhaps you want something like this:

    const char *v;
    struct strbuf key = STRBUF_INIT;
    strbuf_addf(&key, "alias.%s", alias);
    git_config_get_string(key.buf, &v);
    if (v)
        config_error_nonbool(key.buf);
    strbuf_release(&key);
    return v ? xstrdup(v) : NULL;

Show 5 quoted lines

>  }
>
>  #define SPLIT_CMDLINE_BAD_ENDING 1
> --
> 1.9.0.GIT