[PATCH v7 0/1] sideband: highlight keywords in remote sideband output

[PATCH v7 0/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

Fix nits; remove debug printf.

Han-Wen Nienhuys (1):
  sideband: highlight keywords in remote sideband output

 Documentation/config.txt            |  12 +++
 help.c                              |   1 +
 help.h                              |   1 +
 sideband.c                          | 125 ++++++++++++++++++++++++++--
 t/t5409-colorize-remote-messages.sh |  87 +++++++++++++++++++
 5 files changed, 217 insertions(+), 9 deletions(-)
 create mode 100755 t/t5409-colorize-remote-messages.sh

--
2.18.0.597.ga71716f1ad-goog

[PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

The colorization is controlled with the config setting "color.remote".

Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.

   ERROR: commit is missing Change-Id

The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process). This obscures actionable error messages that servers may
send back. Highlighting keywords in the sideband draws more attention
to those messages.

The background for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.

The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").

The highlighting can be configured using color.remote.<KEYWORD>
configuration settings. Since the keys are matched case insensitively,
we match the keywords case insensitively too.

Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.

Helped-by: Duy Nguyen <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 Documentation/config.txt            |  12 +++
 help.c                              |   1 +
 help.h                              |   1 +
 sideband.c                          | 125 ++++++++++++++++++++++++++--
 t/t5409-colorize-remote-messages.sh |  87 +++++++++++++++++++
 5 files changed, 217 insertions(+), 9 deletions(-)
 create mode 100755 t/t5409-colorize-remote-messages.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3d..33bc1a3def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1263,6 +1263,18 @@ color.push::
 color.push.error::
 	Use customized color for push errors.
 
+color.remote::
+	If set, keywords at the start of the line are highlighted. The
+	keywords are "error", "warning", "hint" and "success", and are
+	matched case-insensitively. Maybe set to `always`, `false` (or
+	`never`) or `auto` (or `true`). If unset, then the value of
+	`color.ui` is used (`auto` by default).
+
+color.remote.<slot>::
+	Use customized color for each remote keyword. `<slot>` may be
+	`hint`, `warning`, `success` or `error` which match the
+	corresponding keyword.
+
 color.showBranch::
 	A boolean to enable/disable color in the output of
 	linkgit:git-show-branch[1]. May be set to `always`,
diff --git a/help.c b/help.c
index 3ebf0568db..b6cafcfc0a 100644
--- a/help.c
+++ b/help.c
@@ -425,6 +425,7 @@ void list_config_help(int for_human)
 		{ "color.diff", "<slot>", list_config_color_diff_slots },
 		{ "color.grep", "<slot>", list_config_color_grep_slots },
 		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
+		{ "color.remote", "<slot>", list_config_color_sideband_slots },
 		{ "color.status", "<slot>", list_config_color_status_slots },
 		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
 		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
diff --git a/help.h b/help.h
index f8b15323a6..9eab6a3f89 100644
--- a/help.h
+++ b/help.h
@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list, const char *prefix);
 void list_config_color_grep_slots(struct string_list *list, const char *prefix);
 void list_config_color_interactive_slots(struct string_list *list, const char *prefix);
 void list_config_color_status_slots(struct string_list *list, const char *prefix);
+void list_config_color_sideband_slots(struct string_list *list, const char *prefix);
 void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);
 
 #endif /* HELP_H */
diff --git a/sideband.c b/sideband.c
index 325bf0e974..1c6bb0e25b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,109 @@
 #include "cache.h"
