{"thread":{"id":"35146","subject":"[PATCH] t3600: fix broken \"choking git rm\" test","startedAt":"2013-10-16T00:01:27Z","lastAt":"2013-10-16T00:38:46Z","messageCount":3,"participants":["SZEDER Gábor","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"229049","messageId":"1381881687-11179-1-git-send-email-szeder@ira.uka.de","threadId":"35146","inReplyTo":null,"subject":"[PATCH] t3600: fix broken \"choking git rm\" test","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-10-16T00:01:27Z","receivedAt":"2013-10-16T00:01:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'choking \"git rm\" should not let it die with cruft' is\nsupposed to check 'git rm's behavior when interrupted by provoking a\nSIGPIPE while 'git rm' is busily deleting files from a specially\ncrafted index.\n\nThis test is silently broken for the following reasons:\n\n- The test crafts a special index by feeding a large number of index\n  entries with null shas to 'git update-index --index-info'.  It was\n  OK back then when this test was introduced in commit 0693f9ddad\n  (Make sure lockfiles are unlocked when dying on SIGPIPE,\n  2008-12-18), but since commit 4337b5856f (do not write null sha1s to\n  on-disk index, 2012-07-28) null shas are not allowed in the on-disk\n  index causing 'git update-index' to error out.\n\n- The barfing 'git update-index --index-info' should fail the test,\n  but it remains unnoticed because of the severely broken && chain:\n  the test's result depends solely on whether there is a stale lock\n  file left behind, but after 'git update-index' errors out 'git rm'\n  won't be executed at all.\n\nTo fix this test feed only non-null shas to 'git update-index' and\nrestore the && chain (partly by adding a missing && and by using the\ntest_when_finished helper instead of manual cleanup).\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\nA particularly funny one from the fallout of gmane/236183\n\n t/t3600-rm.sh | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 85854f338f..4dd0130dd9 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -240,18 +240,15 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \tgit reset -q --hard &&\n+\ttest_when_finished \"rm -f .git/index.lock ; git reset -q --hard\" &&\n \ti=0 &&\n \twhile test $i -lt 12000\n \tdo\n-\t    echo \"100644 $_z40 0\tsome-file-$i\"\n+\t    echo \"100644 1234567890123456789012345678901234567890 0\tsome-file-$i\"\n \t    i=$(( $i + 1 ))\n \tdone | git update-index --index-info &&\n-\tgit rm -n \"some-file-*\" | :;\n-\ttest -f .git/index.lock\n-\tstatus=$?\n-\trm -f .git/index.lock\n-\tgit reset -q --hard\n-\ttest \"$status\" != 0\n+\tgit rm -n \"some-file-*\" | : &&\n+\ttest ! -f .git/index.lock\n '\n \n test_expect_success 'rm removes subdirectories recursively' '\n-- \n1.8.4.1.495.gd8d272e\n"},{"id":"229050","messageId":"20131016001804.GJ9464@google.com","threadId":"35146","inReplyTo":"1381881687-11179-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] t3600: fix broken \"choking git rm\" test","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-16T00:18:04Z","receivedAt":"2013-10-16T00:18:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> The test 'choking \"git rm\" should not let it die with cruft' is\n> supposed to check 'git rm's behavior when interrupted by provoking a\n> SIGPIPE while 'git rm' is busily deleting files from a specially\n> crafted index.\n>\n> This test is silently broken for the following reasons:\n[...]\n> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>\n> ---\n> A particularly funny one from the fallout of gmane/236183\n\nFun. :)  Makes sense.\n\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -240,18 +240,15 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n>  \n>  test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n>  \tgit reset -q --hard &&\n> +\ttest_when_finished \"rm -f .git/index.lock ; git reset -q --hard\" &&\n\nI'd use \"&&\" here --- the test_cleanup checks the exit status from\nthis scriptlet, so it's a good habit.\n\n[...]\n> -\ttest -f .git/index.lock\n> -\tstatus=$?\n> -\trm -f .git/index.lock\n> -\tgit reset -q --hard\n> -\ttest \"$status\" != 0\n> +\ttest ! -f .git/index.lock\n\nGah.  Thanks for cleaning it up.\n\nMaybe test_path_is_missing would make sense here?  (It would notice a\n.git/index.lock directory, which is not very likely :), but more\nimportantly, it says why it is failing the test when it fails.)\n\nWith or without the changes mentioned above,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\ndiff --git i/t/t3600-rm.sh w/t/t3600-rm.sh\nindex 8386b54..540c49b 100755\n--- i/t/t3600-rm.sh\n+++ w/t/t3600-rm.sh\n@@ -240,7 +240,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n \n test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \tgit reset -q --hard &&\n-\ttest_when_finished \"rm -f .git/index.lock ; git reset -q --hard\" &&\n+\ttest_when_finished \"rm -f .git/index.lock && git reset -q --hard\" &&\n \ti=0 &&\n \twhile test $i -lt 12000\n \tdo\n@@ -248,7 +248,7 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \t    i=$(( $i + 1 ))\n \tdone | git update-index --index-info &&\n \tgit rm -n \"some-file-*\" | : &&\n-\ttest ! -f .git/index.lock\n+\ttest_path_is_missing .git/index.lock\n '\n \n test_expect_success 'rm removes subdirectories recursively' '\n"},{"id":"229051","messageId":"20131016003846.GU19704@goldbirke","threadId":"35146","inReplyTo":"20131016001804.GJ9464@google.com","subject":"Re: [PATCH] t3600: fix broken \"choking git rm\" test","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-10-16T00:38:46Z","receivedAt":"2013-10-16T00:38:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Oct 15, 2013 at 05:18:04PM -0700, Jonathan Nieder wrote:\n> SZEDER Gábor wrote:\n> > --- a/t/t3600-rm.sh\n> > +++ b/t/t3600-rm.sh\n> > @@ -240,18 +240,15 @@ test_expect_success 'refresh index before checking if it is up-to-date' '\n> >  \n> >  test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n> >  \tgit reset -q --hard &&\n> > +\ttest_when_finished \"rm -f .git/index.lock ; git reset -q --hard\" &&\n> \n> I'd use \"&&\" here --- the test_cleanup checks the exit status from\n> this scriptlet, so it's a good habit.\n\nOK.  My motivation for the ';' was that we should make sure that both\nsteps of the cleanup are executed.  However, now thinking about it if\nregular 'rm -f' can't remove the lock file then all bets are off\nanyway.\n\n> [...]\n> > -\ttest -f .git/index.lock\n> > -\tstatus=$?\n> > -\trm -f .git/index.lock\n> > -\tgit reset -q --hard\n> > -\ttest \"$status\" != 0\n> > +\ttest ! -f .git/index.lock\n> \n> Gah.  Thanks for cleaning it up.\n> \n> Maybe test_path_is_missing would make sense here?  (It would notice a\n> .git/index.lock directory, which is not very likely :), but more\n> importantly, it says why it is failing the test when it fails.)\n\nI was not aware of the test_path_is_missing helper.  I don't think it\nmatters whether it's a file or a directory, because a stale\n.git/index.lock directory would be just as bad.\n"}]}