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

[Han-Wen Nienhuys <hanwen@google.com>]

On Tue, Jul 31, 2018 at 10:21 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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:

Done.

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

Added

> > Co-authored-by: Duy Nguyen <pclouds@gmail.com>
>
> Helped-by: is more typical.

Done.

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

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.

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

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

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

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