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

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

[Junio C Hamano]

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

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

Should I forge your sign-off on this patch?

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

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

[Junio C Hamano]

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

> The colorization is controlled with the config setting "color.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.

Thanks; quite readable.

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

I'll "chmod +x" while queuing.

	Side note: When all problems pointed out are "I'll fix it
	this way while queuing"-kind, and if you agree to the way I
	plan to fix them up, then just saying so is sufficient and
	you do not have to send a new version of the patch(es).

If your "make test" did not catch this as an error, then we may need
to fix t/Makefile, as it is supposed to run test-lint.

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

Nobody tells the end-users what "colored remote output" does;
arguably they can find it out themselves by enabling the feature and
observing remote messages, but that is not user friendly.

	When running commands like `git fetch` and `git push` that
	interact with a remote repository, certain keywords (see
	`color.remote.<slot>`) that appear at the beginning of a
	line of message from the remote end can be painted in color.

	This configuration variable is a boolean to enable/disable
	the feature.  If unset...
	
or something like that, perhaps?

You use `always` in your tests to a good effect, but what the value
means is not described here.

> +color.remote.<slot>::
> +	Use customized color for each remote keywords. `<slot>` may be

Isn't 'each' a singular, i.e. "for each remote keyword"?  If so I do
not mind dropping 's' myself while queuing.

> +	`hint`, `warning`, `success` or `error` which match the
> +	corresponding keyword.

We need to say that keywords are painted case insensitively
somewhere in the doc.  Either do that here, or in the updated
description of `color.remote`---I am not sure which one results in
more readable text offhand.

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

In a block with a dozen more more lines, it is easier to have a
blank line between decls and the first statement, i.e. here.

