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

[Raphael <raphael@pdev.de>]

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.