{"thread":{"id":"45376","subject":"[PATCH] submodule-config: correct error reporting for invalid ignore value","startedAt":"2017-03-14T22:14:53Z","lastAt":"2017-03-15T20:29:41Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314095","messageId":"20170314221440.18259-1-sbeller@google.com","threadId":"45376","inReplyTo":null,"subject":"[PATCH] submodule-config: correct error reporting for invalid ignore value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-14T22:14:40Z","receivedAt":"2017-03-14T22:14:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As 'var' contains the whole value we get error messages that repeat\nthe section and key currently:\n\nwarning: Invalid parameter 'true' for config option 'submodule.submodule.plugins/hooks.ignore.ignore'\n\nFix this by only giving the section name in the warning.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 93453909cf..bb069bc097 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -333,7 +333,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\t strcmp(value, \"all\") &&\n \t\t\t strcmp(value, \"none\"))\n \t\t\twarning(\"Invalid parameter '%s' for config option \"\n-\t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n+\t\t\t\t\t\"'submodule.%s.ignore'\", value, name.buf);\n \t\telse {\n \t\t\tfree((void *) submodule->ignore);\n \t\t\tsubmodule->ignore = xstrdup(value);\n-- \n2.12.0.264.gd6db3f2165.dirty\n\n"},{"id":"314154","messageId":"xmqqefxyfl6b.fsf@gitster.mtv.corp.google.com","threadId":"45376","inReplyTo":"20170314221440.18259-1-sbeller@google.com","subject":"Re: [PATCH] submodule-config: correct error reporting for invalid ignore value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-15T18:29:32Z","receivedAt":"2017-03-15T18:30:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> As 'var' contains the whole value we get error messages that repeat\n> the section and key currently:\n>\n> warning: Invalid parameter 'true' for config option 'submodule.submodule.plugins/hooks.ignore.ignore'\n>\n> Fix this by only giving the section name in the warning.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule-config.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 93453909cf..bb069bc097 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -333,7 +333,7 @@ static int parse_config(const char *var, const char *value, void *data)\n> \t\t\t strcmp(value, \"all\") &&\n> \t\t\t strcmp(value, \"none\"))\n> \t\t\twarning(\"Invalid parameter '%s' for config option \"\n> -\t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n> +\t\t\t\t\t\"'submodule.%s.ignore'\", value, name.buf);\n\nObviously correct. Nobody seems to have complained about this since\nit was first written in 2.6 days, which is a bit sad, though.\n\nThanks.\n"},{"id":"314156","messageId":"CAGZ79kZiM2wkrUZ-KBt0kccfxRZCr79VP-7-YnsqmWX4HAH5-A@mail.gmail.com","threadId":"45376","inReplyTo":"xmqqefxyfl6b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule-config: correct error reporting for invalid ignore value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-15T18:39:33Z","receivedAt":"2017-03-15T18:46:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 15, 2017 at 11:29 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> As 'var' contains the whole value we get error messages that repeat\n>> the section and key currently:\n>>\n>> warning: Invalid parameter 'true' for config option 'submodule.submodule.plugins/hooks.ignore.ignore'\n>>\n>> Fix this by only giving the section name in the warning.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> submodule-config.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/submodule-config.c b/submodule-config.c\n>> index 93453909cf..bb069bc097 100644\n>> --- a/submodule-config.c\n>> +++ b/submodule-config.c\n>> @@ -333,7 +333,7 @@ static int parse_config(const char *var, const char *value, void *data)\n>> strcmp(value, \"all\") &&\n>> strcmp(value, \"none\"))\n>> warning(\"Invalid parameter '%s' for config option \"\n>> - \"'submodule.%s.ignore'\", value, var);\n>> + \"'submodule.%s.ignore'\", value, name.buf);\n>\n> Obviously correct. Nobody seems to have complained about this since\n> it was first written in 2.6 days, which is a bit sad, though.\n>\n> Thanks.\n\nhttps://trends.google.com/trends/explore?date=all&q=submodule,%2Fm%2F05vqwg\n(Comparing \"Submodule\" (search term) with \"Git\" (System Software) over 10 years)\n\n#TiL\n* submodule searches are less than 1% of Git searches,\n so not a lot of users compared to Git itself.\n* submodules are only used in larger countries unlike Git,\n which is used in a lot more countries (well, that may be\n displaying issues)\n* submodules are a German thing (relatively speaking)\n followed by Russia, US, Canada, Australia, Japan.\n\n:-)\n"},{"id":"314163","messageId":"xmqqh92ue2ra.fsf@gitster.mtv.corp.google.com","threadId":"45376","inReplyTo":"xmqqefxyfl6b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule-config: correct error reporting for invalid ignore value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-15T19:52:41Z","receivedAt":"2017-03-15T19:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> As 'var' contains the whole value we get error messages that repeat\n>> the section and key currently:\n>>\n>> warning: Invalid parameter 'true' for config option 'submodule.submodule.plugins/hooks.ignore.ignore'\n>>\n>> Fix this by only giving the section name in the warning.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> submodule-config.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/submodule-config.c b/submodule-config.c\n>> index 93453909cf..bb069bc097 100644\n>> --- a/submodule-config.c\n>> +++ b/submodule-config.c\n>> @@ -333,7 +333,7 @@ static int parse_config(const char *var, const char *value, void *data)\n>> \t\t\t strcmp(value, \"all\") &&\n>> \t\t\t strcmp(value, \"none\"))\n>> \t\t\twarning(\"Invalid parameter '%s' for config option \"\n>> -\t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n>> +\t\t\t\t\t\"'submodule.%s.ignore'\", value, name.buf);\n>\n> Obviously correct.\n\nBut isn't this even more obviously correct?\n\n\twarning(\"invalid parameter '%s' for option %s\", value, var);\n\n"},{"id":"314168","messageId":"CAGZ79kb4rd27rwkYXjerTc=oXUOuo=wb9SOk8vvoMJTEmkRvWw@mail.gmail.com","threadId":"45376","inReplyTo":"xmqqh92ue2ra.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule-config: correct error reporting for invalid ignore value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-15T20:21:39Z","receivedAt":"2017-03-15T20:29:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 15, 2017 at 12:52 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Stefan Beller writes:\n>>\n>>> As 'var' contains the whole value we get error messages that repeat\n>>> the section and key currently:\n>>>\n>>> warning: Invalid parameter 'true' for config option 'submodule.submodule.plugins/hooks.ignore.ignore'\n>>>\n>>> Fix this by only giving the section name in the warning.\n>>>\n>>> Signed-off-by: Stefan Beller \n>>> ---\n>>> submodule-config.c | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/submodule-config.c b/submodule-config.c\n>>> index 93453909cf..bb069bc097 100644\n>>> --- a/submodule-config.c\n>>> +++ b/submodule-config.c\n>>> @@ -333,7 +333,7 @@ static int parse_config(const char *var, const char *value, void *data)\n>>> strcmp(value, \"all\") &&\n>>> strcmp(value, \"none\"))\n>>> warning(\"Invalid parameter '%s' for config option \"\n>>> - \"'submodule.%s.ignore'\", value, var);\n>>> + \"'submodule.%s.ignore'\", value, name.buf);\n>>\n>> Obviously correct.\n>\n> But isn't this even more obviously correct?\n>\n> warning(\"invalid parameter '%s' for option %s\", value, var);\n>\n\nYes. I considered this when writing the patch. It is also obviously correct.\nThe difference is whether you relay funny capitalization to the error message,\nwhich I thought we might not want to do?\n\ngit grep warning yields e.g.\ndiff.c: warning(_(\"Unknown value for 'diff.submodule'\nconfig variable: '%s'\"),\ndiff.c: warning(_(\"Found errors in 'diff.dirstat'\nconfig variable:\\n%s\"),\n\nSo I conclude that we want to present normalized capitalization for\nconfig options\nfor error messages.\n\nThanks,\nStefan\n"}]}