{"thread":{"id":"12013","subject":"Alternative approach to the git config NULL value checking patches..","startedAt":"2008-02-10T20:32:39Z","lastAt":"2008-02-11T17:27:30Z","messageCount":13,"participants":["Linus Torvalds","Junio C Hamano","Pierre Habouzit","Christian Couder","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"68259","messageId":"alpine.LFD.1.00.0802101225110.2896@woody.linux-foundation.org","threadId":"12013","inReplyTo":null,"subject":"Alternative approach to the git config NULL value checking patches..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-10T20:32:39Z","receivedAt":"2008-02-10T20:32:39Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nSo I realize that people have worked on checking for NULL, but I'm kind of \nof the opinion that we could just decide to say that \"value\" just should \nnever be NULL at all, and marking the special case \"true\" value \ndifferently.\n\nNamely by just using a _special_ empty string that can be distinguished \nfrom any regular user-generated empty strings.\n\nThis fairly simple patch just uses a special static \"config_true\" array \nvariable to specify that special case that we used to use NULL for, and \nthen replaces the few explicit places where we knew that NULL was special \nin the config handling with testing whether the value is that special \narray.\n\nIt may be a bit odd, but it automatically means that codepaths that simply \ndon't want to care about the subtle difference between \"true\" and \"empty\" \nwill just automatically work, because to them the two cases will look \nidentical, because the strings will compare the same - they have the same \ndata, just different addresses.\n\nThis also means that code (notably the value regexp parsing) that does \nsomething like\n\n\tvalue ? value : \"\"\n\njust automatically can go away, and just use 'value' directly.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nHmm?\n\n builtin-config.c |   10 +++++-----\n cache.h          |    1 +\n config.c         |    7 ++++---\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex e4a12e3..a58f2a1 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -20,7 +20,7 @@ static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;\n \n static int show_all_config(const char *key_, const char *value_)\n {\n-\tif (value_)\n+\tif (value_ != config_true)\n \t\tprintf(\"%s%c%s%c\", key_, delim, value_, term);\n \telse\n \t\tprintf(\"%s%c\", key_, term);\n@@ -38,11 +38,11 @@ static int show_config(const char* key_, const char* value_)\n \tif (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))\n \t\treturn 0;\n \tif (regexp != NULL &&\n-\t    (do_not_match ^ !!regexec(regexp, (value_?value_:\"\"), 0, NULL, 0)))\n+\t    (do_not_match ^ !!regexec(regexp, value_, 0, NULL, 0)))\n \t\treturn 0;\n \n \tif (show_keys) {\n-\t\tif (value_)\n+\t\tif (value_ != config_true)\n \t\t\tprintf(\"%s%c\", key_, key_delim);\n \t\telse\n \t\t\tprintf(\"%s\", key_);\n@@ -50,11 +50,11 @@ static int show_config(const char* key_, const char* value_)\n \tif (seen && !do_all)\n \t\tdup_error = 1;\n \tif (type == T_INT)\n-\t\tsprintf(value, \"%d\", git_config_int(key_, value_?value_:\"\"));\n+\t\tsprintf(value, \"%d\", git_config_int(key_, value_));\n \telse if (type == T_BOOL)\n \t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n \telse\n-\t\tvptr = value_?value_:\"\";\n+\t\tvptr = value_;\n \tseen++;\n \tif (dup_error) {\n \t\terror(\"More than one value for the key %s: %s\",\ndiff --git a/cache.h b/cache.h\nindex 549f4bb..ffe99fc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -589,6 +589,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int\n extern int git_config_rename_section(const char *, const char *);\n extern const char *git_etc_gitconfig(void);\n extern int check_repository_format_version(const char *var, const char *value);\n+extern const char config_true[];\n \n #define MAX_GITNAME (1000)\n extern char git_default_email[MAX_GITNAME];\ndiff --git a/config.c b/config.c\nindex 498259e..ba381d3 100644\n--- a/config.c\n+++ b/config.c\n@@ -10,6 +10,7 @@\n \n #define MAXNAME (256)\n \n+const char config_true[1] = \"\";\n static FILE *config_file;\n static const char *config_file_name;\n static int config_linenr;\n@@ -114,7 +115,7 @@ static inline int iskeychar(int c)\n static int get_value(config_fn_t fn, char *name, unsigned int len)\n {\n \tint c;\n-\tchar *value;\n+\tconst char *value;\n \n \t/* Get the full name */\n \tfor (;;) {\n@@ -131,7 +132,7 @@ static int get_value(config_fn_t fn, char *name, unsigned int len)\n \twhile (c == ' ' || c == '\\t')\n \t\tc = get_next_char();\n \n-\tvalue = NULL;\n+\tvalue = config_true;\n \tif (c != '\\n') {\n \t\tif (c != '=')\n \t\t\treturn -1;\n@@ -298,7 +299,7 @@ unsigned long git_config_ulong(const char *name, const char *value)\n \n int git_config_bool(const char *name, const char *value)\n {\n-\tif (!value)\n+\tif (value == config_true)\n \t\treturn 1;\n \tif (!*value)\n \t\treturn 0;\n"},{"id":"68275","messageId":"7vir0wfqrz.fsf@gitster.siamese.dyndns.org","threadId":"12013","inReplyTo":"alpine.LFD.1.00.0802101225110.2896@woody.linux-foundation.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-10T21:25:04Z","receivedAt":"2008-02-10T21:25:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> It may be a bit odd, but it automatically means that codepaths that simply \n> don't want to care about the subtle difference between \"true\" and \"empty\" \n> will just automatically work, because to them the two cases will look \n> identical, because the strings will compare the same - they have the same \n> data, just different addresses.\n\nI should have mentioned the reason why I did not suggest doing\nit this way in my [Janitor] message.\n\nIt is not \"suttle difference between true and empty\".  Empty\nmeans false, and with this approach, it switches the meaning of\nvalueless form of config to quite the opposite.\n\nIn addition to fixing existing breakages, a piece of code that\nknew NULL is true and empty is false and coded accordingly, i.e.\n\n\tif (!value)\n        \tAh we have true;\n\telse if (!*value)\n        \tOk this is false;\n\telse if (!strcmp(value, \"something special\")\n        \tOk, this is not bool but special;\n\telse\n        \treturn git_config_bool(var, value);\n\nwill now need to be changed to:\n\n\tif (value == config_true)\n        \tAh we have true;\n\telse if (!*value)\n        \tOk this is false;\n\telse if (!strcmp(value, \"something special\")\n        \tOk, this is not bool but special;\n\telse\n        \treturn git_config_bool(var, value);\n\nif you do this.  And the code that was already broken:\n\n\tif (!strcmp(value, \"somevalue\")\n\t\tOk let's use somevalue;\n\telse if (!strcmp(value, \"someothervalue\")\n\t\tOk let's use someothervalue;\n\telse\n        \tdie(\"oops we do not understand '%s'\", value);\n\nstill need to be fixed to:\n\n\tif (value == config_true)\n        \tdie(\"oops '%s' is not a bool\", var);\n\telse if (!strcmp(value, \"somevalue\")\n\t\tOk let's use somevalue;\n\telse if (!strcmp(value, \"someothervalue\")\n\t\tOk let's use someothervalue;\n\telse\n        \tdie(\"oops we do not understand '%s'\", value);\n\nSo it does not buy us anything.  That is why I did not suggest\ndoing it that way.\n\n> This also means that code (notably the value regexp parsing) that does \n> something like\n>\n> \tvalue ? value : \"\"\n>\n> just automatically can go away, and just use 'value' directly.\n\nYes, but that's broken already, isn't it?\n"},{"id":"68280","messageId":"alpine.LFD.1.00.0802101406560.2896@woody.linux-foundation.org","threadId":"12013","inReplyTo":"7vir0wfqrz.fsf@gitster.siamese.dyndns.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-10T22:08:58Z","receivedAt":"2008-02-10T22:08:58Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Feb 2008, Junio C Hamano wrote:\n> \n> I should have mentioned the reason why I did not suggest doing\n> it this way in my [Janitor] message.\n> \n> It is not \"suttle difference between true and empty\".  Empty\n> means false, and with this approach, it switches the meaning of\n> valueless form of config to quite the opposite.\n\nNo it does not.\n\n> will now need to be changed to:\n> \n> \tif (value == config_true)\n>         \tAh we have true;\n> \telse if (!*value)\n>         \tOk this is false;\n\nAnd that was done by my patch. \n\n> still need to be fixed to:\n> \n> \tif (value == config_true)\n>         \tdie(\"oops '%s' is not a bool\", var);\n> \telse if (!strcmp(value, \"somevalue\")\n> \t\tOk let's use somevalue;\n\nAnd this is different from checking against NULL exactly how?\n\n> > This also means that code (notably the value regexp parsing) that does \n> > something like\n> >\n> > \tvalue ? value : \"\"\n> >\n> > just automatically can go away, and just use 'value' directly.\n> \n> Yes, but that's broken already, isn't it?\n\nNo, it was what the regex parsing logic was - empty matches empty. Whether \nwe really want it or not, I dunno.\n\n\t\tLinus\n"},{"id":"68284","messageId":"7vbq6oe98y.fsf@gitster.siamese.dyndns.org","threadId":"12013","inReplyTo":"alpine.LFD.1.00.0802101406560.2896@woody.linux-foundation.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-10T22:29:01Z","receivedAt":"2008-02-10T22:29:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sun, 10 Feb 2008, Junio C Hamano wrote:\n> ...\n>> will now need to be changed to:\n>> \n>> \tif (value == config_true)\n>>         \tAh we have true;\n>> \telse if (!*value)\n>>         \tOk this is false;\n>\n> And that was done by my patch. \n>\n>> still need to be fixed to:\n>> \n>> \tif (value == config_true)\n>>         \tdie(\"oops '%s' is not a bool\", var);\n>> \telse if (!strcmp(value, \"somevalue\")\n>> \t\tOk let's use somevalue;\n>\n> And this is different from checking against NULL exactly how?\n\nExactly.  My answer to your question is: \"It is not different\nfrom checking against NULL at all.\"\n\nThe first one (i.e. you needed to do so in your patch) shows\nthat the codepath that is already doing the right thing needs to\nbe modified.  If we do not introduce config_true, we do not even\nhave to.  You need additional change to correctly functioning\ncodepaths that you should not have to do.\n\nThe second one shows that even if we introduce config_true, such\nan already broken codepath needs to be fixed to check with\neither NULL or config_true anyway.  The need for fix the\ncodepath has not been reduced.  Changing the rule does not help\nfor this class of codepath.\n\nBut as you seem to imply, it might make sense to equate\n\n\t[some-random-section]\n        \tsome-random-variable\n\nto\n\n\t[some-random-section]\n        \tsome-random-variable = \"\"\n\nfor variables that cannot possibly have any meaningful \"bool\"\nsemantics.  This third class of variables is a possible benefit\nyour patch brings in.  The code can be lax for these variables.\n\nHowever, it would make things inconsistent (\"this variable is\nbool and the above two forms mean completely opposite things,\nwhile that variable is not bool and they mean the same thing\").\nI am just having a hard time convincing myself that this little\ndetail does not matter.\n"},{"id":"68287","messageId":"7v7ihce8ex.fsf@gitster.siamese.dyndns.org","threadId":"12013","inReplyTo":"7vbq6oe98y.fsf@gitster.siamese.dyndns.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-10T22:47:02Z","receivedAt":"2008-02-10T22:47:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> But as you seem to imply, it might make sense to equate\n>\n> \t[some-random-section]\n>         \tsome-random-variable\n>\n> to\n>\n> \t[some-random-section]\n>         \tsome-random-variable = \"\"\n>\n> for variables that cannot possibly have any meaningful \"bool\"\n> semantics.  This third class of variables is a possible benefit\n> your patch brings in.  The code can be lax for these variables.\n>\n> However, it would make things inconsistent (\"this variable is\n> bool and the above two forms mean completely opposite things,\n> while that variable is not bool and they mean the same thing\").\n> I am just having a hard time convincing myself that this little\n> detail does not matter.\n\nHaving said all that, it might be an option to change your patch\nslightly to say:\n\n\tconst char config_true[] = \"true\";\n\nIOW, make\n\n\t[section]\n        \tvar\n\nequivalent to\n\n\t[section]\n\t\tvar = \"true\"\n\nExisting codepaths that deal with variables that were originally\nbool but now bool + more might get upset if we did so, though.\nAn example I could think of offhand was \"d7f4633 (Make AutoCRLF\nternary variable\" but that one is safe.\n"},{"id":"68292","messageId":"20080210232920.GH5129@artemis.madism.org","threadId":"12013","inReplyTo":"7v7ihce8ex.fsf@gitster.siamese.dyndns.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-02-10T23:29:20Z","receivedAt":"2008-02-10T23:29:20Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Feb 10, 2008 at 10:47:02PM +0000, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > But as you seem to imply, it might make sense to equate\n> >\n> > \t[some-random-section]\n> >         \tsome-random-variable\n> >\n> > to\n> >\n> > \t[some-random-section]\n> >         \tsome-random-variable = \"\"\n> >\n> > for variables that cannot possibly have any meaningful \"bool\"\n> > semantics.  This third class of variables is a possible benefit\n> > your patch brings in.  The code can be lax for these variables.\n> >\n> > However, it would make things inconsistent (\"this variable is\n> > bool and the above two forms mean completely opposite things,\n> > while that variable is not bool and they mean the same thing\").\n> > I am just having a hard time convincing myself that this little\n> > detail does not matter.\n> \n> Having said all that, it might be an option to change your patch\n> slightly to say:\n> \n> \tconst char config_true[] = \"true\";\n\nI was about to suggest the same, and testing against \"config_true\" just\nbecomes an optimization, but isn't required. Seems an appropriate hack\nto me.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"68293","messageId":"alpine.LFD.1.00.0802101532070.2920@woody.linux-foundation.org","threadId":"12013","inReplyTo":"7vbq6oe98y.fsf@gitster.siamese.dyndns.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-10T23:36:43Z","receivedAt":"2008-02-10T23:36:43Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Feb 2008, Junio C Hamano wrote:\n> >\n> > And this is different from checking against NULL exactly how?\n> \n> Exactly.  My answer to your question is: \"It is not different\n> from checking against NULL at all.\"\n\nOf course it is.\n\nNot using NULL means that things like\n\n\tstrcmp()\n\tatoi()\n\t..\n\nall work and automatically get the right answer and don't need to care.\n\nTake a look at the NULL-compare patches. 90% of them are just noise.\n\nYes, the 10% that actually *cares* will look exactly the same (the \ndifference being comparing against NULL or config_true), but all the stuff \nthat compares against a special string can just ignore the fact that the \nspecial truth value will never ever match that string.\n\n\t\tLinus\n"},{"id":"68296","messageId":"alpine.LFD.1.00.0802101538030.2920@woody.linux-foundation.org","threadId":"12013","inReplyTo":"alpine.LFD.1.00.0802101532070.2920@woody.linux-foundation.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-10T23:41:23Z","receivedAt":"2008-02-10T23:41:23Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Feb 2008, Linus Torvalds wrote:\n> \n> Yes, the 10% that actually *cares* will look exactly the same (the \n> difference being comparing against NULL or config_true), but all the stuff \n> that compares against a special string can just ignore the fact that the \n> special truth value will never ever match that string.\n\nAnd here's an example of this kind of effect. I'm not actually suggesting \nyou apply this patch, but tell me it isn't simpler done this way?\n\nSo this is where it *does* make a difference whether we use NULL or \nconfig_bool, and where config_bool is simply better: it allows a config \nroutine to simply never care..\n\n\t\tLinus\n\n---\n color.c |   15 ++++++---------\n 1 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/color.c b/color.c\nindex cb70340..e5fbf01 100644\n--- a/color.c\n+++ b/color.c\n@@ -118,15 +118,12 @@ bad:\n \n int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)\n {\n-\tif (value) {\n-\t\tif (!strcasecmp(value, \"never\"))\n-\t\t\treturn 0;\n-\t\tif (!strcasecmp(value, \"always\"))\n-\t\t\treturn 1;\n-\t\tif (!strcasecmp(value, \"auto\"))\n-\t\t\tgoto auto_color;\n-\t}\n-\n+\tif (!strcasecmp(value, \"never\"))\n+\t\treturn 0;\n+\tif (!strcasecmp(value, \"always\"))\n+\t\treturn 1;\n+\tif (!strcasecmp(value, \"auto\"))\n+\t\tgoto auto_color;\n \t/* Missing or explicit false to turn off colorization */\n \tif (!git_config_bool(var, value))\n \t\treturn 0;\n"},{"id":"68297","messageId":"alpine.LFD.1.00.0802101541380.2920@woody.linux-foundation.org","threadId":"12013","inReplyTo":"20080210232920.GH5129@artemis.madism.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-10T23:50:56Z","receivedAt":"2008-02-10T23:50:56Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Feb 2008, Pierre Habouzit wrote:\n>\n> > Having said all that, it might be an option to change your patch\n> > slightly to say:\n> > \n> > \tconst char config_true[] = \"true\";\n> \n> I was about to suggest the same, and testing against \"config_true\" just\n> becomes an optimization, but isn't required. Seems an appropriate hack\n> to me.\n\nWell, I had the thing actually written that way, but it breaks some of the \ntest-suite. Whether the test-suite actually *should* test for what it \ntests for is obviously debatable, but it does. It does test that when you \ndo\n\n\tgit config --get novalue.variable\n\nyou are expected to get an empty result. Is that good? Probably not. But \nit's what you get traditionally, and it's what the tests actually test for \n(t/t1300-repo-config.sh in case you care).\n\nBut yes, I actually think it might be an improvement and have that thing \nreturn \"true\" (which is what happens if you make the 'config_true' array \ncontain that string). And that allows removal of one test from the \n\"git_config_bool()\" function, but on the other hand, it does result in \nsome strange stuff too..\n\nIn particular, *if* somebody just takes a string blindly and uses\n\n\tresult = xstrdup(value);\n\nthen with my patch it would then use an empty string for whatever it \nhappened to pick. So having something like\n\n\t[user]\n\t\tname\n\nwill mean that your name is empty (which will actually trigger an error if \nyou try to commit).\n\nIn contrast, if you do that 'config_true[] = \"true\"' thing, then that \nconfig file entry will make your name be the string \"true\", which is just \n_odd_.\n\nSo I think an empty config_true actually has a nicer and more easily \nexplainable failure case. It causes empty strings when mis-used, not \narbitrary and strange \"true\" strings.\n\nWhatever. \n\n\t\tLinus\n"},{"id":"68300","messageId":"7vhcggcokx.fsf@gitster.siamese.dyndns.org","threadId":"12013","inReplyTo":"alpine.LFD.1.00.0802101538030.2920@woody.linux-foundation.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-11T00:40:46Z","receivedAt":"2008-02-11T00:40:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> And here's an example of this kind of effect. I'm not actually suggesting \n> you apply this patch, but tell me it isn't simpler done this way?\n\nYes, that is a good example of simplification.\n\n> So this is where it *does* make a difference whether we use NULL or \n> config_bool, and where config_bool is simply better: it allows a config \n> routine to simply never care..\n\nBut that applies only to \"originally bool but now has additional\nstates\" kind of variables.\n\nFor a variable that is never about boolean, if the original code\nsaid:\n\n\tif (!strcmp(var, \"section.variable\"))\n        \tfoo = xstrdup(value);\n\nit is wrong (would strdup NULL), and the correct fix would be:\n\n\tif (!strcmp(var, \"section.variable\")) {\n\t\tif (!value)\n\t\t\tdie(\"missing value for '%s'\", var);\n        \tfoo = xstrdup(value);\n\t}\n\nIt does not make much of a difference if that \"if (!value)\"\nbecomes \"if (value == config_true)\".  If you omit that check, as\nyour \"user.name\" example shows, foo may get an empty string or a\nstring \"true\", neither of which is what the user intended to\nsay.\n"},{"id":"68330","messageId":"200802110822.08902.chriscool@tuxfamily.org","threadId":"12013","inReplyTo":"alpine.LFD.1.00.0802101532070.2920@woody.linux-foundation.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-11T07:22:08Z","receivedAt":"2008-02-11T07:22:08Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le lundi 11 février 2008, Linus Torvalds a écrit :\n> On Sun, 10 Feb 2008, Junio C Hamano wrote:\n> >\n> > My answer to your question is: \"It is not different\n> > from checking against NULL at all.\"\n>\n> Of course it is.\n>\n> Not using NULL means that things like\n>\n> \tstrcmp()\n> \tatoi()\n> \t..\n>\n> all work and automatically get the right answer and don't need to care.\n\nI agree.\n\n> Take a look at the NULL-compare patches. 90% of them are just noise.\n\nThat's right.\n\nNow the 10% are only when we have a boolean variable and we want it to \nbe \"false\" when there is:\n\n[foo]\n\tvar =\n\nwhile:\n\n[foo]\n\tvar\n\nis considered \"true\".\n\nAnd let's face it, it's not obvious at all why it should be false if there \nis \"var =\" and true when there is only \"var\". Is it a Microsoft standard ?\nDo we really care about it ?\n\nI also doubt that many users willingly use \"var =\" to mean \"false\". In my \nopinion it is much more likely (95% against 5%) to be a typo than someone \nso lazy as to prefer only \"var =\" over \"var = false\".\n\nSo let's do them (and ourself too) a favor and deprecate \"var =\" to mean \nfalse. I will post my patch to do this.\n\nBy the way deprecating does not mean breaking it and not fixing where it \nbreaks. It just means we think it's bad and it should not be used. We can \neven decide to keep the same boolean meaning (that is \"false\") for \"var =\"\nlatter if we have a good way to deal with the cruft and if we really don't \nwant to break things.\n\nAnd even if we latter change \"var =\" to mean \"true\", we can still keep a \nwarning by checking using Linus' trick:\n\nint git_config_bool(const char *name, const char *value)\n{\n        if (value == config_true)\n                return 1;\n        if (!*value) {\n                fprintf(stderr,\n                        \"Warning: using an empty value for boolean config \"\n                        \"variables is deprecated and dangerous.\\n\"\n                        \"An empty value now means 'true' as a \"\n                        \"boolean, but meant 'false' in previous git \"\n                        \"versions!\\n\"\n                        \"Please consider using a 'true' (or 'false') value \"\n                        \"explicitely for variable '%s', so that your \"\n                        \"config is git version independant. \"\n                        \"You can do that using for example:\\n\"\n                        \"\\tgit config %s true\\n\", name, name);\n               return 1;\n       }\n       if (!strcasecmp(value, \"true\") || !strcasecmp(value, \"yes\"))\n               return 1;\n       if (!strcasecmp(value, \"false\") || !strcasecmp(value, \"no\"))\n\t\treturn 0;\n\treturn git_config_int(name, value) != 0;\n}\n\nOr we could even (using Linus' trick) make it an error and just \"die\" in \nthis case.\n\nChristian (now urgently looking for a flame proof suit).\n"},{"id":"68338","messageId":"7vzlu7ap2y.fsf@gitster.siamese.dyndns.org","threadId":"12013","inReplyTo":"200802110822.08902.chriscool@tuxfamily.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-11T08:12:53Z","receivedAt":"2008-02-11T08:12:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> Now the 10% are only when we have a boolean variable and we want it to \n> be \"false\" when there is:\n>\n> [foo]\n> \tvar =\n>\n> while:\n>\n> [foo]\n> \tvar\n>\n> is considered \"true\".\n>\n> And let's face it, it's not obvious at all why it should be false if there \n> is \"var =\" and true when there is only \"var\". Is it a Microsoft standard ?\n> Do we really care about it ?\n> ...\n> I also doubt that many users willingly use \"var =\" to mean \"false\".\n\nSome variables started their life as boolean and \"[foo] var\" is\ntrue and \"[foo] var = false\" (or \"= 0\") is false for them; later\nthey acquired third or more options to have string values.\n\nFor such a variable:\n\n\t[foo] var\t    ; boolean \"true\"\n        [foo] var = \"true\"  ; boolean \"true\"\n        [foo] var = \"false\" ; boolean \"false\"\n\t[foo] var = \"\"\t    ; boolean \"false\", too.\n        [foo] var =         ; perhaps misspelled, perhaps the same as above.\n\t[foo] var = \"value\" ; string (new semantics)\n\nand adding a warning for the fifth one (but not fourth one)\nmight be a good idea.\n\nI have a bit stronger opinion on keeping \"[foo] var\" (without an\nequal sign) to stay \"true\".  Especially, making it exactly the\nsame as an empty string is unacceptable, if it is done without\nLinus's trick to make it distinguishable internally.\n\nLuckily, the calls to get_config_set() we internally make\nin-tree (e.g. git-init sets core.filemode and friends) are all\nvery explicit and do not write \"[foo] var\" form of truth.  But\nmany existing docs written outside our (my) control that google\nfinds say \"Put '[core] autocrlf' in your .git/config\".  You do\nnot want to break the repositories configured following such an\nadvice.\n\n> So let's do them (and ourself too) a favor and deprecate \"var =\" to mean \n> false. I will post my patch to do this.\n\nDon't do it if you cannot differentiate between the fourth and\nthe fifth in the above listing.\n"},{"id":"68412","messageId":"alpine.LNX.1.00.0802111133490.13593@iabervon.org","threadId":"12013","inReplyTo":"7vir0wfqrz.fsf@gitster.siamese.dyndns.org","subject":"Re: Alternative approach to the git config NULL value checking patches..","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-11T17:27:30Z","receivedAt":"2008-02-11T17:27:30Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 10 Feb 2008, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n> \n> > It may be a bit odd, but it automatically means that codepaths that simply \n> > don't want to care about the subtle difference between \"true\" and \"empty\" \n> > will just automatically work, because to them the two cases will look \n> > identical, because the strings will compare the same - they have the same \n> > data, just different addresses.\n> \n> I should have mentioned the reason why I did not suggest doing\n> it this way in my [Janitor] message.\n> \n> It is not \"suttle difference between true and empty\".  Empty\n> means false, and with this approach, it switches the meaning of\n> valueless form of config to quite the opposite.\n> \n> In addition to fixing existing breakages, a piece of code that\n> knew NULL is true and empty is false and coded accordingly, i.e.\n> \n> \tif (!value)\n>         \tAh we have true;\n> \telse if (!*value)\n>         \tOk this is false;\n> \telse if (!strcmp(value, \"something special\")\n>         \tOk, this is not bool but special;\n> \telse\n>         \treturn git_config_bool(var, value);\n> \n> will now need to be changed to:\n> \n> \tif (value == config_true)\n>         \tAh we have true;\n> \telse if (!*value)\n>         \tOk this is false;\n> \telse if (!strcmp(value, \"something special\")\n>         \tOk, this is not bool but special;\n> \telse\n>         \treturn git_config_bool(var, value);\n\nShouldn't it be simply:\n\n\tif (!strcmp(value, \"something special\"))\n\t\tOk, this is not bool but special;\n\telse\n\t\treturn git_config_bool(var, value);\n\nThat is, don't check for special true or special false at all, but have \ngit_config_bool() sort them out? And Linus's code means that you can do \nthe strcmp without worrying about getting a segfault on special true.\n\n> if you do this.  And the code that was already broken:\n> \n> \tif (!strcmp(value, \"somevalue\")\n> \t\tOk let's use somevalue;\n> \telse if (!strcmp(value, \"someothervalue\")\n> \t\tOk let's use someothervalue;\n> \telse\n>         \tdie(\"oops we do not understand '%s'\", value);\n> \n> still need to be fixed to:\n> \n> \tif (value == config_true)\n>         \tdie(\"oops '%s' is not a bool\", var);\n> \telse if (!strcmp(value, \"somevalue\")\n> \t\tOk let's use somevalue;\n> \telse if (!strcmp(value, \"someothervalue\")\n> \t\tOk let's use someothervalue;\n> \telse\n>         \tdie(\"oops we do not understand '%s'\", value);\n\nIf it isn't changed, you'd get the message \"oops we do not understand ''\" \nfor either true or false empty values, which seems, if anything, better.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}