[PATCH] tests: clean after SANITY tests

[PATCH] tests: clean after SANITY tests

[Junio C Hamano]

Some of our tests try to make sure Git behaves sensibly in a
read-only directory, by dropping 'w' permission bit before doing a
test and then restoring it after it is done.  The latter is needed
for the test framework to clean after itself without leaving a
leftover directory that cannot be removed.

Ancient parts of tests however arrange the above with

	chmod a-w . &&
	... do the test ...
	status=$?
	chmod 775 .
	(exit $status)

which obviously would not work if the test somehow dies before it
has the chance to do "chmod 775".  Rewrite them by following a more
robust pattern recently written tests use, which is

	test_when_finished "chmod 775 ." &&
	chmod a-w . &&
	... do the test ...

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I happened to hit ^C at the right moment and saw "make distclean"
   fail miserably.  I guess you can still kill the process in such a
   way that when-finished hander would not fire, so in that sense
   this is more about consistency and uniformity than the actual
   safety, but anyway...

 t/t0001-init.sh             | 1 +
 t/t0070-fundamental.sh      | 2 +-
 t/t1004-read-tree-m-u-wf.sh | 6 ++----
 t/t5537-fetch-shallow.sh    | 2 +-
 t/t7508-status.sh           | 4 +---
 5 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c413bff9cf..4c865051e7 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' '
 '
 
 test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
+	test_when_finished "chmod +w newdir" &&
 	rm -fr newdir &&
 	mkdir newdir &&
 	chmod -w newdir &&
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 991ed2a48d..60e3de7b7b 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -19,8 +19,8 @@ test_expect_success 'mktemp to nonexistent directory prints filename' '
 
 test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints filename' '
 	mkdir cannotwrite &&
-	chmod -w cannotwrite &&
 	test_when_finished "chmod +w cannotwrite" &&
+	chmod -w cannotwrite &&
 	test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&
 	grep "cannotwrite/test" err
 '
diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index c7ce5d8bb5..826cd32e23 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -179,6 +179,8 @@ test_expect_success 'funny symlink in work tree' '
 
 test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' '
 
+	test_when_finished "chmod a+w a 2>/dev/null; rm -fr a b" &&
+
 	rm -fr a b &&
 	git reset --hard &&
 
@@ -188,10 +190,6 @@ test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' '
 
 '
 
-# clean-up from the above test
-chmod a+w a 2>/dev/null
-rm -fr a b
-
 test_expect_success 'D/F setup' '
 
 	git reset --hard &&
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index df8d2f095a..943231af96 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -175,8 +175,8 @@ EOF
 
 test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
 	cp -R .git read-only.git &&
-	find read-only.git -print | xargs chmod -w &&
 	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
+	find read-only.git -print | xargs chmod -w &&
 	git clone --no-local --depth=2 read-only.git from-read-only &&
 	git --git-dir=from-read-only/.git log --format=%s >actual &&
 	cat >expect <<EOF &&
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 50052e2872..10b084d890 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1099,6 +1099,7 @@ EOF
 '
 
 test_expect_success POSIXPERM,SANITY 'status succeeds in a read-only repository' '
+	test_when_finished "chmod 775 .git" &&
 	(
 		chmod a-w .git &&
 		# make dir1/tracked stat-dirty
@@ -1108,9 +1109,6 @@ test_expect_success POSIXPERM,SANITY 'status succeeds in a read-only repository'
 		# make sure "status" succeeded without writing index out
 		git diff-files | grep dir1/tracked
 	)
-	status=$?
-	chmod 775 .git
-	(exit $status)
 '
 
 (cd sm && echo > bar && git add bar && git commit -q -m 'Add bar') && git add sm
-- 
2.18.0-rc2

Re: [PATCH] tests: clean after SANITY tests

[Eric Sunshine]

On Fri, Jun 15, 2018 at 2:30 PM Junio C Hamano <gitster@pobox.com> wrote:
> Some of our tests try to make sure Git behaves sensibly in a
> read-only directory, by dropping 'w' permission bit before doing a
> test and then restoring it after it is done.  The latter is needed
> for the test framework to clean after itself without leaving a
> leftover directory that cannot be removed.
> [...]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> @@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' '
>  test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
> +       test_when_finished "chmod +w newdir" &&
>         rm -fr newdir &&
>         mkdir newdir &&
>         chmod -w newdir &&

When reading this, I was wondering if there was a "rm -fr newdir" at
the end of the test which could be removed now that it uses
test_when_finished(). Checking beyond the shown context, I see that it
doesn't bother with removal since the next test removes the directory
as a preparatory step. Even before the addition of
test_when_finished() to restore write permission, the subsequent
test's removal of the directory worked because this is test is only
run when POSIXPERM is met, and POSIX allows for such an operation.
Okay, looks good.