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

[Junio C Hamano <gitster@pobox.com>]

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.