Two bugs in --pretty with %C(auto)

Two bugs in --pretty with %C(auto)

[Anatoly Borodin]

Hi All!

First bug:

	git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'

prints %h with the default color (normal yellow), but

	git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'

shows %h with bold yellow, as if only the color was reset, but not
the attributes (blink, ul, reverse also work this way). %d and %s are
printed with the right color both times.

Second bug, maybe related to the first one:

	git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'

The first line looks as expected. Well, almost: the '(' of %d is bold
yellow.

The second line looks like this:

* %h, %s, %an with bold cyan;
* %h with bold yellow;
* %h with normal yellow and %s with normal white (default colors).

PS git version 2.9.2

-- 
Mit freundlichen Grüßen,
Anatoly Borodin

Re: Two bugs in --pretty with %C(auto)

[René Scharfe]

Am 17.09.2016 um 14:51 schrieb Anatoly Borodin:
> Hi All!
> 
> First bug:
> 
> 	git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'
> 
> prints %h with the default color (normal yellow), but
> 
> 	git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'
> 
> shows %h with bold yellow, as if only the color was reset, but not
> the attributes (blink, ul, reverse also work this way). %d and %s are
> printed with the right color both times.
> 
> Second bug, maybe related to the first one:
> 
> 	git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'
> 
> The first line looks as expected. Well, almost: the '(' of %d is bold
> yellow.
> 
> The second line looks like this:
> 
> * %h, %s, %an with bold cyan;
> * %h with bold yellow;
> * %h with normal yellow and %s with normal white (default colors).
> 
> PS git version 2.9.2

Well, in both cases you could add %Creset before %C(auto) to get what
you want.

I'm not sure how just how automatic %C(auto) is supposed to be, but you
expected it do emit the reset for you, right?  Sounds reasonable to me.
The following patch implements that behavior.

Duy, what do you think?

-- >8 --
Subject: pretty: let %C(auto) reset all attributes

