{"thread":{"id":"48717","subject":"[PATCH] tests: clean after SANITY tests","startedAt":"2018-06-15T18:30:53Z","lastAt":"2018-06-15T19:57:28Z","messageCount":2,"participants":["Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"350286","messageId":"xmqqa7rv297r.fsf@gitster-ct.c.googlers.com","threadId":"48717","inReplyTo":null,"subject":"[PATCH] tests: clean after SANITY tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-15T18:30:48Z","receivedAt":"2018-06-15T18:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some of our tests try to make sure Git behaves sensibly in a\nread-only directory, by dropping 'w' permission bit before doing a\ntest and then restoring it after it is done. The latter is needed\nfor the test framework to clean after itself without leaving a\nleftover directory that cannot be removed.\n\nAncient parts of tests however arrange the above with\n\n\tchmod a-w . &&\n\t... do the test ...\n\tstatus=$?\n\tchmod 775 .\n\t(exit $status)\n\nwhich obviously would not work if the test somehow dies before it\nhas the chance to do \"chmod 775\". Rewrite them by following a more\nrobust pattern recently written tests use, which is\n\n\ttest_when_finished \"chmod 775 .\" &&\n\tchmod a-w . &&\n\t... do the test ...\n\nSigned-off-by: Junio C Hamano \n---\n\n * I happened to hit ^C at the right moment and saw \"make distclean\"\n fail miserably. I guess you can still kill the process in such a\n way that when-finished hander would not fire, so in that sense\n this is more about consistency and uniformity than the actual\n safety, but anyway...\n\n t/t0001-init.sh | 1 +\n t/t0070-fundamental.sh | 2 +-\n t/t1004-read-tree-m-u-wf.sh | 6 ++----\n t/t5537-fetch-shallow.sh | 2 +-\n t/t7508-status.sh | 4 +---\n 5 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex c413bff9cf..4c865051e7 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' '\n '\n \n test_expect_success POSIXPERM,SANITY 'init notices EPERM' '\n+\ttest_when_finished \"chmod +w newdir\" &&\n \trm -fr newdir &&\n \tmkdir newdir &&\n \tchmod -w newdir &&\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 991ed2a48d..60e3de7b7b 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -19,8 +19,8 @@ test_expect_success 'mktemp to nonexistent directory prints filename' '\n \n test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints filename' '\n \tmkdir cannotwrite &&\n-\tchmod -w cannotwrite &&\n \ttest_when_finished \"chmod +w cannotwrite\" &&\n+\tchmod -w cannotwrite &&\n \ttest_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&\n \tgrep \"cannotwrite/test\" err\n '\ndiff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh\nindex c7ce5d8bb5..826cd32e23 100755\n--- a/t/t1004-read-tree-m-u-wf.sh\n+++ b/t/t1004-read-tree-m-u-wf.sh\n@@ -179,6 +179,8 @@ test_expect_success 'funny symlink in work tree' '\n \n test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' '\n \n+\ttest_when_finished \"chmod a+w a 2>/dev/null; rm -fr a b\" &&\n+\n \trm -fr a b &&\n \tgit reset --hard &&\n \n@@ -188,10 +190,6 @@ test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' '\n \n '\n \n-# clean-up from the above test\n-chmod a+w a 2>/dev/null\n-rm -fr a b\n-\n test_expect_success 'D/F setup' '\n \n \tgit reset --hard &&\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex df8d2f095a..943231af96 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -175,8 +175,8 @@ EOF\n \n test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n \tcp -R .git read-only.git &&\n-\tfind read-only.git -print | xargs chmod -w &&\n \ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n+\tfind read-only.git -print | xargs chmod -w &&\n \tgit clone --no-local --depth=2 read-only.git from-read-only &&\n \tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n \tcat >expect < bar && git add bar && git commit -q -m 'Add bar') && git add sm\n-- \n2.18.0-rc2\n\n"},{"id":"350296","messageId":"CAPig+cQQEKjM43OqX9bT2=_yFjkiUMqjbZag5TWpsh6d_Hwn_g@mail.gmail.com","threadId":"48717","inReplyTo":"xmqqa7rv297r.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] tests: clean after SANITY tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-15T19:57:13Z","receivedAt":"2018-06-15T19:57:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 15, 2018 at 2:30 PM Junio C Hamano wrote:\n> Some of our tests try to make sure Git behaves sensibly in a\n> read-only directory, by dropping 'w' permission bit before doing a\n> test and then restoring it after it is done. The latter is needed\n> for the test framework to clean after itself without leaving a\n> leftover directory that cannot be removed.\n> [...]\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> @@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' '\n> test_expect_success POSIXPERM,SANITY 'init notices EPERM' '\n> + test_when_finished \"chmod +w newdir\" &&\n> rm -fr newdir &&\n> mkdir newdir &&\n> chmod -w newdir &&\n\nWhen reading this, I was wondering if there was a \"rm -fr newdir\" at\nthe end of the test which could be removed now that it uses\ntest_when_finished(). Checking beyond the shown context, I see that it\ndoesn't bother with removal since the next test removes the directory\nas a preparatory step. Even before the addition of\ntest_when_finished() to restore write permission, the subsequent\ntest's removal of the directory worked because this is test is only\nrun when POSIXPERM is met, and POSIX allows for such an operation.\nOkay, looks good.\n"}]}