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

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

On Mon, Aug 6, 2018 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > The Git push process itself prints lots of non-actionable messages
> > (eg. bandwidth statistics, object counters for different phases of the
> > process), which obscures actionable error messages that servers may
>
> s/which obscures/which obscure/, as I think that "which" refers to
> "messages" above.
>
Done.

> > The highlighting is done on the client rather than server side, so
> > servers don't have to grow capabilities to understand terminal escape
> > codes and terminal state. It also consistent with the current state
> > where Git is control of the local display (eg. prefixing messages with
> > "remote: ").
>
> Yup.
>
> When we introduce "the receiving end asks messages to be sent with
> such and such decoration" protocol support, we would want a lot more
> than just painting messages in color, e.g. i18n, verbosity, and even
> "Hey, I am a script, send them in json".
>
> Until that happens, let's keep things simpler.  No i18n messages and
> no colored output over the wire.

Ack.

>
> > +color.remote::
> > +     If set, keywords at the start of the line are highlighted. The
> > +     keywords are "error", "warning", "hint" and "success", and are
> > +     matched case-insensitively. Maybe set to `always`, `false` (or
> > +     `never`) or `auto` (or `true`). If unset, then the value of
> > +     `color.ui` is used (`auto` by default).
>
> Reads much better.
>
> I am still trying to find a concise way to help readers who saw a
> line that begins with "Warnings: foo bar bla" and accept that it is
> OK the early 7 chars are not painted.  "... case-insensitively and
> honoring word boundary" is the best I came up so far, but  I am
> afraid that is adding more words without hinting what I want to convey
> strongly enough, so I am not going to suggest that (at least not yet).

I would suggest that the phrase "keyword" implies a tokenization, so
I'd leave as is.

> > +     for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > +             strbuf_reset(&sb);
> > +             strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
>
> This design means future enhancement to allow more keywords will
> have to be done in the form of adding more "color.remote.<key>",
> which means a few restrictions on them are cast in stone at the
> end-user facing design level, which we need to be careful about.
>
>         Side note. I do not worry about the implementation level
>         limitation at all.  For example, the current code will not
>         allow end-users and projects to add new keywords to be
>         painted, as it relies on the keywords[] static array we can
>         see above.  But that implementation detail does not prevent
>         us from improving it later to support more code in this
>         codepath that notices "color.remote.failure" in config file
>         and paint a line that begins with "failure:".
>
> Because the third-level "variable" name is case insensive, matching
> of any future keyword MUST be also done case insensitively.
>
> Also, as you mentioned elsewhere in this patch, the string that can
> be in the keyword MUST begin with an alphabetic and consists only of
> alphanumeric or dash.
>
> I do not think these limitations imposed by the design decision this
> patch is making are particularly bad ones---we just need to be aware
> of and firm about them.  When somebody else comes in the future and
> wants to recognize "F A I L" as a custom keyword case sensitively,
> we must be able to comfortably say "no" to it.
>
>         Side note. We _could_ instead use "remotemsg.<key>.color"
>         namespace, as the subsection names at the second level is a
>         lot looser, but I do not think it is a good idea to use in
>         this application, as they are case sensitive.
>
> The above discussion may deserve to be in the log message as a
> record to tell future ourselves why we decided to use
> color.remote.<key>.

I added a note about case insensitivity.

> > +             if (git_config_get_string(sb.buf, &value))
> > +                     continue;
> > +             if (color_parse(value, keywords[i].color))
> > +                     die(_("config value %s is not a color: %s"), sb.buf, value);
>
> That's a bit inconsistent, isn't it?  If the configuration is not
> even a string, we ignore it and continue, but if it is a string, we
> insist that it names a color and otherwise die?

Done; added test.

> > + * Optionally highlight one keyword in remote output if it appears at the start
> > + * of the line. This should be called for a single line only, which must be
> > + * passed as the first N characters of the SRC array.
> > + */
>
> Saying "MUST be" is cheap, but do we have anybody who polices that
> requirement?

rephrased.

> I think the code is OK without any assert() or BUG(), and that is
> because the design is "we just paint the keyword at the beginning of
> what the other side of the sideband wants us to show as a single
> unit".  If the other side sends a payload with an embedded LF in a
> single packet, that's their choice and we are free not to paint the
> beginning of the second line after that LF.  So from that point of
> view, perhaps we shouldn't even talk about "a single line only".

I don't understand this remark. Isn't the call to strpbrk() meant to
split the input on line endings?

> >  #define ANSI_SUFFIX "\033[K"
> > -#define DUMB_SUFFIX "        "
> > +#define DUMB_SUFFIX "             "
> >
>
> Was this change intended and if so for what purpose?  I can drop
> this hunk if this is a mere finger-slip without proofreading, but I
> do not want to do so without making sure I am not missing anything
> and not discarding a meaningful change.

This was my poor use of the tabify function. It would be nice if this
were more explicit though, maybe using a format string of

  sprintf(.. , "%*s", 8, "");

but we can leave that for another time.

> Noticing the dash in "<<-", I would have expected all of the above
> lines to be indented with a tab to align with 'w' in 'write_script'.

Done.

> > +     chmod +x .git/hooks/update &&
>
> No need for this "chmod +x"; that's one of the points in using
> write_script.

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