sideband: color lines with keyword only

When bf1a11f0a1 (sideband: highlight keywords in remote sideband output, 2018-08-07) was introduced, it was carefully considered which strings would be highlighted. However 59a255aef0 (sideband: do not read beyond the end of input, 2018-08-18) brought in a regression that the original did not test for. A line containing only the keyword and nothing else ("SUCCESS") should still be colored.

Signed-off-by Stefan Beller <[email protected]>

---
 sideband.c                          | 5 +++--
 t/t5409-colorize-remote-messages.sh | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

sideband.c +0 -0

 			strbuf_addstr(dest, p->color);
 			strbuf_add(dest, src, len);
 			strbuf_addstr(dest, GIT_COLOR_RESET);

t/t5409-colorize-remote-messages.sh +1 -0

index f81b6813c0..2a8c449661 100755
@@ -17,6 +17,7 @@ test_expect_success 'setup' '
 	echo " " "error: leading space"
 	echo "    "
 	echo Err
+	echo SUCCESS
 	exit 0
 	EOF
 	echo 1 >file &&

sideband.c static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) · line 99

Quoted 9 lines · Stefan Beller

sideband.c +2 -1

 		/*
 		 * Match case insensitively, so we colorize output from existing

@@ -95,7 +95,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		 * messages. We only highlight the word precisely, so
 		 * "successful" stays uncolored.
 		 */
-		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
+		if (!strncasecmp(p->keyword, src, len) &&
+		    (len == n || !isalnum(src[len]))) {

Our custom isalnum treats '\0' as not alphanumeric (sane_ctype[0] == GIT_CNTRL) so this part of the patch is unnecessary. That said, it's good for clarity and defensive programming.

I was curious about what versions of Gerrit this is designed to support (or in other words whether it's a bug fix or a feature). Looking at examples like [1], it seems that Gerrit historically always used "ERROR:" so the 59a255aef0 logic would work for it. More recently, [2] (ReceiveCommits: add a "SUCCESS" marker for successful change updates, 2018-08-21) put SUCCESS on a line of its own. That puts this squarely in the new-feature category.

Quoted 6 lines

sideband.c +0 -0

@@ -87,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
              struct keyword_entry *p = keywords + i;
              int len = strlen(p->keyword);
 

sideband.c static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) · line 91

Quoted 3 lines

sideband.c +1 -1

-             if (n <= len)
+             if (n < len)
                      continue;

In the old code, we would escape early if 'n == len', but we didn't need to. If 'n == len', then

        src[len] == '\0'

src .. &src[len-1] is a valid buffer to read from

so the strncasecmp and strbuf_add operations used in this function are valid. Good.

sideband.c static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) · line 99

Quoted 3 lines

sideband.c +2 -1

-             if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
+             if (!strncasecmp(p->keyword, src, len) &&
+                 (len == n || !isalnum(src[len]))) {

Our custom isalnum treats '\0' as not alphanumeric (sane_ctype[0] == GIT_CNTRL) so this part of the patch is unnecessary. That said, it's good for clarity and defensive programming.

Quoted 3 lines

                      strbuf_addstr(dest, p->color);
                      strbuf_add(dest, src, len);

Quoted 7 lines · Jonathan Nieder

I was curious about what versions of Gerrit this is designed to support (or in other words whether it's a bug fix or a feature). Looking at examples like [1], it seems that Gerrit historically always used "ERROR:" so the 59a255aef0 logic would work for it. More recently, [2] (ReceiveCommits: add a "SUCCESS" marker for successful change updates, 2018-08-21) put SUCCESS on a line of its own. That puts this squarely in the new-feature category.

Ooops. From the internal bug, I assumed this to be long standing Gerrit behavior, which is why I sent it out in -rc to begin with.

Quoted 4 lines

In the old code, we would escape early if 'n == len', but we didn't need to. If 'n == len', then

        src[len] == '\0'

src[len] could also be one of "\n\r", see the caller recv_sideband for sidebase case 2.

Quoted 4 lines · Stefan Beller

Quoted 2 lines · Jonathan Nieder

I was curious about what versions of Gerrit this is designed to support (or in other words whether it's a bug fix or a feature).

No worries. Can't hurt for Junio to have a few patches to apply to "pu" or "next" to practice using the release candidates. :)

Quoted 6 lines

When bf1a11f0a1 (sideband: highlight keywords in remote sideband output, 2018-08-07) was introduced, it was carefully considered which strings would be highlighted. However 59a255aef0 (sideband: do not read beyond the end of input, 2018-08-18) brought in a regression that the original did not test for. A line containing only the keyword and nothing else ("SUCCESS") should still be colored.