+#include "color.h"
+#include "config.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "help.h"
+
+struct keyword_entry {
+	/*
+	 * We use keyword as config key so it should be a single alphanumeric word.
+	 */
+	const char *keyword;
+	char color[COLOR_MAXLEN];
+};
+
+static struct keyword_entry keywords[] = {
+	{ "hint",	GIT_COLOR_YELLOW },
+	{ "warning",	GIT_COLOR_BOLD_YELLOW },
+	{ "success",	GIT_COLOR_BOLD_GREEN },
+	{ "error",	GIT_COLOR_BOLD_RED },
+};
+
+/* Returns a color setting (GIT_COLOR_NEVER, etc). */
+static int use_sideband_colors(void)
+{
+	static int use_sideband_colors_cached = -1;
+
+	const char *key = "color.remote";
+	struct strbuf sb = STRBUF_INIT;
+	char *value;
+	int i;
+
+	if (use_sideband_colors_cached >= 0)
+		return use_sideband_colors_cached;
+
+	if (!git_config_get_string(key, &value)) {
+		use_sideband_colors_cached = git_config_colorbool(key, value);
+	} else if (!git_config_get_string("color.ui", &value)) {
+		use_sideband_colors_cached = git_config_colorbool("color.ui", value);
+	} else {
+		use_sideband_colors_cached = GIT_COLOR_AUTO;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
+		if (git_config_get_string(sb.buf, &value))
+			continue;
+		if (color_parse(value, keywords[i].color))
+			continue;
+	}
+	strbuf_release(&sb);
+	return use_sideband_colors_cached;
+}
+
+void list_config_color_sideband_slots(struct string_list *list, const char *prefix)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(keywords); i++)
+		list_config_item(list, prefix, keywords[i].keyword);
+}
+
+/*
+ * Optionally highlight one keyword in remote output if it appears at the start
+ * of the line. This should be called for a single line only, which is
+ * passed as the first N characters of the SRC array.
+ */
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+	int i;
+
+	if (!want_color_stderr(use_sideband_colors())) {
+		strbuf_add(dest, src, n);
+		return;
+	}
+
+	while (isspace(*src)) {
+		strbuf_addch(dest, *src);
+		src++;
+		n--;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+		struct keyword_entry *p = keywords + i;
+		int len = strlen(p->keyword);
+		/*
+		 * Match case insensitively, so we colorize output from existing
+		 * servers regardless of the case that they use for their
+		 * messages. We only highlight the word precisely, so
+		 * "successful" stays uncolored.
+		 */
+		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
+			strbuf_addstr(dest, p->color);
+			strbuf_add(dest, src, len);
+			strbuf_addstr(dest, GIT_COLOR_RESET);
+			n -= len;
+			src += len;
+			break;
+		}
+	}
+
+	strbuf_add(dest, src, n);
+
+}
+
 
 /*
  * Receive multiplexed output stream over git native protocol.
@@ -48,8 +151,10 @@ int recv_sideband(const char *me, int in_stream, int out)
 		len--;
 		switch (band) {
 		case 3:
-			strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
-				    DISPLAY_PREFIX, buf + 1);
+			strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+				    DISPLAY_PREFIX);
+			maybe_colorize_sideband(&outbuf, buf + 1, len);
+
 			retval = SIDEBAND_REMOTE_ERROR;
 			break;
 		case 2:
@@ -69,20 +174,22 @@ int recv_sideband(const char *me, int in_stream, int out)
 				if (!outbuf.len)
 					strbuf_addstr(&outbuf, DISPLAY_PREFIX);
 				if (linelen > 0) {
-					strbuf_addf(&outbuf, "%.*s%s%c",
-						    linelen, b, suffix, *brk);
-				} else {
-					strbuf_addch(&outbuf, *brk);
+					maybe_colorize_sideband(&outbuf, b, linelen);
+					strbuf_addstr(&outbuf, suffix);
 				}
+
+				strbuf_addch(&outbuf, *brk);
 				xwrite(2, outbuf.buf, outbuf.len);
 				strbuf_reset(&outbuf);
 
 				b = brk + 1;
 			}
 
-			if (*b)
-				strbuf_addf(&outbuf, "%s%s", outbuf.len ?
-					    "" : DISPLAY_PREFIX, b);
+			if (*b) {
+				strbuf_addstr(&outbuf, outbuf.len ?
+					    "" : DISPLAY_PREFIX);
+				maybe_colorize_sideband(&outbuf, b, strlen(b));
+			}
 			break;
 		case 1:
 			write_or_die(out, buf + 1, len);
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
new file mode 100755
index 0000000000..7057277a5f
--- /dev/null
+++ b/t/t5409-colorize-remote-messages.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+
+test_description='remote messages are colorized on the client'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir .git/hooks &&
+	write_script .git/hooks/update <<-\EOF &&
+	echo error: error
+	echo ERROR: also highlighted
+	echo hint: hint
+	echo hinting: not highlighted
+	echo success: success
+	echo warning: warning
+	echo prefixerror: error
+	echo " " "error: leading space"
+	exit 0
+	EOF
+	echo 1 >file &&
+	git add file &&
+	git commit -m 1 &&
+	git clone . child &&
+	(
+		cd child &&
+		test_commit message2 file content2
+	)
+'
+
+test_expect_success 'keywords' '
+	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/keywords 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<BOLD;RED>error<RESET>: error" decoded &&
+	grep "<YELLOW>hint<RESET>:" decoded &&
+	grep "<BOLD;GREEN>success<RESET>:" decoded &&
+	grep "<BOLD;YELLOW>warning<RESET>:" decoded
+'
+
+test_expect_success 'whole words at line start' '
+	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/whole-words 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<YELLOW>hint<RESET>:" decoded &&
+	grep "hinting: not highlighted" decoded &&
+	grep "prefixerror: error" decoded
+'
+
+test_expect_success 'case-insensitive' '
+	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&
+        cat output &&
+	test_decode_color <output >decoded &&
+	grep "<BOLD;RED>error<RESET>: error" decoded &&
+	grep "<BOLD;RED>ERROR<RESET>: also highlighted" decoded
+'
+
+test_expect_success 'leading space' '
+	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output &&        cat output &&
+	test_decode_color <output >decoded &&
+	grep "  <BOLD;RED>error<RESET>: leading space" decoded
+'
+
+test_expect_success 'no coloring for redirected output' '
+	git --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&
+	test_decode_color <output >decoded &&
+	grep "error: error" decoded
+'
+
+test_expect_success 'push with customized color' '
+	git --git-dir child/.git -c color.remote=always -c color.remote.error=blue push -f origin HEAD:refs/heads/customized-color 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<BLUE>error<RESET>:" decoded &&
+	grep "<BOLD;GREEN>success<RESET>:" decoded
+'
+
+
+test_expect_success 'error in customized color' '
+	git --git-dir child/.git -c color.remote=always -c color.remote.error=i-am-not-a-color push -f origin HEAD:refs/heads/error-customized-color 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<BOLD;GREEN>success<RESET>:" decoded
+'
+
+test_expect_success 'fallback to color.ui' '
+	git --git-dir child/.git -c color.ui=always push -f origin HEAD:refs/heads/fallback-color-ui 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<BOLD;RED>error<RESET>: error" decoded
+'
+
+test_done
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> Fix nits; remove debug printf.
>
> Han-Wen Nienhuys (1):
>   sideband: highlight keywords in remote sideband output
>
>  Documentation/config.txt            |  12 +++
>  help.c                              |   1 +
>  help.h                              |   1 +
>  sideband.c                          | 125 ++++++++++++++++++++++++++--
>  t/t5409-colorize-remote-messages.sh |  87 +++++++++++++++++++
>  5 files changed, 217 insertions(+), 9 deletions(-)
>  create mode 100755 t/t5409-colorize-remote-messages.sh
>
> --
> 2.18.0.597.ga71716f1ad-goog

I'll squash in the following while queuing for

    <CAPig+cScb_7s4a_ZSVCsr+nBxAHGHZVMZOtnrOgbhZUi96-VFA@mail.gmail.com>

Thanks for sticking to the topic.  

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 33bc1a3def..9a38dd2cbb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1266,7 +1266,7 @@ color.push.error::
 color.remote::
 	If set, keywords at the start of the line are highlighted. The
 	keywords are "error", "warning", "hint" and "success", and are
-	matched case-insensitively. Maybe set to `always`, `false` (or
+	matched case-insensitively. May be set to `always`, `false` (or
 	`never`) or `auto` (or `true`). If unset, then the value of
 	`color.ui` is used (`auto` by default).
 

Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

On Tue, Aug 7, 2018 at 11:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > Fix nits; remove debug printf.
> >
> > Han-Wen Nienhuys (1):
> >   sideband: highlight keywords in remote sideband output
> >
> >  Documentation/config.txt            |  12 +++
> >  help.c                              |   1 +
> >  help.h                              |   1 +
> >  sideband.c                          | 125 ++++++++++++++++++++++++++--
> >  t/t5409-colorize-remote-messages.sh |  87 +++++++++++++++++++
> >  5 files changed, 217 insertions(+), 9 deletions(-)
> >  create mode 100755 t/t5409-colorize-remote-messages.sh
> >
> > --
> > 2.18.0.597.ga71716f1ad-goog
>
> I'll squash in the following while queuing for
>
>     <CAPig+cScb_7s4a_ZSVCsr+nBxAHGHZVMZOtnrOgbhZUi96-VFA@mail.gmail.com>
>
> Thanks for sticking to the topic.

Thanks, LGTM.

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> +/*
> + * Optionally highlight one keyword in remote output if it appears at the start
> + * of the line. This should be called for a single line only, which is
> + * passed as the first N characters of the SRC array.
> + */
> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> +{
> +	int i;
> +
> +	if (!want_color_stderr(use_sideband_colors())) {
> +		strbuf_add(dest, src, n);
> +		return;
> +	}
> +
> +	while (isspace(*src)) {
> +		strbuf_addch(dest, *src);
> +		src++;
> +		n--;
> +	}

This loop can run out of bytes in src in search of non-space before
n gets to zero or negative, and when that happens ...

> +	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> +		struct keyword_entry *p = keywords + i;
> +		int len = strlen(p->keyword);
> +		/*
> +		 * Match case insensitively, so we colorize output from existing
> +		 * servers regardless of the case that they use for their
> +		 * messages. We only highlight the word precisely, so
> +		 * "successful" stays uncolored.
> +		 */
> +		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {

... these access src[] beyond the end of what the caller intended to
show us, and also ...

> +			strbuf_addstr(dest, p->color);
> +			strbuf_add(dest, src, len);
> +			strbuf_addstr(dest, GIT_COLOR_RESET);
> +			n -= len;
> +			src += len;
> +			break;
> +		}
> +	}
> +
> +	strbuf_add(dest, src, n);

