| Message ID | 20200320213916.604658-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t3419: prevent failure when run with EXPENSIVE | expand |
On Fri, Mar 20, 2020 at 2:39 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > This test runs a function which itself runs several assertions. The > last of these assertions cleans up the .git/rebase-apply directory, > since when run with EXPENSIVE set, the function is invoked a second time > to run the same tests with a larger data set. > > However, as of 2ac0d6273f ("rebase: change the default backend from "am" > to "merge"", 2020-02-15), the default backend of rebase has changed, and > cleaning up the rebase-apply directory has no effect: it no longer > exists, since we're using rebase-merge instead. > > Since we don't really care which rebase backend is in use, let's just > clean up both, which means we'll do the right thing in every case. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > I noticed this when I was working on another series and running the > testsuite with GIT_TEST_LONG=1. We'll probably want to add this before > the release if possible. > > It may also be desirable to have at least once CI run that runs this > way. In my experience, it does not take substantially longer to run the > testsuite on a modern Linux system with this option enabled. > > t/t3419-rebase-patch-id.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh > index 94552669ae..824d84f9ce 100755 > --- a/t/t3419-rebase-patch-id.sh > +++ b/t/t3419-rebase-patch-id.sh > @@ -91,7 +91,7 @@ do_tests () { > git commit -q -m squashed && > git checkout -q other^{} && > test_must_fail git rebase squashed && > - rm -rf .git/rebase-apply > + rm -rf .git/rebase-merge .git/rebase-apply > ' > } Good catch, thanks. Perhaps we just want to invoke 'git rebase --quit' and let it clean up instead of manually doing so ourselves, since it may buy us some future-proofing in case we ever want to move the place we store rebase state?
On 2020-03-20 at 21:44:41, Elijah Newren wrote: > Good catch, thanks. Perhaps we just want to invoke 'git rebase > --quit' and let it clean up instead of manually doing so ourselves, > since it may buy us some future-proofing in case we ever want to move > the place we store rebase state? Sure, I can do that. I think there are other people (e.g., zsh) relying on these locations for the state of the various rebase versions, but nevertheless it seems like a good idea to use our existing cleanup tools. I'll send a v2 shortly.
Elijah Newren <newren@gmail.com> writes: >> - rm -rf .git/rebase-apply >> + rm -rf .git/rebase-merge .git/rebase-apply >> ' >> } > > Good catch, thanks. Perhaps we just want to invoke 'git rebase > --quit' and let it clean up instead of manually doing so ourselves, > since it may buy us some future-proofing in case we ever want to move > the place we store rebase state? I started writing "I would agree 100% if this script were not about testing 'rebase', but using 'rebase' itself for a framework to test 'rebase'???" but then if 'rebase --quit' fails to clear these, what we are likely to see is that the next test to fail, so it probably is an OK approach. Thanks.
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh index 94552669ae..824d84f9ce 100755 --- a/t/t3419-rebase-patch-id.sh +++ b/t/t3419-rebase-patch-id.sh @@ -91,7 +91,7 @@ do_tests () { git commit -q -m squashed && git checkout -q other^{} && test_must_fail git rebase squashed && - rm -rf .git/rebase-apply + rm -rf .git/rebase-merge .git/rebase-apply ' }
This test runs a function which itself runs several assertions. The last of these assertions cleans up the .git/rebase-apply directory, since when run with EXPENSIVE set, the function is invoked a second time to run the same tests with a larger data set. However, as of 2ac0d6273f ("rebase: change the default backend from "am" to "merge"", 2020-02-15), the default backend of rebase has changed, and cleaning up the rebase-apply directory has no effect: it no longer exists, since we're using rebase-merge instead. Since we don't really care which rebase backend is in use, let's just clean up both, which means we'll do the right thing in every case. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- I noticed this when I was working on another series and running the testsuite with GIT_TEST_LONG=1. We'll probably want to add this before the release if possible. It may also be desirable to have at least once CI run that runs this way. In my experience, it does not take substantially longer to run the testsuite on a modern Linux system with this option enabled. t/t3419-rebase-patch-id.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)