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

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Jul 31, 2018 at 09:37:51AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Jul 30 2018, Han-Wen Nienhuys wrote:
> 
> 
> > +	if (sideband_use_color < 0) {
> > +		const char *key = "color.remote";
> > +		char *value = NULL;
> > +		if (!git_config_get_string(key, &value))
> > +			sideband_use_color = git_config_colorbool(key, value);
> > [...]
> > +	struct kwtable {
> > +		const char *keyword;
> > +		const char *color;
> > +	} keywords[] = {
> > +		{"hint", GIT_COLOR_YELLOW},
> > +		{"warning", GIT_COLOR_BOLD_YELLOW},
> > +		{"success", GIT_COLOR_BOLD_GREEN},
> > +		{"error", GIT_COLOR_BOLD_RED},
> 
> 
> FWIW I agree with other reviewers that it would be nice if these could
> be customized, but I also think it can wait for some follow-up patch.
> 
> Users who don't like these colors don't have to use them, and then
> they're no worse off than now, whereas some users will appreciate these
> and be better off than now.
> 
> So great if you want to improve this, but just chiming in on that point
> because I think we should be respectful of the time of contributors, and
> not make perfect the enemy of the good.

Fair enough. I'll scratch my own itch. Can we squash this in then?

-- 8< --
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43b2de7b5f..0783323bec 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 3ebf0568db..b6cafcfc0a 100644
--- a/help.c
+++ b/help.c
@@ -425,6 +425,7 @@ void list_config_help(int for_human)
 		{ "color.diff", "<slot>", list_config_color_diff_slots },
 		{ "color.grep", "<slot>", list_config_color_grep_slots },
 		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
+		{ "color.remote", "<slot>", list_config_color_sideband_slots },
 		{ "color.status", "<slot>", list_config_color_status_slots },
 		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
 		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
diff --git a/help.h b/help.h
index f8b15323a6..9eab6a3f89 100644
--- a/help.h
+++ b/help.h
@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list, const char *prefix);
 void list_config_color_grep_slots(struct string_list *list, const char *prefix);
 void list_config_color_interactive_slots(struct string_list *list, const char *prefix);
 void list_config_color_status_slots(struct string_list *list, const char *prefix);
+void list_config_color_sideband_slots(struct string_list *list, const char *prefix);
 void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);
 
 #endif /* HELP_H */
diff --git a/sideband.c b/sideband.c
index 74b2fcaf64..61c4376a64 100644
--- a/sideband.c
+++ b/sideband.c
@@ -3,7 +3,59 @@
 #include "config.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "help.h"
+
+struct kwtable {
+	/*
+	 * Note, we current 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
@@ -11,24 +63,9 @@
  */
 static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 {
-	static int sideband_use_color = -1;
 	int i;
-	struct kwtable {
-		const char *keyword;
-		const char *color;
-	} keywords[] = {
-		{"hint", GIT_COLOR_YELLOW},
-		{"warning", GIT_COLOR_BOLD_YELLOW},
-		{"success", GIT_COLOR_BOLD_GREEN},
-		{"error", GIT_COLOR_BOLD_RED},
-	};
-
-	if (sideband_use_color < 0) {
-		const char *key = "color.remote";
-		char *value = NULL;
-		if (!git_config_get_string(key, &value))
-			sideband_use_color = git_config_colorbool(key, value);
-	}
+
+	load_sideband_colors();
 
 	if (!want_color_stderr(sideband_use_color)) {
 		strbuf_add(dest, src, n);
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 606386f4fe..69b7cc571d 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -6,7 +6,7 @@ test_description='remote messages are colorized on the client'
 
 test_expect_success 'setup' '
 	mkdir .git/hooks &&
-	cat << EOF > .git/hooks/update &&
+	cat << EOF >.git/hooks/update &&
 #!/bin/sh
 echo error: error
 echo hint: hint
@@ -20,15 +20,26 @@ EOF
 	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 origin HEAD:refs/heads/newbranch 2>output &&
-  test_decode_color < output > decoded &&
-  test_i18ngrep "<BOLD;RED>error<RESET>:" decoded &&
-  test_i18ngrep "<YELLOW>hint<RESET>:" decoded &&
-  test_i18ngrep "<BOLD;GREEN>success<RESET>:" decoded &&
-  test_i18ngrep "<BOLD;YELLOW>warning<RESET>:" decoded'
+test_expect_success 'push' '
+	git -c color.remote=always push origin HEAD:refs/heads/newbranch 2>output &&
+	test_decode_color <output >decoded &&
+	test_i18ngrep "<BOLD;RED>error<RESET>:" decoded &&
+	test_i18ngrep "<YELLOW>hint<RESET>:" decoded &&
+	test_i18ngrep "<BOLD;GREEN>success<RESET>:" decoded &&
+	test_i18ngrep "<BOLD;YELLOW>warning<RESET>:" decoded
+'
+
+test_expect_success 'push with customized color' '
+	git -c color.remote=always -c color.remote.error=white push origin HEAD:refs/heads/newbranch2 2>output &&
+	test_decode_color <output >decoded &&
+	test_i18ngrep "<WHITE>error<RESET>:" decoded &&
+	test_i18ngrep "<YELLOW>hint<RESET>:" decoded &&
+	test_i18ngrep "<BOLD;GREEN>success<RESET>:" decoded &&
+	test_i18ngrep "<BOLD;YELLOW>warning<RESET>:" decoded
+'
 
 test_done
-- 8< --