[PATCH 0/2 v3] Highlight keywords in remote sideband output.

[PATCH 0/2 v3] Highlight keywords in remote sideband output.

[Han-Wen Nienhuys]

squash in Duy's patch

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

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

--
2.18.0.345.g5c9ce644c3-goog

[PATCH 1/2] 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..d39256eb1 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);
+
+/*
+ * The int return values in the functions is 1 if not found, 0 if found, leaving
+ * the found value in teh '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.345.g5c9ce644c3-goog

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

[Eric Sunshine]

On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys <hanwen@google.com> wrote:
> diff --git a/config.h b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
> +/*
> + * The int return values in the functions is 1 if not found, 0 if found, leaving
> + * the found value in teh 'dest' pointer.
> + */

"teh"?

Instead of talking about "the int return values", simpler would be to
say "returns 1 if ...; else 0". Not worth a re-roll, though.

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

[PATCH 2/2] Highlight keywords in remote sideband output.

[Han-Wen Nienhuys]

The highlighting is done on the client-side. Supported keywords are
"error", "warning", "hint" and "success".

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

Co-authored-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                          | 113 +++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh |  47 ++++++++++++
 5 files changed, 162 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..0d67583ec 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,97 @@
 #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 },
+};
+
+static int sideband_use_color = -1;
+
+static void load_sideband_colors(void)
+{
+	const char *key = "color.remote";
+	struct strbuf sb = STRBUF_INIT;
+	char *value;
+	int i;
+
+	if (sideband_use_color >= 0)
+		return;
+
+	if (!git_config_get_string(key, &value))
+		sideband_use_color = 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);
+}
+
+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.
+ */
+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+	int i;
+
+	load_sideband_colors();
+ 	if (!want_color_stderr(sideband_use_color)) {
+		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);
+		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 +139,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 +162,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.345.g5c9ce644c3-goog

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

[Junio C Hamano]

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

> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".
>
> Co-authored-by: Duy Nguyen <pclouds@gmail.com>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>

Thanks.  I'll squash the following in while queuing, though.

 * maybe_colorize_sideband() does not have outside caller; make it
   static to avoid missing-prototype error that breaks compilation.

 * correct space-before-tab whitespace style violation.

 * use write_script.

 * a test script must be executable to avoid triggering test-lint.

 * avoid overlong lines in the test.

 * no SP between redirection operator and its target.

Other than that, the result looks good to me.  So that others can
eyeball the result once more, I'll keep it in 'pu' for a few days,
and if nothing else comes up, hopefully the topic can be merged to
'next' after that.


diff --git a/sideband.c b/sideband.c
index 0d67583ec5..be4635446c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -60,12 +60,12 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
  * Optionally highlight some keywords in remote output if they appear at the
  * start of the line.
  */
-void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 {
 	int i;
 
 	load_sideband_colors();
- 	if (!want_color_stderr(sideband_use_color)) {
+	if (!want_color_stderr(sideband_use_color)) {
 		strbuf_add(dest, src, n);
 		return;
 	}
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
old mode 100644
new mode 100755
index 4e1bd421ff..4547ec95b8
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -6,27 +6,27 @@ test_description='remote messages are colorized on the client'
 
 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 &&
+	write_script .git/hooks/update <<-\EOF &&
+	echo error: error
+	echo hint: hint
+	echo success: success
+	echo warning: warning
+	echo prefixerror: error
+	exit 0
+	EOF
+
 	echo 1 >file &&
 	git add file &&
 	git commit -m 1 &&
 	git clone . child &&
 	cd child &&
-	echo 2 > file &&
+	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 &&
+	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 &&
@@ -36,7 +36,8 @@ test_expect_success 'push' '
 '
 
 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 &&
+	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 &&

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

[Eric Sunshine]

On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys <hanwen@google.com> wrote:
> Highlight keywords in remote sideband output.

Prefix with the module you're touching, don't capitalize, and drop the
period. Perhaps:

    sideband: highlight keywords in remote sideband output

> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".

What's the motivation for this change? The commit message should say
something about that and give an explanation of why this is done
client-side rather than server-side.

> Co-authored-by: Duy Nguyen <pclouds@gmail.com>

Helped-by: is more typical.

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1229,6 +1229,15 @@ color.push::
> +color.remote::
> +       A boolean to enable/disable colored remote output. If unset,
> +       then the value of `color.ui` is used (`auto` by default).

If this is "boolean", what does "auto" mean? Perhaps update the
description to better match other color-related options.

> diff --git a/sideband.c b/sideband.c
> @@ -1,6 +1,97 @@
> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> +{
> +       int i;
> +
> +       load_sideband_colors();
> +       if (!want_color_stderr(sideband_use_color)) {
> +               strbuf_add(dest, src, n);
> +               return;
> +       }

Can load_sideband_colors() be moved below the !want_color_stderr() conditional?

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

Would it make sense to precompute each keyword length so you don't
have to recompute them repeatedly, or is that premature optimization?

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

So, the strncasecmp() is checking if one of the recognized keywords is
at the 'src' position, and the !isalnum() ensures that you won't pick
up something of which the keyword is merely a prefix. For instance,
you won't mistakenly highlight "successful". It also works correctly
when 'len' happens to reference the end-of-string NUL. Okay.

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

So, despite the explanation in the commit message, this function isn't
_generally_ highlighting keywords in the sideband. Instead, it is
highlighting a keyword only if it finds it at the start of string
(ignoring whitespace). Perhaps the commit message could be more clear
about that.

A natural follow-on question is whether strings are fed to this
function one line at a time or if the incoming string may have
embedded newlines (in which case, you might need to find a prefix
following a newline, as well?).

> +       }
> +
> +       strbuf_add(dest, src, n);
> +}

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys <hanwen@google.com> wrote:
>> Highlight keywords in remote sideband output.
>
> Prefix with the module you're touching, don't capitalize, and drop the
> period. Perhaps:
>
>     sideband: highlight keywords in remote sideband output

Yup (I locally did something similar when queued it).

>> The highlighting is done on the client-side. Supported keywords are
>> "error", "warning", "hint" and "success".
>>
>> The colorization is controlled with the config setting "color.remote".
>
> What's the motivation for this change? The commit message should say
> something about that and give an explanation of why this is done
> client-side rather than server-side.

Good suggestion.

>
>> Co-authored-by: Duy Nguyen <pclouds@gmail.com>
>
> Helped-by: is more typical.
>
>> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>> ---
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> @@ -1229,6 +1229,15 @@ color.push::
>> +color.remote::
>> +       A boolean to enable/disable colored remote output. If unset,
>> +       then the value of `color.ui` is used (`auto` by default).
>
> If this is "boolean", what does "auto" mean? Perhaps update the
> description to better match other color-related options.

Existing `color.branch` is already loose in the same way, but others
like color.{diff,grep,interactive} read better.  No, past mistakes
by others is not a good excuse to make things worse, but I'd say it
also is OK to clean this up, together with `color.branch`, after this
change on top.

>> +               if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
>
> So, the strncasecmp() is checking if one of the recognized keywords is
> at the 'src' position, and the !isalnum() ensures that you won't pick
> up something of which the keyword is merely a prefix. For instance,
> you won't mistakenly highlight "successful". It also works correctly
> when 'len' happens to reference the end-of-string NUL. Okay.

Hmm, do we actually say things like "Error: blah"?  I am not sure if
I like this strncasecmp all that much.

>> +                       strbuf_addstr(dest, p->color);
>> +                       strbuf_add(dest, src, len);
>> +                       strbuf_addstr(dest, GIT_COLOR_RESET);
>> +                       n -= len;
>> +                       src += len;
>> +                       break;
>> +               }
>
> So, despite the explanation in the commit message, this function isn't
> _generally_ highlighting keywords in the sideband. Instead, it is
> highlighting a keyword only if it finds it at the start of string
> (ignoring whitespace). Perhaps the commit message could be more clear
> about that.

Sounds good.

