{"thread":{"id":"49006","subject":"[PATCH 0/2 v4] sideband: highlight keywords in remote sideband output","startedAt":"2018-08-02T12:13:15Z","lastAt":"2018-08-06T14:17:53Z","messageCount":10,"participants":["Han-Wen Nienhuys","Eric Sunshine","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"354237","messageId":"20180802121308.17371-1-hanwen@google.com","threadId":"49006","inReplyTo":null,"subject":"[PATCH 0/2 v4] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-02T12:13:06Z","receivedAt":"2018-08-02T12:13:15Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"Address Eric Sunshine's comments.\n\nHan-Wen Nienhuys (2):\n config: document git config getter return value.\n sideband: highlight keywords in remote sideband output\n\n Documentation/config.txt | 9 +++\n config.h | 10 ++-\n help.c | 1 +\n help.h | 1 +\n sideband.c | 119 +++++++++++++++++++++++++---\n t/t5409-colorize-remote-messages.sh | 47 +++++++++++\n 6 files changed, 176 insertions(+), 11 deletions(-)\n create mode 100644 t/t5409-colorize-remote-messages.sh\n\n--\n2.18.0.597.ga71716f1ad-goog\n"},{"id":"354238","messageId":"20180802121308.17371-2-hanwen@google.com","threadId":"49006","inReplyTo":"20180802121308.17371-1-hanwen@google.com","subject":"[PATCH 1/2] config: document git config getter return value.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-02T12:13:07Z","receivedAt":"2018-08-02T12:13:18Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"---\n config.h | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/config.h b/config.h\nindex b95bb7649..41700f40b 100644\n--- a/config.h\n+++ b/config.h\n@@ -178,10 +178,16 @@ struct config_set {\n };\n \n extern void git_configset_init(struct config_set *cs);\n-extern int git_configset_add_file(struct config_set *cs, const char *filename);\n-extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n+\n extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n extern void git_configset_clear(struct config_set *cs);\n+\n+/*\n+ * These functions return 1 if not found, and 0 if found, leaving the found\n+ * value in the 'dest' pointer.\n+ */\n+extern int git_configset_add_file(struct config_set *cs, const char *filename);\n+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354239","messageId":"20180802121308.17371-3-hanwen@google.com","threadId":"49006","inReplyTo":"20180802121308.17371-1-hanwen@google.com","subject":"[PATCH 2/2] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-02T12:13:08Z","receivedAt":"2018-08-02T12:13:21Z","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 rationale 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 Git push process itself prints lots of non-actionable messages\n(eg. bandwidth statistics, object counters for different phases of the\nprocess), and my hypothesis is that these obscure the actionable error\nmessages that our server sends back. Highlighting keywords in the\ndraws more attention to actionable 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\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 | 9 +++\n help.c | 1 +\n help.h | 1 +\n sideband.c | 119 +++++++++++++++++++++++++---\n t/t5409-colorize-remote-messages.sh | 47 +++++++++++\n 5 files changed, 168 insertions(+), 9 deletions(-)\n create mode 100644 t/t5409-colorize-remote-messages.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 43b2de7b5..0783323be 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1229,6 +1229,15 @@ color.push::\n color.push.error::\n \tUse customized color for push errors.\n \n+color.remote::\n+\tA boolean to enable/disable colored remote output. If unset,\n+\tthen the value of `color.ui` is used (`auto` by default).\n+\n+color.remote.::\n+\tUse customized color for each remote keywords. `` 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 3ebf0568d..b6cafcfc0 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 f8b15323a..9eab6a3f8 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 325bf0e97..5c72db83c 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,6 +1,103 @@\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 kwtable {\n+\t/*\n+\t * We use keyword as config key so it can't contain funny chars like\n+\t * spaces. When we do that, we need a separate field for slot name in\n+\t * load_sideband_colors().\n+\t */\n+\tconst char *keyword;\n+\tchar color[COLOR_MAXLEN];\n+};\n+\n+static struct kwtable 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+\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\tdie(_(\"expecting a color: %s\"), value);\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 some keywords in remote output if they appear at the\n+ * start of the line. This should be called for a single line only.\n+ */\n+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+{\n+\tint i;\n+ \tif (!want_color_stderr(use_sideband_colors())) {\n+\t\tstrbuf_add(dest, src, n);\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 kwtable* p = keywords + i;\n+\t\tint len = strlen(p->keyword);\n+ /*\n+ * Match case insensitively, so we colorize output from existing\n+ * servers regardless of the case that they use for their\n+ * messages. We only highlight the word precisely, so\n+ * \"successful\" stays uncolored.\n+ */\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 * Receive multiplexed output stream over git native protocol.\n@@ -48,8 +145,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 +168,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);\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nnew file mode 100644\nindex 000000000..4e1bd421f\n--- /dev/null\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -0,0 +1,47 @@\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+\tcat << EOF > .git/hooks/update &&\n+#!/bin/sh\n+echo error: error\n+echo hint: hint\n+echo success: success\n+echo warning: warning\n+echo prefixerror: error\n+exit 0\n+EOF\n+\tchmod +x .git/hooks/update &&\n+\techo 1 >file &&\n+\tgit add file &&\n+\tgit commit -m 1 &&\n+\tgit clone . child &&\n+\tcd child &&\n+\techo 2 > file &&\n+\tgit commit -a -m 2\n+'\n+\n+test_expect_success 'push' '\n+\tgit -c color.remote=always push -f origin HEAD:refs/heads/newbranch 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error:\" decoded &&\n+\tgrep \"hint:\" decoded &&\n+\tgrep \"success:\" decoded &&\n+\tgrep \"warning:\" decoded &&\n+\tgrep \"prefixerror: error\" decoded\n+'\n+\n+test_expect_success 'push with customized color' '\n+\tgit -c color.remote=always -c color.remote.error=white push -f origin HEAD:refs/heads/newbranch2 2>output &&\n+\ttest_decode_color decoded &&\n+\tgrep \"error:\" decoded &&\n+\tgrep \"hint:\" decoded &&\n+\tgrep \"success:\" decoded &&\n+\tgrep \"warning:\" decoded\n+'\n+\n+test_done\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354277","messageId":"CAPig+cTFmNsxa4bSdUN199Q8YcJz74jn4Ytc3=JezaE-J595Og@mail.gmail.com","threadId":"49006","inReplyTo":"20180802121308.17371-2-hanwen@google.com","subject":"Re: [PATCH 1/2] config: document git config getter return value.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T17:19:00Z","receivedAt":"2018-08-02T17:19:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys wrote:\n> diff --git a/config.h b/config.h\n> @@ -178,10 +178,16 @@ struct config_set {\n> extern void git_configset_init(struct config_set *cs);\n> -extern int git_configset_add_file(struct config_set *cs, const char *filename);\n> -extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n> +/*\n> + * These functions return 1 if not found, and 0 if found, leaving the found\n> + * value in the 'dest' pointer.\n> + */\n> +extern int git_configset_add_file(struct config_set *cs, const char *filename);\n> +extern int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n> extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n> extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n> extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n\nIt doesn't seem like git_configset_add_file() fits in this group. It's\nneither searching for something (thus won't return \"found\" / \"not\nfound\"), nor is it returning 0 or 1. (It returns 0 on success and -1\non failure.)\n"},{"id":"354348","messageId":"20180803032948.GG197924@aiede.svl.corp.google.com","threadId":"49006","inReplyTo":"20180802121308.17371-2-hanwen@google.com","subject":"Re: [PATCH 1/2] config: document git config getter return value.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-03T03:29:48Z","receivedAt":"2018-08-03T03:29:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing peff, config whiz)\nHi,\n\nHan-Wen Nienhuys wrote:\n\n> Subject: config: document git config getter return value.\n\nmicronit: no period at the end of subject line\n\n> ---\n> config.h | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n\nMay we forge your sign-off? See Documentation/SubmittingPatches section\n[[sign-off]] for what this means.\n\n> \n> diff --git a/config.h b/config.h\n> index b95bb7649..41700f40b 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -178,10 +178,16 @@ struct config_set {\n> };\n> \n> extern void git_configset_init(struct config_set *cs);\n> -extern int git_configset_add_file(struct config_set *cs, const char *filename);\n> -extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n> +\n\nnit: this blank line feels out of place (though I don't particularly\nmind it and wouldn't reroll just for that)\n\n> extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n> extern void git_configset_clear(struct config_set *cs);\n> +\n> +/*\n> + * These functions return 1 if not found, and 0 if found, leaving the found\n> + * value in the 'dest' pointer.\n> + */\n> +extern int git_configset_add_file(struct config_set *cs, const char *filename);\n\nThis function doesn't take a 'dest' argument. Is the comment meant to\napply to it? If not, can the comment go below it?\n\n> +extern int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n> extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n> extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n> extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n\nWith a sign-off and whatever subset of the above suggestions makes sense\nto you,\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"354349","messageId":"CAPig+cQcLfN7TjUR6hzFT9h-VhE1o7VGkGkQdznDNxjTLNfzSA@mail.gmail.com","threadId":"49006","inReplyTo":"20180802121308.17371-3-hanwen@google.com","subject":"Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T03:48:37Z","receivedAt":"2018-08-03T03:48:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys wrote:\n> [PATCH 2/2] sideband: highlight keywords in remote sideband output\n\nThe -v option of git-format-patch makes it easy to let reviewers know\nthat this is a new version of a previously-submitted patch series.\nThis (I think) is the second attempt; if you need to send again, you\ncould use \"git format-patch -v3 ...\", which would result in \"[PATCH v3\n2/2] ...\".\n\n> The colorization is controlled with the config setting \"color.remote\".\n>\n> Supported keywords are \"error\", \"warning\", \"hint\" and \"success\". They\n> are highlighted if they appear at the start of the line, which is\n> common in error messages, eg.\n>\n> ERROR: commit is missing Change-Id\n>\n> The rationale for this change is that Gerrit does server-side\n> processing to create or update code reviews, and actionable error\n> messages (eg. missing Change-Id) must be communicated back to the user\n> during the push. User research has shown that new users have trouble\n> seeing these messages.\n> [...snip...]\n\nThanks, this commit message is much more helpful than the previous one.\n\nIf you end up re-rolling, you might consider swapping the order of\nthese paragraphs around a bit to first explain the problem the patch\nis solving (i.e. the justification), then the solution and relevant\ndetails. Documentation/SubmittingPatches has good advice about this.\n\nUsing Gerrit as the sole rationale is underselling this otherwise\ngeneral improvement. Instead, the commit message could sell the change\non its own merits and can then use Gerrit as an example.\n\n> The Git push process itself prints lots of non-actionable messages\n> (eg. bandwidth statistics, object counters for different phases of the\n> process), and my hypothesis is that these obscure the actionable error\n> messages that our server sends back. Highlighting keywords in the\n> draws more attention to actionable messages.\n\nSo, for instance, you might want to use a rewrite of this paragraph to\nopen the commit message. Something like this, perhaps:\n\n A remote Git operation can print many non-actionable messages\n (e.g. bandwidth statistics, object counters for different phases\n of the process, etc.) and such noise can obscure genuine\n actionable messages, such as warnings and outright errors.\n\n As an example, Gerrit does server-side processing to create or\n update code reviews, and actionable error messages (such as\n \"ERROR: commit is missing Change-Id\") must be communicated back to\n the user during a push operation, but new users often have trouble\n spotting these error messages amid the noise.\n\n Address this problem by upgrading 'sideband' to draw attention to\n these messages by highlighting them with color.\n\n [...and so forth...]\n\n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> diff --git a/sideband.c b/sideband.c\n> @@ -1,6 +1,103 @@\n> +struct kwtable {\n> + /*\n> + * We use keyword as config key so it can't contain funny chars like\n> + * spaces. When we do that, we need a separate field for slot name in\n> + * load_sideband_colors().\n> + */\n\nThis comment is outdated; load_sideband_colors() does not exist in this patch.\n\nI get what the first sentence of this comment is saying, but I'm\nhaving trouble understanding what the second sentence is all about.\n\n> + const char *keyword;\n> + char color[COLOR_MAXLEN];\n> +};\n> +\n> +static struct kwtable keywords[] = {\n> + { \"hint\", GIT_COLOR_YELLOW },\n> + { \"warning\", GIT_COLOR_BOLD_YELLOW },\n> + { \"success\", GIT_COLOR_BOLD_GREEN },\n> + { \"error\", GIT_COLOR_BOLD_RED },\n> +};\n> +\n> +// Returns a color setting (GIT_COLOR_NEVER, etc).\n\nUse /* old-style C comments */ in this codebase, not // C++ or new-style C.\n\n> +static int use_sideband_colors(void)\n> +{\n> + static int use_sideband_colors_cached = -1;\n> + const char *key = \"color.remote\";\n> +\n> + if (use_sideband_colors_cached >= 0)\n> + return use_sideband_colors_cached;\n> +\n> + if (!git_config_get_string(key, &value))\n> + use_sideband_colors_cached = git_config_colorbool(key, value);\n\nSo, if \"color.remote\" exists, then 'use_sideband_colors_cached' is\nassigned. However, if \"color.remote\" does not exist, then it's never\nassigned. Is this intended behavior? It seems like you'd want to fall\nback to some other value rather than leaving it unassigned.\n\nWhich leads to the next question: The documentation says that this\nfalls back to \"color.ui\" if \"color.remote\" does not exist, however,\nwhere is this fallback happening? Am I overlooking something obvious?\n\n> + for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> + strbuf_reset(&sb);\n> + strbuf_addf(&sb, \"%s.%s\", key, keywords[i].keyword);\n> + if (git_config_get_string(sb.buf, &value))\n> + continue;\n> + if (color_parse(value, keywords[i].color))\n> + die(_(\"expecting a color: %s\"), value);\n\nThis error message would be much more helpful if it also told the user\nthe name of the config key from which 'value' came.\n\n> + }\n\nDo you need to be doing the work in the above for-loop if, after\ncomputing 'use_sideband_colors_cached', it is determined that you\nwon't be using color?\n\n> + strbuf_release(&sb);\n> + return use_sideband_colors_cached;\n> +}\n> diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\n> @@ -0,0 +1,47 @@\n> +test_expect_success 'setup' '\n> + mkdir .git/hooks &&\n> + cat << EOF > .git/hooks/update &&\n> +#!/bin/sh\n> +echo error: error\n> +echo hint: hint\n> +echo success: success\n> +echo warning: warning\n> +echo prefixerror: error\n\nI addition to checking other cases, such as \"ERROR\", as Junio\nsuggested, I'd think you'd also want to test:\n\n hinting: hint\n\nto verify that it doesn't get colored.\n\n> +exit 0\n> +EOF\n"},{"id":"354350","messageId":"20180803035243.GH197924@aiede.svl.corp.google.com","threadId":"49006","inReplyTo":"20180802121308.17371-3-hanwen@google.com","subject":"Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-03T03:52:43Z","receivedAt":"2018-08-03T03:52:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nHan-Wen Nienhuys wrote:\n\n> The colorization is controlled with the config setting \"color.remote\".\n>\n> Supported keywords are \"error\", \"warning\", \"hint\" and \"success\". They\n> are highlighted if they appear at the start of the line, which is\n> common in error messages, eg.\n>\n> ERROR: commit is missing Change-Id\n\nA few questions:\n\n- should this be documented somewhere in Documentation/technical/*protocol*?\n That way, server implementors can know to take advantage of it.\n\n- how does this interact with (future) internationalization of server\n messages? If my server knows that the client speaks French, should\n they send \"ERROR:\" anyway and rely on the client to translate it?\n Or are we deferring that question to a later day?\n\n[...]\n> The Git push process itself prints lots of non-actionable messages\n> (eg. bandwidth statistics, object counters for different phases of the\n> process), and my hypothesis is that these obscure the actionable error\n> messages that our server sends back. Highlighting keywords in the\n> draws more attention to actionable messages.\n\nI'd be interested in ways to minimize Git's contribution to this as\nwell. E.g. can we make more use of \\r to make client-produced progress\nmessages take less screen real estate? Should some of the servers\ninvolved (e.g., Gerrit) do so as well?\n\n> The highlighting is done on the client rather than server side, so\n> servers don't have to grow capabilities to understand terminal escape\n> codes and terminal state. It also consistent with the current state\n> where Git is control of the local display (eg. prefixing messages with\n> \"remote: \").\n\nAs a strawman idea, what would you think of a way to allow the server\nto do some coloration by using color tags like\n\n Erreur: ...\n\n?\n\nAs an advantage, this would give the server more control. As a\ndisadvantage, it is not so useful as \"semantic markup\", meaning it is\nharder to figure out how to present to accessibility tools in a\nmeaningful way. Plus, there's the issue of usefulness with existing\nservers you mentioned:\n\n> Finally, this solution is backwards compatible: many servers already\n> prefix their messages with \"error\", and they will benefit from this\n> change without requiring a server update.\n\nYes, this seems like a compelling reason to follow this approach.\n\n[...]\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1229,6 +1229,15 @@ color.push::\n> color.push.error::\n> \tUse customized color for push errors.\n> \n> +color.remote::\n> +\tA boolean to enable/disable colored remote output. If unset,\n> +\tthen the value of `color.ui` is used (`auto` by default).\n> +\n> +color.remote.::\n> +\tUse customized color for each remote keywords. `` may be\n> +\t`hint`, `warning`, `success` or `error` which match the\n> +\tcorresponding keyword.\n\nWhat positions in a remote message are matched? If a server prints a\nmessage about something being discouraged because it is error-prone,\nwould the \"error\" in error-prone turn red?\n\n[...]\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -1,6 +1,103 @@\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 kwtable {\n\nnit: perhaps kwtable_entry?\n\n> +\t/*\n> +\t * We use keyword as config key so it can't contain funny chars like\n> +\t * spaces. When we do that, we need a separate field for slot name in\n> +\t * load_sideband_colors().\n> +\t */\n> +\tconst char *keyword;\n> +\tchar color[COLOR_MAXLEN];\n> +};\n> +\n> +static struct kwtable keywords[] = {\n> +\t{ \"hint\",\tGIT_COLOR_YELLOW },\n[...]\n> +// Returns a color setting (GIT_COLOR_NEVER, etc).\n\nnit: Git uses C89-style /* */ comments.\n\n> +static int use_sideband_colors(void)\n\nMakes sense.\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\nNot about this patch: I wonder if we can eliminate this boilerplate some\ntime in the future.\n\n[...]\n> +/*\n> + * Optionally highlight some keywords in remote output if they appear at the\n> + * start of the line. This should be called for a single line only.\n> + */\n> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n\nCan this be static?\n\nWhat does 'n' represent? Can the comment say? (Or if it's the length\nof src, can it be renamed to srclen?)\n\nSuper-pedantic nit: even if there are multiple special words at the\nstart, this will only highlight one. :) So it could say something\nlike \"Optionally check if the first word on the line is a keyword and\nhighlight it if so.\"\n\n> +{\n> +\tint i;\n> + \tif (!want_color_stderr(use_sideband_colors())) {\n\nnit: whitespace damage (you can check for this with \"git show --check\").\nThere's a bit more elsewhere.\n\n> +\t\tstrbuf_add(dest, src, n);\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 kwtable* p = keywords + i;\n\nnit: * should attach to the variable, C-style.\n\nYou can use \"make style\" to do some automatic formatting (though this\nis a bit experimental, so do double-check the results).\n\n> +\t\tint len = strlen(p->keyword);\n> + /*\n> + * Match case insensitively, so we colorize output from existing\n> + * servers regardless of the case that they use for their\n> + * messages. We only highlight the word precisely, so\n> + * \"successful\" stays uncolored.\n> + */\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\nSensible.\n\n[...]\n> @@ -48,8 +145,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 +168,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\nNice. This looks cleaner than what was there before, which is always\na good sign.\n\n[...]\n> --- /dev/null\n> +++ b/t/t5409-colorize-remote-messages.sh\n\nThanks for these. It may make sense to have a test with some leading\nwhitespace as well.\n\n[...]\n> +test_expect_success 'setup' '\n> +\tmkdir .git/hooks &&\n> +\tcat << EOF > .git/hooks/update &&\n> +#!/bin/sh\n> +echo error: error\n> +echo hint: hint\n> +echo success: success\n> +echo warning: warning\n> +echo prefixerror: error\n> +exit 0\n> +EOF\n> +\tchmod +x .git/hooks/update &&\n\nPlease use write_script for this, since on esoteric platforms it picks\nan appropriate shell.\n\nIf you use <<-\\EOF instead of < +\techo 1 >file &&\n> +\tgit add file &&\n> +\tgit commit -m 1 &&\n\nThis can use test_commit. See t/README for details.\n\n[...]\n> +test_expect_success 'push' '\n> +\tgit -c color.remote=always push -f origin HEAD:refs/heads/newbranch 2>output &&\n> +\ttest_decode_color decoded &&\n> +\tgrep \"error:\" decoded &&\n> +\tgrep \"hint:\" decoded &&\n> +\tgrep \"success:\" decoded &&\n> +\tgrep \"warning:\" decoded &&\n> +\tgrep \"prefixerror: error\" decoded\n> +'\n> +\n> +test_expect_success 'push with customized color' '\n> +\tgit -c color.remote=always -c color.remote.error=white push -f origin HEAD:refs/heads/newbranch2 2>output &&\n> +\ttest_decode_color decoded &&\n> +\tgrep \"error:\" decoded &&\n> +\tgrep \"hint:\" decoded &&\n> +\tgrep \"success:\" decoded &&\n> +\tgrep \"warning:\" decoded\n> +'\n\nNice.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"354381","messageId":"20180803130509.GB4671@sigill.intra.peff.net","threadId":"49006","inReplyTo":"20180803032948.GG197924@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/2] config: document git config getter return value.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-03T13:05:09Z","receivedAt":"2018-08-03T13:05:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 08:29:48PM -0700, Jonathan Nieder wrote:\n\n> (cc-ing peff, config whiz)\n\nActually, this is all about the configset caching layer, which I never\nreally worked on. This is mostly from Tanay Abhra ,\nwho was a GSoC student mentored by Matthieu Moy .\n\nThat said...\n\n> > +\n> > +/*\n> > + * These functions return 1 if not found, and 0 if found, leaving the found\n> > + * value in the 'dest' pointer.\n> > + */\n> > +extern int git_configset_add_file(struct config_set *cs, const char *filename);\n> \n> This function doesn't take a 'dest' argument. Is the comment meant to\n> apply to it? If not, can the comment go below it?\n\nThis is returning the value of git_config_from_file(), which is 0/-1. I\nthink it should be left where it is in the original, and not part of\nthis block of functions.\n\nOther than that, the patch seems sane to me (I think the 0/1 return\nvalue from these \"get\" functions is unnecessarily inconsistent with the\nrest of Git, but changing it would be a pain. Documenting it is at least\na step forward).\n\n-Peff\n"},{"id":"354398","messageId":"xmqqpnyzzbmd.fsf@gitster-ct.c.googlers.com","threadId":"49006","inReplyTo":"20180803130509.GB4671@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: document git config getter return value.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T15:57:30Z","receivedAt":"2018-08-03T15:57:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is returning the value of git_config_from_file(), which is 0/-1. I\n> think it should be left where it is in the original, and not part of\n> this block of functions.\n>\n> Other than that, the patch seems sane to me (I think the 0/1 return\n> value from these \"get\" functions is unnecessarily inconsistent with the\n> rest of Git, but changing it would be a pain. Documenting it is at least\n> a step forward).\n\nI do not think changing it would be in the scope of this series.\n\nIt makes sense to document (1) that zero is success, non-zero is\nfailure, to help those who are reading the current callers and\nadding new callers, and (2) that we are aware that the exact\n\"non-zero\" value chosen for these are not in line with the rest of\nthe system. The latter can just be a \"NEEDSWORK\" comment.\n\n"},{"id":"354606","messageId":"CAFQ2z_PfvGgiwWeDbhiBt77s+z7qWzqCMXXZT0EjFNZKx+WUMg@mail.gmail.com","threadId":"49006","inReplyTo":"20180803035243.GH197924@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-06T14:17:39Z","receivedAt":"2018-08-06T14:17:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Aug 3, 2018 at 5:52 AM Jonathan Nieder wrote:\n>\n> Hi,\n>\n> Han-Wen Nienhuys wrote:\n>\n> > The colorization is controlled with the config setting \"color.remote\".\n> >\n> > Supported keywords are \"error\", \"warning\", \"hint\" and \"success\". They\n> > are highlighted if they appear at the start of the line, which is\n> > common in error messages, eg.\n> >\n> > ERROR: commit is missing Change-Id\n>\n> A few questions:\n>\n> - should this be documented somewhere in Documentation/technical/*protocol*?\n> That way, server implementors can know to take advantage of it.\n\nThe protocol docs seem to work on a much different level. Maybe there\nshould be a \"best practices\" document or similar?\n\n> - how does this interact with (future) internationalization of server\n> messages? If my server knows that the client speaks French, should\n> they send \"ERROR:\" anyway and rely on the client to translate it?\n> Or are we deferring that question to a later day?\n\nIt doesn't, and we are deferring that question.\n\n> [...]\n> > The Git push process itself prints lots of non-actionable messages\n> > (eg. bandwidth statistics, object counters for different phases of the\n> > process), and my hypothesis is that these obscure the actionable error\n> > messages that our server sends back. Highlighting keywords in the\n> > draws more attention to actionable messages.\n>\n> I'd be interested in ways to minimize Git's contribution to this as\n> well. E.g. can we make more use of \\r to make client-produced progress\n> messages take less screen real estate? Should some of the servers\n> involved (e.g., Gerrit) do so as well?\n\nYes, I'm interested in this too, but I fear it would veer into a\nterritory that is prone to bikeshedding.\n\nGerrit should definitely also send less noise.\n\n> > Finally, this solution is backwards compatible: many servers already\n> > prefix their messages with \"error\", and they will benefit from this\n> > change without requiring a server update.\n>\n> Yes, this seems like a compelling reason to follow this approach.\n>\n> [...]\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > @@ -1229,6 +1229,15 @@ color.push::\n> > color.push.error::\n> > Use customized color for push errors.\n> >\n> > +color.remote::\n> > + A boolean to enable/disable colored remote output. If unset,\n> > + then the value of `color.ui` is used (`auto` by default).\n> > +\n> > +color.remote.::\n> > + Use customized color for each remote keywords. `` may be\n> > + `hint`, `warning`, `success` or `error` which match the\n> > + corresponding keyword.\n>\n> What positions in a remote message are matched? If a server prints a\n> message about something being discouraged because it is error-prone,\n> would the \"error\" in error-prone turn red?\n\nyes, if it happened to occur after a line-break.\n\nWe could decide that we will only highlight\n\n ':' rest of line\n\nor\n\n '\\n'\n\nthat would work for the Gerrit case, and would be useful forcing\nfunction to uniformize remote error messages.\n\n> > +struct kwtable {\n>\n> nit: perhaps kwtable_entry?\n\ndone.\n\n> > +/*\n> > + * Optionally highlight some keywords in remote output if they appear at the\n> > + * start of the line. This should be called for a single line only.\n> > + */\n> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n>\n> Can this be static?\n\nDone.\n\n> What does 'n' represent? Can the comment say? (Or if it's the length\n> of src, can it be renamed to srclen?)\n\nAdded comment.\n\n> Super-pedantic nit: even if there are multiple special words at the\n> start, this will only highlight one. :) So it could say something\n> like \"Optionally check if the first word on the line is a keyword and\n> highlight it if so.\"\n\nSuper pedantic answer: if people care about it that much, they can\nread the 20 lines of code below :-)\n\n> > +{\n> > + int i;\n> > + if (!want_color_stderr(use_sideband_colors())) {\n>\n> nit: whitespace damage (you can check for this with \"git show --check\").\n> There's a bit more elsewhere.\n\nran tabify on whole file.\n\n> > + for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> > + struct kwtable* p = keywords + i;\n>\n> nit: * should attach to the variable, C-style.\n\nDone.\n\n> You can use \"make style\" to do some automatic formatting (though this\n> is a bit experimental, so do double-check the results).\n\nsad panda:\n\nhanwen@han-wen:~/vc/git$ make style\ngit clang-format --style file --diff --extensions c,h\nTraceback (most recent call last):\n File \"/usr/bin/git-clang-format\", line 580, in \n main()\n File \"/usr/bin/git-clang-format\", line 62, in main\n config = load_git_config()\n File \"/usr/bin/git-clang-format\", line 195, in load_git_config\n name, value = entry.split('\\n', 1)\nValueError: need more than 1 value to unpack\nMakefile:2663: recipe for target 'style' failed\nmake: *** [style] Error 1\n\n> [...]\n> > @@ -48,8 +145,10 @@ int recv_sideband(const char *me, int in_stream, int out)\n> > len--;\n> > switch (band) {\n> > case 3:\n> > - strbuf_addf(&outbuf, \"%s%s%s\", outbuf.len ? \"\\n\" : \"\",\n> > - DISPLAY_PREFIX, buf + 1);\n> > + strbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n> > + DISPLAY_PREFIX);\n> > + maybe_colorize_sideband(&outbuf, buf + 1, len);\n> > +\n> > retval = SIDEBAND_REMOTE_ERROR;\n> > break;\n> > case 2:\n> > @@ -69,20 +168,22 @@ int recv_sideband(const char *me, int in_stream, int out)\n> > if (!outbuf.len)\n> > strbuf_addstr(&outbuf, DISPLAY_PREFIX);\n> > if (linelen > 0) {\n> > - strbuf_addf(&outbuf, \"%.*s%s%c\",\n> > - linelen, b, suffix, *brk);\n> > - } else {\n> > - strbuf_addch(&outbuf, *brk);\n> > + maybe_colorize_sideband(&outbuf, b, linelen);\n> > + strbuf_addstr(&outbuf, suffix);\n> > }\n> > +\n> > + strbuf_addch(&outbuf, *brk);\n> > xwrite(2, outbuf.buf, outbuf.len);\n>\n> Nice. This looks cleaner than what was there before, which is always\n> a good sign.\n>\n> [...]\n> > --- /dev/null\n> > +++ b/t/t5409-colorize-remote-messages.sh\n>\n> Thanks for these. It may make sense to have a test with some leading\n> whitespace as well.\n\nDone.\n\n> > + chmod +x .git/hooks/update &&\n>\n> Please use write_script for this, since on esoteric platforms it picks\n> an appropriate shell.\n\n> If you use <<-\\EOF instead of < - it lets you indent the script\n> - it saves the reviewer from having to look for $ escapes inside the\n> script body\n\nDone (#TIL).\n\n> > + echo 1 >file &&\n> > + git add file &&\n> > + git commit -m 1 &&\n>\n> This can use test_commit. See t/README for details.\n\nDone.\n\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"}]}