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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 31, 2018, 20:21 UTC
Message-ID: <CAPig+cSbibJ7i8LwJqPe06xJObnq6dJdMUnJoC1uAg4zUQq3KA@mail.gmail.com>
In-Reply-To: <20180731173651.184716-3-hanwen@google.com>

On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys <hanwen@google.com> wrote:

> Highlight keywords in remote sideband output.

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

    sideband: highlight keywords in remote sideband output

> 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.

> Co-authored-by: Duy Nguyen <pclouds@gmail.com>

Helped-by: is more typical.

Show 7 quoted lines

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> 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.

Show 11 quoted lines

> 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?

Show 10 quoted lines

> +
> +       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?

> +               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.

Show 7 quoted lines

> +                       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.

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?).

> +       }
> +
> +       strbuf_add(dest, src, n);
> +}