I didn't comment on other parts of your review posed as questions
(that require some digging and thinking), but I think they are all
worthwhile thing to think about.

Thanks.

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

[Han-Wen Nienhuys]

On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano <gitster@pobox.com> wrote:

> Hmm, do we actually say things like "Error: blah"?  I am not sure if
> I like this strncasecmp all that much.

this is for the remote end, so what we (git-core) says isn't all that
relevant. The reason I put this here is that Gerrit has some messages
that say "ERROR: .. "

> >> +                       strbuf_addstr(dest, p->color);
> >> +                       strbuf_add(dest, src, len);
> >> +                       strbuf_addstr(dest, GIT_COLOR_RESET);
> >> +                       n -= len;
> >> +                       src += len;
> >> +                       break;
> >> +               }
> >
> > So, despite the explanation in the commit message, this function isn't
> > _generally_ highlighting keywords in the sideband. Instead, it is
> > highlighting a keyword only if it finds it at the start of string
> > (ignoring whitespace). Perhaps the commit message could be more clear
> > about that.
>
> Sounds good.
>
> I didn't comment on other parts of your review posed as questions
> (that require some digging and thinking), but I think they are all
> worthwhile thing to think about.

Sorry for being dense, but do you want me to send an updated patch or
not based on your and Eric's comments or not?

thanks,
--

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] Highlight keywords in remote sideband output.

[Junio C Hamano]

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

> On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
>> Hmm, do we actually say things like "Error: blah"?  I am not sure if
>> I like this strncasecmp all that much.
>
> this is for the remote end, so what we (git-core) says isn't all that
> relevant.

It is very relevant, I would think.  Because the coloring is
controlled at the client end with this implementation, third-party
remote implementations have strong incentive to follow what our
remote end says and not to deviate.  Preventing them from being
different just to be different does help the users, no?

>> > So, despite the explanation in the commit message, this function isn't
>> > _generally_ highlighting keywords in the sideband. Instead, it is
>> > highlighting a keyword only if it finds it at the start of string
>> > (ignoring whitespace). Perhaps the commit message could be more clear
>> > about that.
>>
>> Sounds good.
>>
>> I didn't comment on other parts of your review posed as questions
>> (that require some digging and thinking), but I think they are all
>> worthwhile thing to think about.
>
> Sorry for being dense, but do you want me to send an updated patch or
> not based on your and Eric's comments or not?

It would help to see the comments responded with either "such a
change is not needed for such and such reasons", "it may make sense
but let's leave it to a follow-up patch later," etc., or with a
"here is an updated patch, taking all the comments to the previous
version into account---note that I rejected that particular comment
because of such and such reasons".

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

[Han-Wen Nienhuys]

On Wed, Aug 1, 2018 at 8:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Hmm, do we actually say things like "Error: blah"?  I am not sure if
> >> I like this strncasecmp all that much.
> >
> > this is for the remote end, so what we (git-core) says isn't all that
> > relevant.
>
> It is very relevant, I would think.  Because the coloring is
> controlled at the client end with this implementation, third-party
> remote implementations have strong incentive to follow what our
> remote end says and not to deviate.  Preventing them from being
> different just to be different does help the users, no?

But the ship has already sailed: Gerrit has been saying "ERROR"
instead of "error" for many years. In the case of Gerrit, the upper
case message is a (poor) way to make the error message stand out from
the sea of progress messages that "git push" prints on the terminal,
without requiring a newer version of git-core.

--

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] Highlight keywords in remote sideband output.

[Eric Sunshine]

On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> Han-Wen Nienhuys <hanwen@google.com> writes:
> > Sorry for being dense, but do you want me to send an updated patch or
> > not based on your and Eric's comments or not?
>
> It would help to see the comments responded with either "such a
> change is not needed for such and such reasons", "it may make sense
> but let's leave it to a follow-up patch later," etc., or with a
> "here is an updated patch, taking all the comments to the previous
> version into account---note that I rejected that particular comment
> because of such and such reasons".

