Re: [PATCH 6/6] t4124: fix test for non-compliance diff

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:
>
>> POSIX's diff(1) requires output in normal diff format.
>> However, busybox's diff's output is written in unified format.
>
> That's a pretty big difference. I'm surprised this only produces one
> problem in the test scripts. ;)
>
>> POSIX requires no option for normal-diff format.
>> 
>> A hint in test-lib-functions::test_cmp said `diff -u` isn't available
>> everywhere.
>> 
>> Workaround this problem by assuming `diff(1)` output is unified
>> if we couldn't make anything from normal-diff format.

I do not mind working it around, but I am a bit disturbed by an
uneven attitude towards POSIX noncompliance this series has.  If
we were willing to break other people's "sed" that does not do BRE
correctly, instead of using '[+]' trick to accomodate them while
making sure that an implementation that does not use nonstandard
extension and does only BRE, we should just similarly be writing
such an implementation of noncompliant diff off as broken, yet we
bend backwards over to make sure we can work with them here.  

IOW, I do not have trouble changing the test so that it works with
noncompliant "diff".  But then in the same series, I would prefer to
see the existing test keeps working with a possibly noncompliant
"sed" implementation that has been working well with the tests.

> But if this is the only spot, then adjusting to handle unified or normal
> diff isn't too bad.

Yup.

> Could we then make a single invocation that covers both diff formats? We
> can further observe that the only thing we do with the "fixed" file is
> count the lines, so we can leave the markers. Which means we could ditch
> sed entirely and use grep. Something like:
>
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 971a5a7512..15cb0c81b7 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -50,8 +50,9 @@ test_fix () {
>  	# fix should not barf
>  	apply_patch --whitespace=fix || return 1
>  
> -	# find touched lines
> -	$DIFF file target | sed -n -e "s/^> //p" >fixed
> +	# find touched lines; handle either normal or unified
> +	# diff, as system diff may generate either
> +	$DIFF file target | grep '^[>+][^+]' >fixed
>  
>  	# the changed lines are all expected to change
>  	fixed_cnt=$(wc -l <fixed)
>
> seems to work for with both busybox diff and GNU diff.

Sounds OK to me.