{"thread":{"id":"21943","subject":"[PATCH/RFC] ignore unknown color configuration","startedAt":"2009-12-12T12:25:24Z","lastAt":"2009-12-16T03:46:48Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"129798","messageId":"20091212122524.GA17547@coredump.intra.peff.net","threadId":"21943","inReplyTo":null,"subject":"[PATCH/RFC] ignore unknown color configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-12T12:25:24Z","receivedAt":"2009-12-12T12:25:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing the config file, if there is a value that is\nsyntactically correct but unused, we generally ignore it.\nThis lets non-core porcelains store arbitrary information in\nthe config file, and it means that configuration files can\nbe shared between new and old versions of git (the old\nversions might simply ignore certain configuration).\n\nThe one exception to this is color configuration; if we\nencounter a color.{diff,branch,status}.$slot variable, we\ndie if it is not one of the recognized slots (presumably as\na safety valve for user misconfiguration). This behavior\nhas existed since 801235c (diff --color: use\n$GIT_DIR/config, 2006-06-24), but hasn't yet caused a\nproblem. No porcelain has wanted to store extra colors, and\nwe once a color area (like color.diff) has been introduced,\nwe've never changed the set of color slots.\n\nHowever, that changed recently with the addition of\ncolor.diff.func. Now a user with color.diff.func in their\nconfig can no longer freely switch between v1.6.6 and older\nversions; the old versions will complain about the existence\nof the variable.\n\nThis patch loosens the check to match the rest of\ngit-config; unknown color slots are simply ignored. This\ndoesn't fix this particular problem, as the older version\n(without this patch) is the problem, but it at least\nprevents it from happening again in the future.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't know if it is worth trying to fix the problem for\ncolor.diff.func. It would require renaming the variable to something\noutside of the color.diff hierarchy, which is quite ugly. I don't know\nhow common a problem this is. Personally I switch between git versions a\nlot doing git development (uncommon), but I also ran into it because I\nshare my ~/.gitconfig across several machines (which may be much more\ncommon).\n\nI suppose we could provide an undocumented alias of \"color.difffunc\",\nand then only people who ran into this problem would have to be exposed\nto the monstrosity. Normal people would use color.diff.func and be none\nthe wiser. But that feels awfully hack-ish to me.\n\n builtin-branch.c |    4 +++-\n builtin-commit.c |    4 +++-\n diff.c           |    4 +++-\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 05e876e..c87e63b 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -65,7 +65,7 @@ static int parse_branch_color_slot(const char *var, int ofs)\n \t\treturn BRANCH_COLOR_LOCAL;\n \tif (!strcasecmp(var+ofs, \"current\"))\n \t\treturn BRANCH_COLOR_CURRENT;\n-\tdie(\"bad config variable '%s'\", var);\n+\treturn -1;\n }\n \n static int git_branch_config(const char *var, const char *value, void *cb)\n@@ -76,6 +76,8 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!prefixcmp(var, \"color.branch.\")) {\n \t\tint slot = parse_branch_color_slot(var, 13);\n+\t\tif (slot < 0)\n+\t\t\treturn 0;\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n \t\tcolor_parse(value, var, branch_colors[slot]);\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex e93a647..326cd63 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -890,7 +890,7 @@ static int parse_status_slot(const char *var, int offset)\n \t\treturn WT_STATUS_NOBRANCH;\n \tif (!strcasecmp(var+offset, \"unmerged\"))\n \t\treturn WT_STATUS_UNMERGED;\n-\tdie(\"bad config variable '%s'\", var);\n+\treturn -1;\n }\n \n static int git_status_config(const char *k, const char *v, void *cb)\n@@ -910,6 +910,8 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t}\n \tif (!prefixcmp(k, \"status.color.\") || !prefixcmp(k, \"color.status.\")) {\n \t\tint slot = parse_status_slot(k, 13);\n+\t\tif (slot < 0)\n+\t\t\treturn -1;\n \t\tif (!v)\n \t\t\treturn config_error_nonbool(k);\n \t\tcolor_parse(v, k, s->color_palette[slot]);\ndiff --git a/diff.c b/diff.c\nindex d952686..08bbd3e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -63,7 +63,7 @@ static int parse_diff_color_slot(const char *var, int ofs)\n \t\treturn DIFF_WHITESPACE;\n \tif (!strcasecmp(var+ofs, \"func\"))\n \t\treturn DIFF_FUNCINFO;\n-\tdie(\"bad config variable '%s'\", var);\n+\treturn -1;\n }\n \n static int git_config_rename(const char *var, const char *value)\n@@ -122,6 +122,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \n \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n \t\tint slot = parse_diff_color_slot(var, 11);\n+\t\tif (slot < 0)\n+\t\t\treturn 0;\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n \t\tcolor_parse(value, var, diff_colors[slot]);\n-- \n1.6.6.rc2.18.gbc86b8\n"},{"id":"129804","messageId":"7vzl5nn9x2.fsf@alter.siamese.dyndns.org","threadId":"21943","inReplyTo":"20091212122524.GA17547@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-12T21:45:45Z","receivedAt":"2009-12-12T21:45:45Z","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> When parsing the config file, if there is a value that is\n> syntactically correct but unused, we generally ignore it.\n> This lets non-core porcelains store arbitrary information in\n> the config file, and it means that configuration files can\n> be shared between new and old versions of git (the old\n> versions might simply ignore certain configuration).\n>\n> The one exception to this is color configuration; if we\n> encounter a color.{diff,branch,status}.$slot variable, we\n> die if it is not one of the recognized slots (presumably as\n> a safety valve for user misconfiguration).\n\nThis reminds me of the issue an earlier patch with a good intention but a\nhorrible consequence wanted to address.\n\n  http://thread.gmane.org/gmane.comp.version-control.git/125925/focus=127629\n\n> This patch loosens the check to match the rest of\n> git-config; unknown color slots are simply ignored.\n\nI am of two minds, even though I am slightly in favor than against the\nchange.\n\nThis is a sane thing to do, as \"slot\" is part of the name of the variable,\nand we generally do not warn upon seeing a misspelled variable name (it\nmakes it worse that \"func\" is not even misspelled but merely unknown to\nolder version of git in your scenario).\n\nOn the other hand, I suspect that most people would apprecfiate if their\ngit pointed out \"diff.color.finc?  What do you mean?\"  before they waste\n30 minutes wondering why the new feature in 1.6.6 does not work for them.\n"},{"id":"129811","messageId":"20091212222046.GA25973@coredump.intra.peff.net","threadId":"21943","inReplyTo":"7vzl5nn9x2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-12T22:20:47Z","receivedAt":"2009-12-12T22:20:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 12, 2009 at 01:45:45PM -0800, Junio C Hamano wrote:\n\n> This is a sane thing to do, as \"slot\" is part of the name of the variable,\n> and we generally do not warn upon seeing a misspelled variable name (it\n> makes it worse that \"func\" is not even misspelled but merely unknown to\n> older version of git in your scenario).\n> \n> On the other hand, I suspect that most people would apprecfiate if their\n> git pointed out \"diff.color.finc?  What do you mean?\"  before they waste\n> 30 minutes wondering why the new feature in 1.6.6 does not work for them.\n\nI would be more sympathetic to that user if this weren't the _only_ set\nof variables with this property. They don't get warned for diff.externel\nor color.show-branch.\n\nIf we are going to declare subsets of the namespace as \"complete\" (and\nwarn about unknown keys in them), then we should probably be more\nthorough about it (but I don't personally think that is a good idea, as\nversion portability is IMHO more useful).\n\n-Peff\n"},{"id":"129827","messageId":"7vzl5mjndi.fsf@alter.siamese.dyndns.org","threadId":"21943","inReplyTo":"20091212222046.GA25973@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-14T02:33:13Z","receivedAt":"2009-12-14T02:33:13Z","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 Sat, Dec 12, 2009 at 01:45:45PM -0800, Junio C Hamano wrote:\n>\n>> This is a sane thing to do, as \"slot\" is part of the name of the variable,\n>> and we generally do not warn upon seeing a misspelled variable name (it\n>> makes it worse that \"func\" is not even misspelled but merely unknown to\n>> older version of git in your scenario).\n>> \n>> On the other hand, I suspect that most people would apprecfiate if their\n>> git pointed out \"diff.color.finc?  What do you mean?\"  before they waste\n>> 30 minutes wondering why the new feature in 1.6.6 does not work for them.\n>\n> I would be more sympathetic to that user if this weren't the _only_ set\n> of variables with this property. They don't get warned for diff.externel\n> or color.show-branch.\n\nTrue and fair enough.  Let's have this in 1.6.6 then.\n"},{"id":"129975","messageId":"7v3a3bwvzj.fsf@alter.siamese.dyndns.org","threadId":"21943","inReplyTo":"7vzl5mjndi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-16T01:25:36Z","receivedAt":"2009-12-16T01:25:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> On Sat, Dec 12, 2009 at 01:45:45PM -0800, Junio C Hamano wrote:\n>>\n>>> This is a sane thing to do, as \"slot\" is part of the name of the variable,\n>>> and we generally do not warn upon seeing a misspelled variable name (it\n>>> makes it worse that \"func\" is not even misspelled but merely unknown to\n>>> older version of git in your scenario).\n>>> \n>>> On the other hand, I suspect that most people would apprecfiate if their\n>>> git pointed out \"diff.color.finc?  What do you mean?\"  before they waste\n>>> 30 minutes wondering why the new feature in 1.6.6 does not work for them.\n>>\n>> I would be more sympathetic to that user if this weren't the _only_ set\n>> of variables with this property. They don't get warned for diff.externel\n>> or color.show-branch.\n>\n> True and fair enough.  Let's have this in 1.6.6 then.\n\nActually I think we should have this in 1.6.5.X as well for it to be\nuseful.  Am I mistaken?\n"},{"id":"129976","messageId":"7vy6l3vh8x.fsf@alter.siamese.dyndns.org","threadId":"21943","inReplyTo":"20091212122524.GA17547@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-16T01:29:18Z","receivedAt":"2009-12-16T01:29:18Z","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> diff --git a/builtin-branch.c b/builtin-branch.c\n> index 05e876e..c87e63b 100644\n> --- a/builtin-branch.c\n> +++ b/builtin-branch.c\n> @@ -65,7 +65,7 @@ static int parse_branch_color_slot(const char *var, int ofs)\n>  \t\treturn BRANCH_COLOR_LOCAL;\n>  \tif (!strcasecmp(var+ofs, \"current\"))\n>  \t\treturn BRANCH_COLOR_CURRENT;\n> -\tdie(\"bad config variable '%s'\", var);\n> +\treturn -1;\n>  }\n>  \n>  static int git_branch_config(const char *var, const char *value, void *cb)\n> @@ -76,6 +76,8 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n>  \t}\n>  \tif (!prefixcmp(var, \"color.branch.\")) {\n>  \t\tint slot = parse_branch_color_slot(var, 13);\n> +\t\tif (slot < 0)\n> +\t\t\treturn 0;\n>  \t\tif (!value)\n>  \t\t\treturn config_error_nonbool(var);\n>  \t\tcolor_parse(value, var, branch_colors[slot]);\n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index e93a647..326cd63 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -890,7 +890,7 @@ static int parse_status_slot(const char *var, int offset)\n>  \t\treturn WT_STATUS_NOBRANCH;\n>  \tif (!strcasecmp(var+offset, \"unmerged\"))\n>  \t\treturn WT_STATUS_UNMERGED;\n> -\tdie(\"bad config variable '%s'\", var);\n> +\treturn -1;\n>  }\n>  \n>  static int git_status_config(const char *k, const char *v, void *cb)\n> @@ -910,6 +910,8 @@ static int git_status_config(const char *k, const char *v, void *cb)\n>  \t}\n>  \tif (!prefixcmp(k, \"status.color.\") || !prefixcmp(k, \"color.status.\")) {\n>  \t\tint slot = parse_status_slot(k, 13);\n> +\t\tif (slot < 0)\n> +\t\t\treturn -1;\n\nShouldn't this return 0, to say \"we handled it (by ignoring), don't\nworry\", instead of saying \"hey it's error\" by returning -1?  That's what\nis done on the \"diff\" side below...\n\n> diff --git a/diff.c b/diff.c\n> index d952686..08bbd3e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -122,6 +122,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n>  \n>  \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n>  \t\tint slot = parse_diff_color_slot(var, 11);\n> +\t\tif (slot < 0)\n> +\t\t\treturn 0;\n>  \t\tif (!value)\n>  \t\t\treturn config_error_nonbool(var);\n>  \t\tcolor_parse(value, var, diff_colors[slot]);\n"},{"id":"129981","messageId":"20091216034529.GA9742@sigill.intra.peff.net","threadId":"21943","inReplyTo":"7v3a3bwvzj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-16T03:45:29Z","receivedAt":"2009-12-16T03:45:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 15, 2009 at 05:25:36PM -0800, Junio C Hamano wrote:\n\n> >> I would be more sympathetic to that user if this weren't the _only_ set\n> >> of variables with this property. They don't get warned for diff.externel\n> >> or color.show-branch.\n> >\n> > True and fair enough.  Let's have this in 1.6.6 then.\n> \n> Actually I think we should have this in 1.6.5.X as well for it to be\n> useful.  Am I mistaken?\n\nThe earlier the better for making it useful, but it is still somewhat of\na lost cause for color.diff.func. The problem comes from using git\nv1.6.6, setting the variable, and then going to back to some older\nversion (either because you are testing multiple versions, or because\nyour config is shared across multiple machines).\n\nSo yes, putting it in v1.6.5.x means switching back there will not be as\npainful. But switching back to existing versions will still be broken\nuntil all older versions you might want to switch to have this patch.\n\nSo we are not so much fixing this color.diff.func problem as\nfuture-proofing against this happening again.\n\n-Peff\n"},{"id":"129982","messageId":"20091216034648.GB9742@sigill.intra.peff.net","threadId":"21943","inReplyTo":"7vy6l3vh8x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] ignore unknown color configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-16T03:46:48Z","receivedAt":"2009-12-16T03:46:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 15, 2009 at 05:29:18PM -0800, Junio C Hamano wrote:\n\n> >  \tif (!prefixcmp(k, \"status.color.\") || !prefixcmp(k, \"color.status.\")) {\n> >  \t\tint slot = parse_status_slot(k, 13);\n> > +\t\tif (slot < 0)\n> > +\t\t\treturn -1;\n> \n> Shouldn't this return 0, to say \"we handled it (by ignoring), don't\n> worry\", instead of saying \"hey it's error\" by returning -1?  That's what\n> is done on the \"diff\" side below...\n\nYes, thank you for catching it. It should definitely be \"return 0\" (as\nthe other two cases are). It was a simple think-o on my part.\n\n-Peff\n"}]}