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

[Tanay Abhra <tanayabh@gmail.com>]

On 6/30/2014 9:26 PM, Karsten Blees wrote:
> Am 30.06.2014 16:39, schrieb Tanay Abhra:
>>
>> 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 <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?
>>>
>>> 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.
> 
> If you don't set the value and continue, git will proceed with the variable's
> default setting.
> 
> Which may not be too harmful in some cases, but if a user changes:
> 
>  gc.pruneexpire=4.weeks.ago
> 
> to
> 
>  gc.pruneexpire=4.monhts.ago
> 
> (note the typo), the next git-gc will warn the user and then happily throw
> away data that the user intended to keep (default is 2.weeks.ago).
> 
> Thus I think git should die() if it encounters an invalid config setting.

Okay, point 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.
> 
> If I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.
> whether the config file is structurally correct).
> 
> The semantic value and correctness of a key (e.g. whether its a boolean or an
> int or a string that denotes a known merge algorithm) is only checked when it is
> accessed via git_config_get_<type>. And at this point, <file>:<line> information
> is already lost.
> 
> With the callback approach, both syntactic (structure) and semantic (meaning)
> errors were checked at load time, resulting in
> 
>   die("bad config file line %d in %s", cf->linenr, cf->name);
> 
> if the callback returned -1.
> 

Yup, you are right, we check only syntax error when loading the file for the cache.
I could save the <filename>:<linenr> when the loading the file for future error
reporting. Thanks.