[PATCH v5 0/2] sideband: highlight keywords in remote sideband output

[PATCH v5 0/2] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

Address Jun's & Jrn's comments.

Han-Wen Nienhuys (2):
  config: document git config getter return value
  sideband: highlight keywords in remote sideband output

 Documentation/config.txt            |  12 +++
 config.h                            |   7 +-
 help.c                              |   1 +
 help.h                              |   1 +
 sideband.c                          | 126 +++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh |  80 ++++++++++++++++++
 6 files changed, 216 insertions(+), 11 deletions(-)
 create mode 100755 t/t5409-colorize-remote-messages.sh

--
2.18.0.597.ga71716f1ad-goog

[PATCH v5 1/2] config: document git config getter return value

[Han-Wen Nienhuys]

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 config.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/config.h b/config.h
index bb2f506b27..183b31ebf4 100644
--- a/config.h
+++ b/config.h
@@ -188,9 +188,14 @@ struct config_set {
 
 extern void git_configset_init(struct config_set *cs);
 extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);
 extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);
 extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * These functions return 1 if not found, and 0 if found, leaving the found
+ * value in the 'dest' pointer.
+ */
+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);
 extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);
 extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);
 extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH v5 2/2] 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), which 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: ").

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                          | 126 +++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh |  80 ++++++++++++++++++
 5 files changed, 210 insertions(+), 10 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..239be2ec85 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,108 @@
 #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))
+			die(_("config value %s is not a color: %s"), sb.buf, value);
+	}
+	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 must be
+ * 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.
@@ -16,7 +118,7 @@
 #define DISPLAY_PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
-#define DUMB_SUFFIX "        "
+#define DUMB_SUFFIX "	     "
 
 int recv_sideband(const char *me, int in_stream, int out)
 {
@@ -48,8 +150,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 +173,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..a9afb55ef1
--- /dev/null
+++ b/t/t5409-colorize-remote-messages.sh
@@ -0,0 +1,80 @@
+#!/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
+	chmod +x .git/hooks/update &&
+	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 &&
+	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 &&
+	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 '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 v5 1/2] config: document git config getter return value

[Junio C Hamano]

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

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  config.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

I think this is good (I read all the comments on v3 and v4 in the
archive before I said this).  Let's split it off from the other
patch and queue it on its own merit (not that I think the other one
is not yet good enough---I haven't even looked at it yet ;-).

Thanks.

>
> diff --git a/config.h b/config.h
> index bb2f506b27..183b31ebf4 100644
> --- a/config.h
> +++ b/config.h
> @@ -188,9 +188,14 @@ struct config_set {
>  
>  extern void git_configset_init(struct config_set *cs);
>  extern int git_configset_add_file(struct config_set *cs, const char *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);
>  extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);
>  extern void git_configset_clear(struct config_set *cs);
> +
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);
>  extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);
>  extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);
>  extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);

Re: [PATCH v5 1/2] config: document git config getter return value

[Han-Wen Nienhuys]

On Mon, Aug 6, 2018 at 6:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> patch and queue it on its own merit (not that I think the other one
> is not yet good enough---I haven't even looked at it yet ;-).

I discovered that emacs tabify will happily also add tabs in the
middle of the line, which breaks in the case of DUMB_SUFFIX.  I can
reroll a v6 if needed, though.

--

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 v5 2/2] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

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

> The Git push process itself prints lots of non-actionable messages
> (eg. bandwidth statistics, object counters for different phases of the
> process), which obscures actionable error messages that servers may

s/which obscures/which obscure/, as I think that "which" refers to
"messages" above.

> 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: ").

Yup.

When we introduce "the receiving end asks messages to be sent with
such and such decoration" protocol support, we would want a lot more
than just painting messages in color, e.g. i18n, verbosity, and even
"Hey, I am a script, send them in json".

Until that happens, let's keep things simpler.  No i18n messages and
no colored output over the wire.

> +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).

Reads much better.

I am still trying to find a concise way to help readers who saw a
line that begins with "Warnings: foo bar bla" and accept that it is
OK the early 7 chars are not painted.  "... case-insensitively and
honoring word boundary" is the best I came up so far, but  I am
afraid that is adding more words without hinting what I want to convey
strongly enough, so I am not going to suggest that (at least not yet).

> 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..239be2ec85 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,6 +1,108 @@
>  #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);

This design means future enhancement to allow more keywords will
have to be done in the form of adding more "color.remote.<key>",
which means a few restrictions on them are cast in stone at the
end-user facing design level, which we need to be careful about.

	Side note. I do not worry about the implementation level
	limitation at all.  For example, the current code will not
	allow end-users and projects to add new keywords to be
	painted, as it relies on the keywords[] static array we can
	see above.  But that implementation detail does not prevent
	us from improving it later to support more code in this
	codepath that notices "color.remote.failure" in config file
	and paint a line that begins with "failure:".