... this will now try to add 0 or negative number of bytes.

> +
> +}
> +

Perhaps this will help (not really tested).  The second hunk is an
unrelated style clean-up.


 sideband.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sideband.c b/sideband.c
index 1c6bb0e25b..d99a559a44 100644
--- a/sideband.c
+++ b/sideband.c
@@ -75,11 +75,13 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		return;
 	}
 
-	while (isspace(*src)) {
+	while (isspace(*src) && n) {
 		strbuf_addch(dest, *src);
 		src++;
 		n--;
 	}
+	if (!n)
+		return;
 
 	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
 		struct keyword_entry *p = keywords + i;
@@ -101,7 +103,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	}
 
 	strbuf_add(dest, src, n);
-
 }
 
 

Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This loop can run out of bytes in src in search of non-space before
> n gets to zero or negative, and when that happens ...
>
>> +	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
>> +		struct keyword_entry *p = keywords + i;
>> +		int len = strlen(p->keyword);
>> +		/*
>> +		 * Match case insensitively, so we colorize output from existing
>> +		 * servers regardless of the case that they use for their
>> +		 * messages. We only highlight the word precisely, so
>> +		 * "successful" stays uncolored.
>> +		 */
>> +		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
>
> ... these access src[] beyond the end of what the caller intended to
> show us, and also ...

