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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.