threads / patch / 45376

patchsubmodule-config: correct error reporting for invalid ignore value

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

## tl;dr

5 messages between Mar 14, 2017 and Mar 15, 2017. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Stefan Beller· Mar 14, 2017, 22:14 UTC · lore

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

Show changes to submodule-config.c +1 −1

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);
 		else {
 			free((void *) submodule->ignore);
 			submodule->ignore = xstrdup(value);

-- 
2.12.0.264.gd6db3f2165.dirty

Junio C Hamano· Mar 15, 2017, 18:29 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:

Show 22 quoted lines

> 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. Nobody seems to have complained about this since it was first written in 2.6 days, which is a bit sad, though.

Thanks.

Stefan Beller· Mar 15, 2017, 18:39 UTC · re: Junio C Hamano · lore

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

On Wed, Mar 15, 2017 at 11:29 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> 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.  Nobody seems to have complained about this since
> it was first written in 2.6 days, which is a bit sad, though.
>
> Thanks.

https://trends.google.com/trends/explore?date=all&q=submodule,%2Fm%2F05vqwg (Comparing "Submodule" (search term) with "Git" (System Software) over 10 years)

#TiL
* submodule searches are less than 1% of Git searches,
   so not a lot of users compared to Git itself.
* submodules are only used in larger countries unlike Git,
  which is used in a lot more countries (well, that may be
  displaying issues)
* submodules are a German thing (relatively speaking)
   followed by Russia, US, Canada, Australia, Japan.

:-)

Junio C Hamano· Mar 15, 2017, 19:52 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:

Show 26 quoted lines

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

Stefan Beller· Mar 15, 2017, 20:21 UTC · re: Junio C Hamano · lore

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

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