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

On Tue, Jul 31, 2018 at 10:21 PM Eric Sunshine [off-list ref] wrote:

On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys [off-list ref] wrote:

quoted

Highlight keywords in remote sideband output.

Prefix with the module you're touching, don't capitalize, and drop the
period. Perhaps:

Done.

    sideband: highlight keywords in remote sideband output

quoted

The highlighting is done on the client-side. Supported keywords are
"error", "warning", "hint" and "success".

The colorization is controlled with the config setting "color.remote".

What's the motivation for this change? The commit message should say
something about that and give an explanation of why this is done
client-side rather than server-side.

Added

quoted

Co-authored-by: Duy Nguyen [off-list ref]

Helped-by: is more typical.

Done.

quoted

Signed-off-by: Han-Wen Nienhuys <redacted>
---

diff --git a/Documentation/config.txt b/Documentation/config.txt

@@ -1229,6 +1229,15 @@ color.push::
+color.remote::
+       A boolean to enable/disable colored remote output. If unset,
+       then the value of `color.ui` is used (`auto` by default).

If this is "boolean", what does "auto" mean? Perhaps update the
description to better match other color-related options.

All other doc entries say "boolean" here too. I'm happy to fix
phrasing of this file in a follow-on change, but let's keep it like
this for consistency.

quoted

diff --git a/sideband.c b/sideband.c

@@ -1,6 +1,97 @@
+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+       int i;
+
+       load_sideband_colors();
+       if (!want_color_stderr(sideband_use_color)) {
+               strbuf_add(dest, src, n);
+               return;
+       }

Can load_sideband_colors() be moved below the !want_color_stderr() conditional?

Reorganized this a bit.

quoted

+
+       while (isspace(*src)) {
+               strbuf_addch(dest, *src);
+               src++;
+               n--;
+       }
+
+       for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+               struct kwtable* p = keywords + i;
+               int len = strlen(p->keyword);

Would it make sense to precompute each keyword length so you don't
have to recompute them repeatedly, or is that premature optimization?

That is premature optimization.  The next line does a strncasecmp on
the same data so the cost (loading the keywords into CPU cache) is the
same, while precomputing the length makes the code more error prone.

quoted

+               if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {

So, the strncasecmp() is checking if one of the recognized keywords is
at the 'src' position, and the !isalnum() ensures that you won't pick
up something of which the keyword is merely a prefix. For instance,
you won't mistakenly highlight "successful". It also works correctly
when 'len' happens to reference the end-of-string NUL. Okay.

added comment.

quoted

+                       strbuf_addstr(dest, p->color);
+                       strbuf_add(dest, src, len);
+                       strbuf_addstr(dest, GIT_COLOR_RESET);
+                       n -= len;
+                       src += len;
+                       break;
+               }

So, despite the explanation in the commit message, this function isn't
_generally_ highlighting keywords in the sideband. Instead, it is
highlighting a keyword only if it finds it at the start of string
(ignoring whitespace). Perhaps the commit message could be more clear
about that.

updated message.

A natural follow-on question is whether strings are fed to this
function one line at a time or if the incoming string may have
embedded newlines (in which case, you might need to find a prefix
following a newline, as well?).

added comment.

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado