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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Aug 6, 2018, 14:17 UTC
Message-ID: <CAFQ2z_PfvGgiwWeDbhiBt77s+z7qWzqCMXXZT0EjFNZKx+WUMg@mail.gmail.com>
In-Reply-To: <20180803035243.GH197924@aiede.svl.corp.google.com>

On Fri, Aug 3, 2018 at 5:52 AM Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 17 quoted lines

>
> Hi,
>
> Han-Wen Nienhuys wrote:
>
> > The colorization is controlled with the config setting "color.remote".
> >
> > Supported keywords are "error", "warning", "hint" and "success". They
> > are highlighted if they appear at the start of the line, which is
> > common in error messages, eg.
> >
> >    ERROR: commit is missing Change-Id
>
> A few questions:
>
> - should this be documented somewhere in Documentation/technical/*protocol*?
>   That way, server implementors can know to take advantage of it.

The protocol docs seem to work on a much different level. Maybe there should be a "best practices" document or similar?

> - how does this interact with (future) internationalization of server
>   messages?  If my server knows that the client speaks French, should
>   they send "ERROR:" anyway and rely on the client to translate it?
>   Or are we deferring that question to a later day?

It doesn't, and we are deferring that question.

Show 11 quoted lines

> [...]
> > The Git push process itself prints lots of non-actionable messages
> > (eg. bandwidth statistics, object counters for different phases of the
> > process), and my hypothesis is that these obscure the actionable error
> > messages that our server sends back. Highlighting keywords in the
> > draws more attention to actionable messages.
>
> I'd be interested in ways to minimize Git's contribution to this as
> well.  E.g. can we make more use of \r to make client-produced progress
> messages take less screen real estate?  Should some of the servers
> involved (e.g., Gerrit) do so as well?

Yes, I'm interested in this too, but I fear it would veer into a territory that is prone to bikeshedding.

Gerrit should definitely also send less noise.

Show 25 quoted lines

> > Finally, this solution is backwards compatible: many servers already
> > prefix their messages with "error", and they will benefit from this
> > change without requiring a server update.
>
> Yes, this seems like a compelling reason to follow this approach.
>
> [...]
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1229,6 +1229,15 @@ color.push::
> >  color.push.error::
> >       Use customized color for push errors.
> >
> > +color.remote::
> > +     A boolean to enable/disable colored remote output. If unset,
> > +     then the value of `color.ui` is used (`auto` by default).
> > +
> > +color.remote.<slot>::
> > +     Use customized color for each remote keywords. `<slot>` may be
> > +     `hint`, `warning`, `success` or `error` which match the
> > +     corresponding keyword.
>
> What positions in a remote message are matched?  If a server prints a
> message about something being discouraged because it is error-prone,
> would the "error" in error-prone turn red?

yes, if it happened to occur after a line-break.

We could decide that we will only highlight

  <sp*><keyword>':' rest of line

or

  <sp*><keyword>'\n'

that would work for the Gerrit case, and would be useful forcing function to uniformize remote error messages.

> > +struct kwtable {
>
> nit: perhaps kwtable_entry?

done.

Show 7 quoted lines

> > +/*
> > + * Optionally highlight some keywords in remote output if they appear at the
> > + * start of the line. This should be called for a single line only.
> > + */
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>
> Can this be static?

Done.

> What does 'n' represent?  Can the comment say?  (Or if it's the length
> of src, can it be renamed to srclen?)

Added comment.

> Super-pedantic nit: even if there are multiple special words at the
> start, this will only highlight one. :)  So it could say something
> like "Optionally check if the first word on the line is a keyword and
> highlight it if so."

Super pedantic answer: if people care about it that much, they can read the 20 lines of code below :-)

Show 6 quoted lines

> > +{
> > +     int i;
> > +     if (!want_color_stderr(use_sideband_colors())) {
>
> nit: whitespace damage (you can check for this with "git show --check").
> There's a bit more elsewhere.

ran tabify on whole file.

> > +     for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > +             struct kwtable* p = keywords + i;
>
> nit: * should attach to the variable, C-style.

Done.

> You can use "make style" to do some automatic formatting (though this
> is a bit experimental, so do double-check the results).

sad panda:

hanwen@han-wen:~/vc/git$ make style
git clang-format --style file --diff --extensions c,h
Traceback (most recent call last):
  File "/usr/bin/git-clang-format", line 580, in <module>
    main()
  File "/usr/bin/git-clang-format", line 62, in main
    config = load_git_config()
  File "/usr/bin/git-clang-format", line 195, in load_git_config
    name, value = entry.split('\n', 1)
ValueError: need more than 1 value to unpack
Makefile:2663: recipe for target 'style' failed
make: *** [style] Error 1

Show 38 quoted lines

> [...]
> > @@ -48,8 +145,10 @@ int recv_sideband(const char *me, int in_stream, int out)
> >               len--;
> >               switch (band) {
> >               case 3:
> > -                     strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
> > -                                 DISPLAY_PREFIX, buf + 1);
> > +                     strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
> > +                                 DISPLAY_PREFIX);
> > +                     maybe_colorize_sideband(&outbuf, buf + 1, len);
> > +
> >                       retval = SIDEBAND_REMOTE_ERROR;
> >                       break;
> >               case 2:
> > @@ -69,20 +168,22 @@ int recv_sideband(const char *me, int in_stream, int out)
> >                               if (!outbuf.len)
> >                                       strbuf_addstr(&outbuf, DISPLAY_PREFIX);
> >                               if (linelen > 0) {
> > -                                     strbuf_addf(&outbuf, "%.*s%s%c",
> > -                                                 linelen, b, suffix, *brk);
> > -                             } else {
> > -                                     strbuf_addch(&outbuf, *brk);
> > +                                     maybe_colorize_sideband(&outbuf, b, linelen);
> > +                                     strbuf_addstr(&outbuf, suffix);
> >                               }
> > +
> > +                             strbuf_addch(&outbuf, *brk);
> >                               xwrite(2, outbuf.buf, outbuf.len);
>
> Nice.  This looks cleaner than what was there before, which is always
> a good sign.
>
> [...]
> > --- /dev/null
> > +++ b/t/t5409-colorize-remote-messages.sh
>
> Thanks for these.  It may make sense to have a test with some leading
> whitespace as well.

Done.

> > +     chmod +x .git/hooks/update &&
>
> Please use write_script for this, since on esoteric platforms it picks
> an appropriate shell.

> If you use <<-\EOF instead of <<EOF, that has two advantages:
> - it lets you indent the script
> - it saves the reviewer from having to look for $ escapes inside the
>   script body

Done (#TIL).

Show 5 quoted lines

> > +     echo 1 >file &&
> > +     git add file &&
> > +     git commit -m 1 &&
>
> This can use test_commit.  See t/README for details.

Done.

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