On Fri, Aug 3, 2018 at 5:52 AM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>
> 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.
> [...]
> > 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.
> > 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.
> > +/*
> > + * 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 :-)
> > +{
> > + 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
> [...]
> > @@ -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).
> > + 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
