| Message ID | 20220405154529.966434-1-raphael@pdev.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pretty format: fix colors on %+ placeholder newline | expand |
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.
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@evledraar.gmail.com/ https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/ 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.
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@evledraar.gmail.com/ > https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/ 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.
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.
diff --git a/pretty.c b/pretty.c index ee6114e3f0..0aabba89ca 100644 --- a/pretty.c +++ b/pretty.c @@ -863,6 +863,7 @@ struct format_commit_context { size_t width, indent1, indent2; int auto_color; int padding; + char curr_color_escape[COLOR_MAXLEN]; /* These offsets are relative to the start of the commit message. */ struct chunk author; @@ -1050,6 +1051,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */ if (color_parse_mem(begin, end - begin, color) < 0) die(_("unable to parse --pretty format")); strbuf_addstr(sb, color); + strlcpy(c->curr_color_escape, color, COLOR_MAXLEN); return end - placeholder + 1; } @@ -1067,8 +1069,10 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */ else if (skip_prefix(placeholder + 1, "reset", &rest)) basic_color = GIT_COLOR_RESET; - if (basic_color && want_color(c->pretty_ctx->color)) + if (basic_color && want_color(c->pretty_ctx->color)) { strbuf_addstr(sb, basic_color); + strlcpy(c->curr_color_escape, basic_color, COLOR_MAXLEN); + } return rest - placeholder; } @@ -1360,8 +1364,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ case 'C': if (starts_with(placeholder + 1, "(auto)")) { c->auto_color = want_color(c->pretty_ctx->color); - if (c->auto_color && sb->len) + if (c->auto_color && sb->len) { strbuf_addstr(sb, GIT_COLOR_RESET); + c->curr_color_escape[0] = 0; + } return 7; /* consumed 7 bytes, "C(auto)" */ } else { int ret = parse_color(sb, placeholder, c); @@ -1813,9 +1819,11 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ while (sb->len && sb->buf[sb->len - 1] == '\n') strbuf_setlen(sb, sb->len - 1); } else if (orig_len != sb->len) { - if (magic == ADD_LF_BEFORE_NON_EMPTY) + if (magic == ADD_LF_BEFORE_NON_EMPTY) { strbuf_insertstr(sb, orig_len, "\n"); - else if (magic == ADD_SP_BEFORE_NON_EMPTY) + strbuf_insertstr(sb, orig_len + 1, + ((struct format_commit_context *)context)->curr_color_escape); + } else if (magic == ADD_SP_BEFORE_NON_EMPTY) strbuf_insertstr(sb, orig_len, " "); } return consumed + 1;