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

From: Tanay Abhra <[email-masked]>
Date: Mon, 30 Jun 2014 20:09:18 +0530

On 6/30/2014 7:04 PM, Karsten Blees wrote:
> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <[email-masked]> wrote:
>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <[email-masked]> 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 <[email-masked]>
>>>>> ---
>>>>>  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?
> 
> IMO its better to Fail Fast than continue with some invalid config (which
> may lead to more severe errors such as data corruption / data loss).

Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL. The only change is that
program will not crash in this case and warn the user not set a NULL value for
a non boolean key.
This won't lead to severe errors as the value will not be set if found value
is a NULL.

>>
>> 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
>>
> 
> I think this would also die in git_parse_source():
> ...
>     if (get_value(fn, data, var) < 0)
>       break;
>   }
>   if (cf->die_on_error)
>     die("bad config file line %d in %s", cf->linenr, cf->name);
> ...
> 
> (AFAICT, die_on_error is always true, except if invoked via 'git-config
> --blob', which isn't used anywhere...)
> 

Noted.

> This, however, raises another issue: switching to the config cache looses
> file/line-precise error reporting for semantic errors. I don't know if
> this feature is important enough to do something about it, though. A
> message of the form "Key 'xyz' is bad" should usually enable a user to
> locate the problematic file and line.
> 

Hmn, but during the config cache construction we parse key-value pairs through
git_config() which still warns users about semantic errors. This happened
yesterday only when I was writing tests for the new API.

Thanks for the review.

Cheers,
Tanay Abhra.