Re: [PATCH v2 1/3] stash: add test to ensure reflog --rewrite --updatref behavior

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Feb 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> This test was already a bit broken in needing the preceding tests, but
>> it will break now if REFFILES isn't true, which you can reproduce
>> e.g. with:
>>
>>     ./t3903-stash.sh --run=1-16,18-50 -vixd
>>
>> Perhaps the least sucky solution to that is:
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index ec9cc5646d6..1d11c9bda20 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -205,13 +205,19 @@ test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>>  	cat >expect <<-EOF &&
>>  	$(test_oid zero) $oid
>>  	EOF
>> -	test_cmp expect actual
>> +	test_cmp expect actual &&
>> +	>dropped-stash
>>  '
>
> If "git stash drop", invoked in earlier part of this test before the
> precontext, fails, then test_cmp would fail and we leave
> dropped-stash untouched, even though we did run "git stash drop"
> already.

Yes, that's an edge case that's exposed here, but which I thought wasn't
worth bothering with. I.e. if you get such a failure on test N getting
N+1 failing as well isn't that big of a deal.

The big deal is rather that we know we're adding a REFFILES dependency
to this, which won't run this at all, which will make the "stash pop"
below fail.

> Why does the next test need to depend on what has happened earlier?

They don't need to, and ideally wouldn't, but most of our test suite has
this issue already. Try e.g. running it with:

    prove t[0-9]*.sh :: --run=10-20 --immediate

And for this particular file running e.g. this on master:

    ./t3903-stash.sh --run=1-10,30-40

Will fail 7 tests in the 30-40 range.

So while it's ideal that we can combine tests with arbitrary --run
parameters, i.e. all tests would tear down fully, not depend on earlier
tests etc. we're really far from that being the case in practice.

So insisting on some general refactoring of this file as part of this
series seems a bit overzelous, which is why I'm suggesting the bare
minimum to expect and work around the inevitable REFFILES failure, as
Han-Wen is actively working in that area.

>>  test_expect_success 'stash pop' '
>>  	git reset --hard &&
>>  	git stash pop &&
>> -	test 9 = $(cat file) &&
>> +	if test -e dropped-stash
>> +	then
>> +		test 9 = $(cat file)
>> +	else
>> +		test 3 = $(cat file)
>> +	fi &&
>>  	test 1 = $(git show :file) &&
>>  	test 1 = $(git show HEAD:file) &&
>>  	test 0 = $(git stash list | wc -l)