{"thread":{"id":"49941","subject":"[PATCH] sideband: color lines with keyword only","startedAt":"2018-12-03T22:37:56Z","lastAt":"2018-12-10T11:03:27Z","messageCount":7,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"364498","messageId":"20181203223713.158394-1-sbeller@google.com","threadId":"49941","inReplyTo":null,"subject":"[PATCH] sideband: color lines with keyword only","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-03T22:37:13Z","receivedAt":"2018-12-03T22:37:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When bf1a11f0a1 (sideband: highlight keywords in remote sideband output,\n2018-08-07) was introduced, it was carefully considered which strings\nwould be highlighted. However 59a255aef0 (sideband: do not read beyond\nthe end of input, 2018-08-18) brought in a regression that the original\ndid not test for. A line containing only the keyword and nothing else\n(\"SUCCESS\") should still be colored.\n\nSigned-off-by: Stefan Beller \n---\n sideband.c | 5 +++--\n t/t5409-colorize-remote-messages.sh | 2 ++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 368647acf8..7c3d33d3f8 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -87,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t\tstruct keyword_entry *p = keywords + i;\n \t\tint len = strlen(p->keyword);\n \n-\t\tif (n <= len)\n+\t\tif (n < len)\n \t\t\tcontinue;\n \t\t/*\n \t\t * Match case insensitively, so we colorize output from existing\n@@ -95,7 +95,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \t\t * messages. We only highlight the word precisely, so\n \t\t * \"successful\" stays uncolored.\n \t\t */\n-\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n+\t\tif (!strncasecmp(p->keyword, src, len) &&\n+\t\t (len == n || !isalnum(src[len]))) {\n \t\t\tstrbuf_addstr(dest, p->color);\n \t\t\tstrbuf_add(dest, src, len);\n \t\t\tstrbuf_addstr(dest, GIT_COLOR_RESET);\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nindex f81b6813c0..2a8c449661 100755\n--- a/t/t5409-colorize-remote-messages.sh\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -17,6 +17,7 @@ test_expect_success 'setup' '\n \techo \" \" \"error: leading space\"\n \techo \" \"\n \techo Err\n+\techo SUCCESS\n \texit 0\n \tEOF\n \techo 1 >file &&\n@@ -35,6 +36,7 @@ test_expect_success 'keywords' '\n \tgrep \"error: error\" decoded &&\n \tgrep \"hint:\" decoded &&\n \tgrep \"success:\" decoded &&\n+\tgrep \"SUCCESS\" decoded &&\n \tgrep \"warning:\" decoded\n '\n \n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364500","messageId":"20181203232353.GA157301@google.com","threadId":"49941","inReplyTo":"20181203223713.158394-1-sbeller@google.com","subject":"Re: [PATCH] sideband: color lines with keyword only","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-03T23:23:53Z","receivedAt":"2018-12-03T23:23:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> When bf1a11f0a1 (sideband: highlight keywords in remote sideband output,\n> 2018-08-07) was introduced, it was carefully considered which strings\n> would be highlighted. However 59a255aef0 (sideband: do not read beyond\n> the end of input, 2018-08-18) brought in a regression that the original\n> did not test for. A line containing only the keyword and nothing else\n> (\"SUCCESS\") should still be colored.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> sideband.c | 5 +++--\n> t/t5409-colorize-remote-messages.sh | 2 ++\n> 2 files changed, 5 insertions(+), 2 deletions(-)\n\nThanks for writing this.\n\nI was curious about what versions of Gerrit this is designed to\nsupport (or in other words whether it's a bug fix or a feature).\nLooking at examples like [1], it seems that Gerrit historically always\nused \"ERROR:\" so the 59a255aef0 logic would work for it. More\nrecently, [2] (ReceiveCommits: add a \"SUCCESS\" marker for successful\nchange updates, 2018-08-21) put SUCCESS on a line of its own. That\nputs this squarely in the new-feature category.\n\n\"success\" on its own line is even less likely to be a false positive\nthan \"success\" followed by punctuation (for example a period marking\nthe end of a sentence). So I like this change.\n\n[1] https://gerrit-review.googlesource.com/c/gerrit/+/22361\n[2] https://gerrit-review.googlesource.com/c/gerrit/+/193570\n\n> diff --git a/sideband.c b/sideband.c\n> index 368647acf8..7c3d33d3f8 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -87,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> \t\tstruct keyword_entry *p = keywords + i;\n> \t\tint len = strlen(p->keyword);\n> \n> -\t\tif (n <= len)\n> +\t\tif (n < len)\n> \t\t\tcontinue;\n\nIn the old code, we would escape early if 'n == len', but we didn't\nneed to. If 'n == len', then\n\n\tsrc[len] == '\\0'\n\tsrc .. &src[len-1] is a valid buffer to read from\n\nso the strncasecmp and strbuf_add operations used in this function are\nvalid. Good.\n\n> \t\t/*\n> \t\t * Match case insensitively, so we colorize output from existing\n> @@ -95,7 +95,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> \t\t * messages. We only highlight the word precisely, so\n> \t\t * \"successful\" stays uncolored.\n> \t\t */\n> -\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n> +\t\tif (!strncasecmp(p->keyword, src, len) &&\n> +\t\t (len == n || !isalnum(src[len]))) {\n\nOur custom isalnum treats '\\0' as not alphanumeric (sane_ctype[0] ==\nGIT_CNTRL) so this part of the patch is unnecessary. That said, it's\ngood for clarity and defensive programming.\n\n> \t\t\tstrbuf_addstr(dest, p->color);\n> \t\t\tstrbuf_add(dest, src, len);\n> \t\t\tstrbuf_addstr(dest, GIT_COLOR_RESET);\n> diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\n> index f81b6813c0..2a8c449661 100755\n> --- a/t/t5409-colorize-remote-messages.sh\n> +++ b/t/t5409-colorize-remote-messages.sh\n> @@ -17,6 +17,7 @@ test_expect_success 'setup' '\n> \techo \" \" \"error: leading space\"\n> \techo \" \"\n> \techo Err\n> +\techo SUCCESS\n> \texit 0\n> \tEOF\n> \techo 1 >file &&\n> @@ -35,6 +36,7 @@ test_expect_success 'keywords' '\n> \tgrep \"error: error\" decoded &&\n> \tgrep \"hint:\" decoded &&\n> \tgrep \"success:\" decoded &&\n> +\tgrep \"SUCCESS\" decoded &&\n> \tgrep \"warning:\" decoded\n> '\n\nNice tests.\n\nThe \"hinting: not highlighted\" example shows that we aren't\nintroducing false positives here, so the coverage seems sufficient.\nIt might be nice to include a line\n\n\techo ERROR:\n\nas well to match another idiom that Gerrit sometimes uses.\n\nReviewed-by: Jonathan Nieder \n\nThanks again for a pleasant read.\n"},{"id":"364501","messageId":"20181203233439.GB157301@google.com","threadId":"49941","inReplyTo":"20181203232353.GA157301@google.com","subject":"Re: [PATCH] sideband: color lines with keyword only","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-03T23:34:39Z","receivedAt":"2018-12-03T23:34:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Stefan Beller wrote:\n\n>> \t\t/*\n>> \t\t * Match case insensitively, so we colorize output from existing\n>> @@ -95,7 +95,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n>> \t\t * messages. We only highlight the word precisely, so\n>> \t\t * \"successful\" stays uncolored.\n>> \t\t */\n>> -\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n>> +\t\tif (!strncasecmp(p->keyword, src, len) &&\n>> +\t\t (len == n || !isalnum(src[len]))) {\n>\n> Our custom isalnum treats '\\0' as not alphanumeric (sane_ctype[0] ==\n> GIT_CNTRL) so this part of the patch is unnecessary. That said, it's\n> good for clarity and defensive programming.\n\nCorrection: I am being silly here. src[len] can be '\\0', '\\n', or\n'\\r' --- it's not always '\\0'. And the contract of this function is\nthat src[len] could be anything. Thanks for having handled it\ncorrectly. :)\n\nJonathan\n"},{"id":"364502","messageId":"CAGZ79kY0w7Zt0Z4KNu7qL4Lz8fFpv2p51D-w_MgZBYPqPFbZKw@mail.gmail.com","threadId":"49941","inReplyTo":"20181203232353.GA157301@google.com","subject":"Re: [PATCH] sideband: color lines with keyword only","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-03T23:35:28Z","receivedAt":"2018-12-03T23:35:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote:\n\n> I was curious about what versions of Gerrit this is designed to\n> support (or in other words whether it's a bug fix or a feature).\n> Looking at examples like [1], it seems that Gerrit historically always\n> used \"ERROR:\" so the 59a255aef0 logic would work for it. More\n> recently, [2] (ReceiveCommits: add a \"SUCCESS\" marker for successful\n> change updates, 2018-08-21) put SUCCESS on a line of its own. That\n> puts this squarely in the new-feature category.\n\nOoops. From the internal bug, I assumed this to be long standing Gerrit\nbehavior, which is why I sent it out in -rc to begin with.\n\n> > --- a/sideband.c\n> > +++ b/sideband.c\n> > @@ -87,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> > struct keyword_entry *p = keywords + i;\n> > int len = strlen(p->keyword);\n> >\n> > - if (n <= len)\n> > + if (n < len)\n> > continue;\n>\n> In the old code, we would escape early if 'n == len', but we didn't\n> need to. If 'n == len', then\n>\n> src[len] == '\\0'\n\nsrc[len] could also be one of \"\\n\\r\", see the caller\nrecv_sideband for sidebase case 2.\n\n> src .. &src[len-1] is a valid buffer to read from\n>\n> so the strncasecmp and strbuf_add operations used in this function are\n> valid. Good.\n\nYes, they are all valid...\n\n> > - if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n> > + if (!strncasecmp(p->keyword, src, len) &&\n> > + (len == n || !isalnum(src[len]))) {\n>\n> Our custom isalnum treats '\\0' as not alphanumeric (sane_ctype[0] ==\n> GIT_CNTRL) so this part of the patch is unnecessary. That said, it's\n> good for clarity and defensive programming.\n\n... but here we need to check for src[len] for validity.\n\nI made no assumptions about isalnum, but rather needed to shortcut\nthe condition, as accessing src[len] would be out of bounds, no?\n\n>\n> > strbuf_addstr(dest, p->color);\n> > strbuf_add(dest, src, len);\n\nunlike here (or the rest of the block), where len is used correctly.\n"},{"id":"364509","messageId":"20181203234257.GC157301@google.com","threadId":"49941","inReplyTo":"CAGZ79kY0w7Zt0Z4KNu7qL4Lz8fFpv2p51D-w_MgZBYPqPFbZKw@mail.gmail.com","subject":"Re: [PATCH] sideband: color lines with keyword only","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-03T23:42:57Z","receivedAt":"2018-12-03T23:43:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote:\n\n>> I was curious about what versions of Gerrit this is designed to\n>> support (or in other words whether it's a bug fix or a feature).\n>> Looking at examples like [1], it seems that Gerrit historically always\n>> used \"ERROR:\" so the 59a255aef0 logic would work for it. More\n>> recently, [2] (ReceiveCommits: add a \"SUCCESS\" marker for successful\n>> change updates, 2018-08-21) put SUCCESS on a line of its own. That\n>> puts this squarely in the new-feature category.\n>\n> Ooops. From the internal bug, I assumed this to be long standing Gerrit\n> behavior, which is why I sent it out in -rc to begin with.\n\nNo worries. Can't hurt for Junio to have a few patches to apply to\n\"pu\" or \"next\" to practice using the release candidates. :)\n\n[...]\n>> In the old code, we would escape early if 'n == len', but we didn't\n>> need to. If 'n == len', then\n>>\n>> src[len] == '\\0'\n>\n> src[len] could also be one of \"\\n\\r\", see the caller\n> recv_sideband for sidebase case 2.\n\nYes, I noticed too late[*]. Sorry for the noise.\n\nThe patch still looks good.\n\nJonathan\n\n[*] https://public-inbox.org/git/20181203233439.GB157301@google.com/\n"},{"id":"364524","messageId":"xmqq5zwaf0cb.fsf@gitster-ct.c.googlers.com","threadId":"49941","inReplyTo":"20181203234257.GC157301@google.com","subject":"Re: [PATCH] sideband: color lines with keyword only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-04T03:16:20Z","receivedAt":"2018-12-04T03:16:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Stefan Beller wrote:\n>> On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote:\n>\n>>> I was curious about what versions of Gerrit this is designed to\n>>> support (or in other words whether it's a bug fix or a feature).\n\nWell, bf1a11f0 (\"sideband: highlight keywords in remote sideband\noutput\", 2018-08-07) clearly wanted to allow a keyword followed by\nanything !isalnum() to be painted, and we accepted that change\nbecause we thought it was a good idea, so anything that made a\nkeyword alone not to be painted is a bug, isn't it? Whether output\nlines from Gerrit benefits from this fix is a different matter, of\ncourse.\n\n> No worries. Can't hurt for Junio to have a few patches to apply to\n> \"pu\" or \"next\" to practice using the release candidates. :)\n\nThis change falls into \"an obvious and small fix to a bug that went\nunnoticed and is in an older release (2.19)\" category, which is not\neligible for the upcoming release this late in the cycle. I think\nenough eyeballs looked at the change already, so let's not waste the\nalready-spent review braincycle and mark it as \"Will merge to 'next'\".\n"},{"id":"364916","messageId":"CAFQ2z_OnekMUu=AomfoBoa=4dYThtEfa4sxf+UkSMAFspxeV3w@mail.gmail.com","threadId":"49941","inReplyTo":"20181203232353.GA157301@google.com","subject":"Re: [PATCH] sideband: color lines with keyword only","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-12-10T11:03:11Z","receivedAt":"2018-12-10T11:03:27Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Dec 4, 2018 at 12:23 AM Jonathan Nieder wrote:\n> > When bf1a11f0a1 (sideband: highlight keywords in remote sideband output,\n> > 2018-08-07) was introduced, it was carefully considered which strings\n> > would be highlighted. However 59a255aef0 (sideband: do not read beyond\n> > the end of input, 2018-08-18) brought in a regression that the original\n> > did not test for. A line containing only the keyword and nothing else\n> > (\"SUCCESS\") should still be colored.\n\nI had intended SUCCESS on a line of its to be highlighted too, and\nsome earlier versions of my patch did that, but it regressed as the\npatch was reworked. The SUCCESS on a line of its own is a recent\nbehavior of Gerrit, and is live in Gerrit 2.16.\n\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"}]}