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

[Tanay Abhra <tanayabh@gmail.com>]

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?

>>                 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
>