Because the third-level "variable" name is case insensive, matching
of any future keyword MUST be also done case insensitively.

Also, as you mentioned elsewhere in this patch, the string that can
be in the keyword MUST begin with an alphabetic and consists only of
alphanumeric or dash.

I do not think these limitations imposed by the design decision this
patch is making are particularly bad ones---we just need to be aware
of and firm about them.  When somebody else comes in the future and
wants to recognize "F A I L" as a custom keyword case sensitively,
we must be able to comfortably say "no" to it.

	Side note. We _could_ instead use "remotemsg.<key>.color"
	namespace, as the subsection names at the second level is a
	lot looser, but I do not think it is a good idea to use in
	this application, as they are case sensitive.

The above discussion may deserve to be in the log message as a
record to tell future ourselves why we decided to use
color.remote.<key>.

> +		if (git_config_get_string(sb.buf, &value))
> +			continue;
> +		if (color_parse(value, keywords[i].color))
> +			die(_("config value %s is not a color: %s"), sb.buf, value);

That's a bit inconsistent, isn't it?  If the configuration is not
even a string, we ignore it and continue, but if it is a string, we
insist that it names a color and otherwise die?

> +	}
> +	strbuf_release(&sb);
> +	return use_sideband_colors_cached;
> +}

> +/*
> + * 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 must be
> + * passed as the first N characters of the SRC array.
> + */

Saying "MUST be" is cheap, but do we have anybody who polices that
requirement?

I think the code is OK without any assert() or BUG(), and that is
because the design is "we just paint the keyword at the beginning of
what the other side of the sideband wants us to show as a single
unit".  If the other side sends a payload with an embedded LF in a
single packet, that's their choice and we are free not to paint the
beginning of the second line after that LF.  So from that point of
view, perhaps we shouldn't even talk about "a single line only".

> +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.
> @@ -16,7 +118,7 @@
>  #define DISPLAY_PREFIX "remote: "
>  
>  #define ANSI_SUFFIX "\033[K"
> -#define DUMB_SUFFIX "        "
> +#define DUMB_SUFFIX "	     "
>  

Was this change intended and if so for what purpose?  I can drop
this hunk if this is a mere finger-slip without proofreading, but I
do not want to do so without making sure I am not missing anything
and not discarding a meaningful change.

>  int recv_sideband(const char *me, int in_stream, int out)
>  {


> diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
> new file mode 100755
> index 0000000000..a9afb55ef1
> --- /dev/null
> +++ b/t/t5409-colorize-remote-messages.sh
> @@ -0,0 +1,80 @@
> +#!/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

Noticing the dash in "<<-", I would have expected all of the above
lines to be indented with a tab to align with 'w' in 'write_script'.

> +	chmod +x .git/hooks/update &&

No need for this "chmod +x"; that's one of the points in using
write_script.

Thanks.

Re: [PATCH v5 2/2] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

On Mon, Aug 6, 2018 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > The Git push process itself prints lots of non-actionable messages
> > (eg. bandwidth statistics, object counters for different phases of the
> > process), which obscures actionable error messages that servers may
>
> s/which obscures/which obscure/, as I think that "which" refers to
> "messages" above.
>
Done.

> > 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: ").
>
> Yup.
>
> When we introduce "the receiving end asks messages to be sent with
> such and such decoration" protocol support, we would want a lot more
> than just painting messages in color, e.g. i18n, verbosity, and even
> "Hey, I am a script, send them in json".
>
> Until that happens, let's keep things simpler.  No i18n messages and
> no colored output over the wire.

Ack.

>
> > +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).
>
> Reads much better.
>
> I am still trying to find a concise way to help readers who saw a
> line that begins with "Warnings: foo bar bla" and accept that it is
> OK the early 7 chars are not painted.  "... case-insensitively and
> honoring word boundary" is the best I came up so far, but  I am
> afraid that is adding more words without hinting what I want to convey
> strongly enough, so I am not going to suggest that (at least not yet).

I would suggest that the phrase "keyword" implies a tokenization, so
I'd leave as is.

