Am 30.06.2014 16:32, schrieb Eric Sunshine:
> On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees <karsten.blees@xxxxxxxxx> wrote:
>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@xxxxxxxxx> wrote:
>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@xxxxxxxxx> 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@xxxxxxxxx>
>>>>>> ---
>>>>>> 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).
>>
>>>
>>> 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);
>> ...
>
> One would expect so, but notes-utils.c:notes_rewrite_config() is
> actually doing this:
>
> if (!c->combine) {
> error(_("Bad notes.rewriteMode value: '%s'"), v);
> return 1;
> }
>
> Rather than returning the -1 result of error(), which would make
> git_parse_source() die(), it's explicitly returning 1, which
> get_parse_source() ignores.
>
Ahh...I missed the '< 0', sorry.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html