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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2018, 15:41 UTC
Message-ID: <xmqqeffi856n.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPig+cSbibJ7i8LwJqPe06xJObnq6dJdMUnJoC1uAg4zUQq3KA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 7 quoted lines

> 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

Yup (I locally did something similar when queued it).

Show 8 quoted lines

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

Good suggestion.

Show 15 quoted lines

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

Existing `color.branch` is already loose in the same way, but others like color.{diff,grep,interactive} read better. No, past mistakes by others is not a good excuse to make things worse, but I'd say it also is OK to clean this up, together with `color.branch`, after this change on top.

Show 7 quoted lines

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

Hmm, do we actually say things like "Error: blah"? I am not sure if I like this strncasecmp all that much.

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

Sounds good.

I didn't comment on other parts of your review posed as questions (that require some digging and thinking), but I think they are all worthwhile thing to think about.

Thanks.