{"thread":{"id":"53041","subject":"[PATCH] t3419: prevent failure when run with EXPENSIVE","startedAt":"2020-03-20T21:39:27Z","lastAt":"2020-03-22T19:19:52Z","messageCount":8,"participants":["brian m. carlson","Elijah Newren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393557","messageId":"20200320213916.604658-1-sandals@crustytoothpaste.net","threadId":"53041","inReplyTo":null,"subject":"[PATCH] t3419: prevent failure when run with EXPENSIVE","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-03-20T21:39:16Z","receivedAt":"2020-03-20T21:39:27Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This test runs a function which itself runs several assertions.  The\nlast of these assertions cleans up the .git/rebase-apply directory,\nsince when run with EXPENSIVE set, the function is invoked a second time\nto run the same tests with a larger data set.\n\nHowever, as of 2ac0d6273f (\"rebase: change the default backend from \"am\"\nto \"merge\"\", 2020-02-15), the default backend of rebase has changed, and\ncleaning up the rebase-apply directory has no effect: it no longer\nexists, since we're using rebase-merge instead.\n\nSince we don't really care which rebase backend is in use, let's just\nclean up both, which means we'll do the right thing in every case.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nI noticed this when I was working on another series and running the\ntestsuite with GIT_TEST_LONG=1.  We'll probably want to add this before\nthe release if possible.\n\nIt may also be desirable to have at least once CI run that runs this\nway.  In my experience, it does not take substantially longer to run the\ntestsuite on a modern Linux system with this option enabled.\n\n t/t3419-rebase-patch-id.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 94552669ae..824d84f9ce 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -91,7 +91,7 @@ do_tests () {\n \t\tgit commit -q -m squashed &&\n \t\tgit checkout -q other^{} &&\n \t\ttest_must_fail git rebase squashed &&\n-\t\trm -rf .git/rebase-apply\n+\t\trm -rf .git/rebase-merge .git/rebase-apply\n \t'\n }\n \n"},{"id":"393558","messageId":"CABPp-BGiP7eG7Ojt8FKV5PUBFk74pkO6jnOwJEZbDzBjdJUm5w@mail.gmail.com","threadId":"53041","inReplyTo":"20200320213916.604658-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] t3419: prevent failure when run with EXPENSIVE","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-20T21:44:41Z","receivedAt":"2020-03-20T21:44:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Mar 20, 2020 at 2:39 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> This test runs a function which itself runs several assertions.  The\n> last of these assertions cleans up the .git/rebase-apply directory,\n> since when run with EXPENSIVE set, the function is invoked a second time\n> to run the same tests with a larger data set.\n>\n> However, as of 2ac0d6273f (\"rebase: change the default backend from \"am\"\n> to \"merge\"\", 2020-02-15), the default backend of rebase has changed, and\n> cleaning up the rebase-apply directory has no effect: it no longer\n> exists, since we're using rebase-merge instead.\n>\n> Since we don't really care which rebase backend is in use, let's just\n> clean up both, which means we'll do the right thing in every case.\n>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n> I noticed this when I was working on another series and running the\n> testsuite with GIT_TEST_LONG=1.  We'll probably want to add this before\n> the release if possible.\n>\n> It may also be desirable to have at least once CI run that runs this\n> way.  In my experience, it does not take substantially longer to run the\n> testsuite on a modern Linux system with this option enabled.\n>\n>  t/t3419-rebase-patch-id.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\n> index 94552669ae..824d84f9ce 100755\n> --- a/t/t3419-rebase-patch-id.sh\n> +++ b/t/t3419-rebase-patch-id.sh\n> @@ -91,7 +91,7 @@ do_tests () {\n>                 git commit -q -m squashed &&\n>                 git checkout -q other^{} &&\n>                 test_must_fail git rebase squashed &&\n> -               rm -rf .git/rebase-apply\n> +               rm -rf .git/rebase-merge .git/rebase-apply\n>         '\n>  }\n\nGood catch, thanks.  Perhaps we just want to invoke 'git rebase\n--quit' and let it clean up instead of manually doing so ourselves,\nsince it may buy us some future-proofing in case we ever want to move\nthe place we store rebase state?\n"},{"id":"393559","messageId":"20200320214947.GH366567@camp.crustytoothpaste.net","threadId":"53041","inReplyTo":"CABPp-BGiP7eG7Ojt8FKV5PUBFk74pkO6jnOwJEZbDzBjdJUm5w@mail.gmail.com","subject":"Re: [PATCH] t3419: prevent failure when run with EXPENSIVE","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-03-20T21:49:47Z","receivedAt":"2020-03-20T21:49:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-03-20 at 21:44:41, Elijah Newren wrote:\n> Good catch, thanks.  Perhaps we just want to invoke 'git rebase\n> --quit' and let it clean up instead of manually doing so ourselves,\n> since it may buy us some future-proofing in case we ever want to move\n> the place we store rebase state?\n\nSure, I can do that.  I think there are other people (e.g., zsh) relying\non these locations for the state of the various rebase versions, but\nnevertheless it seems like a good idea to use our existing cleanup\ntools.\n\nI'll send a v2 shortly.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"393560","messageId":"20200320215241.609049-1-sandals@crustytoothpaste.net","threadId":"53041","inReplyTo":"20200320213916.604658-1-sandals@crustytoothpaste.net","subject":"[PATCH v2] t3419: prevent failure when run with EXPENSIVE","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-03-20T21:52:41Z","receivedAt":"2020-03-20T21:52:51Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This test runs a function which itself runs several assertions.  The\nlast of these assertions cleans up the .git/rebase-apply directory,\nsince when run with EXPENSIVE set, the function is invoked a second time\nto run the same tests with a larger data set.\n\nHowever, as of 2ac0d6273f (\"rebase: change the default backend from \"am\"\nto \"merge\"\", 2020-02-15), the default backend of rebase has changed, and\ncleaning up the rebase-apply directory has no effect: it no longer\nexists, since we're using rebase-merge instead.\n\nSince we don't really care which rebase backend is in use, let's just\nuse the command \"git rebase --quit\", which will do the right thing\nregardless.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n t/t3419-rebase-patch-id.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 94552669ae..d934583776 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -91,7 +91,7 @@ do_tests () {\n \t\tgit commit -q -m squashed &&\n \t\tgit checkout -q other^{} &&\n \t\ttest_must_fail git rebase squashed &&\n-\t\trm -rf .git/rebase-apply\n+\t\tgit rebase --quit\n \t'\n }\n \n"},{"id":"393563","messageId":"xmqqeetmbr6z.fsf@gitster.c.googlers.com","threadId":"53041","inReplyTo":"CABPp-BGiP7eG7Ojt8FKV5PUBFk74pkO6jnOwJEZbDzBjdJUm5w@mail.gmail.com","subject":"Re: [PATCH] t3419: prevent failure when run with EXPENSIVE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-20T22:24:20Z","receivedAt":"2020-03-20T22:24:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> -               rm -rf .git/rebase-apply\n>> +               rm -rf .git/rebase-merge .git/rebase-apply\n>>         '\n>>  }\n>\n> Good catch, thanks.  Perhaps we just want to invoke 'git rebase\n> --quit' and let it clean up instead of manually doing so ourselves,\n> since it may buy us some future-proofing in case we ever want to move\n> the place we store rebase state?\n\nI started writing \"I would agree 100% if this script were not about\ntesting 'rebase', but using 'rebase' itself for a framework to test\n'rebase'???\" but then if 'rebase --quit' fails to clear these, what\nwe are likely to see is that the next test to fail, so it probably\nis an OK approach.\n\nThanks.\n"},{"id":"393693","messageId":"20200322075140.GA600888@coredump.intra.peff.net","threadId":"53041","inReplyTo":"20200320213916.604658-1-sandals@crustytoothpaste.net","subject":"[PATCH] t3419: drop EXPENSIVE tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-22T07:51:40Z","receivedAt":"2020-03-22T07:51:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2020 at 09:39:16PM +0000, brian m. carlson wrote:\n\n> I noticed this when I was working on another series and running the\n> testsuite with GIT_TEST_LONG=1.  We'll probably want to add this before\n> the release if possible.\n> \n> It may also be desirable to have at least once CI run that runs this\n> way.  In my experience, it does not take substantially longer to run the\n> testsuite on a modern Linux system with this option enabled.\n\nI do agree that anything marked with EXPENSIVE is practically useless if\nnobody is running it. In many cases these can be made much less\nexpensive by being smarter about setup (e.g., using fast-import or\ntest_commit_bulk to create commits rather than looping and creating many\nprocesses).\n\nThis one in particular seems to add about 500ms, which isn't too bad.\nBut I actually don't think it's doing anything useful. See below.\n\n-- >8 --\nSubject: [PATCH] t3419: drop EXPENSIVE tests\n\nWhen t3419 was originally written, it was designed to run a smaller test\nfor correctness, and then the same test with a larger number of patches\nfor performance. But it seems unlikely the latter was helping us:\n\n - it was marked with EXPENSIVE, so hardly anybody ran it anyway\n\n - there's no indication that it was more likely to find bugs than the\n   smaller case (the commit message isn't very helpful, but the original\n   cover letter describes it as: \"The first patch adds correctness and\n   (optional) performance tests\".\n\n - the timing results are shown only via test_debug(). So also not run\n   unless the user says \"-d\", and then not provided in any\n   machine-readable form.\n\nIf we're interested in performance regressions, a script in t/perf would\nbe more appropriate. I didn't add one here, because it's not at all\nclear to me that what the script is timing is even all that interesting.\n\nLet's simplify the script by dropping the EXPENSIVE run. That in turn\nlets us drop the do_tests() wrapper, which lets us consistently use\nsingle-quotes for our test snippets. And we can drop the useless\ntest_debug() timings, as well as their run() helper. And finally, while\nwe're here, we can replace the count() helper with the standard\ntest_seq().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nPrepared on top of en/rebase-backend, since it textually conflicts\notherwise. Best viewed with \"diff -w\" because of the re-indenting.\n\n t/t3419-rebase-patch-id.sh | 112 ++++++++++++++-----------------------\n 1 file changed, 41 insertions(+), 71 deletions(-)\n\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex e71560e614..3dc754b13a 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -4,15 +4,6 @@ test_description='git rebase - test patch id computation'\n \n . ./test-lib.sh\n \n-count () {\n-\ti=0\n-\twhile test $i -lt $1\n-\tdo\n-\t\techo \"$i\"\n-\t\ti=$(($i+1))\n-\tdone\n-}\n-\n scramble () {\n \ti=0\n \twhile read x\n@@ -26,75 +17,54 @@ scramble () {\n \tmv -f \"$1.new\" \"$1\"\n }\n \n-run () {\n-\techo \\$ \"$@\"\n-\t/usr/bin/time \"$@\" >/dev/null\n-}\n-\n test_expect_success 'setup' '\n \tgit commit --allow-empty -m initial &&\n \tgit tag root\n '\n \n-do_tests () {\n-\tnlines=$1 pr=${2-}\n-\n-\ttest_expect_success $pr \"setup: $nlines lines\" \"\n-\t\trm -f .gitattributes &&\n-\t\tgit checkout -q -f master &&\n-\t\tgit reset --hard root &&\n-\t\tcount $nlines >file &&\n-\t\tgit add file &&\n-\t\tgit commit -q -m initial &&\n-\t\tgit branch -f other &&\n-\n-\t\tscramble file &&\n-\t\tgit add file &&\n-\t\tgit commit -q -m 'change big file' &&\n-\n-\t\tgit checkout -q other &&\n-\t\t: >newfile &&\n-\t\tgit add newfile &&\n-\t\tgit commit -q -m 'add small file' &&\n-\n-\t\tgit cherry-pick master >/dev/null 2>&1\n-\t\"\n-\n-\ttest_debug \"\n-\t\trun git diff master^\\!\n-\t\"\n-\n-\ttest_expect_success $pr 'setup attributes' \"\n-\t\techo 'file binary' >.gitattributes\n-\t\"\n-\n-\ttest_debug \"\n-\t\trun git format-patch --stdout master &&\n-\t\trun git format-patch --stdout --ignore-if-in-upstream master\n-\t\"\n+test_expect_success 'setup: 500 lines' '\n+\trm -f .gitattributes &&\n+\tgit checkout -q -f master &&\n+\tgit reset --hard root &&\n+\ttest_seq 500 >file &&\n+\tgit add file &&\n+\tgit commit -q -m initial &&\n+\tgit branch -f other &&\n+\n+\tscramble file &&\n+\tgit add file &&\n+\tgit commit -q -m \"change big file\" &&\n+\n+\tgit checkout -q other &&\n+\t: >newfile &&\n+\tgit add newfile &&\n+\tgit commit -q -m \"add small file\" &&\n+\n+\tgit cherry-pick master >/dev/null 2>&1\n+'\n \n-\ttest_expect_success $pr 'detect upstream patch' '\n-\t\tgit checkout -q master &&\n-\t\tscramble file &&\n-\t\tgit add file &&\n-\t\tgit commit -q -m \"change big file again\" &&\n-\t\tgit checkout -q other^{} &&\n-\t\tgit rebase master &&\n-\t\ttest_must_fail test -n \"$(git rev-list master...HEAD~)\"\n-\t'\n+test_expect_success 'setup attributes' '\n+\techo \"file binary\" >.gitattributes\n+'\n \n-\ttest_expect_success $pr 'do not drop patch' '\n-\t\tgit branch -f squashed master &&\n-\t\tgit checkout -q -f squashed &&\n-\t\tgit reset -q --soft HEAD~2 &&\n-\t\tgit commit -q -m squashed &&\n-\t\tgit checkout -q other^{} &&\n-\t\ttest_must_fail git rebase squashed &&\n-\t\tgit rebase --quit\n-\t'\n-}\n+test_expect_success 'detect upstream patch' '\n+\tgit checkout -q master &&\n+\tscramble file &&\n+\tgit add file &&\n+\tgit commit -q -m \"change big file again\" &&\n+\tgit checkout -q other^{} &&\n+\tgit rebase master &&\n+\ttest_must_fail test -n \"$(git rev-list master...HEAD~)\"\n+'\n \n-do_tests 500\n-do_tests 50000 EXPENSIVE\n+test_expect_success 'do not drop patch' '\n+\tgit branch -f squashed master &&\n+\tgit checkout -q -f squashed &&\n+\tgit reset -q --soft HEAD~2 &&\n+\tgit commit -q -m squashed &&\n+\tgit checkout -q other^{} &&\n+\ttest_must_fail git rebase squashed &&\n+\tgit rebase --quit\n+'\n \n test_done\n-- \n2.26.0.rc2.641.gac63ba406a\n\n"},{"id":"393726","messageId":"20200322171111.GB6499@camp.crustytoothpaste.net","threadId":"53041","inReplyTo":"20200322075140.GA600888@coredump.intra.peff.net","subject":"Re: [PATCH] t3419: drop EXPENSIVE tests","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-03-22T17:11:11Z","receivedAt":"2020-03-22T17:11:20Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-03-22 at 07:51:40, Jeff King wrote:\n> When t3419 was originally written, it was designed to run a smaller test\n> for correctness, and then the same test with a larger number of patches\n> for performance. But it seems unlikely the latter was helping us:\n> \n>  - it was marked with EXPENSIVE, so hardly anybody ran it anyway\n> \n>  - there's no indication that it was more likely to find bugs than the\n>    smaller case (the commit message isn't very helpful, but the original\n>    cover letter describes it as: \"The first patch adds correctness and\n>    (optional) performance tests\".\n> \n>  - the timing results are shown only via test_debug(). So also not run\n>    unless the user says \"-d\", and then not provided in any\n>    machine-readable form.\n> \n> If we're interested in performance regressions, a script in t/perf would\n> be more appropriate. I didn't add one here, because it's not at all\n> clear to me that what the script is timing is even all that interesting.\n> \n> Let's simplify the script by dropping the EXPENSIVE run. That in turn\n> lets us drop the do_tests() wrapper, which lets us consistently use\n> single-quotes for our test snippets. And we can drop the useless\n> test_debug() timings, as well as their run() helper. And finally, while\n> we're here, we can replace the count() helper with the standard\n> test_seq().\n\nI'm also fine with this solution.  As long as this test doesn't fail\nwith EXPENSIVE, I'm happy.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"393736","messageId":"20200322191950.GB671552@coredump.intra.peff.net","threadId":"53041","inReplyTo":"20200322171111.GB6499@camp.crustytoothpaste.net","subject":"Re: [PATCH] t3419: drop EXPENSIVE tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-22T19:19:50Z","receivedAt":"2020-03-22T19:19:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2020 at 05:11:11PM +0000, brian m. carlson wrote:\n\n> > Let's simplify the script by dropping the EXPENSIVE run. That in turn\n> > lets us drop the do_tests() wrapper, which lets us consistently use\n> > single-quotes for our test snippets. And we can drop the useless\n> > test_debug() timings, as well as their run() helper. And finally, while\n> > we're here, we can replace the count() helper with the standard\n> > test_seq().\n> \n> I'm also fine with this solution.  As long as this test doesn't fail\n> with EXPENSIVE, I'm happy.\n\nJust to be clear, this is meant to go on top of your fix, which is\nalready in next, so I think that part is fixed either way. :)\n\n-Peff\n"}]}