git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Aug 3, 2018, 03:52 UTC
Message-ID
<20180803035243.GH197924@aiede.svl.corp.google.com>
In-Reply-To
<20180802121308.17371-3-hanwen@google.com>
Hi,
Han-Wen Nienhuys wrote:
Show 7 quoted lines
> 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.
- 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?
[...]
Show 5 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?

Show 5 quoted lines
> 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: ").

As a strawman idea, what would you think of a way to allow the server to do some coloration by using color tags like

 <red>Erreur</red>: ...
?

As an advantage, this would give the server more control. As a disadvantage, it is not so useful as "semantic markup", meaning it is harder to figure out how to present to accessibility tools in a meaningful way. Plus, there's the issue of usefulness with existing servers you mentioned:

> 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.
[...]
Show 14 quoted lines
> --- 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?

[...]
Show 11 quoted lines
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,6 +1,103 @@
>  #include "cache.h"
> +#include "color.h"
> +#include "config.h"
>  #include "pkt-line.h"
>  #include "sideband.h"
> +#include "help.h"
> +
> +struct kwtable {
nit: perhaps kwtable_entry?
Show 11 quoted lines
> +	/*
> +	 * We use keyword as config key so it can't contain funny chars like
> +	 * spaces. When we do that, we need a separate field for slot name in
> +	 * load_sideband_colors().
> +	 */
> +	const char *keyword;
> +	char color[COLOR_MAXLEN];
> +};
> +
> +static struct kwtable keywords[] = {
> +	{ "hint",	GIT_COLOR_YELLOW },
[...]
> +// Returns a color setting (GIT_COLOR_NEVER, etc).
nit: Git uses C89-style /* */ comments.
> +static int use_sideband_colors(void)
Makes sense.
[...]
Show 7 quoted lines
> +void list_config_color_sideband_slots(struct string_list *list, const char *prefix)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(keywords); i++)
> +		list_config_item(list, prefix, keywords[i].keyword);
> +}

Not about this patch: I wonder if we can eliminate this boilerplate some time in the future.

[...]
Show 5 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?

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

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

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

Show 12 quoted lines
> +		strbuf_add(dest, src, n);
> +		return;
> +	}
> +
> +	while (isspace(*src)) {
> +		strbuf_addch(dest, *src);
> +		src++;
> +		n--;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> +		struct kwtable* p = keywords + i;
nit: * should attach to the variable, C-style.

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

Show 15 quoted lines
> +		int len = strlen(p->keyword);
> +                /*
> +                 * Match case insensitively, so we colorize output from existing
> +                 * servers regardless of the case that they use for their
> +                 * messages. We only highlight the word precisely, so
> +                 * "successful" stays uncolored.
> +                 */
> +		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
> +			strbuf_addstr(dest, p->color);
> +			strbuf_add(dest, src, len);
> +			strbuf_addstr(dest, GIT_COLOR_RESET);
> +			n -= len;
> +			src += len;
> +			break;
> +		}
Sensible.
[...]
Show 27 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.

[...]
Show 12 quoted lines
> +test_expect_success 'setup' '
> +	mkdir .git/hooks &&
> +	cat << EOF > .git/hooks/update &&
> +#!/bin/sh
> +echo error: error
> +echo hint: hint
> +echo success: success
> +echo warning: warning
> +echo prefixerror: error
> +exit 0
> +EOF
> +	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
> +	echo 1 >file &&
> +	git add file &&
> +	git commit -m 1 &&
This can use test_commit.  See t/README for details.
[...]
Show 18 quoted lines
> +test_expect_success 'push' '
> +	git -c color.remote=always push -f origin HEAD:refs/heads/newbranch 2>output &&
> +	test_decode_color <output >decoded &&
> +	grep "<BOLD;RED>error<RESET>:" decoded &&
> +	grep "<YELLOW>hint<RESET>:" decoded &&
> +	grep "<BOLD;GREEN>success<RESET>:" decoded &&
> +	grep "<BOLD;YELLOW>warning<RESET>:" decoded &&
> +	grep "prefixerror: error" decoded
> +'
> +
> +test_expect_success 'push with customized color' '
> +	git -c color.remote=always -c color.remote.error=white push -f origin HEAD:refs/heads/newbranch2 2>output &&
> +	test_decode_color <output >decoded &&
> +	grep "<WHITE>error<RESET>:" decoded &&
> +	grep "<YELLOW>hint<RESET>:" decoded &&
> +	grep "<BOLD;GREEN>success<RESET>:" decoded &&
> +	grep "<BOLD;YELLOW>warning<RESET>:" decoded
> +'
Nice.

Thanks and hope that helps, Jonathan

Previous: Eric SunshineNext: Han-Wen Nienhuys
Message 9 of 10 in “sideband: highlight keywords in remote sideband output”
  1. 0/2 sideband: highlight keywords in remote sideband outputHan-Wen Nienhuys, Aug 2, 2018
  2. 1/2 config: document git config getter return value.Han-Wen Nienhuys, Aug 2, 2018
  3. Eric SunshineAug 2, 2018
  4. Jonathan NiederAug 3, 2018
  5. Jeff KingAug 3, 2018
  6. Junio C HamanoAug 3, 2018
  7. 2/2 sideband: highlight keywords in remote sideband outputHan-Wen Nienhuys, Aug 2, 2018
  8. Eric SunshineAug 3, 2018
  9. Jonathan NiederAug 3, 2018
  10. Han-Wen NienhuysAug 6, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.