Re: Two bugs in --pretty with %C(auto)
Hi René, On Thu, Sep 29, 2016 at 8:13 PM, René Scharfewrote: > Reverts the change to t6006, so we'd need another test for this. > Anatoly? :) I've checked my example commands and couldn't find any regressions. -- Mit freundlichen Grüßen, Anatoly Borodin
Re: Two bugs in --pretty with %C(auto)
Am 17.09.2016 um 20:25 schrieb René Scharfe: > diff --git a/pretty.c b/pretty.c > index 9788bd8..493edb0 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1072,6 +1072,8 @@ 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) > + strbuf_addstr(sb, GIT_COLOR_RESET); > return 7; /* consumed 7 bytes, "C(auto)" */ > } else { > int ret = parse_color(sb, placeholder, c); We could optimize this a bit (see below). I can't think of a downside; someone adding a prefix would be responsible for adding a reset as well if needed, right? -- >8 -- Subject: [PATCH] pretty: avoid adding reset for %C(auto) if output is empty We emit an escape sequence for resetting color and attribute for %C(auto) to make sure automatic coloring is displayed as intended. Stop doing that if the output strbuf is empty, i.e. when %C(auto) appears at the start of the format string, because then there is no need for a reset and we save a few bytes in the output. Signed-off-by: Rene Scharfe--- Reverts the change to t6006, so we'd need another test for this. Anatoly? :) pretty.c | 2 +- t/t6006-rev-list-format.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pretty.c b/pretty.c index 493edb0..25efbca 100644 --- a/pretty.c +++ b/pretty.c @@ -1072,7 +1072,7 @@ 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) + if (c->auto_color && sb->len) strbuf_addstr(sb, GIT_COLOR_RESET); return 7; /* consumed 7 bytes, "C(auto)" */ } else { diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index f6020cd..a1dcdb8 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' ' test_expect_success '%C(auto) respects --color' ' git log --color --format="%C(auto)%H" -1 >actual && - printf "\\033[m\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect && + printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect && test_cmp expect actual ' -- 2.10.0
Re: Two bugs in --pretty with %C(auto)
On Sun, Sep 18, 2016 at 1:25 AM, René Scharfewrote: > Am 17.09.2016 um 14:51 schrieb Anatoly Borodin: >> Hi All! >> >> First bug: >> >> git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s' >> >> prints %h with the default color (normal yellow), but >> >> git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s' >> >> shows %h with bold yellow, as if only the color was reset, but not >> the attributes (blink, ul, reverse also work this way). %d and %s are >> printed with the right color both times. >> >> Second bug, maybe related to the first one: >> >> git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s' >> >> The first line looks as expected. Well, almost: the '(' of %d is bold >> yellow. >> >> The second line looks like this: >> >> * %h, %s, %an with bold cyan; >> * %h with bold yellow; >> * %h with normal yellow and %s with normal white (default colors). >> >> PS git version 2.9.2 > > Well, in both cases you could add %Creset before %C(auto) to get what > you want. > > I'm not sure how just how automatic %C(auto) is supposed to be, but you > expected it do emit the reset for you, right? Sounds reasonable to me. > The following patch implements that behavior. > > Duy, what do you think? Even though letting some attributes before %C(auto) through sounds interesting, I'd say it's a bit unpredictable, especially when the main usage of %C(auto) is %d which could use plenty of colors. So yes, your changes look good. -- Duy
Re: Two bugs in --pretty with %C(auto)
Am 18.09.2016 um 14:30 schrieb Anatoly Borodin: On Sat, Sep 17, 2016 at 8:25 PM, René Scharfewrote: I'm not sure how just how automatic %C(auto) is supposed to be, but you expected it do emit the reset for you, right? Sounds reasonable to me. I don't see a good reason not to do so. Spare some bytes?.. The states for colors and attributes are separate; that's how the bold attribute bled into your the auto-colored parts of your output. You could use that property to specify e.g. "give me automatic coloring, but reverse it". This only works by accident now, I think. Full resets are emitted after many placeholders, so attributes don't reach very far in practice. We'd have to be more careful with these full resets if we'd want attributes to cover multiple placeholders. An automatic reset at the start of %C(auto) would go into the opposite direction. René
Re: Two bugs in --pretty with %C(auto)
Hi René! On Sat, Sep 17, 2016 at 8:25 PM, René Scharfewrote: > I'm not sure how just how automatic %C(auto) is supposed to be, but you > expected it do emit the reset for you, right? Sounds reasonable to me. I don't see a good reason not to do so. Spare some bytes?.. > The following patch implements that behavior. Thanks, the patch works great! -- Mit freundlichen Grüßen, Anatoly Borodin
Re: Two bugs in --pretty with %C(auto)
Am 17.09.2016 um 14:51 schrieb Anatoly Borodin: > Hi All! > > First bug: > > git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s' > > prints %h with the default color (normal yellow), but > > git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s' > > shows %h with bold yellow, as if only the color was reset, but not > the attributes (blink, ul, reverse also work this way). %d and %s are > printed with the right color both times. > > Second bug, maybe related to the first one: > > git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s' > > The first line looks as expected. Well, almost: the '(' of %d is bold > yellow. > > The second line looks like this: > > * %h, %s, %an with bold cyan; > * %h with bold yellow; > * %h with normal yellow and %s with normal white (default colors). > > PS git version 2.9.2 Well, in both cases you could add %Creset before %C(auto) to get what you want. I'm not sure how just how automatic %C(auto) is supposed to be, but you expected it do emit the reset for you, right? Sounds reasonable to me. The following patch implements that behavior. Duy, what do you think? -- >8 -- Subject: pretty: let %C(auto) reset all attributes Reset colors and attributes upon %C(auto) to enable full automatic control over them; otherwise attributes like bold or reverse could still be in effect from previous %C placeholders. Signed-off-by: Rene Scharfe--- pretty.c | 2 ++ t/t6006-rev-list-format.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index 9788bd8..493edb0 100644 --- a/pretty.c +++ b/pretty.c @@ -1072,6 +1072,8 @@ 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) + strbuf_addstr(sb, GIT_COLOR_RESET); return 7; /* consumed 7 bytes, "C(auto)" */ } else { int ret = parse_color(sb, placeholder, c); diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index a1dcdb8..f6020cd 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' ' test_expect_success '%C(auto) respects --color' ' git log --color --format="%C(auto)%H" -1 >actual && - printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect && + printf "\\033[m\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect && test_cmp expect actual ' -- 2.10.0
Two bugs in --pretty with %C(auto)
Hi All! First bug: git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s' prints %h with the default color (normal yellow), but git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s' shows %h with bold yellow, as if only the color was reset, but not the attributes (blink, ul, reverse also work this way). %d and %s are printed with the right color both times. Second bug, maybe related to the first one: git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s' The first line looks as expected. Well, almost: the '(' of %d is bold yellow. The second line looks like this: * %h, %s, %an with bold cyan; * %h with bold yellow; * %h with normal yellow and %s with normal white (default colors). PS git version 2.9.2 -- Mit freundlichen Grüßen, Anatoly Borodin