{"thread":{"id":"38515","subject":"BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","startedAt":"2015-02-06T12:45:28Z","lastAt":"2015-02-18T19:02:15Z","messageCount":16,"participants":["Andreas Krey","Jeff King","Junio C Hamano","Mikael Magnusson","Tanay Abhra"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255669","messageId":"20150206124528.GA18859@inner.h.apk.li","threadId":"38515","inReplyTo":null,"subject":"BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2015-02-06T12:45:28Z","receivedAt":"2015-02-06T12:45:28Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi all,\n\nthere seems to be a regression in the behaviour of 'git show_ref'\n(note the underscore). In v2.0.3-711-g586f414 it starts to say:\n\n $ ./git show_ref\n error: invalid key: pager.show_ref\n git: 'show_ref' is not a git command. See 'git --help'.\n\nand somewhere (probably two commits, judging the diffs)\nlater that changes again to:\n\n $ git show_ref\n error: invalid key: pager.show_ref\n error: invalid key: alias.show_ref\n git: 'show_ref' is not a git command. See 'git --help'.\n\nApparently we need to squelch this message from\nwithin git_config_get_* in this case?\n\nStill present in 2.3.0.\n\nAndreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds \nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"255671","messageId":"20150206193313.GA4220@peff.net","threadId":"38515","inReplyTo":"20150206124528.GA18859@inner.h.apk.li","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-06T19:33:13Z","receivedAt":"2015-02-06T19:33:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:\n\n> there seems to be a regression in the behaviour of 'git show_ref'\n> (note the underscore). In v2.0.3-711-g586f414 it starts to say:\n> \n> $ ./git show_ref\n> error: invalid key: pager.show_ref\n> git: 'show_ref' is not a git command. See 'git --help'.\n> \n> and somewhere (probably two commits, judging the diffs)\n> later that changes again to:\n> \n> $ git show_ref\n> error: invalid key: pager.show_ref\n> error: invalid key: alias.show_ref\n> git: 'show_ref' is not a git command. See 'git --help'.\n> \n> Apparently we need to squelch this message from\n> within git_config_get_* in this case?\n\nThis is highlighting the problem with \"pager.*\" that Junio mentioned\nrecently, which is that the keyname has arbitrary data, but\nsyntactically is limited to alnum and \"-\". This should have been:\n\n pager.show_ref.enabled\n\nfrom the beginning. But of course it was not. Even if we transition, we\nwould want to support pager.* for a while.\n\nI don't think squelching the messages is quite the right approach. They\ncome from git_config_parse_key, which barfs on parsing the syntactically\ninvalid keyname. So not only are we complaining, but we are not actually\nlooking up the value. I don't think that's technically a regression in\n586f414, though. The reader started to complain, but AFAICT git would\nnot agree to parse a file containing:\n\n [pager]\n show_ref = true\n\nin the first place. So it is not a new problem, but it is a bug that you\ncannot set pager config for such a command or alias.\n\nI can think of a few possible paths forward:\n\n 1. Squelch the messages, and declare \"show_ref\" and friends\n out-of-luck for pager config or aliases.\n\n 2. Relax the syntactic rules for config keys to allow more characters.\n We cannot make this perfect (e.g., we cannot allow \".\" for reasons\n of ambiguity), but I imagine we could cover most practical cases.\n\n Note that we would need the matching loosening on the file-parsing\n side.\n\n 3. Start phasing in pager.*.enabled (and I guess pager.*.command). We\n would still do the lookup of pager.* for backwards compatibility,\n but we would be careful to do so only when it is syntactically\n valid. IOW, this looks like (1), except the path forward for\n \"show_ref\" is to use the new, more robust, syntax.\n\n-Peff\n"},{"id":"255674","messageId":"xmqqbnl6hljt.fsf@gitster.dls.corp.google.com","threadId":"38515","inReplyTo":"20150206193313.GA4220@peff.net","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T19:44:38Z","receivedAt":"2015-02-06T19:44:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:\n>\n>> $ git show_ref\n>> error: invalid key: pager.show_ref\n>> error: invalid key: alias.show_ref\n>> git: 'show_ref' is not a git command. See 'git --help'.\n>> \n>> Apparently we need to squelch this message from\n>> within git_config_get_* in this case?\n> ...\n> So it is not a new problem, but it is a bug that you\n> cannot set pager config for such a command or alias.\n\nHmm, I think these are two separate issues.\n\n (1) you cannot define \"alias.my_merge\" because that is not a valid\n key. We cannot add a new official subcommand \"git c_m_d\"\n because users cannot define \"pager.c_m_d\" for it for the same\n reason.\n\n (2) \"git no-such-command\" does not get these extraneous error\n messages, but \"git no_such_command\" does.\n\nSolution to (1) would be to move to \"alias.my_merge.command = ...\"\nand \"pager.c_m_d.enabled = true\". But I do not think that would\nsolve (1) until we transition and start ignoring alias.my_merge\nand pager.c_m_d, and I do not think of a way other than squelching\nthe messages to solve (1) during the transition period.\n\n> I can think of a few possible paths forward:\n>\n> 1. Squelch the messages, and declare \"show_ref\" and friends\n> out-of-luck for pager config or aliases.\n>\n> 2. Relax the syntactic rules for config keys to allow more characters.\n> We cannot make this perfect (e.g., we cannot allow \".\" for reasons\n> of ambiguity), but I imagine we could cover most practical cases.\n>\n> Note that we would need the matching loosening on the file-parsing\n> side.\n>\n> 3. Start phasing in pager.*.enabled (and I guess pager.*.command). We\n> would still do the lookup of pager.* for backwards compatibility,\n> but we would be careful to do so only when it is syntactically\n> valid. IOW, this looks like (1), except the path forward for\n> \"show_ref\" is to use the new, more robust, syntax.\n\nI guess I ended up reaching the same conclusion; 3. with also\n\"alias.*.command\" as the longer-term goal.\n"},{"id":"255677","messageId":"xmqq386ihk5w.fsf@gitster.dls.corp.google.com","threadId":"38515","inReplyTo":"20150206193313.GA4220@peff.net","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T20:14:35Z","receivedAt":"2015-02-06T20:14:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is highlighting the problem with \"pager.*\" that Junio mentioned\n> recently, which is that the keyname has arbitrary data,...\n\nYes, even if it is not \"arbitrary\" (imagine we limit ourselves to\nthe official set of commands we know about), the naming rule for the\n\"git\" subcommand names should not be dictated by the naming rule for\nthe configuration variables, as they are unrelated.\n\nThat is one of the reasons why I had the \"unbounded set, including\nthe ones under our control such as subcommand names\" in the draft\nupdate for the guideline. I dropped that part after the discussion\nto keep other \"obviously agreed\" parts moving, but we may have to\nrevisit it later.\n"},{"id":"255678","messageId":"20150206203716.GA10857@peff.net","threadId":"38515","inReplyTo":"xmqq386ihk5w.fsf@gitster.dls.corp.google.com","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-06T20:37:16Z","receivedAt":"2015-02-06T20:37:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 06, 2015 at 12:14:35PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This is highlighting the problem with \"pager.*\" that Junio mentioned\n> > recently, which is that the keyname has arbitrary data,...\n> \n> Yes, even if it is not \"arbitrary\" (imagine we limit ourselves to\n> the official set of commands we know about), the naming rule for the\n> \"git\" subcommand names should not be dictated by the naming rule for\n> the configuration variables, as they are unrelated.\n> \n> That is one of the reasons why I had the \"unbounded set, including\n> the ones under our control such as subcommand names\" in the draft\n> update for the guideline. I dropped that part after the discussion\n> to keep other \"obviously agreed\" parts moving, but we may have to\n> revisit it later.\n\nI think this may be the heart of where we were disagreeing. I took\n\"unbounded set\" to mean \"a set where you might keep adding things\nforever\". So fsck errors would count in that. But if you mean it as \"a\nset where the syntax may be unbounded\", then yeah, we definitely would\nnot want it in the key name, as that becomes an unnecessary restriction.\n\nA list of enum-like values where we are OK confining the names to the\nalnums is OK to use as an unbounded set of key values. Just like we have\ncolor.branch.*, we just pick a name within that syntax for any new\nvalues we add (and that is not even a burden; alnum names are what we\nwould have picked anyway).\n\n-Peff\n"},{"id":"255679","messageId":"20150206203902.GB10857@peff.net","threadId":"38515","inReplyTo":"xmqqbnl6hljt.fsf@gitster.dls.corp.google.com","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-06T20:39:03Z","receivedAt":"2015-02-06T20:39:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 06, 2015 at 11:44:38AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:\n> >\n> >> $ git show_ref\n> >> error: invalid key: pager.show_ref\n> >> error: invalid key: alias.show_ref\n> >> git: 'show_ref' is not a git command. See 'git --help'.\n> >> \n> >> Apparently we need to squelch this message from\n> >> within git_config_get_* in this case?\n> > ...\n> > So it is not a new problem, but it is a bug that you\n> > cannot set pager config for such a command or alias.\n> \n> Hmm, I think these are two separate issues.\n\nYeah, sorry, if I wasn't clear. The error messages are definitely a\nseparate and newer issue, and need to be silenced one way or the other.\nIt is just that they are notifying us of a deeper problem that has\nexisted for a long time, and it probably makes sense to deal with both.\n\n-Peff\n"},{"id":"255684","messageId":"xmqqr3u2d6ru.fsf@gitster.dls.corp.google.com","threadId":"38515","inReplyTo":"20150206203716.GA10857@peff.net","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T22:17:25Z","receivedAt":"2015-02-06T22:17:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> That is one of the reasons why I had the \"unbounded set, including\n>> the ones under our control such as subcommand names\" in the draft\n>> update for the guideline. I dropped that part after the discussion\n>> to keep other \"obviously agreed\" parts moving, but we may have to\n>> revisit it later.\n>\n> I think this may be the heart of where we were disagreeing. I took\n> \"unbounded set\" to mean \"a set where you might keep adding things\n> forever\". So fsck errors would count in that. But if you mean it as \"a\n> set where the syntax may be unbounded\", then yeah, we definitely would\n> not want it in the key name, as that becomes an unnecessary restriction.\n\nWhat I mean is \"possible keys are unbounded and its syntax is not\nunder control of the 'config' subsystem\". The syntax does not have\nto be unbounded; as long as it is wrong for the config subsystem to\ndictate what shape the possible values may take, it shouldn't be\nused as the top or the bottom level in the variable namespace where\nit has its own syntax restriction that may or may not match the\nrequirement of the using code of the config subsystem.\n\nThose who name Git subcommands will be limited to sane looking\nsubcommand names that do not have SP in it, for example, but just\nbecause config subsystem does not want to see \"_\" in its keys, it\nshould not force its world view to those who name subcommands.\n\nIf the names are not \"unbounded\", it becomes easier to live with\nsuch a third-party limitation (imposed by config subsystem), but\notherwise, \"we just pick a name within that syntax\" becomes an\nunnecessary and artificial limitation.\n"},{"id":"255685","messageId":"xmqqk2zud6b0.fsf@gitster.dls.corp.google.com","threadId":"38515","inReplyTo":"20150206203716.GA10857@peff.net","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T22:27:31Z","receivedAt":"2015-02-06T22:27:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> A list of enum-like values where we are OK confining the names to the\n> alnums is OK to use as an unbounded set of key values. Just like we have\n> color.branch.*, we just pick a name within that syntax for any new\n> values we add (and that is not even a burden; alnum names are what we\n> would have picked anyway).\n\nI would say that color.branch. names are very different from\nsubcommand names. The latter is exposed to the end users who do not\nhave to know that they can be used and must be usable as config\nkeys.\n\ncolor.branch. names were invented _only_ to be used to\ninteract with the config, and nowhere else. Of course you can just\npick a name within that \"syntax for configuration variables\" and be\nhappy with it, because the users are very aware that they are using\nthat name to name a configuration variable.\n\nThe names of the subcommands are very different in that they are not\njust for accessing configuration variables---if the user does not\nhave pager., the user will not use it as configuration keys\nanywhere in the system.\n"},{"id":"255689","messageId":"CAHYJk3T8e6DgvQmq-y9iNrQroYu1Gd+kYuAMHDyCUgS2ybb=kQ@mail.gmail.com","threadId":"38515","inReplyTo":"xmqqbnl6hljt.fsf@gitster.dls.corp.google.com","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Mikael Magnusson","fromEmail":"mikachu@gmail.com","sentAt":"2015-02-07T00:03:15Z","receivedAt":"2015-02-07T00:03:15Z","isPatch":false,"sender":{"key":"mikachu@gmail.com","avatar":null},"body":"On Fri, Feb 6, 2015 at 8:44 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:\n>>\n>>> $ git show_ref\n>>> error: invalid key: pager.show_ref\n>>> error: invalid key: alias.show_ref\n>>> git: 'show_ref' is not a git command. See 'git --help'.\n>>>\n>>> Apparently we need to squelch this message from\n>>> within git_config_get_* in this case?\n\nI reported this issue a few months ago,\nhttp://permalink.gmane.org/gmane.comp.version-control.git/258886\nSomeone sent a patch that never went anywhere,\nhttp://comments.gmane.org/gmane.comp.version-control.git/258895\n\n-- \nMikael Magnusson\n"},{"id":"255690","messageId":"20150207045219.GA15548@peff.net","threadId":"38515","inReplyTo":"xmqqk2zud6b0.fsf@gitster.dls.corp.google.com","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-07T04:52:20Z","receivedAt":"2015-02-07T04:52:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 06, 2015 at 02:27:31PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > A list of enum-like values where we are OK confining the names to the\n> > alnums is OK to use as an unbounded set of key values. Just like we have\n> > color.branch.*, we just pick a name within that syntax for any new\n> > values we add (and that is not even a burden; alnum names are what we\n> > would have picked anyway).\n> \n> I would say that color.branch. names are very different from\n> subcommand names. The latter is exposed to the end users who do not\n> have to know that they can be used and must be usable as config\n> keys.\n\nYeah, again, sorry if I wasn't clear. That was the same contrast I was\nmaking. Of the examples given in this thread, color.branch. and\nfsck.* names are in one boat (\"OK to give them configuration-friendly\nnames, they are just a list\") and arbitrary commands are in another.\n\n-Peff\n"},{"id":"255691","messageId":"20150207050112.GB15548@peff.net","threadId":"38515","inReplyTo":"CAHYJk3T8e6DgvQmq-y9iNrQroYu1Gd+kYuAMHDyCUgS2ybb=kQ@mail.gmail.com","subject":"Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-07T05:01:12Z","receivedAt":"2015-02-07T05:01:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 07, 2015 at 01:03:15AM +0100, Mikael Magnusson wrote:\n\n> On Fri, Feb 6, 2015 at 8:44 PM, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> >> On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:\n> >>\n> >>> $ git show_ref\n> >>> error: invalid key: pager.show_ref\n> >>> error: invalid key: alias.show_ref\n> >>> git: 'show_ref' is not a git command. See 'git --help'.\n> >>>\n> >>> Apparently we need to squelch this message from\n> >>> within git_config_get_* in this case?\n> \n> I reported this issue a few months ago,\n> http://permalink.gmane.org/gmane.comp.version-control.git/258886\n> Someone sent a patch that never went anywhere,\n> http://comments.gmane.org/gmane.comp.version-control.git/258895\n\nThanks. I had thought this all seemed familiar, and I did find your\nreport in the archive, but not the follow-up patch[1].\n\nIt looks like that patch just squelches the error message. That fixes\nthe immediate error-message regression, but does not fix the larger\nproblem (that you cannot have an alias with an underscore, or set the\npager config for a command with an underscore). But it is at least a\nstart, and unless somebody is excited about taking it further, maybe it\nis enough for now.\n\nThe thread ended with Tanay mentioning that new patches would be\nforthcoming. I've cc'd him, so hopefully that can still happen.\n\n-Peff\n\n[1] This is a good lesson in why it is nice to make sure that the\n in-reply-to headers for patches are set properly; it makes it easier\n later on to find related parts of the discussion. This is something\n I think that git-send-email doesn't make especially easy.\n"},{"id":"255897","messageId":"54DA5FC1.9010707@gmail.com","threadId":"38515","inReplyTo":"20150206203902.GB10857@peff.net","subject":"[PATCH] config: add show_err flag to git_config_parse_key()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2015-02-10T19:45:05Z","receivedAt":"2015-02-10T19:45:05Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"`git_config_parse_key()` is used to sanitize the input key.\nSome callers of the function like `git_config_set_multivar_in_file()`\nget the per-sanitized key directly from the user so it becomes\nnecessary to raise an error specifying what went wrong when the entered\nkey is defective.\n\nOther callers like `configset_find_element()` get their keys from\nthe git itself so a return value signifying error would be enough.\nThe error output shown to the user is useless and confusing in this\ncase so add a show_err flag to suppress errors in such cases.\n\nSigned-off-by: Tanay Abhra \n---\n\nHi,\n\nI just saw your mail late in the night (I didn't had net for a week).\nThis patch just squelches the error message, I will take a better\nlook tomorrow morning.\n\n-Tanay\n\n builtin/config.c | 2 +-\n cache.h | 2 +-\n config.c | 19 ++++++++++++-------\n 3 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 15a7bea..d5070d7 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)\n \t\t\tgoto free_strings;\n \t\t}\n \t} else {\n-\t\tif (git_config_parse_key(key_, &key, NULL)) {\n+\t\tif (git_config_parse_key(key_, &key, NULL, 1)) {\n \t\t\tret = CONFIG_INVALID_KEY;\n \t\t\tgoto free_strings;\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex f704af5..1c0914d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1358,7 +1358,7 @@ extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n-extern int git_config_parse_key(const char *, char **, int *);\n+extern int git_config_parse_key(const char *, char **, int *, int);\n extern int git_config_set_multivar(const char *, const char *, const char *, int);\n extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 752e2e2..074a671 100644\n--- a/config.c\n+++ b/config.c\n@@ -1309,7 +1309,7 @@ static struct config_set_element *configset_find_element(struct config_set *cs,\n \t * `key` may come from the user, so normalize it before using it\n \t * for querying entries from the hashmap.\n \t */\n-\tret = git_config_parse_key(key, &normalized_key, NULL);\n+\tret = git_config_parse_key(key, &normalized_key, NULL, 0);\n\n \tif (ret)\n \t\treturn NULL;\n@@ -1842,8 +1842,9 @@ int git_config_set(const char *key, const char *value)\n * lowercase section and variable name\n * baselen - pointer to int which will hold the length of the\n * section + subsection part, can be NULL\n+ * show_err - toggle whether the function raises an error on a defective key\n */\n-int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n+int git_config_parse_key(const char *key, char **store_key, int *baselen_, int show_err)\n {\n \tint i, dot, baselen;\n \tconst char *last_dot = strrchr(key, '.');\n@@ -1854,12 +1855,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \t */\n\n \tif (last_dot == NULL || last_dot == key) {\n-\t\terror(\"key does not contain a section: %s\", key);\n+\t\tif (show_err)\n+\t\t\terror(\"key does not contain a section: %s\", key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n\n \tif (!last_dot[1]) {\n-\t\terror(\"key does not contain variable name: %s\", key);\n+\t\tif (show_err)\n+\t\t\terror(\"key does not contain variable name: %s\", key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n\n@@ -1881,12 +1884,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \t\tif (!dot || i > baselen) {\n \t\t\tif (!iskeychar(c) ||\n \t\t\t (i == baselen + 1 && !isalpha(c))) {\n-\t\t\t\terror(\"invalid key: %s\", key);\n+\t\t\t\tif (show_err)\n+\t\t\t\t\terror(\"invalid key: %s\", key);\n \t\t\t\tgoto out_free_ret_1;\n \t\t\t}\n \t\t\tc = tolower(c);\n \t\t} else if (c == '\\n') {\n-\t\t\terror(\"invalid key (newline): %s\", key);\n+\t\t\tif (show_err)\n+\t\t\t\terror(\"invalid key (newline): %s\", key);\n \t\t\tgoto out_free_ret_1;\n \t\t}\n \t\t(*store_key)[i] = c;\n@@ -1936,7 +1941,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tchar *filename_buf = NULL;\n\n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n-\tret = 0 - git_config_parse_key(key, &store.key, &store.baselen);\n+\tret = 0 - git_config_parse_key(key, &store.key, &store.baselen, 1);\n \tif (ret)\n \t\tgoto out_free;\n\n-- \n1.9.0.GIT\n"},{"id":"255917","messageId":"20150211002754.GC30561@peff.net","threadId":"38515","inReplyTo":"54DA5FC1.9010707@gmail.com","subject":"Re: [PATCH] config: add show_err flag to git_config_parse_key()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-11T00:27:54Z","receivedAt":"2015-02-11T00:27:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 11, 2015 at 01:15:05AM +0530, Tanay Abhra wrote:\n\n> I just saw your mail late in the night (I didn't had net for a week).\n> This patch just squelches the error message, I will take a better\n> look tomorrow morning.\n\nThanks, this is probably a good first step. We can worry about making\nthe config look actually _work_ as the next step (which does not even\nhave to happen right now; it is not like it hasn't been this way since\nthe very beginning of git).\n\nAnother option for this first step would be to actually make\ngit_config_parse_key permissive, rather than just squelching the error.\nThat is, to actually look up pager.under_score rather than silently\nerroring out with an invalid key whenever we are reading (whereas on the\nwriting side, we _do_ want to make sure we enforce syntactic validity).\nI doubt it matters, much, though. Such a lookup would never succeed,\nbecause the config file parser will also not allow it. So assuming the\nsyntactic rules here match what the config file parser does, they are at\nworst redundant.\n\n> builtin/config.c | 2 +-\n> cache.h | 2 +-\n> config.c | 19 ++++++++++++-------\n> 3 files changed, 14 insertions(+), 9 deletions(-)\n\nHere's a test that can be squashed in:\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex da958a8..a28a2fd 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -447,4 +447,14 @@ test_expect_success TTY 'external command pagers override sub-commands' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'command with underscores does not complain' '\n+\twrite_script git-under_score <<-\\EOF &&\n+\techo ok\n+\tEOF\n+\tgit --exec-path=. under_score >actual 2>&1 &&\n+\techo ok >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+\n test_done\n\nI was tempted to also add something like:\n\n test_expect_failure TTY 'command with underscores can override pager' '\n\ttest_config pager.under_score \"sed s/^/paged://\" &&\n\tgit --exec-path=. under_score >actual &&\n\techo paged:ok >expect &&\n\ttest_cmp expect actual\n '\n\nbut I am not sure it is worth adding the test, even as a placeholder.\nUnless we are planning to relax the config syntax, the correct spelling\nis more like \"pager.under_score.command\". It's probably better to just\nadd the test along with the code when we know what the final form will\nlook like.\n\n-Peff\n"},{"id":"255939","messageId":"xmqq386cuvxl.fsf@gitster.dls.corp.google.com","threadId":"38515","inReplyTo":"20150211002754.GC30561@peff.net","subject":"Re: [PATCH] config: add show_err flag to git_config_parse_key()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-11T18:47:50Z","receivedAt":"2015-02-11T18:47:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Feb 11, 2015 at 01:15:05AM +0530, Tanay Abhra wrote:\n>\n>> I just saw your mail late in the night (I didn't had net for a week).\n>> This patch just squelches the error message, I will take a better\n>> look tomorrow morning.\n>\n> Thanks, this is probably a good first step. We can worry about making\n> the config look actually _work_ as the next step (which does not even\n> have to happen right now; it is not like it hasn't been this way since\n> the very beginning of git).\n\nI agree this is probably a good first step in the right direction.\nAs to the implementation, there are a few minor things I would\nchange, but they are both minor:\n\n - \"defective\" may want to be a bit more descriptive to clarify what\n kind fo defect is undesired. In the context of this patch, I\n think Tanay meant (syntactically) \"malformed\", perhaps?\n\n - \"int show_err\" should be \"unsigned flags\" with its bit 01 defined\n to be used as QUIET bit.\n\n> Another option for this first step would be to actually make\n> git_config_parse_key permissive, rather than just squelching the\n> error. That is, to actually look up pager.under_score rather than\n> silently erroring out with an invalid key whenever we are reading\n> (whereas on the writing side, we _do_ want to make sure we enforce\n> syntactic validity). I doubt it matters, much, though.\n\nSensible.\n\n> I was tempted to also add something like:\n>\n> test_expect_failure TTY 'command with underscores can override pager' '\n> \ttest_config pager.under_score \"sed s/^/paged://\" &&\n> \tgit --exec-path=. under_score >actual &&\n> \techo paged:ok >expect &&\n> \ttest_cmp expect actual\n> '\n>\n> but I am not sure it is worth adding the test, even as a placeholder.\n> Unless we are planning to relax the config syntax, the correct spelling\n> is more like \"pager.under_score.command\". It's probably better to just\n> add the test along with the code when we know what the final form will\n> look like.\n\nConcurred.\n"},{"id":"256139","messageId":"54E1A30F.5010303@gmail.com","threadId":"38515","inReplyTo":"xmqq386cuvxl.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] add a flag to supress errors in git_config_parse_key()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2015-02-16T07:58:07Z","receivedAt":"2015-02-16T07:58:07Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n`git_config_parse_key()` is used to sanitize the input key.\nSome callers of the function like `git_config_set_multivar_in_file()`\nget the pre-sanitized key directly from the user so it becomes\nnecessary to raise an error specifying what went wrong when the entered\nkey is syntactically malformed.\n\nOther callers like `configset_find_element()` get their keys from\nthe git itself so a return value signifying error would be enough.\nThe error output shown to the user is useless and confusing in this\ncase so add a flag to suppress errors in such cases.\n\nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Tanay Abhra \n---\nHi Jeff,\n\nI went through Junio's config guideline patch series\nand the whole thread of underscore bug report and I also think\nthat pager.*.command is the right path to go.\n\nIf you want to relax the syntactic requirement (such as add '_' to\nthe current set of allowed chacters), I can work upon it but most of the\ncomments point that moving towards pager.*.command would be better.\n\np.s: I hope that I got the unsigned flag suggestion by Junio correctly.\n\n-Tanay\n\n builtin/config.c | 2 +-\n cache.h | 4 +++-\n config.c | 20 +++++++++++++-------\n t/t7006-pager.sh | 9 +++++++++\n 4 files changed, 26 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d32c532..326d3d3 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)\n \t\t\tgoto free_strings;\n \t\t}\n \t} else {\n-\t\tif (git_config_parse_key(key_, &key, NULL)) {\n+\t\tif (git_config_parse_key(key_, &key, NULL, 0)) {\n \t\t\tret = CONFIG_INVALID_KEY;\n \t\t\tgoto free_strings;\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex f704af5..9073ee2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1329,6 +1329,8 @@ extern int update_server_info(int);\n\n #define CONFIG_REGEX_NONE ((void *)1)\n\n+#define CONFIG_ERROR_QUIET 0x0001\n+\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\n@@ -1358,7 +1360,7 @@ extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n-extern int git_config_parse_key(const char *, char **, int *);\n+extern int git_config_parse_key(const char *, char **, int *, unsigned int);\n extern int git_config_set_multivar(const char *, const char *, const char *, int);\n extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex e5e64dc..7e23bb9 100644\n--- a/config.c\n+++ b/config.c\n@@ -1309,7 +1309,7 @@ static struct config_set_element *configset_find_element(struct config_set *cs,\n \t * `key` may come from the user, so normalize it before using it\n \t * for querying entries from the hashmap.\n \t */\n-\tret = git_config_parse_key(key, &normalized_key, NULL);\n+\tret = git_config_parse_key(key, &normalized_key, NULL, CONFIG_ERROR_QUIET);\n\n \tif (ret)\n \t\treturn NULL;\n@@ -1842,8 +1842,10 @@ int git_config_set(const char *key, const char *value)\n * lowercase section and variable name\n * baselen - pointer to int which will hold the length of the\n * section + subsection part, can be NULL\n+ * flags - toggle whether the function raises an error on a syntactically\n+ * malformed key\n */\n-int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n+int git_config_parse_key(const char *key, char **store_key, int *baselen_, unsigned int flags)\n {\n \tint i, dot, baselen;\n \tconst char *last_dot = strrchr(key, '.');\n@@ -1854,12 +1856,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \t */\n\n \tif (last_dot == NULL || last_dot == key) {\n-\t\terror(\"key does not contain a section: %s\", key);\n+\t\tif (!flags)\n+\t\t\terror(\"key does not contain a section: %s\", key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n\n \tif (!last_dot[1]) {\n-\t\terror(\"key does not contain variable name: %s\", key);\n+\t\tif (!flags)\n+\t\t\terror(\"key does not contain variable name: %s\", key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n\n@@ -1881,12 +1885,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \t\tif (!dot || i > baselen) {\n \t\t\tif (!iskeychar(c) ||\n \t\t\t (i == baselen + 1 && !isalpha(c))) {\n-\t\t\t\terror(\"invalid key: %s\", key);\n+\t\t\t\tif (!flags)\n+\t\t\t\t\terror(\"invalid key: %s\", key);\n \t\t\t\tgoto out_free_ret_1;\n \t\t\t}\n \t\t\tc = tolower(c);\n \t\t} else if (c == '\\n') {\n-\t\t\terror(\"invalid key (newline): %s\", key);\n+\t\t\tif (!flags)\n+\t\t\t\terror(\"invalid key (newline): %s\", key);\n \t\t\tgoto out_free_ret_1;\n \t\t}\n \t\t(*store_key)[i] = c;\n@@ -1936,7 +1942,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tchar *filename_buf = NULL;\n\n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n-\tret = 0 - git_config_parse_key(key, &store.key, &store.baselen);\n+\tret = 0 - git_config_parse_key(key, &store.key, &store.baselen, 0);\n \tif (ret)\n \t\tgoto out_free;\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex da958a8..2dd71c0 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -447,4 +447,13 @@ test_expect_success TTY 'external command pagers override sub-commands' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'command with underscores does not complain' '\n+\twrite_script git-under_score <<-\\EOF &&\n+\techo ok\n+\tEOF\n+\tgit --exec-path=. under_score >actual 2>&1 &&\n+\techo ok >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"256296","messageId":"20150218190215.GD7257@peff.net","threadId":"38515","inReplyTo":"54E1A30F.5010303@gmail.com","subject":"Re: [PATCH v2] add a flag to supress errors in git_config_parse_key()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-18T19:02:15Z","receivedAt":"2015-02-18T19:02:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 16, 2015 at 01:28:07PM +0530, Tanay Abhra wrote:\n\n> I went through Junio's config guideline patch series\n> and the whole thread of underscore bug report and I also think\n> that pager.*.command is the right path to go.\n> \n> If you want to relax the syntactic requirement (such as add '_' to\n> the current set of allowed chacters), I can work upon it but most of the\n> comments point that moving towards pager.*.command would be better.\n\nNo, as silly as I find the \"_\" restriction, it is not worth doing. One,\nit would not cover all cases (it is one common case, so it makes the\nproblem more rare but does not eliminate it). And two, there are other\nparsers of git's config format. Technically we do not need to care about\nthem and they can follow our lead, but we do not need to make things\nharder on them than is necessary.\n\n> \tif (last_dot == NULL || last_dot == key) {\n> -\t\terror(\"key does not contain a section: %s\", key);\n> +\t\tif (!flags)\n> +\t\t\terror(\"key does not contain a section: %s\", key);\n\nThe intent of the flag variable is that you would check:\n\n if (!(flags & CONFIG_ERROR_QUIET))\n\nhere. I know that there are no other flags yet, so the two are\nequivalent. But when somebody adds a new flag later, you would not want\nthem to have to tweak each of these sites.\n\n-Peff\n"}]}