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

On Fri, Aug 3, 2018 at 5:52 AM Jonathan Nieder [off-list ref] wrote:

Hi,

Han-Wen Nienhuys wrote:

quoted

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.

[...]

quoted

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.

quoted

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.

[...]

quoted

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

quoted

+struct kwtable {

nit: perhaps kwtable_entry?

done.

quoted

+/*
+ * 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 :-)

quoted

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

quoted

+     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

[...]

quoted

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

[...]

quoted

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

quoted

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

quoted

+     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