Actually, leaving when !n before this loop is insufficient.  src[]
may have 2 bytes "in" remaining, and we may be trying to see if it
begins with "info", for example, and using strncasecmp() with len==4
would of course read beyond the end of src[].

-- >8 --
Subject: sideband: do not read beyond the end of input

The caller of maybe_colorize_sideband() gives a counted buffer
<src,n>, but the callee checked *src as if it were a NUL terminated
buffer.  If src[] had all isspace() bytes in it, we would have made
n negative, and then (1) called number of strncasecmp() to see if
the remaining bytes in src[] matched keywords, reading beyond the
end of the array, and/or (2) called strbuf_add() with negative
count, most likely triggering the "you want to use way too much
memory" error due to unsigned integer overflow.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sideband.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sideband.c b/sideband.c
index 1c6bb0e25b..372039247f 100644
--- a/sideband.c
+++ b/sideband.c
@@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		return;
 	}
 
-	while (isspace(*src)) {
+	while (0 < n && isspace(*src)) {
 		strbuf_addch(dest, *src);
 		src++;
 		n--;
@@ -84,6 +84,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
 		struct keyword_entry *p = keywords + i;
 		int len = strlen(p->keyword);
+
+		if (n <= len)
+			continue;
 		/*
 		 * Match case insensitively, so we colorize output from existing
 		 * servers regardless of the case that they use for their
@@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		}
 	}
 
-	strbuf_add(dest, src, n);
-
+	if (0 < n)
+		strbuf_add(dest, src, n);
 }
 
 

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Jonathan Nieder]

(-cc: my @google.com email)
Hi,

Junio C Hamano wrote:

> Subject: sideband: do not read beyond the end of input
>
> The caller of maybe_colorize_sideband() gives a counted buffer
> <src,n>, but the callee checked *src as if it were a NUL terminated
> buffer.  If src[] had all isspace() bytes in it, we would have made
> n negative, and then (1) called number of strncasecmp() to see if
> the remaining bytes in src[] matched keywords, reading beyond the
> end of the array, and/or (2) called strbuf_add() with negative
> count, most likely triggering the "you want to use way too much
> memory" error due to unsigned integer overflow.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  sideband.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

This indeed avoids the "you want to use way too much memory" error
when I apply it.

> --- a/sideband.c
> +++ b/sideband.c
> @@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

Not about this patch: should the 'n' parameter be a size_t instead of
an int?  It doesn't matter in practice (since the caller has an int,
it can never be more than INT_MAX) but it might make the intent
clearer.

Based on inspecting the caller, using an int seems fine.

>  		return;
>  	}
>  
> -	while (isspace(*src)) {
> +	while (0 < n && isspace(*src)) {

Yes, we need to check 'n && isspace(*src)' to avoid overflowing the
buffer if it consists entirely of spaces.

>  		strbuf_addch(dest, *src);
>  		src++;
>  		n--;
> @@ -84,6 +84,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>  	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
>  		struct keyword_entry *p = keywords + i;
>  		int len = strlen(p->keyword);
> +
> +		if (n <= len)
> +			continue;

Using <= instead of < since we look at the character after the word as
well.  Good.

>  		/*
>  		 * Match case insensitively, so we colorize output from existing
>  		 * servers regardless of the case that they use for their
> 		 * messages. We only highlight the word precisely, so
> 		 * "successful" stays uncolored.
> 		 */
> 		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {

Not about this patch: should this check "&& src[len] == ':'" instead,
as discussed upthread?

> @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>  		}
>  	}
>  
> -	strbuf_add(dest, src, n);
> +	if (0 < n)
> +		strbuf_add(dest, src, n);

This check seems unnecessary.  strbuf_add can cope fine with !n.
Should we put

	assert(n >= 0);

or even

	if (n < 0)
		BUG();

instead, since the earlier part of the fix guarantees that n >= 0?

Thanks for the careful work.  With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Jonathan Nieder]

