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@xxxxxxxxxxxxxxxxxxx/
https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@xxxxxxx/
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.
