Re: [PATCH 2/6] t5400: allow individual tests to fail

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

Jeff King <peff@peff.net> writes:

> On Mon, Feb 09, 2009 at 01:09:21AM -0800, Junio C Hamano wrote:
>
>> Each test chdir'ed around and ended up in a random place if any of the
>> test in the sequence failed but the entire test script was allowed to
>> run.  This wrapps each in a subshell as necessary.
>
> Certainly a good cleanup, but...
>
>> -test_expect_success \
>> -        'push can be used to delete a ref' '
>> +test_expect_success 'push can be used to delete a ref' '
>> +    (
>>  	cd victim &&
>>  	git branch extra master &&
>>  	cd .. &&
>>  	test -f victim/.git/refs/heads/extra &&
>>  	git send-pack ./victim/.git/ :extra master &&
>>  	! test -f victim/.git/refs/heads/extra
>> +    )
>>  '
>
> Wouldn't this be cleaner as:
>
>   (
>     cd victim &&
>     git branch extra master
>   ) &&
>   ...
>
> That is, it is not only safer but (IMHO) a bit easier to see which parts
> are happening in which directory.
>
>> +test_expect_success 'pushing a delete should be denied with denyDeletes' '
>> +    (
>>  	cd victim &&
>>  	git config receive.denyDeletes true &&
>>  	git branch extra master &&
>>  	cd .. &&
>>  	test -f victim/.git/refs/heads/extra &&
>>  	test_must_fail git send-pack ./victim/.git/ :extra master
>> +    )
>
> Ditto (and there are more, but I won't quote each one).
>
>> +test_expect_success 'pushing with --force should be denied with denyNonFastforwards' '
>> +    (
>>  	cd victim &&
>>  	git config receive.denyNonFastforwards true &&
>>  	cd .. &&
>>  	git update-ref refs/heads/master master^ || return 1
>>  	git send-pack --force ./victim/.git/ master && return 1
>>  	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
>> +    )
>
> And here I don't know what in the world is going on with those "return
> 1" lines. Shouldn't this be a chain of &&'s with a test_must_fail?
> I.e.,:
>
>   ( cd victim && git config receive.denyNonFastforwards true ) &&
>   git update-ref refs/heads/master master^ &&
>   test_must_fail git send-pack --force ./victim/.git/ master &&
>   ! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
>
> Not to mention that the final test_cmp would be more robust if written
> to make sure the victim's master ref stayed the same (instead of just
> making sure we didn't screw it up in one particular way). And it should
> probably use a git command rather than looking at the refs files (to be
> future-proof against any automatic ref-packing), but that is just
> nit-picking.
>
>
> All minor things, of course, but while we're cleaning up... :)

Sure.  This was made as a quick-fix to a mess others created, so I did not
study them very deeply.

Will reroll if I have the time but it is likely that I may be tending
other topics first.