> + 	if (!want_color_stderr(use_sideband_colors())) {

The above line is indented with SP followed by HT; don't.

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

	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.
> +                 */

Indent with tabs, not a run of spaces, i.e. 

		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])) {

The code in this function looked OK otherwise.

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

Use write_script, i.e. instead of all the above, say

	write_script .git/hooks/update <<-\EOF &&
	echo error: error
	echo hint: hint
	...
	exit 0
	EOF

so that you do not have to know or care the case where /bin/sh is
not the builder's preferred shell, etc.

Our tests are not written to demonstrate that our code works as
written.  It is to protect our code from getting broken by others
who may not share vision of the original author.  Make sure that you
cast what you care about in stone, e.g. include "echo ERROR: bad" or
something in the above to ensure that future updates to the code
will not turn the match into a case sensitive one without breaking
the test suite.

> +	echo 1 >file &&
> +	git add file &&
> +	git commit -m 1 &&
> +	git clone . child &&
> +	cd child &&
> +	echo 2 > file &&
> +	git commit -a -m 2

Don't chdir the whole testing environment like this.  Depending on
the success and failure in the middle of the above &&-chain, the
next test will start at an unexpected place, which is bad.

Instead, do something like

	git clone . child &&
	echo 2 >child/file &&
	git -C child commit -a -m 2

or

	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

A comment before this test (which covers both of these two) that
explains why many "grep" invocations are necessary, instead of a
comparison with a single multi-line expected result file.  I am
guessing that it is *not* because you cannot rely on the order of
these lines coming out from the update hook, but because the remote
output have lines other than what is given by the update hook and
we cannot afford to write them in the expected result file.

A more readable alternative might be to prepare the message that
come from the other side more parseable, e.g.

	write_script .git/hooks/update <<-\EOF &&
	echo 'BEGIN HOOK OUTPUT
	error: error
	Error: another error
	...
	END HOOK OUTPUT'
	EOF

and then

	sed -ne "/BEGIN HOOK OUTPUT/,/END HOOK OUTPUT/p" output |
	test_decode_color >decoded &&
	cat >expect <<-\EOF &&
	BEGIN HOOK OUTPUT
	<BOLD;RED>error<RESET>: error
	<BOLD;RED>Error<RESET>: another error
	...
	END HOOK OUTPUT
	EOF
	test_cmp expect decoded

Then we do not have to make readers puzzled why we are using bunch
of "grep" and do not explain with an extra comment.

Thanks.

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

[Han-Wen Nienhuys]

On Thu, Aug 2, 2018 at 8:22 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > 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
>
> I'll "chmod +x" while queuing.
>
Done.

> If your "make test" did not catch this as an error, then we may need
> to fix t/Makefile, as it is supposed to run test-lint.

I've been running tests individually as

 sh t5409-colorize-remote-messages.sh  -v -d

> > +color.remote::
> > +     A boolean to enable/disable colored remote output. If unset,
> > +     then the value of `color.ui` is used (`auto` by default).
>
> Nobody tells the end-users what "colored remote output" does;
> arguably they can find it out themselves by enabling the feature and
> observing remote messages, but that is not user friendly.

expanded doc.

> > +color.remote.<slot>::
> > +     Use customized color for each remote keywords. `<slot>` may be
>
> Isn't 'each' a singular, i.e. "for each remote keyword"?  If so I do
> not mind dropping 's' myself while queuing.

Done.

>
> > +     `hint`, `warning`, `success` or `error` which match the
> > +     corresponding keyword.
>
> We need to say that keywords are painted case insensitively
> somewhere in the doc.  Either do that here, or in the updated
> description of `color.remote`---I am not sure which one results in
> more readable text offhand.

Done.

> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> > +{
> > +     int i;
>
> In a block with a dozen more more lines, it is easier to have a
> blank line between decls and the first statement, i.e. here.

Done.

> > +     if (!want_color_stderr(use_sideband_colors())) {
>
> The above line is indented with SP followed by HT; don't.

Fixed. It would be great if there were a pre-commit hook that I could
install to prevent this from ever getting committed.


> > +             struct kwtable* p = keywords + i;
>
>         struct kwtable *p = keywords + i;

Done.

> > +             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.
> > +                 */
>
> Indent with tabs, not a run of spaces, i.e.

Done.

> Use write_script, i.e. instead of all the above, say

Done.


> Our tests are not written to demonstrate that our code works as
> written.  It is to protect our code from getting broken by others
> who may not share vision of the original author.  Make sure that you
> cast what you care about in stone, e.g. include "echo ERROR: bad" or
> something in the above to ensure that future updates to the code
> will not turn the match into a case sensitive one without breaking
> the test suite.

Add some more cases.

> > +     echo 1 >file &&
> > +     git add file &&
> > +     git commit -m 1 &&
> > +     git clone . child &&
> > +     cd child &&
> > +     echo 2 > file &&
> > +     git commit -a -m 2
>
> Don't chdir the whole testing environment like this.  Depending on
> the success and failure in the middle of the above &&-chain, the
> next test will start at an unexpected place, which is bad.
>
> Instead, do something like
>
>         git clone . child &&
>         echo 2 >child/file &&
>         git -C child commit -a -m 2
>
> or
>
Done.

> > +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
>
> A comment before this test (which covers both of these two) that
> explains why many "grep" invocations are necessary, instead of a
> comparison with a single multi-line expected result file.  I am
> guessing that it is *not* because you cannot rely on the order of
> these lines coming out from the update hook, but because the remote
> output have lines other than what is given by the update hook and
> we cannot afford to write them in the expected result file.

Comparing exact outputs is IMO an antipattern in general. It makes the
test more fragile than they need to be. (what if we change the
"remote: " prefix to something else for example?).

If using a golden output, the second test would either require a
repetitive, too long golden file, or it would use loose greps anyway.

Added a comment.

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

[Junio C Hamano]

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

>> If your "make test" did not catch this as an error, then we may need
>> to fix t/Makefile, as it is supposed to run test-lint.
>
> I've been running tests individually as
>
>  sh t5409-colorize-remote-messages.sh  -v -d

During your own development that may be sufficient, but do run "make
test" to run tests by other people; it will help you catch new bugs
you are introducing whey they notice their expectations are broken.

You are breaking 5541 (there may be others, but I noticed a breakage
there) perhaps with your debugging cruft.

[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

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