Fix bug in parse_color that prevented the user from changing the background colors.
PATCH 1/1 Fix bug in parse_color that prevented the user from changing the background colors.
The comments in color.c indicate that the syntax for the color options in the git config is [fg [bg]] [attr], however the implementation fails if strtol is unable to convert the string in its entirety into an integer.
---color.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
color.c +1 -1
@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)return i -1 ; } i =strtol (name, &end,10 ); -if (*name && !*end && i >=-1 && i <=255 ) +if (*name && i >=-1 && i <=255 )return i;return -2 ;
}
Signature · 1 line
1.5.4.29.g43ce-dirty
Quoted 16 lines · Chris Larson
The comments in
color.cindicate that the syntax for the color options in the
git config is [fg [bg]] [attr], however the implementation fails if strtol is
unable to convert the string in its entirety into an integer.Signed-off-by Chris Larson <[email protected]>--- color.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)color.c +0
-0
color.c static int parse_color(const char *name, int len) · line 23
Quoted 8 lines
color.c +1
-1@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)return i -1 ; } i =strtol (name, &end,10 ); -if (*name && !*end && i >=-1 && i <=255 ) +if (*name && i >=-1 && i <=255 )return i;return -2 ; }
My bug, sorry. I should have tested more.
I think this new code accepts "7bold" (didn't test). Maybe you should do something like this instead:
if (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)
Untested of course. BTW, your patch is whitespace corrupted.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 13 lines · Chris Larson
Quoted 11 lines · Timo Hirvonen
Chris Larson <[email protected]> wrote:
unable to convert the string in its entirety into an integer.
color.c static int parse_color(const char *name, int len)
Quoted 5 lines
color.c +1
-1-if (*name && !*end && i >=-1 && i <=255 ) +if (*name && i >=-1 && i <=255 )return i;return -2 ; }My bug, sorry. I should have tested more.
Quoted 8 lines
Quoted 4 lines
I think this new code accepts "7bold" (didn't test). Maybe you should do something like this instead:
if (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)Indeed, I should have tested more too :) Just starting to mess with the codebase, surprised at how clean it is (though I probably shouldn't be).
OK, did some testing.
The old code accepted attribute before color (bold red). With your patch that "bold" is treaded as fg color -1 and red as bg color, so you need that (!*end || isspace(*end)) test.
Quoted 6 lines
Quoted 2 lines
Untested of course. BTW, your patch is whitespace corrupted.
Unsurprising... sometimes I really hate gmail :| Anyone know if there's a way to post with gmail without hosing the patch, or should I switch to a non-web based solution?
I don't know. Maybe attachment is the only way.
PATCH 1/1 Fix parsing numeric color values
Fix bug reported by Chris Larson <[email protected]>. Numeric color only worked if it was at end of line.
--- color.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
color.c +1 -1
@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)return i -1 ; } i =strtol (name, &end,10 ); -if (*name && !*end && i >=-1 && i <=255 ) +if (*name && (!*end ||isspace (*end)) && i >=-1 && i <=255 )return i;return -2 ; }
Signature · 1 line
1.5.4.1134.ge34cf-dirty
Re: [PATCH] Fix parsing numeric color values
Quoted 2 lines · Timo Hirvonen
Fix bug reported by Chris Larson <[email protected]>. Numeric color only worked if it was at end of line.
Signoff?
It is much easier to read if you said that backwards:
Numeric color only worked if it was at end of line. Noticed by Chris Larson <[email protected]>.
color.c static int parse_color(const char *name, int len) · line 20
Quoted 6 lines
color.c +1
-1@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)return i -1 ; } i =strtol (name, &end,10 ); -if (*name && !*end && i >=-1 && i <=255 ) +if (*name && (!*end ||isspace (*end)) && i >=-1 && i <=255 )
Hmph. Is it the same as (end-name) == len?
Please add a test so that your fix won't be broken by others who might later touch this code.
PATCH v2 1/1 Fix parsing numeric color values
Numeric color only worked if it was at end of line. Noticed by Chris Larson <[email protected]>.
--- color.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
color.c +1 -1
@@ -17,7 +17,7 @@ static int parse_color(const char *name, int len)return i -1 ; } i =strtol (name, &end,10 ); -if (*name && !*end && i >=-1 && i <=255 ) +if (end - name == len && i >=-1 && i <=255 )return i;return -2 ; }
Signature · 1 line
1.5.4.1135.gae084
PATCH 1/1 Add tests for diff/status color parser
In reply to Junio C Hamano---
I don't know if t4026-color.sh is good name for this. Feel free to change.
t/t4026-color.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 69 insertions(+), 0 deletions(-) create mode 100755 t/t4026-color.sh
t/t4026-color.sh new file +69 -0
@@ -0,0 +1,69 @@ +#!/bin/sh +# +# Copyright (c) 2008 Timo Hirvonen +# + +test_description='Test diff/status color escape codes' +. ./test-lib.sh + +color() +{ + git config diff.color.new "$1" && + test "`git config --get-color diff.color.new`" = "$2" +} + +invalid_color() +{ + git config diff.color.new "$1" && + test -z "`git config --get-color diff.color.new 2>/dev/null`" +} + +test_expect_success 'reset' ' + color "reset" "[m" +' + +test_expect_success 'attribute before color name' ' + color "bold red" "[1;31m" +' + +test_expect_success 'color name before attribute' ' + color "red bold" "[1;31m" +' + +test_expect_success 'attr fg bg' ' + color "ul blue red" "[4;34;41m" +' + +test_expect_success 'fg attr bg' ' + color "blue ul red" "[4;34;41m" +' + +test_expect_success 'fg bg attr' ' + color "blue red ul" "[4;34;41m" +' + +test_expect_success '256 colors' ' + color "254 bold 255" "[1;38;5;254;48;5;255m" +' + +test_expect_success 'color too small' ' + invalid_color "-2" +' + +test_expect_success 'color too big' ' + invalid_color "256" +' + +test_expect_success 'extra character after color number' ' + invalid_color "3X" +' + +test_expect_success 'extra character after color name' ' + invalid_color "redX" +' + +test_expect_success 'extra character after attribute' ' + invalid_color "dimX" +' + +test_done
Signature · 1 line
1.5.4.1135.gae084
Quoted 12 lines · Timo Hirvonen
color.c static int parse_color(const char *name, int len)
Quoted 7 lines · Chris Larson
unable to convert the string in its entirety into an integer.
color.c +1
-1-if (*name && !*end && i >=-1 && i <=255 ) +if (*name && i >=-1 && i <=255 )return i;return -2 ; }
Quoted 6 lines
My bug, sorry. I should have tested more.
I think this new code accepts "7bold" (didn't test). Maybe you should do something like this instead:
if (*name && (!*end || isspace(*end)) && i >= -1 && i <= 255)
Indeed, I should have tested more too :) Just starting to mess with the codebase, surprised at how clean it is (though I probably shouldn't be).
Quoted 2 lines
Untested of course. BTW, your patch is whitespace corrupted.
Unsurprising... sometimes I really hate gmail :| Anyone know if there's a way to post with gmail without hosing the patch, or should I switch to a non-web based solution?
Thanks,
Signature · 3 lines
Chris Larson - clarson at kergoth dot com Dedicated Engineer - MontaVista - clarson at mvista dot com Core Developer/Architect - TSLib, BitBake, OpenEmbedded, OpenZaurus