Re: [RFC/PATCH] notes-util.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>
> ---
>  notes-utils.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/notes-utils.c b/notes-utils.c
> index a0b1d7b..fdc9912 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>                 return NULL;
>  }
>
> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>  {
> -       struct notes_rewrite_cfg *c = cb;
> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
> -               c->enabled = git_config_bool(k, v);
> -               return 0;
> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
> +       struct strbuf key = STRBUF_INIT;
> +       const char *v;
> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
> +
> +       if (!git_config_get_string(key.buf, &v))
> +               c->enabled = git_config_bool(key.buf, v);
> +
> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>                 if (!v)
> -                       return config_error_nonbool(k);
> +                       config_error_nonbool("notes.rewritemode");

There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.

>                 c->combine = parse_combine_notes_fn(v);

Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.

> -               if (!c->combine) {
> +               if (!c->combine)
>                         error(_("Bad notes.rewriteMode value: '%s'"), v);
> -                       return 1;
> -               }
> -               return 0;
> -       } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
> +       }
> +       if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
>                 /* note that a refs/ prefix is implied in the
>                  * underlying for_each_glob_ref */
>                 if (starts_with(v, "refs/notes/"))
> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
>                 else
>                         warning(_("Refusing to rewrite notes in %s"
>                                 " (outside of refs/notes/)"), v);
> -               return 0;
>         }
> -
> -       return 0;
> +       strbuf_release(&key);

It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.

>  }
>
>
> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
>                 c->refs_from_env = 1;
>                 string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
>         }
> -       git_config(notes_rewrite_config, c);
> +       notes_rewrite_config(c);
>         if (!c->enabled || !c->refs->nr) {
>                 string_list_clear(c->refs, 0);
>                 free(c->refs);
> --
> 1.9.0.GIT