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

[John Cai <johncai86@gmail.com>]

Hi Ævar,

On 23 Feb 2022, at 16:50, Ævar Arnfjörð Bjarmason wrote:

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

Curious what your thoughts are on an effort to isolate these tests from each other.
I like your approach in t/t1417 in creating a test repo and copying it over each time.
Something like this?

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ac345eced8cb..40254f8dc99c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -41,7 +41,9 @@ diff_cmp () {
        rm -f "$1.compare" "$2.compare"
 }

-test_expect_success 'stash some dirty working directory' '
+test_expect_success 'setup' '
+       git init repo &&
+       cd repo &&
        echo 1 >file &&
        git add file &&
        echo unrelated >other-file &&
@@ -54,48 +56,54 @@ test_expect_success 'stash some dirty working directory' '
        test_tick &&
        git stash &&
        git diff-files --quiet &&
-       git diff-index --cached --quiet HEAD
+       git diff-index --cached --quiet HEAD &&
+       cat >expect <<-EOF &&
+       diff --git a/file b/file
+       index 0cfbf08..00750ed 100644
+       --- a/file
+       +++ b/file
+       @@ -1 +1 @@
+       -2
+       +3
+       EOF
+       cd ../
 '

-cat >expect <<EOF
-diff --git a/file b/file
-index 0cfbf08..00750ed 100644
---- a/file
-+++ b/file
-@@ -1 +1 @@
--2
-+3
-EOF
+test_stash () {
+       cp -R repo copy &&
+       cd copy &&
+       test_expect_success "$@" &&
+       cd ../ &&
+       rm -rf copy
+}

-test_expect_success 'parents of stash' '
+test_stash 'parents of stash' '
        test $(git rev-parse stash^) = $(git rev-parse HEAD) &&
        git diff stash^2..stash >output &&
        diff_cmp expect output
 '

Not sure if it's worth it though?


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