Junio C Hamano wrote:

> Subject: sideband: do not read beyond the end of input
>
> The caller of maybe_colorize_sideband() gives a counted buffer
> <src,n>, but the callee checked *src as if it were a NUL terminated
> buffer.  If src[] had all isspace() bytes in it, we would have made
> n negative, and then (1) called number of strncasecmp() to see if
> the remaining bytes in src[] matched keywords, reading beyond the
> end of the array, and/or (2) called strbuf_add() with negative
> count, most likely triggering the "you want to use way too much
> memory" error due to unsigned integer overflow.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  sideband.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

And here are some tests to squash in.

A sideband line consisting entirely of spaces causes us to overflow
our buffer and end up with a negative number of remaining characters,
ultimately resulting in the message

	fatal: you want to use way too much memory

when parsing it in order to add color.

We also forget to limit a strncasecmp and isalnum looking for keywords
to color in to the memory region passed in.  Fortunately all callers
put a delimiter character (\0, \r, or \n) after the end of the region
so this does not cause trouble in practice.  Add a test for
futureproofing to protect the new bounds checking code in case that
ever changes.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks again,
Jonathan

 t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index eb1b8aa05df..f81b6813c03 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -15,6 +15,8 @@ test_expect_success 'setup' '
 	echo warning: warning
 	echo prefixerror: error
 	echo " " "error: leading space"
