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

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.

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

[...]

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?

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.

[...]

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?

[...]

nit: perhaps kwtable_entry?

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

[...]

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

[...]

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

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

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

[...]

Nice.  This looks cleaner than what was there before, which is always
a good sign.

[...]

Thanks for these.  It may make sense to have a test with some leading
whitespace as well.

[...]

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

[...]

+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