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

Ramsay Jones [off-list ref] writes:

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.

On 12/02/18 20:18, Junio C Hamano wrote:

Ramsay Jones [off-list ref] writes:

quoted

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

:-D

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.

"...is done with i18ncmp.", indeed.

The contents of 'actual' would look like:

  warning: # GETTEXT POISON #
  warning: # GETTEXT POISON #

or

  fatal: # GETTEXT POISON #

... depending on which test we are looking at.

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 ;-)

I must admit that I didn't think about the effect of the useless
"| sort" on the exit status!  What I saw was: a process that
received no input, sorted nothing and produced no output - pretty
much the definition of useless! ;-)

Also, the "| grep -v" part does remove the "... hung up ..."
message (when present in the input), since that message has not
been i18n-ed. I thought this was deliberate - but maybe not.
(also, so long a _some_ output gets passed on by that grep, the
exit status will be 0).

There needs an explanation why this commit thinks the invocation of
"sort" useless.

You mean, other than the fact that it is? ;-)

                  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.

I did not look back at the history of this test, so I can't say
if that was the original _intent_ of the "| sort" part of the
pipeline. However, it is not serving any purpose now.

    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.

Hmm, so do you want anything other than a commit message update?

ATB,
Ramsay Jones