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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 31, 2018, 07:37 UTC
Message-ID: <87tvofua7k.fsf@evledraar.gmail.com>
In-Reply-To: <20180730123334.65186-2-hanwen@google.com>

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

Show 14 quoted lines

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

Show 5 quoted lines

> +#!/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

Show 6 quoted lines

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