> > +     for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > +             strbuf_reset(&sb);
> > +             strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
>
> This design means future enhancement to allow more keywords will
> have to be done in the form of adding more "color.remote.<key>",
> which means a few restrictions on them are cast in stone at the
> end-user facing design level, which we need to be careful about.
>
>         Side note. I do not worry about the implementation level
>         limitation at all.  For example, the current code will not
>         allow end-users and projects to add new keywords to be
>         painted, as it relies on the keywords[] static array we can
>         see above.  But that implementation detail does not prevent
>         us from improving it later to support more code in this
>         codepath that notices "color.remote.failure" in config file
>         and paint a line that begins with "failure:".
>
> Because the third-level "variable" name is case insensive, matching
> of any future keyword MUST be also done case insensitively.
>
> Also, as you mentioned elsewhere in this patch, the string that can
> be in the keyword MUST begin with an alphabetic and consists only of
> alphanumeric or dash.
>
> I do not think these limitations imposed by the design decision this
> patch is making are particularly bad ones---we just need to be aware
> of and firm about them.  When somebody else comes in the future and
> wants to recognize "F A I L" as a custom keyword case sensitively,
> we must be able to comfortably say "no" to it.
>
>         Side note. We _could_ instead use "remotemsg.<key>.color"
>         namespace, as the subsection names at the second level is a
>         lot looser, but I do not think it is a good idea to use in
>         this application, as they are case sensitive.
>
> The above discussion may deserve to be in the log message as a
> record to tell future ourselves why we decided to use
> color.remote.<key>.

I added a note about case insensitivity.

> > +             if (git_config_get_string(sb.buf, &value))
> > +                     continue;
> > +             if (color_parse(value, keywords[i].color))
> > +                     die(_("config value %s is not a color: %s"), sb.buf, value);
>
> That's a bit inconsistent, isn't it?  If the configuration is not
> even a string, we ignore it and continue, but if it is a string, we
> insist that it names a color and otherwise die?

Done; added test.

> > + * 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 must be
> > + * passed as the first N characters of the SRC array.
> > + */
>
> Saying "MUST be" is cheap, but do we have anybody who polices that
> requirement?

rephrased.

> I think the code is OK without any assert() or BUG(), and that is
> because the design is "we just paint the keyword at the beginning of
> what the other side of the sideband wants us to show as a single
> unit".  If the other side sends a payload with an embedded LF in a
> single packet, that's their choice and we are free not to paint the
> beginning of the second line after that LF.  So from that point of
> view, perhaps we shouldn't even talk about "a single line only".

I don't understand this remark. Isn't the call to strpbrk() meant to
split the input on line endings?

> >  #define ANSI_SUFFIX "\033[K"
> > -#define DUMB_SUFFIX "        "
> > +#define DUMB_SUFFIX "             "
> >
>
> Was this change intended and if so for what purpose?  I can drop
> this hunk if this is a mere finger-slip without proofreading, but I
> do not want to do so without making sure I am not missing anything
> and not discarding a meaningful change.

This was my poor use of the tabify function. It would be nice if this
were more explicit though, maybe using a format string of

  sprintf(.. , "%*s", 8, "");

but we can leave that for another time.

> Noticing the dash in "<<-", I would have expected all of the above
> lines to be indented with a tab to align with 'w' in 'write_script'.

Done.

> > +     chmod +x .git/hooks/update &&
>
> No need for this "chmod +x"; that's one of the points in using
> write_script.

Done.

--

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 v5 2/2] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

On Mon, Aug 6, 2018 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> > +     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).
>
> Reads much better.
>
> I am still trying to find a concise way to help readers who saw a
> line that begins with "Warnings: foo bar bla" and accept that it is
> OK the early 7 chars are not painted.  "... case-insensitively and
> honoring word boundary" is the best I came up so far, but  I am
> afraid that is adding more words without hinting what I want to convey
> strongly enough, so I am not going to suggest that (at least not yet).
>

what do you think of the idea of requiring a colon explicitly? That
would avoid spurious highlighting in case a keyword happens to be
wrapped onto a line start.

--

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 v5 2/2] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

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

>> I think the code is OK without any assert() or BUG(), and that is
>> because the design is "we just paint the keyword at the beginning of
>> what the other side of the sideband wants us to show as a single
>> unit".  If the other side sends a payload with an embedded LF in a
>> single packet, that's their choice and we are free not to paint the
>> beginning of the second line after that LF.  So from that point of
>> view, perhaps we shouldn't even talk about "a single line only".
>
> I don't understand this remark. Isn't the call to strpbrk() meant to
> split the input on line endings?

Yes, but that happens only for band #2 and not band #3, to which
this coloring also applies if I am reading your changes correctly.

And I still think not splitting band #3 packet into lines and
painting only the beginning of the first line is perfectly OK.

>> >  #define ANSI_SUFFIX "\033[K"
>> > -#define DUMB_SUFFIX "        "
>> > +#define DUMB_SUFFIX "             "
>> >
>>
>> Was this change intended and if so for what purpose?  I can drop
>> this hunk if this is a mere finger-slip without proofreading, but I
>> do not want to do so without making sure I am not missing anything
>> and not discarding a meaningful change.
>
> This was my poor use of the tabify function.

OK, so I can drop this hunk---eh, perhaps v6 won't have it so I
won't have to worry about it ;-)

Thanks.