{"thread":{"id":"11901","subject":"[PATCH] Fix bug in parse_color that prevented the user from changing the background colors.","startedAt":"2008-02-05T17:34:34Z","lastAt":"2008-02-06T12:16:26Z","messageCount":8,"participants":["Chris Larson","Timo Hirvonen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"67532","messageId":"47A89E2A.9010905@kergoth.com","threadId":"11901","inReplyTo":null,"subject":"[PATCH] Fix bug in parse_color that prevented the user from changing the background colors.","fromName":"Chris Larson","fromEmail":"clarson@kergoth.com","sentAt":"2008-02-05T17:34:34Z","receivedAt":"2008-02-05T17:34:34Z","isPatch":true,"sender":{"key":"clarson@kergoth.com","avatar":"https://gravatar.com/avatar/8929179d0d33b0477876eaae4d5ca20dc4207c2eb573360adec7d91f63a3bd71?d=mp&s=160"},"body":"The comments in color.c indicate that the syntax for the color options \nin the\ngit config is [fg [bg]] [attr], however the implementation fails if \nstrtol is\nunable to convert the string in its entirety into an integer.\n\nSigned-off-by: Chris Larson <clarson@kergoth.com>\n---\n color.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/color.c b/color.c\nindex 7f66c29..62518fa 100644\n--- a/color.c\n+++ b/color.c\n@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)\n             return i - 1;\n     }\n     i = strtol(name, &end, 10);\n-    if (*name && !*end && i >= -1 && i <= 255)\n+    if (*name && i >= -1 && i <= 255)\n         return i;\n     return -2;\n }\n-- \n1.5.4.29.g43ce-dirty\n"},{"id":"67533","messageId":"20080205203940.1dcff0ce.tihirvon@gmail.com","threadId":"11901","inReplyTo":"47A89E2A.9010905@kergoth.com","subject":"Re: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2008-02-05T18:39:40Z","receivedAt":"2008-02-05T18:39:40Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"Chris Larson <clarson@kergoth.com> wrote:\n\n> The comments in color.c indicate that the syntax for the color options \n> in the\n> git config is [fg [bg]] [attr], however the implementation fails if \n> strtol is\n> unable to convert the string in its entirety into an integer.\n> \n> Signed-off-by: Chris Larson <clarson@kergoth.com>\n> ---\n>  color.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/color.c b/color.c\n> index 7f66c29..62518fa 100644\n> --- a/color.c\n> +++ b/color.c\n> @@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)\n>              return i - 1;\n>      }\n>      i = strtol(name, &end, 10);\n> -    if (*name && !*end && i >= -1 && i <= 255)\n> +    if (*name && i >= -1 && i <= 255)\n>          return i;\n>      return -2;\n>  }\n\nMy bug, sorry. I should have tested more.\n\nI think this new code accepts \"7bold\" (didn't test).  Maybe you should\ndo something like this instead:\n\n\tif (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)\n\nUntested of course.  BTW, your patch is whitespace corrupted.\n"},{"id":"67534","messageId":"b6ebd0a50802051048s3db55f6yab79e8982fd81b81@mail.gmail.com","threadId":"11901","inReplyTo":"b6ebd0a50802051045t4949df68u7e405ea618403a31@mail.gmail.com","subject":"Re: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.","fromName":"Chris Larson","fromEmail":"clarson@kergoth.com","sentAt":"2008-02-05T18:48:39Z","receivedAt":"2008-02-05T18:48:39Z","isPatch":true,"sender":{"key":"clarson@kergoth.com","avatar":"https://gravatar.com/avatar/8929179d0d33b0477876eaae4d5ca20dc4207c2eb573360adec7d91f63a3bd71?d=mp&s=160"},"body":"On Feb 5, 2008 11:39 AM, Timo Hirvonen <tihirvon@gmail.com> wrote:\n>\n> Chris Larson <clarson@kergoth.com> wrote:\n>\n> > unable to convert the string in its entirety into an integer.\n>\n> > -    if (*name && !*end && i >= -1 && i <= 255)\n> > +    if (*name && i >= -1 && i <= 255)\n> >          return i;\n> >      return -2;\n> >  }\n>\n>\n> My bug, sorry. I should have tested more.\n>\n> I think this new code accepts \"7bold\" (didn't test).  Maybe you should\n> do something like this instead:\n>\n>        if (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)\n\nIndeed, I should have tested more too :)  Just starting to mess with\nthe codebase, surprised at how clean it is (though I probably\nshouldn't be).\n\n> Untested of course.  BTW, your patch is whitespace corrupted.\n>\n\nUnsurprising... sometimes I really hate gmail :|  Anyone know if\nthere's a way to post with gmail without hosing the patch, or should I\nswitch to a non-web based solution?\n\nThanks,\n-- \nChris Larson - clarson at kergoth dot com\nDedicated Engineer - MontaVista - clarson at mvista dot com\nCore Developer/Architect - TSLib, BitBake, OpenEmbedded, OpenZaurus\n"},{"id":"67535","messageId":"20080205205856.76a7cd45.tihirvon@gmail.com","threadId":"11901","inReplyTo":"b6ebd0a50802051045t4949df68u7e405ea618403a31@mail.gmail.com","subject":"Re: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2008-02-05T18:58:56Z","receivedAt":"2008-02-05T18:58:56Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"\"Chris Larson\" <clarson@kergoth.com> wrote:\n\n> On Feb 5, 2008 11:39 AM, Timo Hirvonen <tihirvon@gmail.com> wrote:\n> \n> > Chris Larson <clarson@kergoth.com> wrote:\n> >\n> > unable to convert the string in its entirety into an integer.\n> > > -    if (*name && !*end && i >= -1 && i <= 255)\n> > > +    if (*name && i >= -1 && i <= 255)\n> > >          return i;\n> > >      return -2;\n> > >  }\n> >\n> > My bug, sorry. I should have tested more.\n> >\n> > I think this new code accepts \"7bold\" (didn't test).  Maybe you should\n> > do something like this instead:\n> >\n> >        if (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)\n> \n> \n> Indeed, I should have tested more too :)  Just starting to mess with the\n> codebase, surprised at how clean it is (though I probably shouldn't be).\n\nOK, did some testing.\n\nThe old code accepted attribute before color (bold red). With your patch\nthat \"bold\" is treaded as fg color -1 and red as bg color, so you need\nthat (!*end || isspace(*end)) test.\n\n> > Untested of course.  BTW, your patch is whitespace corrupted.\n> >\n> \n> Unsurprising... sometimes I really hate gmail :|  Anyone know if there's a\n> way to post with gmail without hosing the patch, or should I switch to a\n> non-web based solution?\n\nI don't know.  Maybe attachment is the only way.\n"},{"id":"67537","messageId":"20080205211821.e4a15194.tihirvon@gmail.com","threadId":"11901","inReplyTo":"20080205205856.76a7cd45.tihirvon@gmail.com","subject":"[PATCH] Fix parsing numeric color values","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2008-02-05T19:18:21Z","receivedAt":"2008-02-05T19:18:21Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"Fix bug reported by Chris Larson <clarson@kergoth.com>.  Numeric color\nonly worked if it was at end of line.\n---\n color.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/color.c b/color.c\nindex 7f66c29..3c999c3 100644\n--- a/color.c\n+++ b/color.c\n@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)\n \t\t\treturn i - 1;\n \t}\n \ti = strtol(name, &end, 10);\n-\tif (*name && !*end && i >= -1 && i <= 255)\n+\tif (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)\n \t\treturn i;\n \treturn -2;\n }\n-- \n1.5.4.1134.ge34cf-dirty\n"},{"id":"67643","messageId":"7vzluez9q9.fsf@gitster.siamese.dyndns.org","threadId":"11901","inReplyTo":"20080205211821.e4a15194.tihirvon@gmail.com","subject":"Re: [PATCH] Fix parsing numeric color values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T09:59:26Z","receivedAt":"2008-02-06T09:59:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Timo Hirvonen <tihirvon@gmail.com> writes:\n\n> Fix bug reported by Chris Larson <clarson@kergoth.com>.  Numeric color\n> only worked if it was at end of line.\n\nSignoff?\n\nIt is much easier to read if you said that backwards:\n\n        Numeric color only worked if it was at end of line.\n        Noticed by Chris Larson <clarson@kergoth.com>.  \n\n> @@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)\n>  \t\t\treturn i - 1;\n>  \t}\n>  \ti = strtol(name, &end, 10);\n> -\tif (*name && !*end && i >= -1 && i <= 255)\n> +\tif (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)\n\nHmph.  Is it the same as (end-name) == len?\n\nPlease add a test so that your fix won't be broken by others who\nmight later touch this code.\n"},{"id":"67663","messageId":"20080206141608.1a992041.tihirvon@gmail.com","threadId":"11901","inReplyTo":"7vzluez9q9.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] Fix parsing numeric color values","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2008-02-06T12:16:08Z","receivedAt":"2008-02-06T12:16:08Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"Numeric color only worked if it was at end of line.\nNoticed by Chris Larson <clarson@kergoth.com>.\n\nSigned-off-by: Timo Hirvonen <tihirvon@gmail.com>\n---\n color.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/color.c b/color.c\nindex 7f66c29..cb70340 100644\n--- a/color.c\n+++ b/color.c\n@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)\n \t\t\treturn i - 1;\n \t}\n \ti = strtol(name, &end, 10);\n-\tif (*name && !*end && i >= -1 && i <= 255)\n+\tif (end - name == len && i >= -1 && i <= 255)\n \t\treturn i;\n \treturn -2;\n }\n-- \n1.5.4.1135.gae084\n"},{"id":"67664","messageId":"20080206141626.0f2b532c.tihirvon@gmail.com","threadId":"11901","inReplyTo":"7vzluez9q9.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Add tests for diff/status color parser","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2008-02-06T12:16:26Z","receivedAt":"2008-02-06T12:16:26Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>\n---\n I don't know if t4026-color.sh is good name for this. Feel free to change.\n\n t/t4026-color.sh |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 69 insertions(+), 0 deletions(-)\n create mode 100755 t/t4026-color.sh\n\ndiff --git a/t/t4026-color.sh b/t/t4026-color.sh\nnew file mode 100755\nindex 0000000..b61e516\n--- /dev/null\n+++ b/t/t4026-color.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Timo Hirvonen\n+#\n+\n+test_description='Test diff/status color escape codes'\n+. ./test-lib.sh\n+\n+color()\n+{\n+\tgit config diff.color.new \"$1\" &&\n+\ttest \"`git config --get-color diff.color.new`\" = \"\u001b$2\"\n+}\n+\n+invalid_color()\n+{\n+\tgit config diff.color.new \"$1\" &&\n+\ttest -z \"`git config --get-color diff.color.new 2>/dev/null`\"\n+}\n+\n+test_expect_success 'reset' '\n+\tcolor \"reset\" \"[m\"\n+'\n+\n+test_expect_success 'attribute before color name' '\n+\tcolor \"bold red\" \"[1;31m\"\n+'\n+\n+test_expect_success 'color name before attribute' '\n+\tcolor \"red bold\" \"[1;31m\"\n+'\n+\n+test_expect_success 'attr fg bg' '\n+\tcolor \"ul blue red\" \"[4;34;41m\"\n+'\n+\n+test_expect_success 'fg attr bg' '\n+\tcolor \"blue ul red\" \"[4;34;41m\"\n+'\n+\n+test_expect_success 'fg bg attr' '\n+\tcolor \"blue red ul\" \"[4;34;41m\"\n+'\n+\n+test_expect_success '256 colors' '\n+\tcolor \"254 bold 255\" \"[1;38;5;254;48;5;255m\"\n+'\n+\n+test_expect_success 'color too small' '\n+\tinvalid_color \"-2\"\n+'\n+\n+test_expect_success 'color too big' '\n+\tinvalid_color \"256\"\n+'\n+\n+test_expect_success 'extra character after color number' '\n+\tinvalid_color \"3X\"\n+'\n+\n+test_expect_success 'extra character after color name' '\n+\tinvalid_color \"redX\"\n+'\n+\n+test_expect_success 'extra character after attribute' '\n+\tinvalid_color \"dimX\"\n+'\n+\n+test_done\n-- \n1.5.4.1135.gae084\n"}]}