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.
