{"thread":{"id":"49049","subject":"[PATCH v6 0/1] sideband: highlight keywords in remote sideband output","startedAt":"2018-08-06T17:45:06Z","lastAt":"2018-08-06T18:58:43Z","messageCount":3,"participants":["Han-Wen Nienhuys","Eric Sunshine"],"isPatch":true,"patchVersion":6,"patchTotal":1},"messages":[{"id":"354656","messageId":"20180806174459.199589-1-hanwen@google.com","threadId":"49049","inReplyTo":null,"subject":"[PATCH v6 0/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-06T17:44:58Z","receivedAt":"2018-08-06T17:45:06Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"address Jun's comments.\n\nHan-Wen Nienhuys (1):\n sideband: highlight keywords in remote sideband output\n\n Documentation/config.txt | 12 +++\n help.c | 1 +\n help.h | 1 +\n sideband.c | 127 ++++++++++++++++++++++++++--\n t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++\n 5 files changed, 219 insertions(+), 9 deletions(-)\n create mode 100755 t/t5409-colorize-remote-messages.sh\n\n--\n2.18.0.597.ga71716f1ad-goog\n"},{"id":"354657","messageId":"20180806174459.199589-2-hanwen@google.com","threadId":"49049","inReplyTo":"20180806174459.199589-1-hanwen@google.com","subject":"[PATCH v6 1/1] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-06T17:44:59Z","receivedAt":"2018-08-06T17:45:08Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"The colorization is controlled with the config setting \"color.remote\".\n\nSupported keywords are \"error\", \"warning\", \"hint\" and \"success\". They\nare highlighted if they appear at the start of the line, which is\ncommon in error messages, eg.\n\n ERROR: commit is missing Change-Id\n\nThe Git push process itself prints lots of non-actionable messages\n(eg. bandwidth statistics, object counters for different phases of the\nprocess). This obscures actionable error messages that servers may\nsend back. Highlighting keywords in the sideband draws more attention\nto those messages.\n\nThe background for this change is that Gerrit does server-side\nprocessing to create or update code reviews, and actionable error\nmessages (eg. missing Change-Id) must be communicated back to the user\nduring the push. User research has shown that new users have trouble\nseeing these messages.\n\nThe highlighting is done on the client rather than server side, so\nservers don't have to grow capabilities to understand terminal escape\ncodes and terminal state. It also consistent with the current state\nwhere Git is control of the local display (eg. prefixing messages with\n\"remote: \").\n\nThe highlighting can be configured using color.remote.\nconfiguration settings. Since the keys are matched case insensitively,\nwe match the keywords case insensitively too.\n\nFinally, this solution is backwards compatible: many servers already\nprefix their messages with \"error\", and they will benefit from this\nchange without requiring a server update. By contrast, a server-side\nsolution would likely require plumbing the TERM variable through the\ngit protocol, so it would require changes to both server and client.\n\nHelped-by: Duy Nguyen \nSigned-off-by: Han-Wen Nienhuys \n---\n Documentation/config.txt | 12 +++\n help.c | 1 +\n help.h | 1 +\n sideband.c | 127 ++++++++++++++++++++++++++--\n t/t5409-colorize-remote-messages.sh | 87 +++++++++++++++++++\n 5 files changed, 219 insertions(+), 9 deletions(-)\n create mode 100755 t/t5409-colorize-remote-messages.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 63365dcf3d..33bc1a3def 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1263,6 +1263,18 @@ color.push::\n color.push.error::\n \tUse customized color for push errors.\n \n+color.remote::\n+\tIf set, keywords at the start of the line are highlighted. The\n+\tkeywords are \"error\", \"warning\", \"hint\" and \"success\", and are\n+\tmatched case-insensitively. Maybe set to `always`, `false` (or\n+\t`never`) or `auto` (or `true`). If unset, then the value of\n+\t`color.ui` is used (`auto` by default).\n+\n+color.remote.::\n+\tUse customized color for each remote keyword. `` may be\n+\t`hint`, `warning`, `success` or `error` which match the\n+\tcorresponding keyword.\n+\n color.showBranch::\n \tA boolean to enable/disable color in the output of\n \tlinkgit:git-show-branch[1]. May be set to `always`,\ndiff --git a/help.c b/help.c\nindex 3ebf0568db..b6cafcfc0a 100644\n--- a/help.c\n+++ b/help.c\n@@ -425,6 +425,7 @@ void list_config_help(int for_human)\n \t\t{ \"color.diff\", \"\", list_config_color_diff_slots },\n \t\t{ \"color.grep\", \"\", list_config_color_grep_slots },\n \t\t{ \"color.interactive\", \"\", list_config_color_interactive_slots },\n+\t\t{ \"color.remote\", \"\", list_config_color_sideband_slots },\n \t\t{ \"color.status\", \"\", list_config_color_status_slots },\n \t\t{ \"fsck\", \"\", list_config_fsck_msg_ids },\n \t\t{ \"receive.fsck\", \"\", list_config_fsck_msg_ids },\ndiff --git a/help.h b/help.h\nindex f8b15323a6..9eab6a3f89 100644\n--- a/help.h\n+++ b/help.h\n@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list, const char *prefix);\n void list_config_color_grep_slots(struct string_list *list, const char *prefix);\n void list_config_color_interactive_slots(struct string_list *list, const char *prefix);\n void list_config_color_status_slots(struct string_list *list, const char *prefix);\n+void list_config_color_sideband_slots(struct string_list *list, const char *prefix);\n void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);\n \n #endif /* HELP_H */\ndiff --git a/sideband.c b/sideband.c\nindex 325bf0e974..d882e79a5f 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,6 +1,110 @@\n #include \"cache.h\"\n+#include \"color.h\"\n+#include \"config.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n+#include \"help.h\"\n+\n+struct keyword_entry {\n+\t/*\n+\t * We use keyword as config key so it should be a single alphanumeric word.\n+\t */\n+\tconst char *keyword;\n+\tchar color[COLOR_MAXLEN];\n+};\n+\n+static struct keyword_entry keywords[] = {\n+\t{ \"hint\",\tGIT_COLOR_YELLOW },\n+\t{ \"warning\",\tGIT_COLOR_BOLD_YELLOW },\n+\t{ \"success\",\tGIT_COLOR_BOLD_GREEN },\n+\t{ \"error\",\tGIT_COLOR_BOLD_RED },\n+};\n+\n+/* Returns a color setting (GIT_COLOR_NEVER, etc). */\n+static int use_sideband_colors(void)\n+{\n+\tstatic int use_sideband_colors_cached = -1;\n+\n+\tconst char *key = \"color.remote\";\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *value;\n+\tint i;\n+\n+\tif (use_sideband_colors_cached >= 0)\n+\t\treturn use_sideband_colors_cached;\n+\n+\tif (!git_config_get_string(key, &value)) {\n+\t\tuse_sideband_colors_cached = git_config_colorbool(key, value);\n+\t} else if (!git_config_get_string(\"color.ui\", &value)) {\n+\t\tuse_sideband_colors_cached = git_config_colorbool(\"color.ui\", value);\n+\t} else {\n+\t\tuse_sideband_colors_cached = GIT_COLOR_AUTO;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s.%s\", key, keywords[i].keyword);\n+\t\tif (git_config_get_string(sb.buf, &value))\n+\t\t\tcontinue;\n+\t\tif (color_parse(value, keywords[i].color))\n+\t\t\tcontinue;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn use_sideband_colors_cached;\n+}\n+\n+void list_config_color_sideband_slots(struct string_list *list, const char *prefix)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(keywords); i++)\n+\t\tlist_config_item(list, prefix, keywords[i].keyword);\n+}\n+\n+/*\n+ * Optionally highlight one keyword in remote output if it appears at the start\n+ * of the line. This should be called for a single line only, which is\n+ * passed as the first N characters of the SRC array.\n+ */\n+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+{\n+\tint i;\n+\n+\tif (!want_color_stderr(use_sideband_colors())) {\n+\t\tstrbuf_add(dest, src, n);\n+\t\tfprintf(stderr, \"added: '%s'\\n\", dest->buf);\n+\t\treturn;\n+\t}\n+\n+\twhile (isspace(*src)) {\n+\t\tstrbuf_addch(dest, *src);\n+\t\tsrc++;\n+\t\tn--;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n+\t\tstruct keyword_entry *p = keywords + i;\n+\t\tint len = strlen(p->keyword);\n+\t\t/*\n+\t\t * Match case insensitively, so we colorize output from existing\n+\t\t * servers regardless of the case that they use for their\n+\t\t * messages. We only highlight the word precisely, so\n+\t\t * \"successful\" stays uncolored.\n+\t\t */\n+\t\tif (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n+\t\t\tstrbuf_addstr(dest, p->color);\n+\t\t\tstrbuf_add(dest, src, len);\n+\t\t\tstrbuf_addstr(dest, GIT_COLOR_RESET);\n+\t\t\tn -= len;\n+\t\t\tsrc += len;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tstrbuf_add(dest, src, n);\n+\n+}\n+\n \n /*\n * Receive multiplexed output stream over git native protocol.\n@@ -48,8 +152,10 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\tlen--;\n \t\tswitch (band) {\n \t\tcase 3:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t DISPLAY_PREFIX, buf + 1);\n+\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n+\t\t\t\t DISPLAY_PREFIX);\n+\t\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n+\n \t\t\tretval = SIDEBAND_REMOTE_ERROR;\n \t\t\tbreak;\n \t\tcase 2:\n@@ -69,20 +175,22 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t\tif (!outbuf.len)\n \t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n \t\t\t\tif (linelen > 0) {\n-\t\t\t\t\tstrbuf_addf(&outbuf, \"%.*s%s%c\",\n-\t\t\t\t\t\t linelen, b, suffix, *brk);\n-\t\t\t\t} else {\n-\t\t\t\t\tstrbuf_addch(&outbuf, *brk);\n+\t\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n+\t\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n \t\t\t\t}\n+\n+\t\t\t\tstrbuf_addch(&outbuf, *brk);\n \t\t\t\txwrite(2, outbuf.buf, outbuf.len);\n \t\t\t\tstrbuf_reset(&outbuf);\n \n \t\t\t\tb = brk + 1;\n \t\t\t}\n \n-\t\t\tif (*b)\n-\t\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ?\n-\t\t\t\t\t \"\" : DISPLAY_PREFIX, b);\n+\t\t\tif (*b) {\n+\t\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n+\t\t\t\t\t \"\" : DISPLAY_PREFIX);\n+\t\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 1:\n \t\t\twrite_or_die(out, buf + 1, len);\n@@ -96,6 +204,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t}\n \n \tif (outbuf.len) {\n+\t\tfprintf(stderr, \"last bit '%s'\\n\", outbuf.buf);\n \t\tstrbuf_addch(&outbuf, '\\n');\n \t\txwrite(2, outbuf.buf, outbuf.len);\n \t}\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nnew file mode 100755\nindex 0000000000..7057277a5f\n--- /dev/null\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -0,0 +1,87 @@\n+#!/bin/sh\n+\n+test_description='remote messages are colorized on the client'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir .git/hooks &&\n+\twrite_script .git/hooks/update <<-\\EOF &&\n+\techo error: error\n+\techo ERROR: also highlighted\n+\techo hint: hint\n+\techo hinting: not highlighted\n+\techo success: success\n+\techo warning: warning\n+\techo prefixerror: error\n+\techo \" \" \"error: leading space\"\n+\texit 0\n+\tEOF\n+\techo 1 >file &&\n+\tgit add file &&\n+\tgit commit -m 1 &&\n+\tgit clone . child &&\n+\t(\n+\t\tcd child &&\n+\t\ttest_commit message2 file content2\n+\t)\n+'\n+\n+test_expect_success 'keywords' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/keywords 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded &&\n+\tgrep \"hint:\" decoded &&\n+\tgrep \"success:\" decoded &&\n+\tgrep \"warning:\" decoded\n+'\n+\n+test_expect_success 'whole words at line start' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/whole-words 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"hint:\" decoded &&\n+\tgrep \"hinting: not highlighted\" decoded &&\n+\tgrep \"prefixerror: error\" decoded\n+'\n+\n+test_expect_success 'case-insensitive' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&\n+ cat output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded &&\n+\tgrep \"ERROR: also highlighted\" decoded\n+'\n+\n+test_expect_success 'leading space' '\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output && cat output &&\n+\ttest_decode_color decoded &&\n+\tgrep \" error: leading space\" decoded\n+'\n+\n+test_expect_success 'no coloring for redirected output' '\n+\tgit --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded\n+'\n+\n+test_expect_success 'push with customized color' '\n+\tgit --git-dir child/.git -c color.remote=always -c color.remote.error=blue push -f origin HEAD:refs/heads/customized-color 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error:\" decoded &&\n+\tgrep \"success:\" decoded\n+'\n+\n+\n+test_expect_success 'error in customized color' '\n+\tgit --git-dir child/.git -c color.remote=always -c color.remote.error=i-am-not-a-color push -f origin HEAD:refs/heads/error-customized-color 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"success:\" decoded\n+'\n+\n+test_expect_success 'fallback to color.ui' '\n+\tgit --git-dir child/.git -c color.ui=always push -f origin HEAD:refs/heads/fallback-color-ui 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error: error\" decoded\n+'\n+\n+test_done\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354668","messageId":"CAPig+cScb_7s4a_ZSVCsr+nBxAHGHZVMZOtnrOgbhZUi96-VFA@mail.gmail.com","threadId":"49049","inReplyTo":"20180806174459.199589-2-hanwen@google.com","subject":"Re: [PATCH v6 1/1] sideband: highlight keywords in remote sideband output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-06T18:58:28Z","receivedAt":"2018-08-06T18:58:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 6, 2018 at 1:45 PM Han-Wen Nienhuys wrote:\n> The colorization is controlled with the config setting \"color.remote\".\n> [...]\n> Helped-by: Duy Nguyen \n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -1263,6 +1263,18 @@ color.push::\n> +color.remote::\n> + If set, keywords at the start of the line are highlighted. The\n> + keywords are \"error\", \"warning\", \"hint\" and \"success\", and are\n> + matched case-insensitively. Maybe set to `always`, `false` (or\n\ns/Maybe/May be/\n\n> + `never`) or `auto` (or `true`). If unset, then the value of\n> + `color.ui` is used (`auto` by default).\n> diff --git a/sideband.c b/sideband.c\n> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> +{\n> + if (!want_color_stderr(use_sideband_colors())) {\n> + strbuf_add(dest, src, n);\n> + fprintf(stderr, \"added: '%s'\\n\", dest->buf);\n\nDebugging gunk?\n\n> + return;\n> + }\n> @@ -96,6 +204,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n> if (outbuf.len) {\n> + fprintf(stderr, \"last bit '%s'\\n\", outbuf.buf);\n\nDebugging gunk?\n\n> strbuf_addch(&outbuf, '\\n');\n> xwrite(2, outbuf.buf, outbuf.len);\n> }\n"}]}