Re: [PATCH 1/1] Highlight keywords in remote sideband output.

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 30 2018, Han-Wen Nienhuys wrote:


> +	if (sideband_use_color < 0) {
> +		const char *key = "color.remote";
> +		char *value = NULL;
> +		if (!git_config_get_string(key, &value))
> +			sideband_use_color = git_config_colorbool(key, value);
> [...]
> +	struct kwtable {
> +		const char *keyword;
> +		const char *color;
> +	} keywords[] = {
> +		{"hint", GIT_COLOR_YELLOW},
> +		{"warning", GIT_COLOR_BOLD_YELLOW},
> +		{"success", GIT_COLOR_BOLD_GREEN},
> +		{"error", GIT_COLOR_BOLD_RED},


FWIW I agree with other reviewers that it would be nice if these could
be customized, but I also think it can wait for some follow-up patch.

Users who don't like these colors don't have to use them, and then
they're no worse off than now, whereas some users will appreciate these
and be better off than now.

So great if you want to improve this, but just chiming in on that point
because I think we should be respectful of the time of contributors, and
not make perfect the enemy of the good.

> +test_expect_success 'setup' '
> +	mkdir .git/hooks &&
> +        cat << EOF > .git/hooks/update &&

It's the coding style of git.git to snuggle the ">" with the
filename. I.e. "<<EOF >file", not "<< EOF > file". Same for the other
occurrences of ">" etc. below.

> +#!/bin/sh
> +echo error: error
> +echo hint: hint
> +echo success: success
> +echo warning: warning

Here I think it makes sense to more exhaustively test the sort of output
we do and don't hilight, both for regression testing and to explain this
in code. E.g. let's do all of these:

    error: text
    hint: text
    success: text
    warning: text

    PREFIXerror: text
    PREFIXhint: text
    PREFIXsuccess: text
    PREFIXwarning: text

    errorSUFFIX: text
    hintSUFFIX: text
    successSUFFIX: text
    warningSUFFIX: text

	this is an error: text
	this is a hint: text
	this is success: text
	this is a warning: text

> +test_expect_success 'push' 'git -c color.remote=always push origin HEAD:refs/heads/newbranch 2>output &&
> +  test_decode_color < output > decoded &&
> +  test_i18ngrep "<BOLD;RED>error<RESET>:" decoded &&
> +  test_i18ngrep "<YELLOW>hint<RESET>:" decoded &&
> +  test_i18ngrep "<BOLD;GREEN>success<RESET>:" decoded &&
> +  test_i18ngrep "<BOLD;YELLOW>warning<RESET>:" decoded'

I think this is a case where we want just "grep", not "test_i18ngrep",
that output you just emitted won't be translated.