+	echo "    "
+	echo Err
 	exit 0
 	EOF
 	echo 1 >file &&
@@ -44,6 +46,12 @@ test_expect_success 'whole words at line start' '
 	grep "prefixerror: error" decoded
 '
 
+test_expect_success 'short line' '
+	git -C child -c color.remote=always push -f origin HEAD:short-line 2>output &&
+	test_decode_color <output >decoded &&
+	grep "remote: Err" decoded
+'
+
 test_expect_success 'case-insensitive' '
 	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&
 	cat output &&
@@ -58,6 +66,12 @@ test_expect_success 'leading space' '
 	grep "  <BOLD;RED>error<RESET>: leading space" decoded
 '
 
+test_expect_success 'spaces only' '
+	git -C child -c color.remote=always push -f origin HEAD:only-space 2>output &&
+	test_decode_color <output >decoded &&
+	grep "remote:     " decoded
+'
+
 test_expect_success 'no coloring for redirected output' '
 	git --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&
 	test_decode_color <output >decoded &&
-- 
2.18.0.1017.ga543ac7ca45

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> --- a/sideband.c
>> +++ b/sideband.c
>> @@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>
> Not about this patch: should the 'n' parameter be a size_t instead of
> an int?  It doesn't matter in practice (since the caller has an int,
> it can never be more than INT_MAX) but it might make the intent
> clearer.

I tend to agree, but I think a separate "clean-up" patch to do so is
more appropriate than rolling it into this fix.

>>  		/*
>>  		 * Match case insensitively, so we colorize output from existing
>>  		 * servers regardless of the case that they use for their
>> 		 * messages. We only highlight the word precisely, so
>> 		 * "successful" stays uncolored.
>> 		 */
>> 		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
>
> Not about this patch: should this check "&& src[len] == ':'" instead,
> as discussed upthread?

I originally was of an opinion that we should take only lowercase
keyword followed by a colon, primarily because that is what we
produce.  Then "the real world need" told us that we are better off
catching the keyword case-insensitively.  Recalling that lesson, I
am not sure I would support "let's limit to the colon, rejecting any
other punctionation letter".

In any case, we should make such a policy decision outside a patch
like this one that is about fixing a behaviour which all users would
consider as a bug regardless of the policy they support.

>> @@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>>  		}
>>  	}
>>  
>> -	strbuf_add(dest, src, n);
>> +	if (0 < n)
>> +		strbuf_add(dest, src, n);
>
> This check seems unnecessary.  strbuf_add can cope fine with !n.

I was primarily interested in catching negatives, and !n was a mere
optimization, but you are correct to point out that negative n at
this point in the codeflow is a BUG().

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>> -	strbuf_add(dest, src, n);
>>> +	if (0 < n)
>>> +		strbuf_add(dest, src, n);
>>
>> This check seems unnecessary.  strbuf_add can cope fine with !n.
>
> I was primarily interested in catching negatives, and !n was a mere
> optimization, but you are correct to point out that negative n at
> this point in the codeflow is a BUG().

Actually, let's just lose the conditional.  strbuf_add() would catch
and issue an error message when it notices that we fed negative
count anyway ;-).

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> And here are some tests to squash in.

Thanks, will do.

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>>> -	strbuf_add(dest, src, n);
>>>> +	if (0 < n)
>>>> +		strbuf_add(dest, src, n);
>>>
>>> This check seems unnecessary.  strbuf_add can cope fine with !n.
>>
>> I was primarily interested in catching negatives, and !n was a mere
>> optimization, but you are correct to point out that negative n at
>> this point in the codeflow is a BUG().
>
> Actually, let's just lose the conditional.  strbuf_add() would catch
> and issue an error message when it notices that we fed negative
> count anyway ;-).

