{"thread":{"id":"44103","subject":"Two bugs in --pretty with %C(auto)","startedAt":"2016-09-17T12:52:11Z","lastAt":"2016-10-01T21:01:42Z","messageCount":7,"participants":["Anatoly Borodin","René Scharfe","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"302100","messageId":"nrje96$q7s$1@blaine.gmane.org","threadId":"44103","inReplyTo":null,"subject":"Two bugs in --pretty with %C(auto)","fromName":"Anatoly Borodin","fromEmail":"anatoly.borodin@gmail.com","sentAt":"2016-09-17T12:51:50Z","receivedAt":"2016-09-17T12:52:11Z","isPatch":false,"sender":{"key":"anatoly.borodin@gmail.com","avatar":"https://gravatar.com/avatar/59f11f1f45cf5ebb7bcca8da94b43cfe5f596d91c0a3e27ad0d2a861cc2bbce7?d=mp&s=160"},"body":"Hi All!\n\nFirst bug:\n\n\tgit log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'\n\nprints %h with the default color (normal yellow), but\n\n\tgit log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'\n\nshows %h with bold yellow, as if only the color was reset, but not\nthe attributes (blink, ul, reverse also work this way). %d and %s are\nprinted with the right color both times.\n\nSecond bug, maybe related to the first one:\n\n\tgit log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'\n\nThe first line looks as expected. Well, almost: the '(' of %d is bold\nyellow.\n\nThe second line looks like this:\n\n* %h, %s, %an with bold cyan;\n* %h with bold yellow;\n* %h with normal yellow and %s with normal white (default colors).\n\nPS git version 2.9.2\n\n-- \nMit freundlichen Grüßen,\nAnatoly Borodin\n\n"},{"id":"302104","messageId":"db20ae0c-9c33-1e65-b201-1b6a9ed11340@web.de","threadId":"44103","inReplyTo":"nrje96$q7s$1@blaine.gmane.org","subject":"Re: Two bugs in --pretty with %C(auto)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-17T18:25:24Z","receivedAt":"2016-09-17T18:26:12Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.09.2016 um 14:51 schrieb Anatoly Borodin:\n> Hi All!\n> \n> First bug:\n> \n> \tgit log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'\n> \n> prints %h with the default color (normal yellow), but\n> \n> \tgit log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'\n> \n> shows %h with bold yellow, as if only the color was reset, but not\n> the attributes (blink, ul, reverse also work this way). %d and %s are\n> printed with the right color both times.\n> \n> Second bug, maybe related to the first one:\n> \n> \tgit log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'\n> \n> The first line looks as expected. Well, almost: the '(' of %d is bold\n> yellow.\n> \n> The second line looks like this:\n> \n> * %h, %s, %an with bold cyan;\n> * %h with bold yellow;\n> * %h with normal yellow and %s with normal white (default colors).\n> \n> PS git version 2.9.2\n\nWell, in both cases you could add %Creset before %C(auto) to get what\nyou want.\n\nI'm not sure how just how automatic %C(auto) is supposed to be, but you\nexpected it do emit the reset for you, right?  Sounds reasonable to me.\nThe following patch implements that behavior.\n\nDuy, what do you think?\n\n-- >8 --\nSubject: pretty: let %C(auto) reset all attributes\n\nReset colors and attributes upon %C(auto) to enable full automatic\ncontrol over them; otherwise attributes like bold or reverse could\nstill be in effect from previous %C placeholders.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n pretty.c                   | 2 ++\n t/t6006-rev-list-format.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 9788bd8..493edb0 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1072,6 +1072,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n \t\t\tc->auto_color = want_color(c->pretty_ctx->color);\n+\t\t\tif (c->auto_color)\n+\t\t\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n \t\t\treturn 7; /* consumed 7 bytes, \"C(auto)\" */\n \t\t} else {\n \t\t\tint ret = parse_color(sb, placeholder, c);\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex a1dcdb8..f6020cd 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '\n \n test_expect_success '%C(auto) respects --color' '\n \tgit log --color --format=\"%C(auto)%H\" -1 >actual &&\n-\tprintf \"\\\\033[33m%s\\\\033[m\\\\n\" $(git rev-parse HEAD) >expect &&\n+\tprintf \"\\\\033[m\\\\033[33m%s\\\\033[m\\\\n\" $(git rev-parse HEAD) >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.10.0\n\n\n"},{"id":"302112","messageId":"CACNzp2mkD7Se9R=SVwXhyRstkGAs1N9YbNgaZRQYKkpPM1kj8A@mail.gmail.com","threadId":"44103","inReplyTo":"db20ae0c-9c33-1e65-b201-1b6a9ed11340@web.de","subject":"Re: Two bugs in --pretty with %C(auto)","fromName":"Anatoly Borodin","fromEmail":"anatoly.borodin@gmail.com","sentAt":"2016-09-18T12:30:52Z","receivedAt":"2016-09-18T12:31:37Z","isPatch":false,"sender":{"key":"anatoly.borodin@gmail.com","avatar":"https://gravatar.com/avatar/59f11f1f45cf5ebb7bcca8da94b43cfe5f596d91c0a3e27ad0d2a861cc2bbce7?d=mp&s=160"},"body":"Hi René!\n\n\nOn Sat, Sep 17, 2016 at 8:25 PM, René Scharfe <l.s.r@web.de> wrote:\n> I'm not sure how just how automatic %C(auto) is supposed to be, but you\n> expected it do emit the reset for you, right?  Sounds reasonable to me.\n\nI don't see a good reason not to do so. Spare some bytes?..\n\n> The following patch implements that behavior.\n\nThanks, the patch works great!\n\n\n-- \nMit freundlichen Grüßen,\nAnatoly Borodin\n"},{"id":"302113","messageId":"bddd85de-76d1-fbd1-06f5-4ced57879f2c@web.de","threadId":"44103","inReplyTo":"CACNzp2mkD7Se9R=SVwXhyRstkGAs1N9YbNgaZRQYKkpPM1kj8A@mail.gmail.com","subject":"Re: Two bugs in --pretty with %C(auto)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-18T13:21:51Z","receivedAt":"2016-09-18T13:22:23Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.09.2016 um 14:30 schrieb Anatoly Borodin:\n> On Sat, Sep 17, 2016 at 8:25 PM, René Scharfe <l.s.r@web.de> wrote:\n>> I'm not sure how just how automatic %C(auto) is supposed to be, but you\n>> expected it do emit the reset for you, right?  Sounds reasonable to me.\n>\n> I don't see a good reason not to do so. Spare some bytes?..\n\nThe states for colors and attributes are separate; that's how the bold \nattribute bled into your the auto-colored parts of your output.  You \ncould use that property to specify e.g. \"give me automatic coloring, but \nreverse it\".\n\nThis only works by accident now, I think.  Full resets are emitted after \nmany placeholders, so attributes don't reach very far in practice.  We'd \nhave to be more careful with these full resets if we'd want attributes \nto cover multiple placeholders.  An automatic reset at the start of \n%C(auto) would go into the opposite direction.\n\nRené\n"},{"id":"302126","messageId":"CACsJy8BUT6wypU5yn4izCadEF0y32n4m9x8ZLgfod+HNfSZiqg@mail.gmail.com","threadId":"44103","inReplyTo":"db20ae0c-9c33-1e65-b201-1b6a9ed11340@web.de","subject":"Re: Two bugs in --pretty with %C(auto)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-09-19T12:59:03Z","receivedAt":"2016-09-19T12:59:40Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 18, 2016 at 1:25 AM, René Scharfe <l.s.r@web.de> wrote:\n> Am 17.09.2016 um 14:51 schrieb Anatoly Borodin:\n>> Hi All!\n>>\n>> First bug:\n>>\n>>       git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'\n>>\n>> prints %h with the default color (normal yellow), but\n>>\n>>       git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'\n>>\n>> shows %h with bold yellow, as if only the color was reset, but not\n>> the attributes (blink, ul, reverse also work this way). %d and %s are\n>> printed with the right color both times.\n>>\n>> Second bug, maybe related to the first one:\n>>\n>>       git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'\n>>\n>> The first line looks as expected. Well, almost: the '(' of %d is bold\n>> yellow.\n>>\n>> The second line looks like this:\n>>\n>> * %h, %s, %an with bold cyan;\n>> * %h with bold yellow;\n>> * %h with normal yellow and %s with normal white (default colors).\n>>\n>> PS git version 2.9.2\n>\n> Well, in both cases you could add %Creset before %C(auto) to get what\n> you want.\n>\n> I'm not sure how just how automatic %C(auto) is supposed to be, but you\n> expected it do emit the reset for you, right?  Sounds reasonable to me.\n> The following patch implements that behavior.\n>\n> Duy, what do you think?\n\nEven though letting some attributes before %C(auto) through sounds\ninteresting, I'd say it's a bit unpredictable, especially when the\nmain usage of %C(auto) is %d which could use plenty of colors. So yes,\nyour changes look good.\n-- \nDuy\n"},{"id":"302905","messageId":"b032fcbc-045b-7822-f9d4-84930cbf6b41@web.de","threadId":"44103","inReplyTo":"db20ae0c-9c33-1e65-b201-1b6a9ed11340@web.de","subject":"Re: Two bugs in --pretty with %C(auto)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-29T18:13:05Z","receivedAt":"2016-09-29T18:13:39Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.09.2016 um 20:25 schrieb René Scharfe:\n> diff --git a/pretty.c b/pretty.c\n> index 9788bd8..493edb0 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1072,6 +1072,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n>  \tcase 'C':\n>  \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n>  \t\t\tc->auto_color = want_color(c->pretty_ctx->color);\n> +\t\t\tif (c->auto_color)\n> +\t\t\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n>  \t\t\treturn 7; /* consumed 7 bytes, \"C(auto)\" */\n>  \t\t} else {\n>  \t\t\tint ret = parse_color(sb, placeholder, c);\n\nWe could optimize this a bit (see below).  I can't think of a downside;\nsomeone adding a prefix would be responsible for adding a reset as well\nif needed, right?\n\n-- >8 --\nSubject: [PATCH] pretty: avoid adding reset for %C(auto) if output is empty\n\nWe emit an escape sequence for resetting color and attribute for\n%C(auto) to make sure automatic coloring is displayed as intended.\nStop doing that if the output strbuf is empty, i.e. when %C(auto)\nappears at the start of the format string, because then there is no\nneed for a reset and we save a few bytes in the output.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nReverts the change to t6006, so we'd need another test for this.\nAnatoly? :)\n\n pretty.c                   | 2 +-\n t/t6006-rev-list-format.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 493edb0..25efbca 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1072,7 +1072,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n \t\t\tc->auto_color = want_color(c->pretty_ctx->color);\n-\t\t\tif (c->auto_color)\n+\t\t\tif (c->auto_color && sb->len)\n \t\t\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n \t\t\treturn 7; /* consumed 7 bytes, \"C(auto)\" */\n \t\t} else {\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex f6020cd..a1dcdb8 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '\n \n test_expect_success '%C(auto) respects --color' '\n \tgit log --color --format=\"%C(auto)%H\" -1 >actual &&\n-\tprintf \"\\\\033[m\\\\033[33m%s\\\\033[m\\\\n\" $(git rev-parse HEAD) >expect &&\n+\tprintf \"\\\\033[33m%s\\\\033[m\\\\n\" $(git rev-parse HEAD) >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.10.0\n\n"},{"id":"303055","messageId":"CACNzp2=tRrAunuP0x2QtBfToXSLj1bW54-U63dpyUokpywxOOw@mail.gmail.com","threadId":"44103","inReplyTo":"b032fcbc-045b-7822-f9d4-84930cbf6b41@web.de","subject":"Re: Two bugs in --pretty with %C(auto)","fromName":"Anatoly Borodin","fromEmail":"anatoly.borodin@gmail.com","sentAt":"2016-10-01T21:01:15Z","receivedAt":"2016-10-01T21:01:42Z","isPatch":false,"sender":{"key":"anatoly.borodin@gmail.com","avatar":"https://gravatar.com/avatar/59f11f1f45cf5ebb7bcca8da94b43cfe5f596d91c0a3e27ad0d2a861cc2bbce7?d=mp&s=160"},"body":"Hi René,\n\n\nOn Thu, Sep 29, 2016 at 8:13 PM, René Scharfe <l.s.r@web.de> wrote:\n> Reverts the change to t6006, so we'd need another test for this.\n> Anatoly? :)\n\nI've checked my example commands and couldn't find any regressions.\n\n-- \nMit freundlichen Grüßen,\nAnatoly Borodin\n"}]}