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

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

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.