So I'll have this applied on top of the original topic to prevent a
buggy version from escaping the lab.

-- >8 --
Subject: [PATCH] sideband: do not read beyond the end of input

The caller of maybe_colorize_sideband() gives a counted buffer
<src, n>, but the callee checked src[] as if it were a NUL terminated
buffer.  If src[] had all isspace() bytes in it, we would have made
n negative, and then

 (1) made number of strncasecmp() calls to see if the remaining
     bytes in src[] matched keywords, reading beyond the end of the
     array (this actually happens even if n does not go negative),
     and/or

 (2) called strbuf_add() with negative count, most likely triggering
     the "you want to use way too much memory" error due to unsigned
     integer overflow.

Fix both issues by making sure we do not go beyond &src[n].

In the longer term we may want to accept size_t as parameter for
clarity (even though we know that a sideband message we are painting
typically would fit on a line on a terminal and int is sufficient).
Write it down as a NEEDSWORK comment.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sideband.c                          |  8 ++++++--
 t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/sideband.c b/sideband.c
index 1c6bb0e25b..368647acf8 100644
--- a/sideband.c
+++ b/sideband.c
@@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
  * Optionally highlight one keyword in remote output if it appears at the start
  * of the line. This should be called for a single line only, which is
  * passed as the first N characters of the SRC array.
+ *
+ * NEEDSWORK: use "size_t n" instead for clarity.
  */
 static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 {
@@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		return;
 	}
 
