{"thread":{"id":"48993","subject":"[PATCH 0/2 v3] Highlight keywords in remote sideband output.","startedAt":"2018-07-31T17:37:00Z","lastAt":"2018-08-02T17:44:28Z","messageCount":15,"participants":["Han-Wen Nienhuys","Junio C Hamano","Eric Sunshine","Stefan Beller"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"354069","messageId":"20180731173651.184716-1-hanwen@google.com","threadId":"48993","inReplyTo":null,"subject":"[PATCH 0/2 v3] Highlight keywords in remote sideband output.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-07-31T17:36:49Z","receivedAt":"2018-07-31T17:37:00Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"squash in Duy's patch\n\nHan-Wen Nienhuys (2):\n Document git config getter return value.\n 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 | 113 +++++++++++++++++++++++++---\n t/t5409-colorize-remote-messages.sh | 47 ++++++++++++\n 6 files changed, 170 insertions(+), 11 deletions(-)\n create mode 100644 t/t5409-colorize-remote-messages.sh\n\n--\n2.18.0.345.g5c9ce644c3-goog\n"},{"id":"354070","messageId":"20180731173651.184716-2-hanwen@google.com","threadId":"48993","inReplyTo":"20180731173651.184716-1-hanwen@google.com","subject":"[PATCH 1/2] Document git config getter return value.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-07-31T17:36:50Z","receivedAt":"2018-07-31T17:37:06Z","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..d39256eb1 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+ * The int return values in the functions is 1 if not found, 0 if found, leaving\n+ * the found value in teh '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.345.g5c9ce644c3-goog\n\n"},{"id":"354071","messageId":"20180731173651.184716-3-hanwen@google.com","threadId":"48993","inReplyTo":"20180731173651.184716-1-hanwen@google.com","subject":"[PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-07-31T17:36:51Z","receivedAt":"2018-07-31T17:37:11Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"The highlighting is done on the client-side. Supported keywords are\n\"error\", \"warning\", \"hint\" and \"success\".\n\nThe colorization is controlled with the config setting \"color.remote\".\n\nCo-authored-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 | 113 +++++++++++++++++++++++++---\n t/t5409-colorize-remote-messages.sh | 47 ++++++++++++\n 5 files changed, 162 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..0d67583ec 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,6 +1,97 @@\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+static int sideband_use_color = -1;\n+\n+static void load_sideband_colors(void)\n+{\n+\tconst char *key = \"color.remote\";\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *value;\n+\tint i;\n+\n+\tif (sideband_use_color >= 0)\n+\t\treturn;\n+\n+\tif (!git_config_get_string(key, &value))\n+\t\tsideband_use_color = 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+}\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.\n+ */\n+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+{\n+\tint i;\n+\n+\tload_sideband_colors();\n+ \tif (!want_color_stderr(sideband_use_color)) {\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+\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 +139,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 +162,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.345.g5c9ce644c3-goog\n\n"},{"id":"354110","messageId":"xmqqa7q7b35v.fsf@gitster-ct.c.googlers.com","threadId":"48993","inReplyTo":"20180731173651.184716-3-hanwen@google.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T19:45:00Z","receivedAt":"2018-07-31T19:45:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> The highlighting is done on the client-side. Supported keywords are\n> \"error\", \"warning\", \"hint\" and \"success\".\n>\n> The colorization is controlled with the config setting \"color.remote\".\n>\n> Co-authored-by: Duy Nguyen \n> Signed-off-by: Han-Wen Nienhuys \n\nThanks. I'll squash the following in while queuing, though.\n\n * maybe_colorize_sideband() does not have outside caller; make it\n static to avoid missing-prototype error that breaks compilation.\n\n * correct space-before-tab whitespace style violation.\n\n * use write_script.\n\n * a test script must be executable to avoid triggering test-lint.\n\n * avoid overlong lines in the test.\n\n * no SP between redirection operator and its target.\n\nOther than that, the result looks good to me. So that others can\neyeball the result once more, I'll keep it in 'pu' for a few days,\nand if nothing else comes up, hopefully the topic can be merged to\n'next' after that.\n\n\ndiff --git a/sideband.c b/sideband.c\nindex 0d67583ec5..be4635446c 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -60,12 +60,12 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n * Optionally highlight some keywords in remote output if they appear at the\n * start of the line.\n */\n-void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n {\n \tint i;\n \n \tload_sideband_colors();\n- \tif (!want_color_stderr(sideband_use_color)) {\n+\tif (!want_color_stderr(sideband_use_color)) {\n \t\tstrbuf_add(dest, src, n);\n \t\treturn;\n \t}\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nold mode 100644\nnew mode 100755\nindex 4e1bd421ff..4547ec95b8\n--- a/t/t5409-colorize-remote-messages.sh\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -6,27 +6,27 @@ test_description='remote messages are colorized on the client'\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+\twrite_script .git/hooks/update <<-\\EOF &&\n+\techo error: error\n+\techo hint: hint\n+\techo success: success\n+\techo warning: warning\n+\techo prefixerror: error\n+\texit 0\n+\tEOF\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+\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+\tgit -c color.remote=always \\\n+\t\tpush -f origin HEAD:refs/heads/newbranch 2>output &&\n \ttest_decode_color decoded &&\n \tgrep \"error:\" decoded &&\n \tgrep \"hint:\" decoded &&\n@@ -36,7 +36,8 @@ test_expect_success 'push' '\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+\tgit -c color.remote=always -c color.remote.error=white \\\n+\t\tpush -f origin HEAD:refs/heads/newbranch2 2>output &&\n \ttest_decode_color decoded &&\n \tgrep \"error:\" decoded &&\n \tgrep \"hint:\" decoded &&\n\n"},{"id":"354119","messageId":"CAPig+cSbibJ7i8LwJqPe06xJObnq6dJdMUnJoC1uAg4zUQq3KA@mail.gmail.com","threadId":"48993","inReplyTo":"20180731173651.184716-3-hanwen@google.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T20:21:24Z","receivedAt":"2018-07-31T20:21:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:\n> Highlight keywords in remote sideband output.\n\nPrefix with the module you're touching, don't capitalize, and drop the\nperiod. Perhaps:\n\n sideband: highlight keywords in remote sideband output\n\n> The highlighting is done on the client-side. Supported keywords are\n> \"error\", \"warning\", \"hint\" and \"success\".\n>\n> The colorization is controlled with the config setting \"color.remote\".\n\nWhat's the motivation for this change? The commit message should say\nsomething about that and give an explanation of why this is done\nclient-side rather than server-side.\n\n> Co-authored-by: Duy Nguyen \n\nHelped-by: is more typical.\n\n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -1229,6 +1229,15 @@ color.push::\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\nIf this is \"boolean\", what does \"auto\" mean? Perhaps update the\ndescription to better match other color-related options.\n\n> diff --git a/sideband.c b/sideband.c\n> @@ -1,6 +1,97 @@\n> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> +{\n> + int i;\n> +\n> + load_sideband_colors();\n> + if (!want_color_stderr(sideband_use_color)) {\n> + strbuf_add(dest, src, n);\n> + return;\n> + }\n\nCan load_sideband_colors() be moved below the !want_color_stderr() conditional?\n\n> +\n> + while (isspace(*src)) {\n> + strbuf_addch(dest, *src);\n> + src++;\n> + n--;\n> + }\n> +\n> + for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> + struct kwtable* p = keywords + i;\n> + int len = strlen(p->keyword);\n\nWould it make sense to precompute each keyword length so you don't\nhave to recompute them repeatedly, or is that premature optimization?\n\n> + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n\nSo, the strncasecmp() is checking if one of the recognized keywords is\nat the 'src' position, and the !isalnum() ensures that you won't pick\nup something of which the keyword is merely a prefix. For instance,\nyou won't mistakenly highlight \"successful\". It also works correctly\nwhen 'len' happens to reference the end-of-string NUL. Okay.\n\n> + strbuf_addstr(dest, p->color);\n> + strbuf_add(dest, src, len);\n> + strbuf_addstr(dest, GIT_COLOR_RESET);\n> + n -= len;\n> + src += len;\n> + break;\n> + }\n\nSo, despite the explanation in the commit message, this function isn't\n_generally_ highlighting keywords in the sideband. Instead, it is\nhighlighting a keyword only if it finds it at the start of string\n(ignoring whitespace). Perhaps the commit message could be more clear\nabout that.\n\nA natural follow-on question is whether strings are fed to this\nfunction one line at a time or if the incoming string may have\nembedded newlines (in which case, you might need to find a prefix\nfollowing a newline, as well?).\n\n> + }\n> +\n> + strbuf_add(dest, src, n);\n> +}\n"},{"id":"354150","messageId":"CAPig+cRv5sHSoAPVi=Dnu2HXcbGThdMsgOfvs-Kh2NbnGg-j8w@mail.gmail.com","threadId":"48993","inReplyTo":"20180731173651.184716-2-hanwen@google.com","subject":"Re: [PATCH 1/2] Document git config getter return value.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-01T08:24:01Z","receivedAt":"2018-08-01T08:24:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:\n> diff --git a/config.h b/config.h\n> @@ -178,10 +178,16 @@ struct config_set {\n> +/*\n> + * The int return values in the functions is 1 if not found, 0 if found, leaving\n> + * the found value in teh 'dest' pointer.\n> + */\n\n\"teh\"?\n\nInstead of talking about \"the int return values\", simpler would be to\nsay \"returns 1 if ...; else 0\". Not worth a re-roll, though.\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"},{"id":"354164","messageId":"xmqqeffi856n.fsf@gitster-ct.c.googlers.com","threadId":"48993","inReplyTo":"CAPig+cSbibJ7i8LwJqPe06xJObnq6dJdMUnJoC1uAg4zUQq3KA@mail.gmail.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T15:41:52Z","receivedAt":"2018-08-01T15:41:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:\n>> Highlight keywords in remote sideband output.\n>\n> Prefix with the module you're touching, don't capitalize, and drop the\n> period. Perhaps:\n>\n> sideband: highlight keywords in remote sideband output\n\nYup (I locally did something similar when queued it).\n\n>> The highlighting is done on the client-side. Supported keywords are\n>> \"error\", \"warning\", \"hint\" and \"success\".\n>>\n>> The colorization is controlled with the config setting \"color.remote\".\n>\n> What's the motivation for this change? The commit message should say\n> something about that and give an explanation of why this is done\n> client-side rather than server-side.\n\nGood suggestion.\n\n>\n>> Co-authored-by: Duy Nguyen \n>\n> Helped-by: is more typical.\n>\n>> Signed-off-by: Han-Wen Nienhuys \n>> ---\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> @@ -1229,6 +1229,15 @@ color.push::\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> If this is \"boolean\", what does \"auto\" mean? Perhaps update the\n> description to better match other color-related options.\n\nExisting `color.branch` is already loose in the same way, but others\nlike color.{diff,grep,interactive} read better. No, past mistakes\nby others is not a good excuse to make things worse, but I'd say it\nalso is OK to clean this up, together with `color.branch`, after this\nchange on top.\n\n>> + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n>\n> So, the strncasecmp() is checking if one of the recognized keywords is\n> at the 'src' position, and the !isalnum() ensures that you won't pick\n> up something of which the keyword is merely a prefix. For instance,\n> you won't mistakenly highlight \"successful\". It also works correctly\n> when 'len' happens to reference the end-of-string NUL. Okay.\n\nHmm, do we actually say things like \"Error: blah\"? I am not sure if\nI like this strncasecmp all that much.\n\n>> + strbuf_addstr(dest, p->color);\n>> + strbuf_add(dest, src, len);\n>> + strbuf_addstr(dest, GIT_COLOR_RESET);\n>> + n -= len;\n>> + src += len;\n>> + break;\n>> + }\n>\n> So, despite the explanation in the commit message, this function isn't\n> _generally_ highlighting keywords in the sideband. Instead, it is\n> highlighting a keyword only if it finds it at the start of string\n> (ignoring whitespace). Perhaps the commit message could be more clear\n> about that.\n\nSounds good.\n\nI didn't comment on other parts of your review posed as questions\n(that require some digging and thinking), but I think they are all\nworthwhile thing to think about.\n\nThanks.\n"},{"id":"354169","messageId":"CAFQ2z_PXfp60C-aiizUURjcqr-A+VJQDjMJ+fU_5DOo10x+rcQ@mail.gmail.com","threadId":"48993","inReplyTo":"xmqqeffi856n.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-01T17:18:31Z","receivedAt":"2018-08-01T17:18:48Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano wrote:\n\n> Hmm, do we actually say things like \"Error: blah\"? I am not sure if\n> I like this strncasecmp all that much.\n\nthis is for the remote end, so what we (git-core) says isn't all that\nrelevant. The reason I put this here is that Gerrit has some messages\nthat say \"ERROR: .. \"\n\n> >> + strbuf_addstr(dest, p->color);\n> >> + strbuf_add(dest, src, len);\n> >> + strbuf_addstr(dest, GIT_COLOR_RESET);\n> >> + n -= len;\n> >> + src += len;\n> >> + break;\n> >> + }\n> >\n> > So, despite the explanation in the commit message, this function isn't\n> > _generally_ highlighting keywords in the sideband. Instead, it is\n> > highlighting a keyword only if it finds it at the start of string\n> > (ignoring whitespace). Perhaps the commit message could be more clear\n> > about that.\n>\n> Sounds good.\n>\n> I didn't comment on other parts of your review posed as questions\n> (that require some digging and thinking), but I think they are all\n> worthwhile thing to think about.\n\nSorry for being dense, but do you want me to send an updated patch or\nnot based on your and Eric's comments or not?\n\nthanks,\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"354174","messageId":"xmqqsh3y6jfp.fsf@gitster-ct.c.googlers.com","threadId":"48993","inReplyTo":"CAFQ2z_PXfp60C-aiizUURjcqr-A+VJQDjMJ+fU_5DOo10x+rcQ@mail.gmail.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T18:16:58Z","receivedAt":"2018-08-01T18:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano wrote:\n>\n>> Hmm, do we actually say things like \"Error: blah\"? I am not sure if\n>> I like this strncasecmp all that much.\n>\n> this is for the remote end, so what we (git-core) says isn't all that\n> relevant.\n\nIt is very relevant, I would think. Because the coloring is\ncontrolled at the client end with this implementation, third-party\nremote implementations have strong incentive to follow what our\nremote end says and not to deviate. Preventing them from being\ndifferent just to be different does help the users, no?\n\n>> > So, despite the explanation in the commit message, this function isn't\n>> > _generally_ highlighting keywords in the sideband. Instead, it is\n>> > highlighting a keyword only if it finds it at the start of string\n>> > (ignoring whitespace). Perhaps the commit message could be more clear\n>> > about that.\n>>\n>> Sounds good.\n>>\n>> I didn't comment on other parts of your review posed as questions\n>> (that require some digging and thinking), but I think they are all\n>> worthwhile thing to think about.\n>\n> Sorry for being dense, but do you want me to send an updated patch or\n> not based on your and Eric's comments or not?\n\nIt would help to see the comments responded with either \"such a\nchange is not needed for such and such reasons\", \"it may make sense\nbut let's leave it to a follow-up patch later,\" etc., or with a\n\"here is an updated patch, taking all the comments to the previous\nversion into account---note that I rejected that particular comment\nbecause of such and such reasons\".\n"},{"id":"354223","messageId":"CAFQ2z_Mazv3jQF6+juLRu214Ddem3=TOd4tnVcm13ZThYMNH1g@mail.gmail.com","threadId":"48993","inReplyTo":"xmqqsh3y6jfp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-02T07:34:08Z","receivedAt":"2018-08-02T07:34:22Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Aug 1, 2018 at 8:17 PM Junio C Hamano wrote:\n> >> Hmm, do we actually say things like \"Error: blah\"? I am not sure if\n> >> I like this strncasecmp all that much.\n> >\n> > this is for the remote end, so what we (git-core) says isn't all that\n> > relevant.\n>\n> It is very relevant, I would think. Because the coloring is\n> controlled at the client end with this implementation, third-party\n> remote implementations have strong incentive to follow what our\n> remote end says and not to deviate. Preventing them from being\n> different just to be different does help the users, no?\n\nBut the ship has already sailed: Gerrit has been saying \"ERROR\"\ninstead of \"error\" for many years. In the case of Gerrit, the upper\ncase message is a (poor) way to make the error message stand out from\nthe sea of progress messages that \"git push\" prints on the terminal,\nwithout requiring a newer version of git-core.\n\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"354227","messageId":"CAPig+cR3WdYpZftK72MKMs4BBF2ZiuO-Bm_-edJ4tr1bRQvoGQ@mail.gmail.com","threadId":"48993","inReplyTo":"xmqqsh3y6jfp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T10:24:01Z","receivedAt":"2018-08-02T10:24:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano wrote:\n> Han-Wen Nienhuys writes:\n> > Sorry for being dense, but do you want me to send an updated patch or\n> > not based on your and Eric's comments or not?\n>\n> It would help to see the comments responded with either \"such a\n> change is not needed for such and such reasons\", \"it may make sense\n> but let's leave it to a follow-up patch later,\" etc., or with a\n> \"here is an updated patch, taking all the comments to the previous\n> version into account---note that I rejected that particular comment\n> because of such and such reasons\".\n\nRight. The way to know whether or not an updated patch is warranted is\nto respond to review comments, saying that you agree or disagree with\nvarious points raised (and why), and by answering the (genuine)\nquestions raised during review. The outcome of the dialogue with\nreviewers will make it clear if an updated patch is necessary. (It's\nalso a courtesy to respond to review comments since reviewing is\ntime-consuming business and it's good to let reviewers know that the\ntime spent reviewing was not in vain.)\n\nRegarding my question about whether load_sideband_colors() can be\nmoved below the '!want_color_stderr(sideband_use_color)' conditional,\nafter studying the code further, I see that it can't be, because\nload_sideband_colors() is responsible for setting\n'sideband_use_color'. The fact that this code confused or left\nquestions in the mind of a reviewer may indicate that responsibilities\nare not partitioned in the best manner, and that the code could be\nstructured to be more clear.\n\nFor instance, it might make sense to rip all the 'sideband_use_color'\ngunk out of load_sideband_colors() and move it to\nmaybe_colorize_sideband(), perhaps like this:\n\n void maybe_colorize_sideband(...)\n {\n /* one-time initialization */\n if (sideband_use_color < 0) {\n if (!git_config_get_string(key, &value))\n sideband_use_color = git_config_colorbool(key, value);\n if (sideband_use_color > 0)\n load_sideband_colors();\n }\n\n if (!want_color_stderr(sideband_use_color)) {\n strbuf_add(dest, src, n);\n return;\n }\n ...as before...\n }\n\nYou may or may not agree with the above suggestion; it's good to let\nthe reviewer know how you feel about it.\n\nYour follow-on comment about how Gerrit has for years used \"ERROR\" is\nexactly the sort of information which should be in the commit messages\nsince it saves reviewers (and future readers, months or years down the\nroad) from head-scratching, wondering why the code was written the way\nit was (strcasecmp() vs. strcmp(), for instance).\n\nThe more pertinent information you can say up front in the commit\nmessage, the less likely reviewers will be confused or wonder why you\nmade the choices you did. My question about whether\nmaybe_colorize_sideband() is fed lines one-by-one or whether its input\nmay contain embedded newlines is a good example of how a more complete\ncommit message could help. As the author of the patch, you have been\nworking in this code and likely know the answer, but reviewers won't\nnecessarily have this information at hand. If the commit message says\nup front that this function processes lines one-by-one, then the\nreviewer feels reassured that the patch author understands the\nimplications of the change (as opposed to the patch author perhaps not\nhaving thought of the possibility of embedded newlines). So, it's a\ngenuine question (I still don't know the answer.)\n"},{"id":"354231","messageId":"CAFQ2z_OZjGykJ6xUwXB69MSChJpPQzc0xBPDw82oA6+tOs00qQ@mail.gmail.com","threadId":"48993","inReplyTo":"CAPig+cSbibJ7i8LwJqPe06xJObnq6dJdMUnJoC1uAg4zUQq3KA@mail.gmail.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-02T11:43:53Z","receivedAt":"2018-08-02T11:44:09Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Jul 31, 2018 at 10:21 PM Eric Sunshine wrote:\n>\n> On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:\n> > Highlight keywords in remote sideband output.\n>\n> Prefix with the module you're touching, don't capitalize, and drop the\n> period. Perhaps:\n\nDone.\n\n> sideband: highlight keywords in remote sideband output\n>\n> > The highlighting is done on the client-side. Supported keywords are\n> > \"error\", \"warning\", \"hint\" and \"success\".\n> >\n> > The colorization is controlled with the config setting \"color.remote\".\n>\n> What's the motivation for this change? The commit message should say\n> something about that and give an explanation of why this is done\n> client-side rather than server-side.\n\nAdded\n\n> > Co-authored-by: Duy Nguyen \n>\n> Helped-by: is more typical.\n\nDone.\n\n> > Signed-off-by: Han-Wen Nienhuys \n> > ---\n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > @@ -1229,6 +1229,15 @@ color.push::\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> If this is \"boolean\", what does \"auto\" mean? Perhaps update the\n> description to better match other color-related options.\n\nAll other doc entries say \"boolean\" here too. I'm happy to fix\nphrasing of this file in a follow-on change, but let's keep it like\nthis for consistency.\n\n> > diff --git a/sideband.c b/sideband.c\n> > @@ -1,6 +1,97 @@\n> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> > +{\n> > + int i;\n> > +\n> > + load_sideband_colors();\n> > + if (!want_color_stderr(sideband_use_color)) {\n> > + strbuf_add(dest, src, n);\n> > + return;\n> > + }\n>\n> Can load_sideband_colors() be moved below the !want_color_stderr() conditional?\n\nReorganized this a bit.\n\n> > +\n> > + while (isspace(*src)) {\n> > + strbuf_addch(dest, *src);\n> > + src++;\n> > + n--;\n> > + }\n> > +\n> > + for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> > + struct kwtable* p = keywords + i;\n> > + int len = strlen(p->keyword);\n>\n> Would it make sense to precompute each keyword length so you don't\n> have to recompute them repeatedly, or is that premature optimization?\n\nThat is premature optimization. The next line does a strncasecmp on\nthe same data so the cost (loading the keywords into CPU cache) is the\nsame, while precomputing the length makes the code more error prone.\n\n> > + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {\n>\n> So, the strncasecmp() is checking if one of the recognized keywords is\n> at the 'src' position, and the !isalnum() ensures that you won't pick\n> up something of which the keyword is merely a prefix. For instance,\n> you won't mistakenly highlight \"successful\". It also works correctly\n> when 'len' happens to reference the end-of-string NUL. Okay.\n\nadded comment.\n\n> > + strbuf_addstr(dest, p->color);\n> > + strbuf_add(dest, src, len);\n> > + strbuf_addstr(dest, GIT_COLOR_RESET);\n> > + n -= len;\n> > + src += len;\n> > + break;\n> > + }\n>\n> So, despite the explanation in the commit message, this function isn't\n> _generally_ highlighting keywords in the sideband. Instead, it is\n> highlighting a keyword only if it finds it at the start of string\n> (ignoring whitespace). Perhaps the commit message could be more clear\n> about that.\n\nupdated message.\n\n> A natural follow-on question is whether strings are fed to this\n> function one line at a time or if the incoming string may have\n> embedded newlines (in which case, you might need to find a prefix\n> following a newline, as well?).\n\nadded comment.\n\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"354232","messageId":"CAFQ2z_NOxB4E13hoUPwoP+kZ0rVofCf3mb+bdopyTLnzWu1kPA@mail.gmail.com","threadId":"48993","inReplyTo":"CAPig+cR3WdYpZftK72MKMs4BBF2ZiuO-Bm_-edJ4tr1bRQvoGQ@mail.gmail.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2018-08-02T11:46:20Z","receivedAt":"2018-08-02T11:46:34Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Aug 2, 2018 at 12:24 PM Eric Sunshine wrote:\n>\n> On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano wrote:\n> > Han-Wen Nienhuys writes:\n> > > Sorry for being dense, but do you want me to send an updated patch or\n> > > not based on your and Eric's comments or not?\n> >\n> > It would help to see the comments responded with either \"such a\n> > change is not needed for such and such reasons\", \"it may make sense\n> > but let's leave it to a follow-up patch later,\" etc., or with a\n> > \"here is an updated patch, taking all the comments to the previous\n> > version into account---note that I rejected that particular comment\n> > because of such and such reasons\".\n>\n> Right. The way to know whether or not an updated patch is warranted is\n> to respond to review comments, saying that you agree or disagree with\n> various points raised (and why), and by answering the (genuine)\n> questions raised during review. The outcome of the dialogue with\n> reviewers will make it clear if an updated patch is necessary. (It's\n> also a courtesy to respond to review comments since reviewing is\n> time-consuming business and it's good to let reviewers know that the\n> time spent reviewing was not in vain.)\n\nSure. My doubt is that it's hard to tell what the state of my patch is\nat any given time.\n\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"354281","messageId":"CAPig+cS5Nwnau36t_G-PBgY_8aoQQT6b_TfQPeyLt0tFXSppeQ@mail.gmail.com","threadId":"48993","inReplyTo":"CAFQ2z_NOxB4E13hoUPwoP+kZ0rVofCf3mb+bdopyTLnzWu1kPA@mail.gmail.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T17:37:36Z","receivedAt":"2018-08-02T17:37:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys wrote:\n> Sure. My doubt is that it's hard to tell what the state of my patch is\n> at any given time.\n\nUnderstandable. Junio sends out a periodic \"What's cooking\" email\nsummarizing the state of patches sent to the list. The most recent one\nis here [1].\n\nA patch series may be annotated with \"will merge to next\" (meaning he\nthinks it's stable and probably ready to expose to a wider audience),\n\"waiting for review\" (meaning not enough people have shown interest to\nlook it over), \"waiting for a re-roll\" (meaning he expects a new\nversion). Other annotations are possible. If a patch series isn't\nannotated, it means he hasn't made any decision about it yet.\n\nAlso, if your patch series is not in \"What's cooking\", it may mean\nthat Junio just hasn't gotten around to your submission yet, and there\nis a good chance it will be in the following \"What's cooking\". If,\nhowever, a couple weeks or more elapse and your submission doesn't\nshow up, then it may have gotten lost in the noise, in which case it's\na good idea to send a \"ping\" as a reminder.\n\n[1]: https://public-inbox.org/git/xmqqd0vbt14e.fsf@gitster-ct.c.googlers.com/\n"},{"id":"354282","messageId":"CAGZ79kYGS4DvoetyGX01ciNrxxLCqgXoVSpLhmgYZ8B51LzhSg@mail.gmail.com","threadId":"48993","inReplyTo":"CAPig+cS5Nwnau36t_G-PBgY_8aoQQT6b_TfQPeyLt0tFXSppeQ@mail.gmail.com","subject":"Re: [PATCH 2/2] Highlight keywords in remote sideband output.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T17:44:14Z","receivedAt":"2018-08-02T17:44:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 10:37 AM Eric Sunshine wrote:\n>\n> On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys wrote:\n> > Sure. My doubt is that it's hard to tell what the state of my patch is\n> > at any given time.\n>\n> Understandable. Junio sends out a periodic \"What's cooking\" email\n> summarizing the state of patches sent to the list. The most recent one\n> is here [1].\n\nPlease contrast that with [2], specifically:\n\n The discussion thread in the list archive is\n the authoritative record of the discussion; treat \"What's cooking\"\n as my personal note, nothing more.\n\n[2] https://public-inbox.org/git/xmqqsh3x69ap.fsf@gitster-ct.c.googlers.com/\n"}]}