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

[Eric Sunshine <sunshine@sunshineco.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.

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

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

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

> +                       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);
> +}