threads / patch / 11901

patchFix bug in parse_color that prevented the user from changing the background colors.

Subject: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.

## tl;dr

8 messages between Feb 5, 2008 and Feb 6, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Chris Larson· Feb 5, 2008, 17:34 UTC · lore

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.

Signed-off-by: Chris Larson <clarson@kergoth.com>
---
 color.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to color.c +1 −1
diff --git a/color.c b/color.c
index 7f66c29..62518fa 100644
--- a/color.c
+++ b/color.c
@@ -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;
 }

-- 
1.5.4.29.g43ce-dirty
Timo Hirvonen· Feb 5, 2008, 18:39 UTC · re: Chris Larson · lore

Re: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.

Chris Larson <clarson@kergoth.com> wrote:
Show 24 quoted lines
> 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.
> 
> Signed-off-by: Chris Larson <clarson@kergoth.com>
> ---
>  color.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/color.c b/color.c
> index 7f66c29..62518fa 100644
> --- a/color.c
> +++ b/color.c
> @@ -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.
Chris Larson· Feb 5, 2008, 18:48 UTC · lore

Re: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.

On Feb 5, 2008 11:39 AM, Timo Hirvonen <tihirvon@gmail.com> wrote:
Show 18 quoted lines
>
> Chris Larson <clarson@kergoth.com> wrote:
>
> > unable to convert the string in its entirety into an integer.
>
> > -    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)

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).

> 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,
-- 
Chris Larson - clarson at kergoth dot com
Dedicated Engineer - MontaVista - clarson at mvista dot com
Core Developer/Architect - TSLib, BitBake, OpenEmbedded, OpenZaurus
Timo Hirvonen· Feb 5, 2008, 18:58 UTC · lore

Re: [PATCH] Fix bug in parse_color that prevented the user from changing the background colors.

"Chris Larson" <clarson@kergoth.com> wrote:
Show 21 quoted lines
> On Feb 5, 2008 11:39 AM, Timo Hirvonen <tihirvon@gmail.com> wrote:
> 
> > Chris Larson <clarson@kergoth.com> wrote:
> >
> > unable to convert the string in its entirety into an integer.
> > > -    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)
> 
> 
> 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.

Show 6 quoted 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.
Timo Hirvonen· Feb 5, 2008, 19:18 UTC · re: Timo Hirvonen · lore

[PATCH] Fix parsing numeric color values

Fix bug reported by Chris Larson <clarson@kergoth.com>.  Numeric color
only worked if it was at end of line.
---
 color.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to color.c +1 −1
diff --git a/color.c b/color.c
index 7f66c29..3c999c3 100644
--- a/color.c
+++ b/color.c
@@ -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;
 }

-- 
1.5.4.1134.ge34cf-dirty
Junio C Hamano· Feb 6, 2008, 09:59 UTC · re: Timo Hirvonen · lore

Re: [PATCH] Fix parsing numeric color values

Timo Hirvonen <tihirvon@gmail.com> writes:
> Fix bug reported by Chris Larson <clarson@kergoth.com>.  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 <clarson@kergoth.com>.
Show 6 quoted lines
> @@ -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.

Timo Hirvonen· Feb 6, 2008, 12:16 UTC · re: Junio C Hamano · lore

[PATCH v2] Fix parsing numeric color values

Numeric color only worked if it was at end of line. Noticed by Chris Larson <clarson@kergoth.com>.

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 color.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to color.c +1 −1
diff --git a/color.c b/color.c
index 7f66c29..cb70340 100644
--- a/color.c
+++ b/color.c
@@ -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;
 }

-- 
1.5.4.1135.gae084
Timo Hirvonen· Feb 6, 2008, 12:16 UTC · re: Junio C Hamano · lore

[PATCH] Add tests for diff/status color parser

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 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
Show changes to t/t4026-color.sh +69 −0
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
new file mode 100755
index 0000000..b61e516
--- /dev/null
+++ b/t/t4026-color.sh
@@ -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

-- 
1.5.4.1135.gae084

← back to recent threads