Right. The way to know whether or not an updated patch is warranted is
to respond to review comments, saying that you agree or disagree with
various points raised (and why), and by answering the (genuine)
questions raised during review. The outcome of the dialogue with
reviewers will make it clear if an updated patch is necessary. (It's
also a courtesy to respond to review comments since reviewing is
time-consuming business and it's good to let reviewers know that the
time spent reviewing was not in vain.)

Regarding my question about whether load_sideband_colors() can be
moved below the '!want_color_stderr(sideband_use_color)' conditional,
after studying the code further, I see that it can't be, because
load_sideband_colors() is responsible for setting
'sideband_use_color'. The fact that this code confused or left
questions in the mind of a reviewer may indicate that responsibilities
are not partitioned in the best manner, and that the code could be
structured to be more clear.

For instance, it might make sense to rip all the 'sideband_use_color'
gunk out of load_sideband_colors() and move it to
maybe_colorize_sideband(), perhaps like this:

    void maybe_colorize_sideband(...)
    {
        /* one-time initialization */
        if (sideband_use_color < 0) {
            if (!git_config_get_string(key, &value))
                sideband_use_color = git_config_colorbool(key, value);
            if (sideband_use_color > 0)
                load_sideband_colors();
        }

        if (!want_color_stderr(sideband_use_color)) {
            strbuf_add(dest, src, n);
            return;
        }
        ...as before...
    }

You may or may not agree with the above suggestion; it's good to let
the reviewer know how you feel about it.

Your follow-on comment about how Gerrit has for years used "ERROR" is
exactly the sort of information which should be in the commit messages
since it saves reviewers (and future readers, months or years down the
road) from head-scratching, wondering why the code was written the way
it was (strcasecmp() vs. strcmp(), for instance).

The more pertinent information you can say up front in the commit
message, the less likely reviewers will be confused or wonder why you
made the choices you did. My question about whether
maybe_colorize_sideband() is fed lines one-by-one or whether its input
may contain embedded newlines is a good example of how a more complete
commit message could help. As the author of the patch, you have been
working in this code and likely know the answer, but reviewers won't
necessarily have this information at hand. If the commit message says
up front that this function processes lines one-by-one, then the
reviewer feels reassured that the patch author understands the
implications of the change (as opposed to the patch author perhaps not
having thought of the possibility of embedded newlines). So, it's a
genuine question (I still don't know the answer.)

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

[Han-Wen Nienhuys]

On Thu, Aug 2, 2018 at 12:24 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Han-Wen Nienhuys <hanwen@google.com> writes:
> > > Sorry for being dense, but do you want me to send an updated patch or
> > > not based on your and Eric's comments or not?
> >
> > It would help to see the comments responded with either "such a
> > change is not needed for such and such reasons", "it may make sense
> > but let's leave it to a follow-up patch later," etc., or with a
> > "here is an updated patch, taking all the comments to the previous
> > version into account---note that I rejected that particular comment
> > because of such and such reasons".
>
> Right. The way to know whether or not an updated patch is warranted is
> to respond to review comments, saying that you agree or disagree with
> various points raised (and why), and by answering the (genuine)
> questions raised during review. The outcome of the dialogue with
> reviewers will make it clear if an updated patch is necessary. (It's
> also a courtesy to respond to review comments since reviewing is
> time-consuming business and it's good to let reviewers know that the
> time spent reviewing was not in vain.)

Sure. My doubt is that it's hard to tell what the state of my patch is
at any given time.

--

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] Highlight keywords in remote sideband output.

[Eric Sunshine]

On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> Sure. My doubt is that it's hard to tell what the state of my patch is
> at any given time.

Understandable. Junio sends out a periodic "What's cooking" email
summarizing the state of patches sent to the list. The most recent one
is here [1].

