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

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

[Han-Wen Nienhuys]

Address Eric Sunshine's comments.

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

 Documentation/config.txt            |   9 +++
 config.h                            |  10 ++-
 help.c                              |   1 +
 help.h                              |   1 +
 sideband.c                          | 119 +++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh |  47 +++++++++++
 6 files changed, 176 insertions(+), 11 deletions(-)
 create mode 100644 t/t5409-colorize-remote-messages.sh

--
2.18.0.597.ga71716f1ad-goog

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

[Han-Wen Nienhuys]

Address Eric Sunshine's comments.

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

 Documentation/config.txt            |   9 +++
 config.h                            |  10 ++-
 help.c                              |   1 +
 help.h                              |   1 +
 sideband.c                          | 119 +++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh |  47 +++++++++++
 6 files changed, 176 insertions(+), 11 deletions(-)
 create mode 100644 t/t5409-colorize-remote-messages.sh

--
2.18.0.597.ga71716f1ad-goog

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

[Han-Wen Nienhuys]

---
 config.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/config.h b/config.h
index b95bb7649..41700f40b 100644
--- a/config.h
+++ b/config.h
@@ -178,10 +178,16 @@ 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_add_file(struct config_set *cs, const char *filename);
+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 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 rationale 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 Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process), and my hypothesis is that these obscure the actionable error
messages that our server sends back. Highlighting keywords in the
draws more attention to actionable 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            |   9 +++
 help.c                              |   1 +
 help.h                              |   1 +
 sideband.c                          | 119 +++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh |  47 +++++++++++
 5 files changed, 168 insertions(+), 9 deletions(-)
 create mode 100644 t/t5409-colorize-remote-messages.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43b2de7b5..0783323be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1229,6 +1229,15 @@ color.push::
 color.push.error::
 	Use customized color for push errors.
 
