{"thread":{"id":"19024","subject":"[PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default","startedAt":"2009-04-23T13:49:05Z","lastAt":"2009-04-24T16:19:12Z","messageCount":6,"participants":["Johannes Sixt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"112088","messageId":"1240494546-25775-1-git-send-email-j6t@kdbg.org","threadId":"19024","inReplyTo":null,"subject":"[PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-23T13:49:05Z","receivedAt":"2009-04-23T13:49:05Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"If the config file contains a section like this:\n\n    [remote]\n            default = foo\n\n(it should be '[remotes]') then commands like\n\n    git status\n    git checkout\n    git branch -v\n\nfail even though they are not obviously related to remotes. (These commands\nwrite \"ahead, behind\" information and, therefore, access the configuration\nof remotes.)\n\nTypos in configuration keys usually do not hurt because they never match\nin look-ups. But this case is different: it does match, but it does not\nhave the expected format. With this patch this situation is treated more\nlike a typo.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n remote.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 91f7485..d66e2f3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -366,7 +366,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t}\n \tsubkey = strrchr(name, '.');\n \tif (!subkey)\n-\t\treturn error(\"Config with no key for remote %s\", name);\n+\t\treturn 0;\n \tremote = make_remote(name, subkey - name);\n \tremote->origin = REMOTE_CONFIG;\n \tif (!strcmp(subkey, \".mirror\"))\n-- \n1.6.3.rc1.88.g1bf9\n"},{"id":"112089","messageId":"1240494546-25775-2-git-send-email-j6t@kdbg.org","threadId":"19024","inReplyTo":"1240494546-25775-1-git-send-email-j6t@kdbg.org","subject":"[PATCH 2/2] builtin-help: silently tolerate unknown keys","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-23T13:49:06Z","receivedAt":"2009-04-23T13:49:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"If for some reason the config file contains a key without a subkey like\n\n    [man]\n        foo = bar\n\nthen even a plain\n\n    git help\n\nproduces an error message. With this patch such an entry is ignored.\n\nAdditionally, the warning about unknown sub-keys is removed. It could\nbecome annoying if new sub-keys are introduced in the future, and then\nthe configuration is read by an old version of git that does not know\nabout it.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n builtin-help.c |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-help.c b/builtin-help.c\nindex 9b57a74..e7fbe9a 100644\n--- a/builtin-help.c\n+++ b/builtin-help.c\n@@ -236,7 +236,7 @@ static int add_man_viewer_info(const char *var, const char *value)\n \tconst char *subkey = strrchr(name, '.');\n \n \tif (!subkey)\n-\t\treturn error(\"Config with no key for man viewer: %s\", name);\n+\t\treturn 0;\n \n \tif (!strcmp(subkey, \".path\")) {\n \t\tif (!value)\n@@ -249,7 +249,6 @@ static int add_man_viewer_info(const char *var, const char *value)\n \t\treturn add_man_viewer_cmd(name, subkey - name, value);\n \t}\n \n-\twarning(\"'%s': unsupported man viewer sub key.\", subkey);\n \treturn 0;\n }\n \n-- \n1.6.3.rc1.88.g1bf9\n"},{"id":"112096","messageId":"20090423164008.GA26346@coredump.intra.peff.net","threadId":"19024","inReplyTo":"1240494546-25775-1-git-send-email-j6t@kdbg.org","subject":"Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-23T16:40:09Z","receivedAt":"2009-04-23T16:40:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 23, 2009 at 03:49:05PM +0200, Johannes Sixt wrote:\n\n> Typos in configuration keys usually do not hurt because they never match\n> in look-ups. But this case is different: it does match, but it does not\n> have the expected format. With this patch this situation is treated more\n> like a typo.\n\nI definitely think causing the command to fail is bad, but should we\nperhaps still warn the user? I know that we can't catch _every_ typo in\nthe config, but if there is something obviously wrong that we've\ndetected, it is nice to let the user know.\n\n> -\t\treturn error(\"Config with no key for remote %s\", name);\n> +\t\treturn 0;\n\nIOW,\n\n  + warn(\"Config with no key for remote %s\", name);\n  + return 0;\n\n-Peff\n"},{"id":"112108","messageId":"200904232037.27095.j6t@kdbg.org","threadId":"19024","inReplyTo":"20090423164008.GA26346@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-23T18:37:26Z","receivedAt":"2009-04-23T18:37:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 23. April 2009, Jeff King wrote:\n> On Thu, Apr 23, 2009 at 03:49:05PM +0200, Johannes Sixt wrote:\n> > Typos in configuration keys usually do not hurt because they never match\n> > in look-ups. But this case is different: it does match, but it does not\n> > have the expected format. With this patch this situation is treated more\n> > like a typo.\n>\n> I definitely think causing the command to fail is bad, but should we\n> perhaps still warn the user? I know that we can't catch _every_ typo in\n> the config, but if there is something obviously wrong that we've\n> detected, it is nice to let the user know.\n>\n> > -\t\treturn error(\"Config with no key for remote %s\", name);\n> > +\t\treturn 0;\n>\n> IOW,\n>\n>   + warn(\"Config with no key for remote %s\", name);\n>   + return 0;\n\nI don't like this. This would warn in a number of situations where it's not \nobvious that remotes are involved, for example in 'git status'.\n\nAlso observe (and I forgot to mention that in the commit message) that this \nsame exit\n\n   if (!subkey)\n        return 0;\n\nis also taken for url.foo and branch.foo configurations a few lines above this \nchange.\n\n-- Hannes\n"},{"id":"112124","messageId":"20090423200136.GA3056@coredump.intra.peff.net","threadId":"19024","inReplyTo":"200904232037.27095.j6t@kdbg.org","subject":"Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-23T20:01:37Z","receivedAt":"2009-04-23T20:01:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 23, 2009 at 08:37:26PM +0200, Johannes Sixt wrote:\n\n> >   + warn(\"Config with no key for remote %s\", name);\n> >   + return 0;\n> \n> I don't like this. This would warn in a number of situations where it's not \n> obvious that remotes are involved, for example in 'git status'.\n\nAh, I didn't think of that (I didn't really look too closely into the\naffected code, and you obviously have). So let me withdraw my comment.\n\n-Peff\n"},{"id":"112211","messageId":"7vzle6dny7.fsf@gitster.siamese.dyndns.org","threadId":"19024","inReplyTo":"1240494546-25775-1-git-send-email-j6t@kdbg.org","subject":"Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-24T16:19:12Z","receivedAt":"2009-04-24T16:19:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> If the config file contains a section like this:\n>\n>     [remote]\n>             default = foo\n>\n> (it should be '[remotes]') then commands like\n>\n>     git status\n>     git checkout\n>     git branch -v\n>\n> fail even though they are not obviously related to remotes. (These commands\n> write \"ahead, behind\" information and, therefore, access the configuration\n> of remotes.)\n>\n> Typos in configuration keys usually do not hurt because they never match\n> in look-ups. But this case is different: it does match, but it does not\n> have the expected format. With this patch this situation is treated more\n> like a typo.\n\nI agree with the patch text, but we should realize that this is not\n\"silently tolerating typos\".  The existing per-remote variables such as\nurl, proxy, fetch, etc. are defined to be, eh, per-remote, and by\ndefinition require three-level syntax.  There is nothing that prevents us\nfrom actually fixing the stupid mistake that is \"remotes.default = foo\"\nand make it \"remote.default = foo\".\n\nTo avoid confusion, we might want to deprecate remotes.default and make it\nremotegroup.default or something like that, though.\n"}]}