Pretty format color bug

Pretty format color bug

[Raphael Bauer]

Hello,

I think I found a minor formatting bug when using the custom pretty format:

* What did you do before the bug happened? (Steps to reproduce your issue)
git log --pretty=format:'%h%Cred%+d test'

* What did you expect to happen? (Expected behavior)
A listing of commit hashes and, if ref names for this commit exist, a 
second line.
This line should be colored in red and contain the ref names as well as 
the string 'test'.
In the case of no refs, the string 'test' should appear in line with the 
commit hash, also in red.

* What happened instead? (Actual behavior)
In case of ref names / a second line, the color is missing completely.
The colors work correctly for the single line case (when no ref names 
are available).

* What's different between what you expected and what actually happened?
The %+d placeholder inserts newlines if the string is non-empty, but in 
doing so, resets any coloring information.
This is demonstrated by the string 'test' which should always show in 
red, but does so only if %+d is not expanded.
This makes it currently impossible to color anything with the %+ 
placeholder.

[System Info]
git version:
git version 2.35.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.16.10-arch1-1 #1 SMP PREEMPT Wed, 16 Feb 2022 19:35:18 
+0000 x86_64
compiler info: gnuc: 11.1
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]

Re: Pretty format color bug

[Ævar Arnfjörð Bjarmason]

> I think I found a minor formatting bug when using the custom pretty format:
>
> * What did you do before the bug happened? (Steps to reproduce your issue)
> git log --pretty=format:'%h%Cred%+d test'
> [...]
> In case of ref names / a second line, the color is missing completely.

I think you've found a "bug" or unexpected feature, but it's not in git,
but in your pager.

Try:

    PAGER=cat git log --pretty=format:'%h%Cred%+d test'

I'm assuming you're using "less" and are about to read about its
behavior of resetting colors that are still in effect when it hits a
newline.

I hope that helps.

Re: Pretty format color bug

[Raphael Bauer]

> I think you've found a "bug" or unexpected feature, but it's not in git,
> but in your pager.
> 
> Try:
>      PAGER=cat git log --pretty=format:'%h%Cred%+d test'

Interesting, cat works correctly.
But this still fails for me:
PAGER=cat git log --pretty=format:'%h%Cred%+d test' --graph

The pipe symbol printed (because of --graph) for the second line is 
colored and resets the color of the refnames and 'test'.
Now this looks like a bug in git?

> I'm assuming you're using "less" and are about to read about its
> behavior of resetting colors that are still in effect when it hits a
> newline.

There is a section (-R) that mentions less assumes every line starts 
normal / uncolored. Shouldn't git follow this assumption when printing 
colorized to less?

BUG: %+ format placeholder and colors

[Raphael Bauer]

Hello,

I am not familiar with using mailing list for bug tracking. I already 
reported this bug, but got no answer, so I wanted to mention it again.

* What did you do before the bug happened? (Steps to reproduce your issue)
git log --pretty=format:'%h%Cred%+d test' --graph

This also happens with a PAGER=cat environment, just to preclude this is 
related to the pager.

* What did you expect to happen? (Expected behavior)
A graph listing of commit hashes and, if ref names for this commit 
exist, a second line.
This line should be colored in red and contain the ref names as well as 
the string 'test'.
In the case of no refs, the string 'test' should appear in line with the 
commit hash, also in red.

* What happened instead? (Actual behavior)
In case of ref names / a second line, the color is missing completely.
The colors work correctly for the single line case (when no ref names 
are available).

* What's different between what you expected and what actually happened?
The %+d placeholder inserts newlines if the string is non-empty, but in 
doing so, resets any coloring information.
This is demonstrated by the string 'test' which should always show in 
red, but does so only if %+d is not expanded.
This makes it currently impossible to color anything with the %+ 
placeholder.


If this is already tracked, I apologize.

Re: BUG: %+ format placeholder and colors

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 23 2022, Raphael Bauer wrote:

> Hello,
>
> I am not familiar with using mailing list for bug tracking. I already
> reported this bug, but got no answer, so I wanted to mention it again.

There was my answer at
https://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/
and your reply at
https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/;
Perhaps not the answer you wanted, but not "no answer"...

> * What did you do before the bug happened? (Steps to reproduce your issue)
> git log --pretty=format:'%h%Cred%+d test' --graph
>
> This also happens with a PAGER=cat environment, just to preclude this
> is related to the pager.

This example also renders differently under PAGER=cat for me. I.e. it's
the pager that's resetting the colors, but maybe I'm not understanding
the subtlety here...