Reset colors and attributes upon %C(auto) to enable full automatic
control over them; otherwise attributes like bold or reverse could
still be in effect from previous %C placeholders.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 pretty.c                   | 2 ++
 t/t6006-rev-list-format.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 9788bd8..493edb0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1072,6 +1072,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	case 'C':
 		if (starts_with(placeholder + 1, "(auto)")) {
 			c->auto_color = want_color(c->pretty_ctx->color);
+			if (c->auto_color)
+				strbuf_addstr(sb, GIT_COLOR_RESET);
 			return 7; /* consumed 7 bytes, "C(auto)" */
 		} else {
 			int ret = parse_color(sb, placeholder, c);
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index a1dcdb8..f6020cd 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
 
 test_expect_success '%C(auto) respects --color' '
 	git log --color --format="%C(auto)%H" -1 >actual &&
-	printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+	printf "\\033[m\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
 	test_cmp expect actual
 '
 
-- 
2.10.0

Re: Two bugs in --pretty with %C(auto)

[Anatoly Borodin]

Hi René!


On Sat, Sep 17, 2016 at 8:25 PM, René Scharfe <l.s.r@web.de> wrote:
> I'm not sure how just how automatic %C(auto) is supposed to be, but you
> expected it do emit the reset for you, right?  Sounds reasonable to me.

I don't see a good reason not to do so. Spare some bytes?..

> The following patch implements that behavior.

Thanks, the patch works great!


-- 
Mit freundlichen Grüßen,
Anatoly Borodin

Re: Two bugs in --pretty with %C(auto)

[René Scharfe]

Am 18.09.2016 um 14:30 schrieb Anatoly Borodin:
> On Sat, Sep 17, 2016 at 8:25 PM, René Scharfe <l.s.r@web.de> wrote:
>> I'm not sure how just how automatic %C(auto) is supposed to be, but you
>> expected it do emit the reset for you, right?  Sounds reasonable to me.
>
> I don't see a good reason not to do so. Spare some bytes?..

The states for colors and attributes are separate; that's how the bold 
attribute bled into your the auto-colored parts of your output.  You 
could use that property to specify e.g. "give me automatic coloring, but 
reverse it".

This only works by accident now, I think.  Full resets are emitted after 
many placeholders, so attributes don't reach very far in practice.  We'd 
have to be more careful with these full resets if we'd want attributes 
to cover multiple placeholders.  An automatic reset at the start of 
%C(auto) would go into the opposite direction.

René

Re: Two bugs in --pretty with %C(auto)

[Duy Nguyen]

On Sun, Sep 18, 2016 at 1:25 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 17.09.2016 um 14:51 schrieb Anatoly Borodin:
>> Hi All!
>>
>> First bug:
>>
>>       git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'
>>
>> prints %h with the default color (normal yellow), but
>>
>>       git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'
>>
>> shows %h with bold yellow, as if only the color was reset, but not
>> the attributes (blink, ul, reverse also work this way). %d and %s are
>> printed with the right color both times.
>>
>> Second bug, maybe related to the first one:
>>
>>       git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'
>>
>> The first line looks as expected. Well, almost: the '(' of %d is bold
>> yellow.
>>
>> The second line looks like this:
>>
>> * %h, %s, %an with bold cyan;
>> * %h with bold yellow;
>> * %h with normal yellow and %s with normal white (default colors).
>>
>> PS git version 2.9.2
>
> Well, in both cases you could add %Creset before %C(auto) to get what
> you want.
>
> I'm not sure how just how automatic %C(auto) is supposed to be, but you
> expected it do emit the reset for you, right?  Sounds reasonable to me.
> The following patch implements that behavior.
>
> Duy, what do you think?

Even though letting some attributes before %C(auto) through sounds
interesting, I'd say it's a bit unpredictable, especially when the
main usage of %C(auto) is %d which could use plenty of colors. So yes,
your changes look good.
-- 
Duy

Re: Two bugs in --pretty with %C(auto)

[René Scharfe]

Am 17.09.2016 um 20:25 schrieb René Scharfe:
> diff --git a/pretty.c b/pretty.c
> index 9788bd8..493edb0 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1072,6 +1072,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  	case 'C':
>  		if (starts_with(placeholder + 1, "(auto)")) {
>  			c->auto_color = want_color(c->pretty_ctx->color);
> +			if (c->auto_color)
> +				strbuf_addstr(sb, GIT_COLOR_RESET);
>  			return 7; /* consumed 7 bytes, "C(auto)" */
>  		} else {
>  			int ret = parse_color(sb, placeholder, c);

We could optimize this a bit (see below).  I can't think of a downside;
someone adding a prefix would be responsible for adding a reset as well
if needed, right?

-- >8 --
Subject: [PATCH] pretty: avoid adding reset for %C(auto) if output is empty

We emit an escape sequence for resetting color and attribute for
%C(auto) to make sure automatic coloring is displayed as intended.
Stop doing that if the output strbuf is empty, i.e. when %C(auto)
appears at the start of the format string, because then there is no
need for a reset and we save a few bytes in the output.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Reverts the change to t6006, so we'd need another test for this.
Anatoly? :)

 pretty.c                   | 2 +-
 t/t6006-rev-list-format.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pretty.c b/pretty.c
index 493edb0..25efbca 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1072,7 +1072,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	case 'C':
 		if (starts_with(placeholder + 1, "(auto)")) {
 			c->auto_color = want_color(c->pretty_ctx->color);
-			if (c->auto_color)
+			if (c->auto_color && sb->len)
 				strbuf_addstr(sb, GIT_COLOR_RESET);
 			return 7; /* consumed 7 bytes, "C(auto)" */
 		} else {
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index f6020cd..a1dcdb8 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
 
 test_expect_success '%C(auto) respects --color' '
 	git log --color --format="%C(auto)%H" -1 >actual &&
-	printf "\\033[m\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+	printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
 	test_cmp expect actual
 '
 
-- 
2.10.0

Re: Two bugs in --pretty with %C(auto)

[Anatoly Borodin]

Hi René,


On Thu, Sep 29, 2016 at 8:13 PM, René Scharfe <l.s.r@web.de> wrote:
> Reverts the change to t6006, so we'd need another test for this.
> Anatoly? :)

I've checked my example commands and couldn't find any regressions.

-- 
Mit freundlichen Grüßen,
Anatoly Borodin