{"thread":{"id":"22575","subject":"[PATCH] Generate a warning message if we find an unrecognized option.","startedAt":"2010-02-08T22:33:35Z","lastAt":"2010-02-09T05:59:38Z","messageCount":6,"participants":["Jeremy White","Jeff King","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133963","messageId":"4B70913F.7060809@winehq.org","threadId":"22575","inReplyTo":null,"subject":"[PATCH] Generate a warning message if we find an unrecognized option.","fromName":"Jeremy White","fromEmail":"jwhite@winehq.org","sentAt":"2010-02-08T22:33:35Z","receivedAt":"2010-02-08T22:33:35Z","isPatch":true,"sender":{"key":"jwhite@winehq.org","avatar":null},"body":"---\n imap-send.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 51f371b..885da22 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1360,6 +1360,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)\n \t\tserver.ssl_verify = git_config_bool(key, val);\n \telse if (!strcmp(\"preformattedHTML\", key))\n \t\tserver.use_html = git_config_bool(key, val);\n+        else imap_info(\"Unknown imap configuration option '%s'\\n\", key);\n \treturn 0;\n }\n \n-- \n1.7.0.rc2.1.gd78df.dirty\n"},{"id":"133975","messageId":"20100209004514.GB4065@coredump.intra.peff.net","threadId":"22575","inReplyTo":"4B70913F.7060809@winehq.org","subject":"Re: [PATCH] Generate a warning message if we find an unrecognized option.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-09T00:45:14Z","receivedAt":"2010-02-09T00:45:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 04:33:35PM -0600, Jeremy White wrote:\n\n> diff --git a/imap-send.c b/imap-send.c\n> index 51f371b..885da22 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1360,6 +1360,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)\n>  \t\tserver.ssl_verify = git_config_bool(key, val);\n>  \telse if (!strcmp(\"preformattedHTML\", key))\n>  \t\tserver.use_html = git_config_bool(key, val);\n> +        else imap_info(\"Unknown imap configuration option '%s'\\n\", key);\n>  \treturn 0;\n>  }\n\nSlight NAK from me on this. When we later add new options, it makes\nusing the same config for multiple versions of git difficult (the old\nversions will complain about the unknown option).\n\nAnd obviously that is weighed against the ability to notice things like\ntypos. But if we are going to start complaining about unknown config, we\nwould probably do better to complain about _all_ unknown config, and not\njust this one subsection.\n\n-Peff\n"},{"id":"133979","messageId":"7vvde7z0kf.fsf@alter.siamese.dyndns.org","threadId":"22575","inReplyTo":"20100209004514.GB4065@coredump.intra.peff.net","subject":"Re: [PATCH] Generate a warning message if we find an unrecognized option.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T00:59:12Z","receivedAt":"2010-02-09T00:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Feb 08, 2010 at 04:33:35PM -0600, Jeremy White wrote:\n>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 51f371b..885da22 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -1360,6 +1360,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)\n>>  \t\tserver.ssl_verify = git_config_bool(key, val);\n>>  \telse if (!strcmp(\"preformattedHTML\", key))\n>>  \t\tserver.use_html = git_config_bool(key, val);\n>> +        else imap_info(\"Unknown imap configuration option '%s'\\n\", key);\n>>  \treturn 0;\n>>  }\n>\n> Slight NAK from me on this. When we later add new options, it makes\n> using the same config for multiple versions of git difficult (the old\n> versions will complain about the unknown option).\n>\n> And obviously that is weighed against the ability to notice things like\n> typos. But if we are going to start complaining about unknown config, we\n> would probably do better to complain about _all_ unknown config, and not\n> just this one subsection.\n\nWe would probably want something like:\n\n\tstatic int do_warn_unknown_config;\n\n\tvoid warn_unknown_config(const char *key)\n        {\n\t\tif (do_warn_unknown_config)\n                \twarn(\"Unknown configuration variable %s\", key);\n        }\n\nand sprinkle that everywhere.\n\nAn interesting issue is where to flip do_warn_unknown_config.  A naïve\nand obvious implementation would do:\n\n        static int git_default_core_config(const char *var, const char *value)\n        {\n\t\t...\n\t\tif (!strcmp(var, \"core.warnunknownconfig\")) {\n\t\t\tdo_warn_unknown_config = git_config_bool(var, value);\n\t\t\treturn 0;\n\t\t}\n\t\t...\n\t}\n\nbut that means the definition of this variable has to come very early in\nthe configuration file to be effective.\n"},{"id":"133983","messageId":"20100209030151.GA5370@coredump.intra.peff.net","threadId":"22575","inReplyTo":"7vvde7z0kf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Generate a warning message if we find an unrecognized option.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-09T03:01:51Z","receivedAt":"2010-02-09T03:01:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 04:59:12PM -0800, Junio C Hamano wrote:\n\n> > And obviously that is weighed against the ability to notice things like\n> > typos. But if we are going to start complaining about unknown config, we\n> > would probably do better to complain about _all_ unknown config, and not\n> > just this one subsection.\n> \n> We would probably want something like:\n> \n> \tstatic int do_warn_unknown_config;\n> \n> \tvoid warn_unknown_config(const char *key)\n>         {\n> \t\tif (do_warn_unknown_config)\n>                 \twarn(\"Unknown configuration variable %s\", key);\n>         }\n> \n> and sprinkle that everywhere.\n\nYour \"sprinkle that everywhere\" is a little harder than one might hope.\nThere is no code path that reads and claims to recognize _all_ of the\ngit config. In many cases, the git_*_config for individual subsystems\ncan reasonably lay claim to the whole of a \"[heading]\" section. But\nthere are exceptions even to that.\n\nDiff config is split across git_diff_basic_config and\ngit_diff_ui_config. You would certainly not want to warn about\ndiff.color.* just because you are running a plumbing command which\nhappens not to recognize those entries.\n\nAnd some headings have entries for several subsystems. remote.*.fetch is\nused many places for calculating upstream branches. But only git-remote\nlooks at remote.*.skipDefaultUpdate.\n\nSo in practice I think you will get quite spotty coverage. Which isn't\nto say it isn't necessarily worth doing, but I am personally not very\nexcited about working on it. I do like the suggestion of making it\noptional, so that people who don't care about having a portable config\ncan have the benefit of sanity-checking their config.\n\n> An interesting issue is where to flip do_warn_unknown_config.  A naïve\n> and obvious implementation would do:\n> \n>         static int git_default_core_config(const char *var, const char *value)\n>         {\n> \t\t...\n> \t\tif (!strcmp(var, \"core.warnunknownconfig\")) {\n> \t\t\tdo_warn_unknown_config = git_config_bool(var, value);\n> \t\t\treturn 0;\n> \t\t}\n> \t\t...\n> \t}\n> \n> but that means the definition of this variable has to come very early in\n> the configuration file to be effective.\n\nI would rather have a \"git config --lint\" command, but that is even\nharder, since we are not even loading most of the subsystems which know\nabout the valid config options. And it presupposes that people will\nbother to actually run such a lint command.\n\nYou could always just hoist into an environment variable which neatly\ngets rid of the ordering problem.\n\n-Peff\n"},{"id":"133996","messageId":"20100209051730.GA28599@gmail.com","threadId":"22575","inReplyTo":"20100209030151.GA5370@coredump.intra.peff.net","subject":"Re: [PATCH] Generate a warning message if we find an unrecognized option.","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-02-09T05:17:31Z","receivedAt":"2010-02-09T05:17:31Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Feb 08, 2010 at 10:01:51PM -0500, Jeff King wrote:\n> On Mon, Feb 08, 2010 at 04:59:12PM -0800, Junio C Hamano wrote:\n> \n> > > And obviously that is weighed against the ability to notice things like\n> > > typos. But if we are going to start complaining about unknown config, we\n> > > would probably do better to complain about _all_ unknown config, and not\n> > > just this one subsection.\n\nNo, please, please no.\n\n\n> So in practice I think you will get quite spotty coverage. Which isn't\n> to say it isn't necessarily worth doing, but I am personally not very\n> excited about working on it. I do like the suggestion of making it\n> optional, so that people who don't care about having a portable config\n> can have the benefit of sanity-checking their config.\n> \n> I would rather have a \"git config --lint\" command, but that is even\n> harder, since we are not even loading most of the subsystems which know\n> about the valid config options. And it presupposes that people will\n> bother to actually run such a lint command.\n\nThis runs up against the same issue you pointed out\nearlier--that older versions of git cannot adequately lint\nconfigs from newer versions.\n\nThere are also config variables from unknown git scripts outside\nof git.git that happen to use the git-config mechanism because\nit is convenient.  It would be unfortunate to punish those who\nchose to make up their own config variables by warning them\nthat git doesn't know about them.\n\nI have to wonder if this is a non-existent problem.\n\nConfig variables are one-shot things.  You set them and forget\nabout them.  When you set it you are usually pretty well aware\nof whether it's typoed because it simply does't work.\ncolor.ui is a perfect example.  If it's typoed, you don't need\n'git config --lint' to tell you, you already know by virtue of\nusing the thing.\n\nMaintaining 'git config --lint' would also be a PITA since we'd\nthen have to remember to update yet another place in addition to\ncode and documentation.  And who's to say what gets to be \"in\"\nand what doesn't?  git-gui, for example, has its own [gui]\nnamespace.  git-p4 has its own [git-p4] namespace, but it\nlives in contrib/, etc. etc.\n\nThis seems like a bad idea to me.\n\n-- \n\t\tDavid\n"},{"id":"134004","messageId":"20100209055938.GA14736@coredump.intra.peff.net","threadId":"22575","inReplyTo":"20100209051730.GA28599@gmail.com","subject":"Re: [PATCH] Generate a warning message if we find an unrecognized option.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-09T05:59:38Z","receivedAt":"2010-02-09T05:59:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 09:17:31PM -0800, David Aguilar wrote:\n\n> > > > And obviously that is weighed against the ability to notice things like\n> > > > typos. But if we are going to start complaining about unknown config, we\n> > > > would probably do better to complain about _all_ unknown config, and not\n> > > > just this one subsection.\n> \n> No, please, please no.\n\nI would only be OK with it if it were optional.\n\n> > I would rather have a \"git config --lint\" command, but that is even\n> > harder, since we are not even loading most of the subsystems which know\n> > about the valid config options. And it presupposes that people will\n> > bother to actually run such a lint command.\n> \n> This runs up against the same issue you pointed out\n> earlier--that older versions of git cannot adequately lint\n> configs from newer versions.\n\nYeah, but that isn't a big deal. You just don't run \"config --lint\" with\nthe older version. But if, for example, \"git diff\" breaks because your\nconfig is too new, then that is a real pain (and that was what happened\nwith color.diff.func recently).\n\n> There are also config variables from unknown git scripts outside\n> of git.git that happen to use the git-config mechanism because\n> it is convenient.  It would be unfortunate to punish those who\n> chose to make up their own config variables by warning them\n> that git doesn't know about them.\n\nYes, I don't think anyone is proposing to lint _all_ variables. But it\ndoes not seem unreasonable for certain subsystems to claim portions of\nthe namespace. I would expect git-core to own \"core.*\". And I would\nexpect git-gui to own \"git-gui\", etc.\n\n> I have to wonder if this is a non-existent problem.\n> \n> Config variables are one-shot things.  You set them and forget\n> about them.  When you set it you are usually pretty well aware\n> of whether it's typoed because it simply does't work.\n> color.ui is a perfect example.  If it's typoed, you don't need\n> 'git config --lint' to tell you, you already know by virtue of\n> using the thing.\n\nI think I agree with you on this. It is _much_ more annoying to me not\nto have version portability than it is not to have strict config\nchecking. I was mainly trying to put myself in the shoes of \"regular\"\nusers, who are less likely to be running the same config file on many\ndifferent versions, and are more likely to be clueless about config. But\nI may have overcompensated.\n\n-Peff\n"}]}