Re: [PATCH] t5534: fix misleading grep invocation

[Michael J Gruber <git@grubix.eu>]

Junio C Hamano venit, vidit, dixit 05.07.2017 18:26:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> It seems to be a little-known feature of `grep` (and it certainly came
>> as a surprise to this here developer who believed to know the Unix tools
>> pretty well) that multiple patterns can be passed in the same
>> command-line argument simply by separating them by newlines. Watch, and
>> learn:
>>
>> 	$ printf '1\n2\n3\n' | grep "$(printf '1\n3\n')"
>> 	1
>> 	3
>>
>> That behavior also extends to patterns passed via `-e`, and it is not
>> modified by passing the option `-E` (but trying this with -P issues the
>> error "grep: the -P option only supports a single pattern").
>>
>> It seems that there are more old Unix hands who are surprised by this
>> behavior, as grep invocations of the form
>>
>> 	grep "$(git rev-parse A B) C" file
>>
>> were introduced in a85b377d041 (push: the beginning of "git push
>> --signed", 2014-09-12), and later faithfully copy-edited in b9459019bbb
>> (push: heed user.signingkey for signed pushes, 2014-10-22).
>>
>> Please note that the output of `git rev-parse A B` separates the object
>> IDs via *newlines*, not via spaces, and those newlines are preserved
>> because the interpolation is enclosed in double quotes.
>>
>> As a consequence, these tests try to validate that the file contains
>> either A's object ID, or B's object ID followed by C, or both. Clearly,
>> however, what the test wanted to see is that there is a line that
>> contains all of them.
>>
>> This is clearly unintended, and the grep invocations in question really
>> match too many lines.
>>
>> Fix the test by avoiding the newlines in the patterns.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
> 
> The invocation this fixes is not just misleading but simply wrong.
> Nicely spotted.

In addition, the patch makes sure to catch any rev-parse failures which
the original invocation shove under the rug.

> Thanks, will queue.

Thanks from the faithful copy-editor ;)

How did you spot this? Are there grep versions that behave differently?

>>  t/t5534-push-signed.sh | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
>> index 5bcb288f5c4..464ffdd147a 100755
>> --- a/t/t5534-push-signed.sh
>> +++ b/t/t5534-push-signed.sh
>> @@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push certificate' '
>>  		sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
>>  	) >expect &&
>>  
>> -	grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
>> -	grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
>> +	noop=$(git rev-parse noop) &&
>> +	ff=$(git rev-parse ff) &&
>> +	noff=$(git rev-parse noff) &&
>> +	grep "$noop $ff refs/heads/ff" dst/push-cert &&
>> +	grep "$noop $noff refs/heads/noff" dst/push-cert &&
>>  	test_cmp expect dst/push-cert-status
>>  '
>>  
>> @@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '
>>  		sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
>>  	) >expect &&
>>  
>> -	grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
>> -	grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
>> +	noop=$(git rev-parse noop) &&
>> +	ff=$(git rev-parse ff) &&
>> +	noff=$(git rev-parse noff) &&
>> +	grep "$noop $ff refs/heads/ff" dst/push-cert &&
>> +	grep "$noop $noff refs/heads/noff" dst/push-cert &&
>>  	test_cmp expect dst/push-cert-status
>>  '
>>  
>>
>> base-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a