# [PATCH] submodule-config: correct error reporting for invalid ignore value 5 messages from 2017-03-14 to 2017-03-15. Participants: Stefan Beller, Junio C Hamano. Thread: https://gitlist.dev/t/45376 ## Stefan Beller, 2017-03-14 22:14 Subject: [PATCH] submodule-config: correct error reporting for invalid ignore value Message-ID: <20170314221440.18259-1-sbeller@google.com> URL: https://gitlist.dev/e/20170314221440.18259-1-sbeller%40google.com ``` 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 --- 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); else { free((void *) submodule->ignore); submodule->ignore = xstrdup(value); -- 2.12.0.264.gd6db3f2165.dirty ``` ## Junio C Hamano, 2017-03-15 18:29 Subject: Re: [PATCH] submodule-config: correct error reporting for invalid ignore value Message-ID: URL: https://gitlist.dev/e/xmqqefxyfl6b.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170314221440.18259-1-sbeller@google.com> ``` Stefan Beller 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 > --- > 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, 2017-03-15 18:39 Subject: Re: [PATCH] submodule-config: correct error reporting for invalid ignore value Message-ID: URL: https://gitlist.dev/e/CAGZ79kZiM2wkrUZ-KBt0kccfxRZCr79VP-7-YnsqmWX4HAH5-A%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 15, 2017 at 11:29 AM, Junio C Hamano wrote: > Stefan Beller 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 >> --- >> 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, 2017-03-15 19:52 Subject: Re: [PATCH] submodule-config: correct error reporting for invalid ignore value Message-ID: URL: https://gitlist.dev/e/xmqqh92ue2ra.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Stefan Beller 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 >> --- >> 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, 2017-03-15 20:21 Subject: Re: [PATCH] submodule-config: correct error reporting for invalid ignore value Message-ID: URL: https://gitlist.dev/e/CAGZ79kb4rd27rwkYXjerTc%3DoXUOuo%3Dwb9SOk8vvoMJTEmkRvWw%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 15, 2017 at 12:52 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Stefan Beller 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 >>> --- >>> 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 ```