Re: [PATCH] submodule-config: correct error reporting for invalid ignore value

From: Stefan Beller <sbeller@google.com>
Date: Mar 15, 2017, 20:21 UTC
Message-ID: <CAGZ79kb4rd27rwkYXjerTc=oXUOuo=wb9SOk8vvoMJTEmkRvWw@mail.gmail.com>
In-Reply-To: <xmqqh92ue2ra.fsf@gitster.mtv.corp.google.com>

On Wed, Mar 15, 2017 at 12:52 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> As 'var' contains the whole value we get error messages that repeat
>>> the section and key currently:
>>>
>>> warning: Invalid parameter 'true' for config option 'submodule.submodule.plugins/hooks.ignore.ignore'
>>>
>>> Fix this by only giving the section name in the warning.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>  submodule-config.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/submodule-config.c b/submodule-config.c
>>> index 93453909cf..bb069bc097 100644
>>> --- a/submodule-config.c
>>> +++ b/submodule-config.c
>>> @@ -333,7 +333,7 @@ static int parse_config(const char *var, const char *value, void *data)
>>>                       strcmp(value, "all") &&
>>>                       strcmp(value, "none"))
>>>                      warning("Invalid parameter '%s' for config option "
>>> -                                    "'submodule.%s.ignore'", value, var);
>>> +                                    "'submodule.%s.ignore'", value, name.buf);
>>
>> Obviously correct.
>
> But isn't this even more obviously correct?
>
>         warning("invalid parameter '%s' for option %s", value, var);
>

Yes. I considered this when writing the patch. It is also obviously correct. The difference is whether you relay funny capitalization to the error message, which I thought we might not want to do?

git grep warning yields e.g. diff.c: warning(_("Unknown value for 'diff.submodule' config variable: '%s'"), diff.c: warning(_("Found errors in 'diff.dirstat' config variable:\n%s"),

So I conclude that we want to present normalized capitalization for config options for error messages.

Thanks, Stefan