* Pretty format color bug @ 2022-02-21 22:38 Raphael Bauer 2022-02-21 23:29 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 11+ messages in thread From: Raphael Bauer @ 2022-02-21 22:38 UTC (permalink / raw) To: git Hello, I think I found a minor formatting bug when using the custom pretty format: * What did you do before the bug happened? (Steps to reproduce your issue) git log --pretty=format:'%h%Cred%+d test' * What did you expect to happen? (Expected behavior) A listing of commit hashes and, if ref names for this commit exist, a second line. This line should be colored in red and contain the ref names as well as the string 'test'. In the case of no refs, the string 'test' should appear in line with the commit hash, also in red. * What happened instead? (Actual behavior) In case of ref names / a second line, the color is missing completely. The colors work correctly for the single line case (when no ref names are available). * What's different between what you expected and what actually happened? The %+d placeholder inserts newlines if the string is non-empty, but in doing so, resets any coloring information. This is demonstrated by the string 'test' which should always show in red, but does so only if %+d is not expanded. This makes it currently impossible to color anything with the %+ placeholder. [System Info] git version: git version 2.35.1 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 5.16.10-arch1-1 #1 SMP PREEMPT Wed, 16 Feb 2022 19:35:18 +0000 x86_64 compiler info: gnuc: 11.1 libc info: glibc: 2.35 $SHELL (typically, interactive shell): /bin/bash [Enabled Hooks] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Pretty format color bug 2022-02-21 22:38 Pretty format color bug Raphael Bauer @ 2022-02-21 23:29 ` Ævar Arnfjörð Bjarmason 2022-02-22 0:15 ` Raphael Bauer 0 siblings, 1 reply; 11+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-02-21 23:29 UTC (permalink / raw) To: Raphael Bauer; +Cc: Git ML > I think I found a minor formatting bug when using the custom pretty format: > > * What did you do before the bug happened? (Steps to reproduce your issue) > git log --pretty=format:'%h%Cred%+d test' > [...] > In case of ref names / a second line, the color is missing completely. I think you've found a "bug" or unexpected feature, but it's not in git, but in your pager. Try: PAGER=cat git log --pretty=format:'%h%Cred%+d test' I'm assuming you're using "less" and are about to read about its behavior of resetting colors that are still in effect when it hits a newline. I hope that helps. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Pretty format color bug 2022-02-21 23:29 ` Ævar Arnfjörð Bjarmason @ 2022-02-22 0:15 ` Raphael Bauer 2022-03-22 23:42 ` BUG: %+ format placeholder and colors Raphael Bauer 0 siblings, 1 reply; 11+ messages in thread From: Raphael Bauer @ 2022-02-22 0:15 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Git ML > I think you've found a "bug" or unexpected feature, but it's not in git, > but in your pager. > > Try: > PAGER=cat git log --pretty=format:'%h%Cred%+d test' Interesting, cat works correctly. But this still fails for me: PAGER=cat git log --pretty=format:'%h%Cred%+d test' --graph The pipe symbol printed (because of --graph) for the second line is colored and resets the color of the refnames and 'test'. Now this looks like a bug in git? > I'm assuming you're using "less" and are about to read about its > behavior of resetting colors that are still in effect when it hits a > newline. There is a section (-R) that mentions less assumes every line starts normal / uncolored. Shouldn't git follow this assumption when printing colorized to less? ^ permalink raw reply [flat|nested] 11+ messages in thread
* BUG: %+ format placeholder and colors 2022-02-22 0:15 ` Raphael Bauer @ 2022-03-22 23:42 ` Raphael Bauer 2022-03-23 2:49 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 11+ messages in thread From: Raphael Bauer @ 2022-03-22 23:42 UTC (permalink / raw) To: Git ML Hello, I am not familiar with using mailing list for bug tracking. I already reported this bug, but got no answer, so I wanted to mention it again. * What did you do before the bug happened? (Steps to reproduce your issue) git log --pretty=format:'%h%Cred%+d test' --graph This also happens with a PAGER=cat environment, just to preclude this is related to the pager. * What did you expect to happen? (Expected behavior) A graph listing of commit hashes and, if ref names for this commit exist, a second line. This line should be colored in red and contain the ref names as well as the string 'test'. In the case of no refs, the string 'test' should appear in line with the commit hash, also in red. * What happened instead? (Actual behavior) In case of ref names / a second line, the color is missing completely. The colors work correctly for the single line case (when no ref names are available). * What's different between what you expected and what actually happened? The %+d placeholder inserts newlines if the string is non-empty, but in doing so, resets any coloring information. This is demonstrated by the string 'test' which should always show in red, but does so only if %+d is not expanded. This makes it currently impossible to color anything with the %+ placeholder. If this is already tracked, I apologize. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: BUG: %+ format placeholder and colors 2022-03-22 23:42 ` BUG: %+ format placeholder and colors Raphael Bauer @ 2022-03-23 2:49 ` Ævar Arnfjörð Bjarmason 2022-03-23 18:12 ` Raphael 0 siblings, 1 reply; 11+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-23 2:49 UTC (permalink / raw) To: Raphael Bauer; +Cc: Git ML On Wed, Mar 23 2022, Raphael Bauer wrote: > Hello, > > I am not familiar with using mailing list for bug tracking. I already > reported this bug, but got no answer, so I wanted to mention it again. There was my answer at https://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/ and your reply at https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/; Perhaps not the answer you wanted, but not "no answer"... > * What did you do before the bug happened? (Steps to reproduce your issue) > git log --pretty=format:'%h%Cred%+d test' --graph > > This also happens with a PAGER=cat environment, just to preclude this > is related to the pager. This example also renders differently under PAGER=cat for me. I.e. it's the pager that's resetting the colors, but maybe I'm not understanding the subtlety here... > * What did you expect to happen? (Expected behavior) > A graph listing of commit hashes and, if ref names for this commit > exist, a second line. > This line should be colored in red and contain the ref names as well > as the string 'test'. > In the case of no refs, the string 'test' should appear in line with > the commit hash, also in red. > > * What happened instead? (Actual behavior) > In case of ref names / a second line, the color is missing completely. > The colors work correctly for the single line case (when no ref names > are available). > > * What's different between what you expected and what actually happened? > The %+d placeholder inserts newlines if the string is non-empty, but > in doing so, resets any coloring information. > This is demonstrated by the string 'test' which should always show in > red, but does so only if %+d is not expanded. > This makes it currently impossible to color anything with the %+ > placeholder. At least for me a screenshot & a link to a repo you expect to run this on would really help, since the output is very different depending on branch topology, ref names etc. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: BUG: %+ format placeholder and colors 2022-03-23 2:49 ` Ævar Arnfjörð Bjarmason @ 2022-03-23 18:12 ` Raphael 2022-04-05 15:45 ` [PATCH] pretty format: fix colors on %+ placeholder newline Raphael Bauer 0 siblings, 1 reply; 11+ messages in thread From: Raphael @ 2022-03-23 18:12 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Git ML > At least for me a screenshot & a link to a repo you expect to run this > on would really help, since the output is very different depending on > branch topology, ref names etc. I believe the branch topology is not relevant to reproduce this bug, it happens in my small demo repo with two commits as well as big repos. I uploaded a screenshot at https://qu.ax/Mzuv.png The key is that lines starting with the pipe symbol should have their text printed in red, which does not happen. The general idea is to highlight when a commit has refnames, which I want to print in an optional second line and colorized. The latter seems not possible currently. The exact command used was: git log --pretty=format:'%h %s%Cred%+d test' --graph ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] pretty format: fix colors on %+ placeholder newline 2022-03-23 18:12 ` Raphael @ 2022-04-05 15:45 ` Raphael Bauer 2022-04-06 21:16 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Raphael Bauer @ 2022-04-05 15:45 UTC (permalink / raw) To: git; +Cc: Raphael Bauer Previously, the color escape codes were not printed again when a %+ placeholder was expanded, which could lead to missing colors. 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. --- pretty.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) 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; -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] pretty format: fix colors on %+ placeholder newline 2022-04-05 15:45 ` [PATCH] pretty format: fix colors on %+ placeholder newline Raphael Bauer @ 2022-04-06 21:16 ` Junio C Hamano 2022-04-08 13:18 ` Raphael 0 siblings, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2022-04-06 21:16 UTC (permalink / raw) To: Raphael Bauer; +Cc: git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pretty format: fix colors on %+ placeholder newline 2022-04-06 21:16 ` Junio C Hamano @ 2022-04-08 13:18 ` Raphael 2022-04-08 15:14 ` Ævar Arnfjörð Bjarmason 2022-04-08 23:40 ` Junio C Hamano 0 siblings, 2 replies; 11+ messages in thread From: Raphael @ 2022-04-08 13:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pretty format: fix colors on %+ placeholder newline 2022-04-08 13:18 ` Raphael @ 2022-04-08 15:14 ` Ævar Arnfjörð Bjarmason 2022-04-08 23:40 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-08 15:14 UTC (permalink / raw) To: Raphael; +Cc: Junio C Hamano, git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pretty format: fix colors on %+ placeholder newline 2022-04-08 13:18 ` Raphael 2022-04-08 15:14 ` Ævar Arnfjörð Bjarmason @ 2022-04-08 23:40 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2022-04-08 23:40 UTC (permalink / raw) To: Raphael; +Cc: git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-08 23:40 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-21 22:38 Pretty format color bug Raphael Bauer 2022-02-21 23:29 ` Ævar Arnfjörð Bjarmason 2022-02-22 0:15 ` Raphael Bauer 2022-03-22 23:42 ` BUG: %+ format placeholder and colors Raphael Bauer 2022-03-23 2:49 ` Ævar Arnfjörð Bjarmason 2022-03-23 18:12 ` Raphael 2022-04-05 15:45 ` [PATCH] pretty format: fix colors on %+ placeholder newline Raphael Bauer 2022-04-06 21:16 ` Junio C Hamano 2022-04-08 13:18 ` Raphael 2022-04-08 15:14 ` Ævar Arnfjörð Bjarmason 2022-04-08 23:40 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).