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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 6, 2022, 21:16 UTC
Message-ID: <xmqq8rshx687.fsf@gitster.g>
In-Reply-To: <20220405154529.966434-1-raphael@pdev.de>

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").

Show 10 quoted lines

> 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.