| Message ID | 6e136b62ca4588cc58f2cb59b635eeaf14e6e20d.1645554652.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libify reflog | expand |
On Tue, Feb 22 2022, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > There is missing test coverage to ensure that the resulting reflogs > after a git stash drop has had its old oid rewritten if applicable, and > if the refs/stash has been updated if applicable. This test looks good, and if 3/3 is applied and either of the flags you're passing is omitted they'll fail, so we know we have the missing coverage here. > Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: John Cai <johncai86@gmail.com> > --- > t/t3903-stash.sh | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index b149e2af441..ec9cc5646d6 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -185,10 +185,33 @@ test_expect_success 'drop middle stash by index' ' > test 1 = $(git show HEAD:file) > ' > > +test_expect_success 'drop stash reflog updates refs/stash' ' > + git reset --hard && > + git rev-parse refs/stash >expect && > + echo 9 >file && > + git stash && > + git stash drop stash@{0} && > + git rev-parse refs/stash >actual && > + test_cmp expect actual > +' This one will be portable to the reftable backend. > +test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' But as I noted in <220222.86fsob88h7.gmgdl@evledraar.gmail.com> (but it was easy to miss) this test will need to depend on REFFILES. So just changing this line to: test_expect_success REFFILES 'drop stash[...]' > + git reset --hard && > + echo 9 >file && > + git stash && > + oid="$(git rev-parse stash@{0})" && > + git stash drop stash@{1} && > + cut -d" " -f1-2 .git/logs/refs/stash >actual && > + cat >expect <<-EOF && > + $(test_oid zero) $oid > + EOF > + test_cmp expect actual > +' Then: > test_expect_success 'stash pop' ' > git reset --hard && > git stash pop && > - test 3 = $(cat file) && > + test 9 = $(cat file) && > test 1 = $(git show :file) && > test 1 = $(git show HEAD:file) && > test 0 = $(git stash list | wc -l) 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 ' 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)
Æ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. Why does the next test need to depend on what has happened earlier? > 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)
Hi Junio, On 23 Feb 2022, at 16:27, 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. > > Why does the next test need to depend on what has happened earlier? Ideally it shouldn't, but it seems like the way these test have been written makes subsequent tests depend on what previous tests have stashed. I'm wondering now that while we're at it, if we should just clean up these tests so there are no dependencies between the tests. Otherwise it's quite painful the next time someone needs to add a test here. We could follow the pattern in 5ac15ad2509 (reflog tests: add --updateref tests, 2021-10-16), where Ævar set up the tests and copied over the repo so each test is isolated from each other. > >> 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)
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)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> +test_expect_success 'drop stash reflog updates refs/stash' ' >> + git reset --hard && >> + git rev-parse refs/stash >expect && >> + echo 9 >file && >> + git stash && >> + git stash drop stash@{0} && >> + git rev-parse refs/stash >actual && >> + test_cmp expect actual >> +' > > This one will be portable to the reftable backend. > >> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' > > But as I noted in <220222.86fsob88h7.gmgdl@evledraar.gmail.com> (but it > was easy to miss) this test will need to depend on REFFILES. So just > changing this line to: > > test_expect_success REFFILES 'drop stash[...]' > >> + git reset --hard && >> + echo 9 >file && >> + git stash && >> + oid="$(git rev-parse stash@{0})" && >> + git stash drop stash@{1} && >> + cut -d" " -f1-2 .git/logs/refs/stash >actual && >> + cat >expect <<-EOF && >> + $(test_oid zero) $oid >> + EOF >> + test_cmp expect actual >> +' Why should this be tested with "cut" in the first place, though? If we start from stash@{0} = A stash@{1} = B stash@{2} = C and after saying "drop stash@{1}", what we need to check is that stash@{0} = A stash@{1} = C now holds, which can be done with "git rev-parse", and the fact that the ref-files backend happens to record both before-and-after object IDs is an irrelevant implementation detail, no? I am still puzzled.
Hi Junio, On 23 Feb 2022, at 17:51, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> +test_expect_success 'drop stash reflog updates refs/stash' ' >>> + git reset --hard && >>> + git rev-parse refs/stash >expect && >>> + echo 9 >file && >>> + git stash && >>> + git stash drop stash@{0} && >>> + git rev-parse refs/stash >actual && >>> + test_cmp expect actual >>> +' >> >> This one will be portable to the reftable backend. >> >>> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' >> >> But as I noted in <220222.86fsob88h7.gmgdl@evledraar.gmail.com> (but it >> was easy to miss) this test will need to depend on REFFILES. So just >> changing this line to: >> >> test_expect_success REFFILES 'drop stash[...]' >> >>> + git reset --hard && >>> + echo 9 >file && >>> + git stash && >>> + oid="$(git rev-parse stash@{0})" && >>> + git stash drop stash@{1} && >>> + cut -d" " -f1-2 .git/logs/refs/stash >actual && >>> + cat >expect <<-EOF && >>> + $(test_oid zero) $oid >>> + EOF >>> + test_cmp expect actual >>> +' > > Why should this be tested with "cut" in the first place, though? > > If we start from > > stash@{0} = A > stash@{1} = B > stash@{2} = C > > and after saying "drop stash@{1}", what we need to check is that > > stash@{0} = A > stash@{1} = C Yes, this is true but that doesn't seem to test the --rewrite functionality. I could be missing something, but it seems that the reflog --rewrite option will write the LHS old oid value in the .git/logs/refs/stash file. When --rewrite isn't used, the reflog delete still does the right thing to the RHS entry. I couldn't find any way to check this LFS value other than reaching into the actual file. If there is a way that would be preferable. > > now holds, which can be done with "git rev-parse", and the fact that > the ref-files backend happens to record both before-and-after object > IDs is an irrelevant implementation detail, no? > > I am still puzzled.
On Wed, Feb 23 2022, John Cai wrote: > Hi Junio, > > On 23 Feb 2022, at 17:51, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>>> +test_expect_success 'drop stash reflog updates refs/stash' ' >>>> + git reset --hard && >>>> + git rev-parse refs/stash >expect && >>>> + echo 9 >file && >>>> + git stash && >>>> + git stash drop stash@{0} && >>>> + git rev-parse refs/stash >actual && >>>> + test_cmp expect actual >>>> +' >>> >>> This one will be portable to the reftable backend. >>> >>>> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' >>> >>> But as I noted in <220222.86fsob88h7.gmgdl@evledraar.gmail.com> (but it >>> was easy to miss) this test will need to depend on REFFILES. So just >>> changing this line to: >>> >>> test_expect_success REFFILES 'drop stash[...]' >>> >>>> + git reset --hard && >>>> + echo 9 >file && >>>> + git stash && >>>> + oid="$(git rev-parse stash@{0})" && >>>> + git stash drop stash@{1} && >>>> + cut -d" " -f1-2 .git/logs/refs/stash >actual && >>>> + cat >expect <<-EOF && >>>> + $(test_oid zero) $oid >>>> + EOF >>>> + test_cmp expect actual >>>> +' >> >> Why should this be tested with "cut" in the first place, though? >> >> If we start from >> >> stash@{0} = A >> stash@{1} = B >> stash@{2} = C >> >> and after saying "drop stash@{1}", what we need to check is that >> >> stash@{0} = A >> stash@{1} = C > > Yes, this is true but that doesn't seem to test the --rewrite functionality. > I could be missing something, but it seems that the reflog --rewrite option > will write the LHS old oid value in the .git/logs/refs/stash file. When > --rewrite isn't used, the reflog delete still does the right thing to the > RHS entry. > > I couldn't find any way to check this LFS value other than reaching into the > actual file. If there is a way that would be preferable. Thanks for that summary that's accurate as far as I know. I think that's how this all works, and I don't know of another way to extract this information than this reaching behind the curtain. Which, I think is a lot clearer if we amend the test like this. Note that this doesn't really add anything for catching a regression goes, but I think helps guide the human reader through this step-by-step. So perhaps it would be good to fix the test up to have it (or maybe not): diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index ec9cc5646d6..bc58e99e3e6 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -198,12 +198,25 @@ test_expect_success 'drop stash reflog updates refs/stash' ' test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' git reset --hard && echo 9 >file && + + # Our two stashes + old_oid="$(git rev-parse stash@{0})" && git stash && - oid="$(git rev-parse stash@{0})" && + new_oid="$(git rev-parse stash@{0})" && + + # Our stash <old oid>/<new oid> before "drop" + cat >expect <<-EOF && + $(test_oid zero) $old_oid + $old_oid $new_oid + EOF + cut -d" " -f1-2 .git/logs/refs/stash >actual && + test_cmp expect actual && + + # Our stash <old oid>/<new oid> after "drop" git stash drop stash@{1} && cut -d" " -f1-2 .git/logs/refs/stash >actual && cat >expect <<-EOF && - $(test_oid zero) $oid + $(test_oid zero) $new_oid EOF test_cmp expect actual ' If this series is amended to drop the "EXPIRE_REFLOGS_REWRITE" flag then this will fail on that last test_cmp like: + diff -u expect actual --- expect 2022-02-23 23:37:40.438221222 +0000 +++ actual 2022-02-23 23:37:40.434221258 +0000 @@ -1 +1 @@ -0000000000000000000000000000000000000000 236c59f58e239e74e90b6832a98fa4b7f4b33647 +5c6ad4ca28e71ae3a007e6c77043d04bc42fa9ee 236c59f58e239e74e90b6832a98fa4b7f4b33647 I.e. our <old oid> is now referring to the now-deleted stash entry we just deleted, since we didn't rewrite it. And as we can see with some manual inspection the state before we dropped stash@{1} was: 0000000000000000000000000000000000000000 5c6ad4ca28e71ae3a007e6c77043d04bc42fa9ee 5c6ad4ca28e71ae3a007e6c77043d04bc42fa9ee 236c59f58e239e74e90b6832a98fa4b7f4b33647 My usual method of checking my assumption about this not being otherwise inspectable would be something like: diff --git a/refs/files-backend.c b/refs/files-backend.c index f59589d6cce..590c13e7a2b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3133,7 +3133,7 @@ static int files_reflog_expire(struct ref_store *ref_store, const struct object_id *oid; memset(&cb, 0, sizeof(cb)); - cb.rewrite = !!(expire_flags & EXPIRE_REFLOGS_REWRITE); + cb.rewrite = 0; cb.dry_run = !!(expire_flags & EXPIRE_REFLOGS_DRY_RUN); cb.policy_cb = policy_cb_data; cb.should_prune_fn = should_prune_fn; I.e. let's intentionally break the flag, and see what else fails (it's set in a few places, but this is the only place where it's checked). That should normally find the other things that are testing this, maybe there's a better way. But, no such luck :) The only thing that'll fail is this new test being added here. So just like my 5ac15ad2509 (reflog tests: add --updateref tests, 2021-10-16) this is covering a true blindspot in the "git reflog" functionality. The only tests that used --rewrite were a test added in c41a87dd80c (refs: make rev-parse --quiet actually quiet, 2014-09-18), which will pass if --rewrite is omitted. And the ones I added in 5ac15ad2509, which I added not to test --rewrite per-se, but to test that the --updateref part of it behaved as expected in combination with whatever effect it was having.
John Cai <johncai86@gmail.com> writes: > Yes, this is true but that doesn't seem to test the --rewrite functionality. > I could be missing something, but it seems that the reflog --rewrite option > will write the LHS old oid value in the .git/logs/refs/stash file. When > --rewrite isn't used, the reflog delete still does the right thing to the > RHS entry. > > I couldn't find any way to check this LFS value other than reaching into the > actual file. If there is a way that would be preferable. Ah, that one. As 2b81fab2 (git-reflog: add option --rewrite to update reflog entries while expiring, 2008-02-22) says, the redundant half of the reflog entry only matters to "certain sanity checks" and would not be even visible to normal ref API users. I wonder why we need to even say "--rewrite" in the first place. Perhaps we should implicitly set it always and eventually deprecate that option.
Hi Junio, On 23 Feb 2022, at 18:50, Junio C Hamano wrote: > John Cai <johncai86@gmail.com> writes: > >> Yes, this is true but that doesn't seem to test the --rewrite functionality. >> I could be missing something, but it seems that the reflog --rewrite option >> will write the LHS old oid value in the .git/logs/refs/stash file. When >> --rewrite isn't used, the reflog delete still does the right thing to the >> RHS entry. >> >> I couldn't find any way to check this LFS value other than reaching into the >> actual file. If there is a way that would be preferable. > > Ah, that one. > > As 2b81fab2 (git-reflog: add option --rewrite to update reflog > entries while expiring, 2008-02-22) says, the redundant half of the > reflog entry only matters to "certain sanity checks" and would not > be even visible to normal ref API users. I wonder why we need to > even say "--rewrite" in the first place. Perhaps we should > implicitly set it always and eventually deprecate that option. Yeah, that makes sense. I had this thought as I was figuring out how to test this. I can take care of this in a separate patch series
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)
On Thu, Feb 24 2022, John Cai wrote: > 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? That looks good to me if you're willing to do that legwork, probably better in a preceding cleanup commit. > 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 && nit: you can add \ to that, i.e. <<-\EOF. Helps readability, i.e. it's obvious right away that no variables are in play.. > + 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 > ' For this sort of thing I think it's usually better to override "test_expect_success" as a last resort, i.e. to have that "test_setup_stash_copy" just be a "setup_stash" or whatever function called from within your test_expect_success. And instead of the "rm -rf" later, just do: test_when_finished "rm -rf copy" && cp -R repo copy && [...] The test still needs to deal with the sub-repo, but it could cd or use "-C". It's bad to add "cd .." in a &&-chain, because if earlier steps fail we're in the wrong directory for the next test, so either -C or a sub-shell... > Not sure if it's worth it though? Maybe not, which is why I suggested upthread to maybe go for some smallest possible change here and focus on the lib-ificitaion :) > > >> >>>> 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)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> 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? > > That looks good to me if you're willing to do that legwork, probably > better in a preceding cleanup commit. Yup. Thanks for helping other contributors. I agree with many things you said in your review. >> 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 && We do not want to "chdir" around without isolating it in a subprocess. If this test fails after it goes to "repo" but before it does "cd ..", the next test begins in the "repo" directory, but it is most likely not expecting that. >> -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 >> +} This will create an anti-pattern, because you would want to have the part between "cd copy" and "cd .." in a subshell, but you do not want to do test_expect_success inside a subshell. Hence, this is a bad helper that does not help and should not be used, I would think. >> -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 >> ' > > For this sort of thing I think it's usually better to override > "test_expect_success" as a last resort, i.e. to have that > "test_setup_stash_copy" just be a "setup_stash" or whatever function > called from within your test_expect_success. > > And instead of the "rm -rf" later, just do: > > test_when_finished "rm -rf copy" && > cp -R repo copy && > [...] Yup. I think this is how we would write it: test_expect_success 'parents of stash' ' test_when_finished "rm -fr copy" && cp -R repo copy && ( cd copy && ... the real body of the test here, like ... test $(git rev-parse stash^) = $(git rev-parse HEAD) && ) ' > The test still needs to deal with the sub-repo, but it could cd or use > "-C". I am not sure about this. test_expect_success does not take "-C".
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b149e2af441..ec9cc5646d6 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -185,10 +185,33 @@ test_expect_success 'drop middle stash by index' ' test 1 = $(git show HEAD:file) ' +test_expect_success 'drop stash reflog updates refs/stash' ' + git reset --hard && + git rev-parse refs/stash >expect && + echo 9 >file && + git stash && + git stash drop stash@{0} && + git rev-parse refs/stash >actual && + test_cmp expect actual +' + +test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' + git reset --hard && + echo 9 >file && + git stash && + oid="$(git rev-parse stash@{0})" && + git stash drop stash@{1} && + cut -d" " -f1-2 .git/logs/refs/stash >actual && + cat >expect <<-EOF && + $(test_oid zero) $oid + EOF + test_cmp expect actual +' + test_expect_success 'stash pop' ' git reset --hard && git stash pop && - test 3 = $(cat file) && + test 9 = $(cat file) && test 1 = $(git show :file) && test 1 = $(git show HEAD:file) && test 0 = $(git stash list | wc -l)