{"thread":{"id":"49005","subject":"[PATCH 0/2 v4] sideband: highlight keywords in remote sideband output","startedAt":"2018-08-02T11:48:01Z","lastAt":"2018-08-06T22:12:18Z","messageCount":7,"participants":["Han-Wen Nienhuys","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"354233","messageId":"20180802114753.9715-1-hanwen@google.com","threadId":"49005","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-02T11:47:51Z","receivedAt":"2018-08-02T11:48:01Z","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":"354234","messageId":"20180802114753.9715-2-hanwen@google.com","threadId":"49005","inReplyTo":"20180802114753.9715-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-02T11:47:52Z","receivedAt":"2018-08-02T11:48:04Z","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":"354235","messageId":"20180802114753.9715-3-hanwen@google.com","threadId":"49005","inReplyTo":"20180802114753.9715-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-02T11:47:53Z","receivedAt":"2018-08-02T11:48:06Z","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":"354280","messageId":"xmqqbmak3c11.fsf@gitster-ct.c.googlers.com","threadId":"49005","inReplyTo":"20180802114753.9715-2-hanwen@google.com","subject":"Re: [PATCH 1/2] config: document git config getter return value.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T17:37:30Z","receivedAt":"2018-08-02T17:37:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> ---\n> config.h | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n\nShould I forge your sign-off on this patch?\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> 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"},{"id":"354289","messageId":"xmqqpnz01vdr.fsf@gitster-ct.c.googlers.com","threadId":"49005","inReplyTo":"20180802114753.9715-3-hanwen@google.com","subject":"Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T18:22:24Z","receivedAt":"2018-08-02T18:22:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> The colorization is controlled with the config setting \"color.remote\".\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. By contrast, a server-side\n> solution would likely require plumbing the TERM variable through the\n> git protocol, so it would require changes to both server and client.\n\nThanks; quite readable.\n\n>\n> Helped-by: Duy Nguyen \n> Signed-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\nI'll \"chmod +x\" while queuing.\n\n\tSide note: When all problems pointed out are \"I'll fix it\n\tthis way while queuing\"-kind, and if you agree to the way I\n\tplan to fix them up, then just saying so is sufficient and\n\tyou do not have to send a new version of the patch(es).\n\nIf your \"make test\" did not catch this as an error, then we may need\nto fix t/Makefile, as it is supposed to run test-lint.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 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\nNobody tells the end-users what \"colored remote output\" does;\narguably they can find it out themselves by enabling the feature and\nobserving remote messages, but that is not user friendly.\n\n\tWhen running commands like `git fetch` and `git push` that\n\tinteract with a remote repository, certain keywords (see\n\t`color.remote.`) that appear at the beginning of a\n\tline of message from the remote end can be painted in color.\n\n\tThis configuration variable is a boolean to enable/disable\n\tthe feature. If unset...\n\t\nor something like that, perhaps?\n\nYou use `always` in your tests to a good effect, but what the value\nmeans is not described here.\n\n> +color.remote.::\n> +\tUse customized color for each remote keywords. `` may be\n\nIsn't 'each' a singular, i.e. \"for each remote keyword\"? If so I do\nnot mind dropping 's' myself while queuing.\n\n> +\t`hint`, `warning`, `success` or `error` which match the\n> +\tcorresponding keyword.\n\nWe need to say that keywords are painted case insensitively\nsomewhere in the doc. Either do that here, or in the updated\ndescription of `color.remote`---I am not sure which one results in\nmore readable text offhand.\n\n> diff --git a/sideband.c b/sideband.c\n> index 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\nIn a block with a dozen more more lines, it is easier to have a\nblank line between decls and the first statement, i.e. here.\n\n> + \tif (!want_color_stderr(use_sideband_colors())) {\n\nThe above line is indented with SP followed by HT; don't.\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\n\tstruct kwtable *p = keywords + i;\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\nIndent with tabs, not a run of spaces, i.e. \n\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\nThe code in this function looked OK otherwise.\n\n> diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\n> new file mode 100644\n> index 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\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\nUse write_script, i.e. instead of all the above, say\n\n\twrite_script .git/hooks/update <<-\\EOF &&\n\techo error: error\n\techo hint: hint\n\t...\n\texit 0\n\tEOF\n\nso that you do not have to know or care the case where /bin/sh is\nnot the builder's preferred shell, etc.\n\nOur tests are not written to demonstrate that our code works as\nwritten. It is to protect our code from getting broken by others\nwho may not share vision of the original author. Make sure that you\ncast what you care about in stone, e.g. include \"echo ERROR: bad\" or\nsomething in the above to ensure that future updates to the code\nwill not turn the match into a case sensitive one without breaking\nthe test suite.\n\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\nDon't chdir the whole testing environment like this. Depending on\nthe success and failure in the middle of the above &&-chain, the\nnext test will start at an unexpected place, which is bad.\n\nInstead, do something like\n\n\tgit clone . child &&\n\techo 2 >child/file &&\n\tgit -C child commit -a -m 2\n\nor\n\n\tgit clone . child &&\n\t(\n\t\tcd child &&\n\t\techo 2 >file &&\n\t\tgit commit -a -m 2\n\t)\n\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\nA comment before this test (which covers both of these two) that\nexplains why many \"grep\" invocations are necessary, instead of a\ncomparison with a single multi-line expected result file. I am\nguessing that it is *not* because you cannot rely on the order of\nthese lines coming out from the update hook, but because the remote\noutput have lines other than what is given by the update hook and\nwe cannot afford to write them in the expected result file.\n\nA more readable alternative might be to prepare the message that\ncome from the other side more parseable, e.g.\n\n\twrite_script .git/hooks/update <<-\\EOF &&\n\techo 'BEGIN HOOK OUTPUT\n\terror: error\n\tError: another error\n\t...\n\tEND HOOK OUTPUT'\n\tEOF\n\nand then\n\n\tsed -ne \"/BEGIN HOOK OUTPUT/,/END HOOK OUTPUT/p\" output |\n\ttest_decode_color >decoded &&\n\tcat >expect <<-\\EOF &&\n\tBEGIN HOOK OUTPUT\n\terror: error\n\tError: another error\n\t...\n\tEND HOOK OUTPUT\n\tEOF\n\ttest_cmp expect decoded\n\nThen we do not have to make readers puzzled why we are using bunch\nof \"grep\" and do not explain with an extra comment.\n\nThanks.\n"},{"id":"354604","messageId":"CAFQ2z_OnUHcJSryq7vTdkvBuna64y0Q3+C+NX7o4_tFwSqjcDg@mail.gmail.com","threadId":"49005","inReplyTo":"xmqqpnz01vdr.fsf@gitster-ct.c.googlers.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:06:12Z","receivedAt":"2018-08-06T14:06:41Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Aug 2, 2018 at 8:22 PM Junio C Hamano wrote:\n> >\n> > Helped-by: Duy Nguyen \n> > Signed-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>\n> I'll \"chmod +x\" while queuing.\n>\nDone.\n\n> If your \"make test\" did not catch this as an error, then we may need\n> to fix t/Makefile, as it is supposed to run test-lint.\n\nI've been running tests individually as\n\n sh t5409-colorize-remote-messages.sh -v -d\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> Nobody tells the end-users what \"colored remote output\" does;\n> arguably they can find it out themselves by enabling the feature and\n> observing remote messages, but that is not user friendly.\n\nexpanded doc.\n\n> > +color.remote.::\n> > + Use customized color for each remote keywords. `` may be\n>\n> Isn't 'each' a singular, i.e. \"for each remote keyword\"? If so I do\n> not mind dropping 's' myself while queuing.\n\nDone.\n\n>\n> > + `hint`, `warning`, `success` or `error` which match the\n> > + corresponding keyword.\n>\n> We need to say that keywords are painted case insensitively\n> somewhere in the doc. Either do that here, or in the updated\n> description of `color.remote`---I am not sure which one results in\n> more readable text offhand.\n\nDone.\n\n> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> > +{\n> > + int i;\n>\n> In a block with a dozen more more lines, it is easier to have a\n> blank line between decls and the first statement, i.e. here.\n\nDone.\n\n> > + if (!want_color_stderr(use_sideband_colors())) {\n>\n> The above line is indented with SP followed by HT; don't.\n\nFixed. It would be great if there were a pre-commit hook that I could\ninstall to prevent this from ever getting committed.\n\n\n> > + struct kwtable* p = keywords + i;\n>\n> struct kwtable *p = keywords + i;\n\nDone.\n\n> > + int 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>\n> Indent with tabs, not a run of spaces, i.e.\n\nDone.\n\n> Use write_script, i.e. instead of all the above, say\n\nDone.\n\n\n> Our tests are not written to demonstrate that our code works as\n> written. It is to protect our code from getting broken by others\n> who may not share vision of the original author. Make sure that you\n> cast what you care about in stone, e.g. include \"echo ERROR: bad\" or\n> something in the above to ensure that future updates to the code\n> will not turn the match into a case sensitive one without breaking\n> the test suite.\n\nAdd some more cases.\n\n> > + echo 1 >file &&\n> > + git add file &&\n> > + git commit -m 1 &&\n> > + git clone . child &&\n> > + cd child &&\n> > + echo 2 > file &&\n> > + git commit -a -m 2\n>\n> Don't chdir the whole testing environment like this. Depending on\n> the success and failure in the middle of the above &&-chain, the\n> next test will start at an unexpected place, which is bad.\n>\n> Instead, do something like\n>\n> git clone . child &&\n> echo 2 >child/file &&\n> git -C child commit -a -m 2\n>\n> or\n>\nDone.\n\n> > +test_expect_success 'push' '\n> > + git -c color.remote=always push -f origin HEAD:refs/heads/newbranch 2>output &&\n> > + test_decode_color decoded &&\n> > + grep \"error:\" decoded &&\n> > + grep \"hint:\" decoded &&\n> > + grep \"success:\" decoded &&\n> > + grep \"warning:\" decoded &&\n> > + grep \"prefixerror: error\" decoded\n>\n> A comment before this test (which covers both of these two) that\n> explains why many \"grep\" invocations are necessary, instead of a\n> comparison with a single multi-line expected result file. I am\n> guessing that it is *not* because you cannot rely on the order of\n> these lines coming out from the update hook, but because the remote\n> output have lines other than what is given by the update hook and\n> we cannot afford to write them in the expected result file.\n\nComparing exact outputs is IMO an antipattern in general. It makes the\ntest more fragile than they need to be. (what if we change the\n\"remote: \" prefix to something else for example?).\n\nIf using a golden output, the second test would either require a\nrepetitive, too long golden file, or it would use loose greps anyway.\n\nAdded a comment.\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"},{"id":"354686","messageId":"xmqqy3djm9fo.fsf@gitster-ct.c.googlers.com","threadId":"49005","inReplyTo":"CAFQ2z_OnUHcJSryq7vTdkvBuna64y0Q3+C+NX7o4_tFwSqjcDg@mail.gmail.com","subject":"Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T22:12:11Z","receivedAt":"2018-08-06T22:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n>> If your \"make test\" did not catch this as an error, then we may need\n>> to fix t/Makefile, as it is supposed to run test-lint.\n>\n> I've been running tests individually as\n>\n> sh t5409-colorize-remote-messages.sh -v -d\n\nDuring your own development that may be sufficient, but do run \"make\ntest\" to run tests by other people; it will help you catch new bugs\nyou are introducing whey they notice their expectations are broken.\n\nYou are breaking 5541 (there may be others, but I noticed a breakage\nthere) perhaps with your debugging cruft.\n\n"}]}