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

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

[Chris Larson]

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;
 }
-- 
1.5.4.29.g43ce-dirty

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

[Timo Hirvonen]

Chris Larson <clarson@kergoth.com> wrote:

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

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

[Chris Larson]

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

> 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

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

[Timo Hirvonen]

"Chris Larson" <clarson@kergoth.com> wrote:

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

> > 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] Fix parsing numeric color values

[Timo Hirvonen]

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

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

Re: [PATCH] Fix parsing numeric color values

[Junio C Hamano]

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

> @@ -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] Fix parsing numeric color values

[Timo Hirvonen]

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

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

[PATCH] Add tests for diff/status color parser

[Timo Hirvonen]

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

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