Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2018, 20:18 UTC
Message-ID: <xmqq3726t11d.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <d0e6c6cf-7166-bef6-f179-c4e6acf7b0ac@ramsayjones.plus.com>

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

Show 25 quoted lines

> Attempting to grep the output of test_i18ngrep will not work under a
> poison build, since the output is (almost) guaranteed not to have the
> string you are looking for. In this case, the output of test_i18ngrep
> is further filtered by a simple piplined grep to exclude an '... remote
> end hung up unexpectedly' warning message. Use a regular 'grep -E' to
> replace the call to test_i18ngrep in the filter pipeline.
> Also, remove a useless invocation of 'sort' as the final element of the
> pipeline.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>  t/t5536-fetch-conflicts.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
> index 2e42cf331..38381df5e 100755
> --- a/t/t5536-fetch-conflicts.sh
> +++ b/t/t5536-fetch-conflicts.sh
> @@ -22,7 +22,7 @@ verify_stderr () {
>  	cat >expected &&
>  	# We're not interested in the error
>  	# "fatal: The remote end hung up unexpectedly":
> -	test_i18ngrep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual | sort &&
> +	grep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual &&
>  	test_i18ncmp expected actual

OK, but not quite OK.

Two grep invocations will not leave anything useful in 'actual' under poison build, and is almost guaranteed that 'expected' would not match, but that is perfectly OK because the final comparison is done.

However, it is brittle to rely on the latter "grep -v" to exit with status 0 for the &&-chain to work. The original was most likely masked by the "| sort" exiting with 0 status all the time ;-)

There needs an explanation why this commit thinks the invocation of "sort" useless. I do not particularly think "suppressing a not-found non-zero exit from 'grep'" is a useful thing, but are we committed to show the two warnings seen in the last test in this file to always in the shown order (i.e. the same order as the refspecs are given to us)? If not, then "sort" does serve a purpose. Note that I do not think we want to be able to reorder the warning messages in future versions of Git for that last case, so making that explicit may be a good justification.

    The "sort" as the last step in the pipeline makes sure that we
    do not have to guarantee the order in which we give the two
    warning messages from the last test in this script, but
    processing the refspecs in the same order as they are given on
    the command line and warning against them as we discover
    problems is a property we would rather want to keep, so drop it
    to make sure that we would catch regression when we accidentally
    change the order in which these messages are given.

or something like that, perhaps.