A patch series may be annotated with "will merge to next" (meaning he
thinks it's stable and probably ready to expose to a wider audience),
"waiting for review" (meaning not enough people have shown interest to
look it over), "waiting for a re-roll" (meaning he expects a new
version). Other annotations are possible. If a patch series isn't
annotated, it means he hasn't made any decision about it yet.

Also, if your patch series is not in "What's cooking", it may mean
that Junio just hasn't gotten around to your submission yet, and there
is a good chance it will be in the following "What's cooking". If,
however, a couple weeks or more elapse and your submission doesn't
show up, then it may have gotten lost in the noise, in which case it's
a good idea to send a "ping" as a reminder.

[1]: https://public-inbox.org/git/xmqqd0vbt14e.fsf@gitster-ct.c.googlers.com/

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

[Stefan Beller]

On Thu, Aug 2, 2018 at 10:37 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> > Sure. My doubt is that it's hard to tell what the state of my patch is
> > at any given time.
>
> Understandable. Junio sends out a periodic "What's cooking" email
> summarizing the state of patches sent to the list. The most recent one
> is here [1].

Please contrast that with [2], specifically:

  The discussion thread in the list archive is
  the authoritative record of the discussion; treat "What's cooking"
  as my personal note, nothing more.

[2] https://public-inbox.org/git/xmqqsh3x69ap.fsf@gitster-ct.c.googlers.com/

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

[Han-Wen Nienhuys]

On Tue, Jul 31, 2018 at 10:21 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys <hanwen@google.com> wrote:
> > Highlight keywords in remote sideband output.
>
> Prefix with the module you're touching, don't capitalize, and drop the
> period. Perhaps:

Done.

>     sideband: highlight keywords in remote sideband output
>
> > The highlighting is done on the client-side. Supported keywords are
> > "error", "warning", "hint" and "success".
> >
> > The colorization is controlled with the config setting "color.remote".
>
> What's the motivation for this change? The commit message should say
> something about that and give an explanation of why this is done
> client-side rather than server-side.

Added

> > Co-authored-by: Duy Nguyen <pclouds@gmail.com>
>
> Helped-by: is more typical.

Done.

> > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> > ---
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > @@ -1229,6 +1229,15 @@ color.push::
> > +color.remote::
> > +       A boolean to enable/disable colored remote output. If unset,
> > +       then the value of `color.ui` is used (`auto` by default).
>
> If this is "boolean", what does "auto" mean? Perhaps update the
> description to better match other color-related options.

All other doc entries say "boolean" here too. I'm happy to fix
phrasing of this file in a follow-on change, but let's keep it like
this for consistency.

> > diff --git a/sideband.c b/sideband.c
> > @@ -1,6 +1,97 @@
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> > +{
> > +       int i;
> > +
> > +       load_sideband_colors();
> > +       if (!want_color_stderr(sideband_use_color)) {
> > +               strbuf_add(dest, src, n);
> > +               return;
> > +       }
>
> Can load_sideband_colors() be moved below the !want_color_stderr() conditional?

Reorganized this a bit.

> > +
> > +       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);
>
> Would it make sense to precompute each keyword length so you don't
> have to recompute them repeatedly, or is that premature optimization?

That is premature optimization.  The next line does a strncasecmp on
the same data so the cost (loading the keywords into CPU cache) is the
same, while precomputing the length makes the code more error prone.

> > +               if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
>
> So, the strncasecmp() is checking if one of the recognized keywords is
> at the 'src' position, and the !isalnum() ensures that you won't pick
> up something of which the keyword is merely a prefix. For instance,
> you won't mistakenly highlight "successful". It also works correctly
> when 'len' happens to reference the end-of-string NUL. Okay.

added comment.

> > +                       strbuf_addstr(dest, p->color);
> > +                       strbuf_add(dest, src, len);
> > +                       strbuf_addstr(dest, GIT_COLOR_RESET);
> > +                       n -= len;
> > +                       src += len;
> > +                       break;
> > +               }
>
> So, despite the explanation in the commit message, this function isn't
> _generally_ highlighting keywords in the sideband. Instead, it is
> highlighting a keyword only if it finds it at the start of string
> (ignoring whitespace). Perhaps the commit message could be more clear
> about that.

updated message.

> A natural follow-on question is whether strings are fed to this
> function one line at a time or if the incoming string may have
> embedded newlines (in which case, you might need to find a prefix
> following a newline, as well?).

added 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