+color.remote::
+	A boolean to enable/disable colored remote output. If unset,
+	then the value of `color.ui` is used (`auto` by default).
+
+color.remote.<slot>::
+	Use customized color for each remote keywords. `<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 3ebf0568d..b6cafcfc0 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 f8b15323a..9eab6a3f8 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 325bf0e97..5c72db83c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,103 @@
 #include "cache.h"
+#include "color.h"
+#include "config.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "help.h"
+
+struct kwtable {
+	/*
+	 * We use keyword as config key so it can't contain funny chars like
+	 * spaces. When we do that, we need a separate field for slot name in
+	 * load_sideband_colors().
+	 */
+	const char *keyword;
+	char color[COLOR_MAXLEN];
+};
+
+static struct kwtable 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);
+
+	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(_("expecting a color: %s"), 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 some keywords in remote output if they appear at the
+ * start of the line. This should be called for a single line only.
+ */
+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 kwtable* 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 +145,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 +168,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 100644
index 000000000..4e1bd421f
--- /dev/null
+++ b/t/t5409-colorize-remote-messages.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+
+test_description='remote messages are colorized on the client'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir .git/hooks &&
+	cat << EOF > .git/hooks/update &&
+#!/bin/sh
+echo error: error
+echo hint: hint
+echo success: success
+echo warning: warning
+echo prefixerror: error
+exit 0
+EOF
+	chmod +x .git/hooks/update &&
+	echo 1 >file &&
+	git add file &&
+	git commit -m 1 &&
+	git clone . child &&
+	cd child &&
+	echo 2 > file &&
+	git commit -a -m 2
+'
+
+test_expect_success 'push' '
+	git -c color.remote=always push -f origin HEAD:refs/heads/newbranch 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<BOLD;RED>error<RESET>:" decoded &&
+	grep "<YELLOW>hint<RESET>:" decoded &&
+	grep "<BOLD;GREEN>success<RESET>:" decoded &&
+	grep "<BOLD;YELLOW>warning<RESET>:" decoded &&
+	grep "prefixerror: error" decoded
+'
+
+test_expect_success 'push with customized color' '
+	git -c color.remote=always -c color.remote.error=white push -f origin HEAD:refs/heads/newbranch2 2>output &&
+	test_decode_color <output >decoded &&
+	grep "<WHITE>error<RESET>:" decoded &&
+	grep "<YELLOW>hint<RESET>:" decoded &&
+	grep "<BOLD;GREEN>success<RESET>:" decoded &&
+	grep "<BOLD;YELLOW>warning<RESET>:" decoded
+'
+
+test_done
-- 
2.18.0.597.ga71716f1ad-goog

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

[Eric Sunshine]

On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> diff --git a/config.h b/config.h
> @@ -178,10 +178,16 @@ 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);
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +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 **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);

It doesn't seem like git_configset_add_file() fits in this group. It's
neither searching for something (thus won't return "found" / "not
found"), nor is it returning 0 or 1. (It returns 0 on success and -1
on failure.)

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

[Jonathan Nieder]

(cc-ing peff, config whiz)
Hi,

Han-Wen Nienhuys wrote:

> Subject: config: document git config getter return value.

micronit: no period at the end of subject line

> ---
>  config.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

May we forge your sign-off?  See Documentation/SubmittingPatches section
[[sign-off]] for what this means.

> 
> diff --git a/config.h b/config.h
> index b95bb7649..41700f40b 100644
> --- a/config.h
> +++ b/config.h
> @@ -178,10 +178,16 @@ 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);
> +

nit: this blank line feels out of place (though I don't particularly
mind it and wouldn't reroll just for that)

>  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_add_file(struct config_set *cs, const char *filename);

This function doesn't take a 'dest' argument.  Is the comment meant to
apply to it?  If not, can the comment go below it?

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

With a sign-off and whatever subset of the above suggestions makes sense
to you,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

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

[Eric Sunshine]

On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> [PATCH 2/2] sideband: highlight keywords in remote sideband output

The -v option of git-format-patch makes it easy to let reviewers know
that this is a new version of a previously-submitted patch series.
This (I think) is the second attempt; if you need to send again, you
could use "git format-patch -v3 ...", which would result in "[PATCH v3
2/2] ...".

> 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 rationale 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.
> [...snip...]

Thanks, this commit message is much more helpful than the previous one.

If you end up re-rolling, you might consider swapping the order of
these paragraphs around a bit to first explain the problem the patch
is solving (i.e. the justification), then the solution and relevant
details. Documentation/SubmittingPatches has good advice about this.

Using Gerrit as the sole rationale is underselling this otherwise
general improvement. Instead, the commit message could sell the change
on its own merits and can then use Gerrit as an example.

> The Git push process itself prints lots of non-actionable messages
> (eg. bandwidth statistics, object counters for different phases of the
> process), and my hypothesis is that these obscure the actionable error
> messages that our server sends back. Highlighting keywords in the
> draws more attention to actionable messages.

So, for instance, you might want to use a rewrite of this paragraph to
open the commit message. Something like this, perhaps:

    A remote Git operation can print many non-actionable messages
    (e.g. bandwidth statistics, object counters for different phases
    of the process, etc.) and such noise can obscure genuine
    actionable messages, such as warnings and outright errors.

    As an example, Gerrit does server-side processing to create or
    update code reviews, and actionable error messages (such as
    "ERROR: commit is missing Change-Id") must be communicated back to
    the user during a push operation, but new users often have trouble
    spotting these error messages amid the noise.

    Address this problem by upgrading 'sideband' to draw attention to
    these messages by highlighting them with color.

    [...and so forth...]

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> diff --git a/sideband.c b/sideband.c
> @@ -1,6 +1,103 @@
> +struct kwtable {
> +       /*
> +        * We use keyword as config key so it can't contain funny chars like
> +        * spaces. When we do that, we need a separate field for slot name in
> +        * load_sideband_colors().
> +        */

This comment is outdated; load_sideband_colors() does not exist in this patch.

I get what the first sentence of this comment is saying, but I'm
having trouble understanding what the second sentence is all about.

> +       const char *keyword;
> +       char color[COLOR_MAXLEN];
> +};
> +
> +static struct kwtable 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).

Use /* old-style C comments */ in this codebase, not // C++ or new-style C.

> +static int use_sideband_colors(void)
> +{
> +       static int use_sideband_colors_cached = -1;
> +       const char *key = "color.remote";
> +
> +       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);

So, if "color.remote" exists, then 'use_sideband_colors_cached' is
assigned. However, if "color.remote" does not exist, then it's never
assigned. Is this intended behavior? It seems like you'd want to fall
back to some other value rather than leaving it unassigned.

Which leads to the next question: The documentation says that this
falls back to "color.ui" if "color.remote" does not exist, however,
where is this fallback happening? Am I overlooking something obvious?

> +       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(_("expecting a color: %s"), value);

This error message would be much more helpful if it also told the user
the name of the config key from which 'value' came.

> +       }

Do you need to be doing the work in the above for-loop if, after
computing 'use_sideband_colors_cached', it is determined that you
won't be using color?

> +       strbuf_release(&sb);
> +       return use_sideband_colors_cached;
> +}
> diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
> @@ -0,0 +1,47 @@
> +test_expect_success 'setup' '
> +       mkdir .git/hooks &&
> +       cat << EOF > .git/hooks/update &&
> +#!/bin/sh
> +echo error: error
> +echo hint: hint
> +echo success: success
> +echo warning: warning
> +echo prefixerror: error

I addition to checking other cases, such as "ERROR", as Junio
suggested, I'd think you'd also want to test:

    hinting: hint

to verify that it doesn't get colored.

> +exit 0
> +EOF

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

[Jonathan Nieder]

Hi,

Han-Wen Nienhuys wrote:

> 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

A few questions:

- should this be documented somewhere in Documentation/technical/*protocol*?
  That way, server implementors can know to take advantage of it.

- how does this interact with (future) internationalization of server
  messages?  If my server knows that the client speaks French, should
  they send "ERROR:" anyway and rely on the client to translate it?
  Or are we deferring that question to a later day?

[...]
> The Git push process itself prints lots of non-actionable messages
> (eg. bandwidth statistics, object counters for different phases of the
> process), and my hypothesis is that these obscure the actionable error
> messages that our server sends back. Highlighting keywords in the
> draws more attention to actionable messages.

I'd be interested in ways to minimize Git's contribution to this as
well.  E.g. can we make more use of \r to make client-produced progress
messages take less screen real estate?  Should some of the servers
involved (e.g., Gerrit) do so as well?

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

As a strawman idea, what would you think of a way to allow the server
to do some coloration by using color tags like

 <red>Erreur</red>: ...

?

As an advantage, this would give the server more control.  As a
disadvantage, it is not so useful as "semantic markup", meaning it is
harder to figure out how to present to accessibility tools in a
meaningful way.  Plus, there's the issue of usefulness with existing
servers you mentioned:

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

Yes, this seems like a compelling reason to follow this approach.

[...]
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1229,6 +1229,15 @@ color.push::
>  color.push.error::
>  	Use customized color for push errors.
>  
> +color.remote::
> +	A boolean to enable/disable colored remote output. If unset,
> +	then the value of `color.ui` is used (`auto` by default).
> +
> +color.remote.<slot>::
> +	Use customized color for each remote keywords. `<slot>` may be
> +	`hint`, `warning`, `success` or `error` which match the
> +	corresponding keyword.

What positions in a remote message are matched?  If a server prints a
message about something being discouraged because it is error-prone,
would the "error" in error-prone turn red?

[...]
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,6 +1,103 @@
>  #include "cache.h"
> +#include "color.h"
> +#include "config.h"
>  #include "pkt-line.h"
>  #include "sideband.h"
> +#include "help.h"
> +
> +struct kwtable {

nit: perhaps kwtable_entry?

> +	/*
> +	 * We use keyword as config key so it can't contain funny chars like
> +	 * spaces. When we do that, we need a separate field for slot name in
> +	 * load_sideband_colors().
> +	 */
> +	const char *keyword;
> +	char color[COLOR_MAXLEN];
> +};
> +
> +static struct kwtable keywords[] = {
> +	{ "hint",	GIT_COLOR_YELLOW },
[...]
> +// Returns a color setting (GIT_COLOR_NEVER, etc).

nit: Git uses C89-style /* */ comments.

> +static int use_sideband_colors(void)

Makes sense.

[...]
> +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);
> +}

Not about this patch: I wonder if we can eliminate this boilerplate some
time in the future.

[...]
> +/*
> + * Optionally highlight some keywords in remote output if they appear at the
> + * start of the line. This should be called for a single line only.
> + */
> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

Can this be static?

What does 'n' represent?  Can the comment say?  (Or if it's the length
of src, can it be renamed to srclen?)

Super-pedantic nit: even if there are multiple special words at the
start, this will only highlight one. :)  So it could say something
like "Optionally check if the first word on the line is a keyword and
highlight it if so."

> +{
> +	int i;
> + 	if (!want_color_stderr(use_sideband_colors())) {

nit: whitespace damage (you can check for this with "git show --check").
There's a bit more elsewhere.

> +		strbuf_add(dest, src, n);
> +		return;
> +	}
> +
> +	while (isspace(*src)) {
> +		strbuf_addch(dest, *src);
> +		src++;
> +		n--;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> +		struct kwtable* p = keywords + i;

nit: * should attach to the variable, C-style.

You can use "make style" to do some automatic formatting (though this
is a bit experimental, so do double-check the results).

> +		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;
> +		}

Sensible.

[...]
> @@ -48,8 +145,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 +168,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);

Nice.  This looks cleaner than what was there before, which is always
a good sign.

[...]
> --- /dev/null
> +++ b/t/t5409-colorize-remote-messages.sh

Thanks for these.  It may make sense to have a test with some leading
whitespace as well.

[...]
> +test_expect_success 'setup' '
> +	mkdir .git/hooks &&
> +	cat << EOF > .git/hooks/update &&
> +#!/bin/sh
> +echo error: error
> +echo hint: hint
> +echo success: success
> +echo warning: warning
> +echo prefixerror: error
> +exit 0
> +EOF
> +	chmod +x .git/hooks/update &&

Please use write_script for this, since on esoteric platforms it picks
an appropriate shell.

If you use <<-\EOF instead of <<EOF, that has two advantages:
- it lets you indent the script
- it saves the reviewer from having to look for $ escapes inside the
  script body

> +	echo 1 >file &&
> +	git add file &&
> +	git commit -m 1 &&

This can use test_commit.  See t/README for details.

[...]
> +test_expect_success 'push' '
> +	git -c color.remote=always push -f origin HEAD:refs/heads/newbranch 2>output &&
> +	test_decode_color <output >decoded &&
> +	grep "<BOLD;RED>error<RESET>:" decoded &&
> +	grep "<YELLOW>hint<RESET>:" decoded &&
> +	grep "<BOLD;GREEN>success<RESET>:" decoded &&
> +	grep "<BOLD;YELLOW>warning<RESET>:" decoded &&
> +	grep "prefixerror: error" decoded
> +'
> +
> +test_expect_success 'push with customized color' '
> +	git -c color.remote=always -c color.remote.error=white push -f origin HEAD:refs/heads/newbranch2 2>output &&
> +	test_decode_color <output >decoded &&
> +	grep "<WHITE>error<RESET>:" decoded &&
> +	grep "<YELLOW>hint<RESET>:" decoded &&
> +	grep "<BOLD;GREEN>success<RESET>:" decoded &&
> +	grep "<BOLD;YELLOW>warning<RESET>:" decoded
> +'

Nice.

Thanks and hope that helps,
Jonathan

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

[Jeff King]

On Thu, Aug 02, 2018 at 08:29:48PM -0700, Jonathan Nieder wrote:

> (cc-ing peff, config whiz)

Actually, this is all about the configset caching layer, which I never
really worked on. This is mostly from Tanay Abhra <tanayabh@gmail.com>,
who was a GSoC student mentored by Matthieu Moy <Matthieu.Moy@imag.fr>.

That said...

> > +
> > +/*
> > + * These functions return 1 if not found, and 0 if found, leaving the found
> > + * value in the 'dest' pointer.
> > + */
> > +extern int git_configset_add_file(struct config_set *cs, const char *filename);
> 
> This function doesn't take a 'dest' argument.  Is the comment meant to
> apply to it?  If not, can the comment go below it?

This is returning the value of git_config_from_file(), which is 0/-1. I
think it should be left where it is in the original, and not part of
this block of functions.

Other than that, the patch seems sane to me (I think the 0/1 return
value from these "get" functions is unnecessarily inconsistent with the
rest of Git, but changing it would be a pain. Documenting it is at least
a step forward).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This is returning the value of git_config_from_file(), which is 0/-1. I
> think it should be left where it is in the original, and not part of
> this block of functions.
>
> Other than that, the patch seems sane to me (I think the 0/1 return
> value from these "get" functions is unnecessarily inconsistent with the
> rest of Git, but changing it would be a pain. Documenting it is at least
> a step forward).

I do not think changing it would be in the scope of this series.

It makes sense to document (1) that zero is success, non-zero is
failure, to help those who are reading the current callers and
adding new callers, and (2) that we are aware that the exact
"non-zero" value chosen for these are not in line with the rest of
the system.  The latter can just be a "NEEDSWORK" comment.

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

[Han-Wen Nienhuys]

On Fri, Aug 3, 2018 at 5:52 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Han-Wen Nienhuys wrote:
>
> > 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
>
> A few questions:
>
> - should this be documented somewhere in Documentation/technical/*protocol*?
>   That way, server implementors can know to take advantage of it.

The protocol docs seem to work on a much different level. Maybe there
should be a "best practices" document or similar?

> - how does this interact with (future) internationalization of server
>   messages?  If my server knows that the client speaks French, should
>   they send "ERROR:" anyway and rely on the client to translate it?
>   Or are we deferring that question to a later day?

It doesn't, and we are deferring that question.

> [...]
> > The Git push process itself prints lots of non-actionable messages
> > (eg. bandwidth statistics, object counters for different phases of the
> > process), and my hypothesis is that these obscure the actionable error
> > messages that our server sends back. Highlighting keywords in the
> > draws more attention to actionable messages.
>
> I'd be interested in ways to minimize Git's contribution to this as
> well.  E.g. can we make more use of \r to make client-produced progress
> messages take less screen real estate?  Should some of the servers
> involved (e.g., Gerrit) do so as well?

Yes, I'm interested in this too, but I fear it would veer into a
territory that is prone to bikeshedding.

Gerrit should definitely also send less noise.

> > 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.
>
> Yes, this seems like a compelling reason to follow this approach.
>
> [...]
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1229,6 +1229,15 @@ color.push::
> >  color.push.error::
> >       Use customized color for push errors.
> >
> > +color.remote::
> > +     A boolean to enable/disable colored remote output. If unset,
> > +     then the value of `color.ui` is used (`auto` by default).
> > +
> > +color.remote.<slot>::
> > +     Use customized color for each remote keywords. `<slot>` may be
> > +     `hint`, `warning`, `success` or `error` which match the
> > +     corresponding keyword.
>
> What positions in a remote message are matched?  If a server prints a
> message about something being discouraged because it is error-prone,
> would the "error" in error-prone turn red?

yes, if it happened to occur after a line-break.

We could decide that we will only highlight

  <sp*><keyword>':' rest of line

or

  <sp*><keyword>'\n'

that would work for the Gerrit case, and would be useful forcing
function to uniformize remote error messages.

> > +struct kwtable {
>
> nit: perhaps kwtable_entry?

done.

> > +/*
> > + * Optionally highlight some keywords in remote output if they appear at the
> > + * start of the line. This should be called for a single line only.
> > + */
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>
> Can this be static?

Done.

> What does 'n' represent?  Can the comment say?  (Or if it's the length
> of src, can it be renamed to srclen?)

Added comment.

> Super-pedantic nit: even if there are multiple special words at the
> start, this will only highlight one. :)  So it could say something
> like "Optionally check if the first word on the line is a keyword and
> highlight it if so."

Super pedantic answer: if people care about it that much, they can
read the 20 lines of code below :-)

> > +{
> > +     int i;
> > +     if (!want_color_stderr(use_sideband_colors())) {
>
> nit: whitespace damage (you can check for this with "git show --check").
> There's a bit more elsewhere.

ran tabify on whole file.

> > +     for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > +             struct kwtable* p = keywords + i;
>
> nit: * should attach to the variable, C-style.

Done.

> You can use "make style" to do some automatic formatting (though this
> is a bit experimental, so do double-check the results).

sad panda:

hanwen@han-wen:~/vc/git$ make style
git clang-format --style file --diff --extensions c,h
Traceback (most recent call last):
  File "/usr/bin/git-clang-format", line 580, in <module>
    main()
  File "/usr/bin/git-clang-format", line 62, in main
    config = load_git_config()
  File "/usr/bin/git-clang-format", line 195, in load_git_config
    name, value = entry.split('\n', 1)
ValueError: need more than 1 value to unpack
Makefile:2663: recipe for target 'style' failed
make: *** [style] Error 1

> [...]
> > @@ -48,8 +145,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 +168,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);
>
> Nice.  This looks cleaner than what was there before, which is always
> a good sign.
>
> [...]
> > --- /dev/null
> > +++ b/t/t5409-colorize-remote-messages.sh
>
> Thanks for these.  It may make sense to have a test with some leading
> whitespace as well.

Done.

> > +     chmod +x .git/hooks/update &&
>
> Please use write_script for this, since on esoteric platforms it picks
> an appropriate shell.

> If you use <<-\EOF instead of <<EOF, that has two advantages:
> - it lets you indent the script
> - it saves the reviewer from having to look for $ escapes inside the
>   script body

Done (#TIL).

> > +     echo 1 >file &&
> > +     git add file &&
> > +     git commit -m 1 &&
>
> This can use test_commit.  See t/README for details.

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