Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> On 6/25/2014 1:24 PM, 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>
>>> ---
>>>  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.
>
> Is this change serious enough? Can I ignore it?

I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:

- if "notes.rewritemode" is bool, it die()s.

- if "notes.rewritemode" doesn't specify a recognized mode, it
error()s but continues

- if "notes.rewriteref" doesn't start with "refs/notes/, it warning()s
and continues

It would be nice to hear an opinion from someone more invested in the
config system.

>>>                 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.
>>
>
> Noted.
>
>>> -               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.
>>
>
> Noted. Thanks.
>
>>>  }
>>>
>>>
>>> @@ -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
>>