> * What did you expect to happen? (Expected behavior)
> A graph listing of commit hashes and, if ref names for this commit
> exist, a second line.
> This line should be colored in red and contain the ref names as well
> as the string 'test'.
> In the case of no refs, the string 'test' should appear in line with
> the commit hash, also in red.
>
> * What happened instead? (Actual behavior)
> In case of ref names / a second line, the color is missing completely.
> The colors work correctly for the single line case (when no ref names
> are available).
>
> * What's different between what you expected and what actually happened?
> The %+d placeholder inserts newlines if the string is non-empty, but
> in doing so, resets any coloring information.
> This is demonstrated by the string 'test' which should always show in
> red, but does so only if %+d is not expanded.
> This makes it currently impossible to color anything with the %+
> placeholder.

At least for me a screenshot & a link to a repo you expect to run this
on would really help, since the output is very different depending on
branch topology, ref names etc.

Re: BUG: %+ format placeholder and colors

[Raphael]

 > At least for me a screenshot & a link to a repo you expect to run this
 > on would really help, since the output is very different depending on
 > branch topology, ref names etc.

I believe the branch topology is not relevant to reproduce this bug, it 
happens in my small demo repo with two commits as well as big repos.

I uploaded a screenshot at https://qu.ax/Mzuv.png
The key is that lines starting with the pipe symbol should have their 
text printed in red, which does not happen.

The general idea is to highlight when a commit has refnames, which I 
want to print in an optional second line and colorized. The latter seems 
not possible currently.

The exact command used was:
git log --pretty=format:'%h %s%Cred%+d test' --graph

[PATCH] pretty format: fix colors on %+ placeholder newline

[Raphael Bauer]

Previously, the color escape codes were not printed again when a %+
placeholder was expanded, which could lead to missing colors.

In particular, the following command on any commit history exercised the
problem:

git log --pretty=format:'%h%Cred%+d test' --graph

The string 'test' should always be in red, but is not when commits have
ref names associated and the %+d placeholder is expanded.
This is also not a problem of any pager since the --graph option adds a
colored pipe symbol at the beginning of the line, which makes re-setting
the color again afterwards necessary.
---
 pretty.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index ee6114e3f0..0aabba89ca 100644
--- a/pretty.c
+++ b/pretty.c
@@ -863,6 +863,7 @@ struct format_commit_context {
 	size_t width, indent1, indent2;
 	int auto_color;
 	int padding;
+	char curr_color_escape[COLOR_MAXLEN];
 
 	/* These offsets are relative to the start of the commit message. */
 	struct chunk author;
@@ -1050,6 +1051,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
 		if (color_parse_mem(begin, end - begin, color) < 0)
 			die(_("unable to parse --pretty format"));
 		strbuf_addstr(sb, color);
+		strlcpy(c->curr_color_escape, color, COLOR_MAXLEN);
 		return end - placeholder + 1;
 	}
 
@@ -1067,8 +1069,10 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
 	else if (skip_prefix(placeholder + 1, "reset", &rest))
 		basic_color = GIT_COLOR_RESET;
 
-	if (basic_color && want_color(c->pretty_ctx->color))
+	if (basic_color && want_color(c->pretty_ctx->color)) {
 		strbuf_addstr(sb, basic_color);
+		strlcpy(c->curr_color_escape, basic_color, COLOR_MAXLEN);
+	}
 
 	return rest - placeholder;
 }
@@ -1360,8 +1364,10 @@ 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 && sb->len)
+			if (c->auto_color && sb->len) {
 				strbuf_addstr(sb, GIT_COLOR_RESET);
+				c->curr_color_escape[0] = 0;
+			}
 			return 7; /* consumed 7 bytes, "C(auto)" */
 		} else {
 			int ret = parse_color(sb, placeholder, c);
@@ -1813,9 +1819,11 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 		while (sb->len && sb->buf[sb->len - 1] == '\n')
 			strbuf_setlen(sb, sb->len - 1);
 	} else if (orig_len != sb->len) {
-		if (magic == ADD_LF_BEFORE_NON_EMPTY)
+		if (magic == ADD_LF_BEFORE_NON_EMPTY) {
 			strbuf_insertstr(sb, orig_len, "\n");
-		else if (magic == ADD_SP_BEFORE_NON_EMPTY)
+			strbuf_insertstr(sb, orig_len + 1,
+				((struct format_commit_context *)context)->curr_color_escape);
+		} else if (magic == ADD_SP_BEFORE_NON_EMPTY)
 			strbuf_insertstr(sb, orig_len, " ");
 	}
 	return consumed + 1;
-- 
2.35.1

Re: [PATCH] pretty format: fix colors on %+ placeholder newline

[Junio C Hamano]

Raphael Bauer <raphael@pdev.de> writes:

> Previously, the color escape codes were not printed again when a %+
> placeholder was expanded, which could lead to missing colors.

It is very good to start the proposed log message by describing the
current behaviour, highlighting what problem it has.  Because the
message is merely _proposing_ to make this change, which may or may
not be accepted into the codebase, however, please describe the
status quo in the present tense.  "We behave this way", not "we used
to behave this way".  There is no need to say "Currently" (and it is
simply misleading to say "Previously").

> In particular, the following command on any commit history exercised the
> problem:
>
> git log --pretty=format:'%h%Cred%+d test' --graph
>
> The string 'test' should always be in red, but is not when commits have
> ref names associated and the %+d placeholder is expanded.
> This is also not a problem of any pager since the --graph option adds a
> colored pipe symbol at the beginning of the line, which makes re-setting
> the color again afterwards necessary.

Isn't it the problem with the "--graph" codepath, then?

It is unclear from the proposed log message why it is a good idea to
add the new behaviour to the code that handles "%+" unconditionally.
It also is unclear why the new behaviour to save only one "color
escape" is sufficient.  For example, if we used

    git log --pretty='format:%h%C(green)%C(reverse)%+d test' --graph

wouldn't the effects of these two add up?  Would it be sufficient to
remember the last one we saw and re-enable it?

Combining the above two together, it makes me wonder if the right
approach is instead to (1) stop resetting at the end of --graph, and
(2) instead "save" the attribute before starting to draw --graph,
draw the --graph in color, and then "restore" the attribute.

Whatever approach we decide to take to solve this issue, let's have
a test case or two added to the test suite to better document the
issue.

Thanks.

Re: [PATCH] pretty format: fix colors on %+ placeholder newline

[Raphael]

On 06.04.22 23:16, Junio C Hamano wrote:
> It is very good to start the proposed log message by describing the
> current behaviour, highlighting what problem it has.  Because the
> message is merely _proposing_ to make this change, which may or may
> not be accepted into the codebase, however, please describe the
> status quo in the present tense. 
I understand that for proposing changes this makes sense.
But if the change would be accepted into the code, wouldn't the text 
become the commit message? I assumed it is sensible to write the message 
for that case: Using the present tense for the state of the code at this 
commit / after the patch.

> Isn't it the problem with the "--graph" codepath, then?
> 
> It is unclear from the proposed log message why it is a good idea to
> add the new behaviour to the code that handles "%+" unconditionally.

Good point, let me explain my thinking:
I first reported this bug without the --graph option where the color on 
the second line is missing as well.
It was pointed out that this is a problem with the pager "less" and not 
a bug in git:
https://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/
https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/

Using "cat" as a pager produces the correct coloring, but since "less" 
is the default pager I find it useful to follow its conventions: Namely 
that lines are started non-colored and escape sequences must be repeated 
at the beginning of each colored line.
This is achieved as well by my implementation.

> It also is unclear why the new behaviour to save only one "color
> escape" is sufficient.  For example, if we used
> 
>      git log --pretty='format:%h%C(green)%C(reverse)%+d test' --graph
> 
> wouldn't the effects of these two add up?

You are right, I forgot about this case.
A naive solution would be to concatenate the format escapes and clearing 
the string when the color is reset.
Is there already existing code for keeping track of which format strings 
override each other, so that only the required escape sequences must be 
stored/printed?
Or do you see a different, more elegant solution?

> Whatever approach we decide to take to solve this issue, let's have
> a test case or two added to the test suite to better document the
> issue.

Sure, I will take a look after solving the core issue.

Re: [PATCH] pretty format: fix colors on %+ placeholder newline

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 08 2022, Raphael wrote:

> On 06.04.22 23:16, Junio C Hamano wrote:
> Good point, let me explain my thinking:
> I first reported this bug without the --graph option where the color
> on the second line is missing as well.
> It was pointed out that this is a problem with the pager "less" and
> not a bug in git:
> https://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/
> https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/

To be clear I meant there that at least 1/2 of what you were proposing
was really a feature request. I.e. that we pro-actively work around a
detected pager when coloring is still in effect when we hit a \n.

> Using "cat" as a pager produces the correct coloring, but since "less"
> is the default pager I find it useful to follow its conventions:
> Namely that lines are started non-colored and escape sequences must be
> repeated at the beginning of each colored line.
> This is achieved as well by my implementation.

*nod*, do we forsee any fallout from doing that where ANSI escapes now
reach "across" lines for people who were relying on the previous
behavior?

I.e. you're changing how %Cred works, which is a synonym for
%C(red). Perhaps this should be %C(red:across-nl).

