{"thread":{"id":"23701","subject":"1.7.2 cycle will open soon","startedAt":"2010-05-05T05:39:59Z","lastAt":"2010-05-06T20:20:24Z","messageCount":11,"participants":["Junio C Hamano","Jeff King","Jonathan Nieder","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"140965","messageId":"7vaaselxe8.fsf@alter.siamese.dyndns.org","threadId":"23701","inReplyTo":null,"subject":"1.7.2 cycle will open soon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-05T05:39:59Z","receivedAt":"2010-05-05T05:39:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The usual post-release ritual.\n\n - The tip of 'next' has been rebuilt on top of the release, with topics\n   that have been cooking;\n\n - The 'maint' branch is now for 1.7.1.X series; and\n\n - What have been cooking in 'next' during the pre-release freeze period\n   for 1.7.1 should start graduating to 'master' soon.\n\nIt appears that the tip of 'maint' and 'next' (but curiously not 'master')\ndo not pass test t1504 on at least FreeBSD right now, but I don't have\nenough energy to hunt things down tonight.\n"},{"id":"141029","messageId":"20100506055236.GA16151@coredump.intra.peff.net","threadId":"23701","inReplyTo":"7vaaselxe8.fsf@alter.siamese.dyndns.org","subject":"Re: 1.7.2 cycle will open soon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-06T05:52:36Z","receivedAt":"2010-05-06T05:52:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 04, 2010 at 10:39:59PM -0700, Junio C Hamano wrote:\n\n> It appears that the tip of 'maint' and 'next' (but curiously not 'master')\n> do not pass test t1504 on at least FreeBSD right now, but I don't have\n> enough energy to hunt things down tonight.\n\nThe culprit is Jonathan's 3bf7886 (test-lib: Let tests specify commands\nto be run at end of test, 2010-05-02). FreeBSD's /bin/sh doesn't\npropagate $? over an eval:\n\n  $ /bin/sh\n  $ false; echo $?\n  1\n  $ false; eval 'echo $?'\n  0\n  $ eval 'false; echo $?' ;# but it still works inside, of course\n  1\n\nLooking at that patch, I don't see any reason that eval_ret needs to be\nset inside the eval. If we have multiple test_when_finished calls, we\nkeep setting and propagating eval_ret, which doesn't make much sense to\nme.  Why not just:\n\n  test_run_ () {\n    test_cleanup=\n    eval >&3 2>&4 \"$1\"\n    eval_ret=$?\n    eval >&3 2>&4 \"$test_cleanup\"\n    return 0\n  }\n\n  test_when_finished () {\n    test_cleanup=\"$*; $test_cleanup\"\n  }\n\nAm I missing something?\n\n-Peff\n"},{"id":"141036","messageId":"20100506064428.GA29360@progeny.tock","threadId":"23701","inReplyTo":"20100506055236.GA16151@coredump.intra.peff.net","subject":"Re: 1.7.2 cycle will open soon","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-06T06:44:28Z","receivedAt":"2010-05-06T06:44:28Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> FreeBSD's /bin/sh doesn't propagate $? over an eval\n\nThanks for tracking it down.\n\n> Looking at that patch, I don't see any reason that eval_ret needs to be\n> set inside the eval. If we have multiple test_when_finished calls, we\n> keep setting and propagating eval_ret, which doesn't make much sense to\n> me.  Why not just:\n> \n>   test_run_ () {\n>     test_cleanup=\n>     eval >&3 2>&4 \"$1\"\n>     eval_ret=$?\n>     eval >&3 2>&4 \"$test_cleanup\"\n>     return 0\n>   }\n> \n>   test_when_finished () {\n>     test_cleanup=\"$*; $test_cleanup\"\n>   }\n> \n> Am I missing something?\n\nIf cleanup fails, I want to catch it.  Would something like this do?\n\n test_run_ () {\n\ttest_cleanup=:\n\teval >&3 2>&4 \"$1\"\n\teval_ret=$?\n\teval >&3 2>&4 \"$test_cleanup\" && (exit \"$eval_ret\")\n\teval_ret=$?\n\treturn 0\n }\n\n test_when_finished () {\n\ttest_cleanup=\"$* && $test_cleanup\"\n }\n\nTo permit a line break at the end of a cleanup command, one can do\n\n test_when_finished () {\n\ttest_cleanup=\"{ $*\n\t\t} && $test_cleanup\"\n }\n\nbut that might not be worth the ugliness.\n\nJonathan\n"},{"id":"141038","messageId":"20100506065419.GA21009@coredump.intra.peff.net","threadId":"23701","inReplyTo":"20100506064428.GA29360@progeny.tock","subject":"Re: 1.7.2 cycle will open soon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-06T06:54:19Z","receivedAt":"2010-05-06T06:54:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 06, 2010 at 01:44:28AM -0500, Jonathan Nieder wrote:\n\n> > Am I missing something?\n> \n> If cleanup fails, I want to catch it.  Would something like this do?\n\nAh, I see.\n\n>  test_run_ () {\n> \ttest_cleanup=:\n> \teval >&3 2>&4 \"$1\"\n> \teval_ret=$?\n> \teval >&3 2>&4 \"$test_cleanup\" && (exit \"$eval_ret\")\n> \teval_ret=$?\n> \treturn 0\n>  }\n> \n>  test_when_finished () {\n> \ttest_cleanup=\"$* && $test_cleanup\"\n>  }\n\nDoesn't that violate your rule that the cleanup will _always_ be run?\nHere, if the first cleanup fails, we don't run subsequent ones.\n\nPerhaps the simplest would be to just keep a cleanup_ret in the second\neval (where you have eval_ret in the original patch), and then act\nappropriately after the eval finishes. That would be easier on the eyes,\ntoo, I think.\n\n> To permit a line break at the end of a cleanup command, one can do\n> \n>  test_when_finished () {\n> \ttest_cleanup=\"{ $*\n> \t\t} && $test_cleanup\"\n>  }\n> \n> but that might not be worth the ugliness.\n\nI doubt anyone will put a linebreak in, but it is probably better to be\ndefensive, and the ugliness is at least contained only in that function.\n\n-Peff\n"},{"id":"141039","messageId":"4BE2684F.1080004@viscovery.net","threadId":"23701","inReplyTo":"20100506055236.GA16151@coredump.intra.peff.net","subject":"Re: 1.7.2 cycle will open soon","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-05-06T06:57:19Z","receivedAt":"2010-05-06T06:57:19Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/6/2010 7:52, schrieb Jeff King:\n> Why not just:\n> \n>   test_run_ () {\n>     test_cleanup=\n>     eval >&3 2>&4 \"$1\"\n>     eval_ret=$?\n>     eval >&3 2>&4 \"$test_cleanup\"\n>     return 0\n>   }\n> \n>   test_when_finished () {\n>     test_cleanup=\"$*; $test_cleanup\"\n>   }\n> \n> Am I missing something?\n\nI ask myself the same question.\n\nFor my taste, this change went production far too fast. Did I miss any\ndiscussion on it? (I didn't speak up when it passed by on the ML because I\nwas on a short trip at that time.)\n\n-- Hannes\n"},{"id":"141041","messageId":"4BE26A83.4010708@viscovery.net","threadId":"23701","inReplyTo":"20100506064428.GA29360@progeny.tock","subject":"Re: 1.7.2 cycle will open soon","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-05-06T07:06:43Z","receivedAt":"2010-05-06T07:06:43Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/6/2010 8:44, schrieb Jonathan Nieder:\n>  test_when_finished () {\n> \ttest_cleanup=\"$* && $test_cleanup\"\n>  }\n\nI'm wondering why you want this test_cleanup at all?\n\nIs it so that subsequent tests can succeed even if an earlier test failed\nbefore its regular cleanup?\n\nI don't see what this buys you. If a test case uncovers a regression, you\ngot to fix it - who cares how many later tests fail or not? Once you are\nfinished with your change, all tests will pass anyway (including their\nregular cleanups).\n\n-- Hannes\n"},{"id":"141045","messageId":"20100506074943.GA24399@coredump.intra.peff.net","threadId":"23701","inReplyTo":"4BE26A83.4010708@viscovery.net","subject":"Re: 1.7.2 cycle will open soon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-06T07:49:43Z","receivedAt":"2010-05-06T07:49:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 06, 2010 at 09:06:43AM +0200, Johannes Sixt wrote:\n\n> Am 5/6/2010 8:44, schrieb Jonathan Nieder:\n> >  test_when_finished () {\n> > \ttest_cleanup=\"$* && $test_cleanup\"\n> >  }\n> \n> I'm wondering why you want this test_cleanup at all?\n> \n> Is it so that subsequent tests can succeed even if an earlier test failed\n> before its regular cleanup?\n> \n> I don't see what this buys you. If a test case uncovers a regression, you\n> got to fix it - who cares how many later tests fail or not? Once you are\n> finished with your change, all tests will pass anyway (including their\n> regular cleanups).\n\nI have to agree. Yes, using test_when_finished can make _some_ tests\nmore robust, but there will still be many tests whose breakage will\nbreak future tests. And many of those will never be fixed, because the\ntests simply build on one another.\n\nSo I don't think we will ever \"solve\" this problem, which means testers\nwill continue to have to fix early failures before looking at later\nones, because they don't know if the later test is a false negative or\nnot. And as you point out, it is simply not that big a deal in the first\nplace.\n\n-Peff\n"},{"id":"141046","messageId":"20100506080144.GA25876@progeny.tock","threadId":"23701","inReplyTo":"4BE26A83.4010708@viscovery.net","subject":"Re: 1.7.2 cycle will open soon","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-06T08:01:44Z","receivedAt":"2010-05-06T08:01:44Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n> Am 5/6/2010 8:44, schrieb Jonathan Nieder:\n>>  test_when_finished () {\n>> \ttest_cleanup=\"$* && $test_cleanup\"\n>>  }\n>\n> I'm wondering why you want this test_cleanup at all?\n> \n> Is it so that subsequent tests can succeed even if an earlier test failed\n> before its regular cleanup?\n\nYes.  In some cases (permissions-related), if a test fails, even a\n‘make clean’ afterwards fails.\n\n> I don't see what this buys you. If a test case uncovers a regression, you\n> got to fix it - who cares how many later tests fail or not? Once you are\n> finished with your change, all tests will pass anyway (including their\n> regular cleanups).\n\nWhy do we support the non --immediate mode at all, then?  Just like it can\nbe easier to understand the result when a compile uncovers more than one\nerror, it can help in debugging to see which later tests were broken.\n\nIf there is a consensus that this is not worth it, I am fine with\nthat, though.  The current status is that each test where this matters\ndoes things its own way.\n\nJonathan\n"},{"id":"141049","messageId":"20100506084045.GA25917@progeny.tock","threadId":"23701","inReplyTo":"20100506065419.GA21009@coredump.intra.peff.net","subject":"[PATCH] test-lib: some shells do not let $? propagate into an eval","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-06T08:41:10Z","receivedAt":"2010-05-06T08:41:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Thu, May 06, 2010 at 01:44:28AM -0500, Jonathan Nieder wrote:\n\n>>  test_when_finished () {\n>> \ttest_cleanup=\"$* && $test_cleanup\"\n>>  }\n>\n> Doesn't that violate your rule that the cleanup will _always_ be run?\n\nThanks for the sanity check.\n\n> Perhaps the simplest would be to just keep a cleanup_ret in the second\n> eval (where you have eval_ret in the original patch), and then act\n> appropriately after the eval finishes. That would be easier on the eyes,\n> too, I think.\n\nI tried the cleanup_ret idea; test_run_ ended up looking like this:\n\n\ttest_cleanup=:\n\teval >&3 2>&4 \"$1\"\n\teval_ret=$?\n\teval >&3 2>&4 \":; $test_cleanup\"\n\tcleanup_ret=?\n\t(exit \"$test_ret\") && (exit \"$cleanup_ret\")\n\teval_ret=$?\n\treturn 0\n\nThat breaks the principle of keeping the ugliness in test_when_finished.\nSo here’s the minimal fix.\n\n-- 8< --\nSubject: test-lib: some shells do not let $? propagate into an eval\n\nIn 3bf7886 (test-lib: Let tests specify commands to be run at end of\ntest, 2010-05-02), the git test harness learned to run cleanup\ncommands unconditionally at the end of a test.  During each test,\nthe intended cleanup actions are collected in the test_cleanup variable\nand evaluated.  That variable looks something like this:\n\n\teval_ret=$?; clean_something && (exit \"$eval_ret\")\n\teval_ret=$?; clean_something_else && (exit \"$eval_ret\")\n\teval_ret=$?; final_cleanup && (exit \"$eval_ret\")\n\teval_ret=$?\n\nAll cleanup actions are run unconditionally but if one of them fails\nit is properly reported through $eval_ret.\n\nOn FreeBSD, unfortunately, $? is set at the beginning of an ‘eval’\nto 0 instead of the exit status of the previous command.  This results\nin tests using test_expect_code appearing to fail and all others\nappearing to pass, unless their cleanup fails.  Avoid the problem by\nsetting eval_ret before the ‘eval’ begins.\n\nThanks to Jeff King for the explanation.\n\nCc: Jeff King <peff@peff.net>\nCc: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nI was also surprised to see this migrate to maint so quickly, but I\nwas happy to see it broke early and loudly.\n\nBecause there is some unhappiness with the feature[1], it might make\nmore sense to revert it for now.  If that discussion can be taken as\nlicense to write tests that take down other tests with them on\nfailure, then such a revert would not interfere with other work.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/146375/focus=146451\n\n t/t0000-basic.sh |   21 +++++++++++++++++++++\n t/test-lib.sh    |    7 ++++---\n 2 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex f4ca4fc..3ec9cbe 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -73,6 +73,27 @@ then\n \texit 1\n fi\n \n+clean=no\n+test_expect_success 'tests clean up after themselves' '\n+    test_when_finished clean=yes\n+'\n+\n+cleaner=no\n+test_expect_code 1 'tests clean up even after a failure' '\n+    test_when_finished cleaner=yes &&\n+    (exit 1)\n+'\n+\n+if test $clean$cleaner != yesyes\n+then\n+\tsay \"bug in test framework: cleanup commands do not work reliably\"\n+\texit 1\n+fi\n+\n+test_expect_code 2 'failure to clean up causes the test to fail' '\n+    test_when_finished \"(exit 2)\"\n+'\n+\n ################################################################\n # Basics of the basics\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex acce3d0..7422bba 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -366,8 +366,9 @@ test_debug () {\n }\n \n test_run_ () {\n-\ttest_cleanup='eval_ret=$?'\n+\ttest_cleanup=:\n \teval >&3 2>&4 \"$1\"\n+\teval_ret=$?\n \teval >&3 2>&4 \"$test_cleanup\"\n \treturn 0\n }\n@@ -567,8 +568,8 @@ test_cmp() {\n # the test to pass.\n \n test_when_finished () {\n-\ttest_cleanup=\"eval_ret=\\$?; { $*\n-\t\t} && (exit \\\"\\$eval_ret\\\"); $test_cleanup\"\n+\ttest_cleanup=\"{ $*\n+\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n }\n \n # Most tests can use the created repository, but some may need to create more.\n-- \n1.7.1\n"},{"id":"141052","messageId":"20100506085720.GA31873@coredump.intra.peff.net","threadId":"23701","inReplyTo":"20100506084045.GA25917@progeny.tock","subject":"Re: [PATCH] test-lib: some shells do not let $? propagate into an eval","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-06T08:57:20Z","receivedAt":"2010-05-06T08:57:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 06, 2010 at 03:41:10AM -0500, Jonathan Nieder wrote:\n\n> I tried the cleanup_ret idea; test_run_ ended up looking like this:\n> \n> \ttest_cleanup=:\n> \teval >&3 2>&4 \"$1\"\n> \teval_ret=$?\n> \teval >&3 2>&4 \":; $test_cleanup\"\n> \tcleanup_ret=?\n> \t(exit \"$test_ret\") && (exit \"$cleanup_ret\")\n> \teval_ret=$?\n\nMaybe it is just me, but those last two lines would be much more\nreadable as:\n\n  if test \"$eval_ret\" = 0; then\n    eval_ret=$cleanup_ret\n  fi\n\nassuming your \"$test_ret\" was supposed to be $eval_ret.\n\n> That breaks the principle of keeping the ugliness in test_when_finished.\n> So here’s the minimal fix.\n\nI don't know that we have necessarily have that principle. The most\nimportant thing is that ugliness not escape to the test scripts\nthemselves, where we have to write many thousands of (hopefully\nreadable) lines.\n\nBut...\n\n> -- 8< --\n> Subject: test-lib: some shells do not let $? propagate into an eval\n\nThis fix looks fine to me, and I tested it on FreeBSD 8.0. So:\n\n Acked-by: Jeff King <peff@peff.net>\n\n> I was also surprised to see this migrate to maint so quickly, but I\n> was happy to see it broke early and loudly.\n\nI think Junio is much more carefree with changes that only touch test\ninfrastructure, as they cannot possibly be breaking git itself for\nanybody.\n\n> Because there is some unhappiness with the feature[1], it might make\n\nTo clarify my position, I am not that negative on the feature. I just\nthink it is a bit of a fool's errand, and I don't want to personally\nspend any time on making it happen. If you think it is worth proceeding\nand you can do so without breaking anything[1], I won't object.\n\n[1] Yes, I am teasing you. But probably the complex part is now done,\nand you and others can use test_when_finished() at will now, and we can\nsee if it actually makes anybody's life better.\n\n-Peff\n"},{"id":"141082","messageId":"7vocgshjef.fsf@alter.siamese.dyndns.org","threadId":"23701","inReplyTo":"20100506085720.GA31873@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: some shells do not let $? propagate into an eval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-06T20:20:24Z","receivedAt":"2010-05-06T20:20:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think Junio is much more carefree with changes that only touch test\n> infrastructure, as they cannot possibly be breaking git itself for\n> anybody.\n\nYes, and also I deliberately queued this with a known breakage because\nI've been swamped with non-git issues (sorry), I knew it would be very\nisolated issue, and it was the easiest way to get people's attention.\n\nThanks for helping.\n"}]}