-	while (isspace(*src)) {
+	while (0 < n && isspace(*src)) {
 		strbuf_addch(dest, *src);
 		src++;
 		n--;
@@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
 		struct keyword_entry *p = keywords + i;
 		int len = strlen(p->keyword);
+
+		if (n <= len)
+			continue;
 		/*
 		 * Match case insensitively, so we colorize output from existing
 		 * servers regardless of the case that they use for their
@@ -101,7 +106,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	}
 
 	strbuf_add(dest, src, n);
-
 }
 
 
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index eb1b8aa05d..f81b6813c0 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -15,6 +15,8 @@ test_expect_success 'setup' '
 	echo warning: warning
 	echo prefixerror: error
 	echo " " "error: leading space"
+	echo "    "
+	echo Err
 	exit 0
 	EOF
 	echo 1 >file &&
@@ -44,6 +46,12 @@ test_expect_success 'whole words at line start' '
 	grep "prefixerror: error" decoded
 '
 
+test_expect_success 'short line' '
+	git -C child -c color.remote=always push -f origin HEAD:short-line 2>output &&
+	test_decode_color <output >decoded &&
+	grep "remote: Err" decoded
+'
+
 test_expect_success 'case-insensitive' '
 	git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&
 	cat output &&
@@ -58,6 +66,12 @@ test_expect_success 'leading space' '
 	grep "  <BOLD;RED>error<RESET>: leading space" decoded
 '
 
+test_expect_success 'spaces only' '
+	git -C child -c color.remote=always push -f origin HEAD:only-space 2>output &&
+	test_decode_color <output >decoded &&
+	grep "remote:     " decoded
+'
+
 test_expect_success 'no coloring for redirected output' '
 	git --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&
 	test_decode_color <output >decoded &&
-- 
2.18.0-748-gfa03cdc39b

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Jeff King]

On Sat, Aug 18, 2018 at 09:16:28AM -0700, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] sideband: do not read beyond the end of input
> 
> The caller of maybe_colorize_sideband() gives a counted buffer
> <src, n>, but the callee checked src[] as if it were a NUL terminated
> buffer.  If src[] had all isspace() bytes in it, we would have made
> n negative, and then
> 
>  (1) made number of strncasecmp() calls to see if the remaining
>      bytes in src[] matched keywords, reading beyond the end of the
>      array (this actually happens even if n does not go negative),
>      and/or
> 
>  (2) called strbuf_add() with negative count, most likely triggering
>      the "you want to use way too much memory" error due to unsigned
>      integer overflow.
> 
> Fix both issues by making sure we do not go beyond &src[n].

Thanks. I've been sporadically seeing "fatal: you want to use way too
much memory" the last few days while running 'next', and finally managed
to catch a reproducible case. This patch definitely fixes it.

> In the longer term we may want to accept size_t as parameter for
> clarity (even though we know that a sideband message we are painting
> typically would fit on a line on a terminal and int is sufficient).
> Write it down as a NEEDSWORK comment.

This "typically" made me nervous about what happens in the non-typical
case, but I think we can say something even stronger: the length comes
from a pktline, so the maximum is less than 16 bits. I wondered if we
might ever call this on the accumulated string from multiple sidebands,
but it doesn't look like it.

-Peff

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

On Sat, Aug 18, 2018 at 6:16 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Actually, let's just lose the conditional.  strbuf_add() would catch
> > and issue an error message when it notices that we fed negative
> > count anyway ;-).
>
> So I'll have this applied on top of the original topic to prevent a
> buggy version from escaping the lab.
>
> -- >8 --
> Subject: [PATCH] sideband: do not read beyond the end of input
>
> The caller of maybe_colorize_sideband() gives a counted buffer
> <src, n>, but the callee checked src[] as if it were a NUL terminated
> buffer.  If src[] had all isspace() bytes in it, we would have made
> n negative, and then
>
>  (1) made number of strncasecmp() calls to see if the remaining
>      bytes in src[] matched keywords, reading beyond the end of the
>      array (this actually happens even if n does not go negative),
>      and/or
>
>  (2) called strbuf_add() with negative count, most likely triggering
>      the "you want to use way too much memory" error due to unsigned
>      integer overflow.
>
> Fix both issues by making sure we do not go beyond &src[n].
>
> In the longer term we may want to accept size_t as parameter for
> clarity (even though we know that a sideband message we are painting
> typically would fit on a line on a terminal and int is sufficient).
> Write it down as a NEEDSWORK comment.
>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  sideband.c                          |  8 ++++++--
>  t/t5409-colorize-remote-messages.sh | 14 ++++++++++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/sideband.c b/sideband.c
> index 1c6bb0e25b..368647acf8 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
>   * Optionally highlight one keyword in remote output if it appears at the start
>   * of the line. This should be called for a single line only, which is
>   * passed as the first N characters of the SRC array.
> + *
> + * NEEDSWORK: use "size_t n" instead for clarity.
>   */
>  static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>  {
> @@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>                 return;
>         }
>
> -       while (isspace(*src)) {
> +       while (0 < n && isspace(*src)) {
>                 strbuf_addch(dest, *src);
>                 src++;
>                 n--;
> @@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>         for (i = 0; i < ARRAY_SIZE(keywords); i++) {
>                 struct keyword_entry *p = keywords + i;
>                 int len = strlen(p->keyword);
> +
> +               if (n <= len)
> +                       continue;

I would suggest

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

so we colorize a single line that looks like "warning" as well


Other than that, LGTM.

--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

and, thanks for cleaning up after me :)

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> In the longer term we may want to accept size_t as parameter for
>> clarity (even though we know that a sideband message we are painting
>> typically would fit on a line on a terminal and int is sufficient).
>> Write it down as a NEEDSWORK comment.
>
> This "typically" made me nervous about what happens in the non-typical
> case, but I think we can say something even stronger: the length comes
> from a pktline, so the maximum is less than 16 bits. I wondered if we
> might ever call this on the accumulated string from multiple sidebands,
> but it doesn't look like it.

I think a sideband packet may be split on one but not the other
codepath to result in multiple calls, but I do not think we coalesce
them together, so I agree that int is sufficient.

Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

>> @@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>>         for (i = 0; i < ARRAY_SIZE(keywords); i++) {
>>                 struct keyword_entry *p = keywords + i;
>>                 int len = strlen(p->keyword);
>> +
>> +               if (n <= len)
>> +                       continue;
>
> I would suggest
>
>  if (n < len) continue;
> ..
> if (!strncasecmp(p->keyword, src, len) && (n == len || !isalnum(src[len]))) {
>
> so we colorize a single line that looks like "warning" as well

That's the kind of thing I would have mentioned in the initial
review of the feature, and I do not think it is a bad idea.

I do think it is a bad idea to roll it into this patch, though.