On Fri, Apr 08 2022, Raphael wrote: > On 06.04.22 23:16, Junio C Hamano wrote: > 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/ To be clear I meant there that at least 1/2 of what you were proposing was really a feature request. I.e. that we pro-actively work around a detected pager when coloring is still in effect when we hit a \n. > 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. *nod*, do we forsee any fallout from doing that where ANSI escapes now reach "across" lines for people who were relying on the previous behavior? I.e. you're changing how %Cred works, which is a synonym for %C(red). Perhaps this should be %C(red:across-nl). >> 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? Right now when we emit any color we do e.g.: %C(red)<thing>%C(reset) Where as what you're really asking for, if I'm understandng it correctly, is: %C(red)%C(save)<thing>%C(reset)%C(restore) Or equivalent, and then you'd like to have the vertical pipe (and anything else) that's printed at the beginning of a line to do the "restore". Is that correct? >> 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. See "test_decode_color" for numerous examples. Anyway, just take the above as suggestions. I really haven't looked deeply enough into this to form any sort of strong opinion. Except that I really think it would be useful to split up these logical changes, and have a smal series that: 1. Tests for the current behavior of both 2. Does just the "across lines" care, perhaps only if we detect less as a pager? 3. Does the "don't just reset, but reset back to what was the state one before the coloring preceding the reset" But now as I'm finishing up this E-Mail & testing your patch I was expecting that this would "keep" the color such that my %s would always be red, i.e. across the vertical bars: git log --oneline --graph --pretty=format:"%s => %Cred%aN <%aE>" Which it's not, but it is what we do before this patch with: git -c core.pager=cat -c color.ui=always log --oneline --pretty=format:"%s => %Cred%aN <%aE>" | head -n 3 But if I do it with your patch it does it for some things (the branch names) if I put %+d in there: git -c color.ui=always log --oneline --pretty=format:"%s => %Cred%aN%+d<%aE>" But still not the subjects? I'm also confused by: 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. Since I can't find any way to do this with --graph that'll emit coloring across the various colored bars it emits with your patch. Hrm, I partially take that back, it'll do it for the ref names, but not the subjects, so I suppose it's the same. :) Anyway, I think all of the above might make a good case that it would be really good to do this more incrementally, and with tests before/after for the various formats/resets etc.