>> It also is unclear why the new behaviour to save only one "color
>> escape" is sufficient.  For example, if we used
>>      git log --pretty='format:%h%C(green)%C(reverse)%+d test'
>> --graph
>> wouldn't the effects of these two add up?
>
> You are right, I forgot about this case.
> A naive solution would be to concatenate the format escapes and
> clearing the string when the color is reset.
> Is there already existing code for keeping track of which format
> strings override each other, so that only the required escape
> sequences must be stored/printed?
> Or do you see a different, more elegant solution?

Right now when we emit any color we do e.g.:

    %C(red)<thing>%C(reset)

Where as what you're really asking for, if I'm understandng it
correctly, is:

    %C(red)%C(save)<thing>%C(reset)%C(restore)

Or equivalent, and then you'd like to have the vertical pipe (and
anything else) that's printed at the beginning of a line to do the
"restore". Is that correct?

>> Whatever approach we decide to take to solve this issue, let's have
>> a test case or two added to the test suite to better document the
>> issue.
>
> Sure, I will take a look after solving the core issue.

See "test_decode_color" for numerous examples.

Anyway, just take the above as suggestions. I really haven't looked
deeply enough into this to form any sort of strong opinion.

Except that I really think it would be useful to split up these logical
changes, and have a smal series that:

 1. Tests for the current behavior of both
 2. Does just the "across lines" care, perhaps only if we detect less as a pager?
 3. Does the "don't just reset, but reset back to what was the state one
    before the coloring preceding the reset"

But now as I'm finishing up this E-Mail & testing your patch I was
expecting that this would "keep" the color such that my %s would always
be red, i.e. across the vertical bars:

	git log --oneline --graph --pretty=format:"%s => %Cred%aN <%aE>"

Which it's not, but it is what we do before this patch with:

    git -c core.pager=cat -c color.ui=always log --oneline --pretty=format:"%s => %Cred%aN <%aE>" | head -n 3

But if I do it with your patch it does it for some things (the branch
names) if I put %+d in there:

    git -c color.ui=always log --oneline --pretty=format:"%s => %Cred%aN%+d<%aE>"

But still not the subjects? I'm also confused by:
	
	This is also not a problem of any pager since the --graph option adds a
	colored pipe symbol at the beginning of the line, which makes re-setting
	the color again afterwards necessary.

Since I can't find any way to do this with --graph that'll emit coloring
across the various colored bars it emits with your patch.

Hrm, I partially take that back, it'll do it for the ref names, but not
the subjects, so I suppose it's the same.

:)

Anyway, I think all of the above might make a good case that it would be
really good to do this more incrementally, and with tests before/after
for the various formats/resets etc.

Re: [PATCH] pretty format: fix colors on %+ placeholder newline

[Junio C Hamano]

Raphael <raphael@pdev.de> writes:

> On 06.04.22 23:16, Junio C Hamano wrote:
>> It is very good to start the proposed log message by describing the
>> current behaviour, highlighting what problem it has.  Because the
>> message is merely _proposing_ to make this change, which may or may
>> not be accepted into the codebase, however, please describe the
>> status quo in the present tense. 
> I understand that for proposing changes this makes sense.

OK

> But if the change would be accepted into the code, wouldn't the text
> become the commit message?

Yes.  Our history records what proposals were made in the past, and
"git log" is the way to learn about them.

> I assumed it is sensible to write the
> message for that case: Using the present tense for the state of the
> code at this commit / after the patch.

I was not saying what you did was unconditionally wrong.  I
understand that you wrote in the past tense with good intentions.
The conventions are different from project and project, and I was
merely telling you what the project convention around here is.

> Using "cat" as a pager produces the correct coloring, but since "less"
> is the default pager I find it useful to follow its conventions:
> Namely that lines are started non-colored and escape sequences must be
> repeated at the beginning of each colored line.

Even if it may be a problem with the pagers, we are OK to help such
pagers if the workaround we need to make is reasonable and does not
involve too much 'bending over backwards' work.  That much we have
already established before seeing this patch in the previous
discussion thread on this, I thought ;-)

But this is a behaviour change, if not limited to the --graph thing,
that probably would affect other usecases negatively.

>> It also is unclear why the new behaviour to save only one "color
>> escape" is sufficient.  For example, if we used
>>      git log --pretty='format:%h%C(green)%C(reverse)%+d test'
>> --graph
>> wouldn't the effects of these two add up?
>
> You are right, I forgot about this case.
> A naive solution would be to concatenate the format escapes and
> clearing the string when the color is reset.

There is vt100/ansi sequence to save/restore attributes, no?  I am
not sure if it passes the "reasonable and not too much" bar if we
have to save all the previous attribute requests ourselves.