Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY

Torsten Bögershausen [off-list ref] writes:

 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.

This no longer is relevant, I think.

+# Special check for CYGWIN (or Windows in general):

Misleading comment in the end result, as your new test drops SANITY
correctly on POSIX for the root user, too.  In a commit log message
it is correct to say "This adds special check for Cygwin", but the
resulting code is sensible with or without Cygwin, I would think,
with the justification to "test by checking what we really want, not
by inferring from the result of indirectly testing something else".

+# A file can be deleted, even if the containing directory does'nt
+# have write permissions

We also rely on SANITY to make sure that "chmod -rx directory" makes
"directory/file" undiscoverable.

How about extending it like this (not tested)?

-- >8 --
From: Torsten Bögershausen <redacted>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need

What we wanted out of the SANITY precondition is that the filesystem
behaves sensibly with permission bits settings.

 - You should not be able to remove a file in a read-only directory,

 - You should not be able to tell if a file in a directory exists if
   the directory lacks read or execute permission bits.

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test.  Neither test
is sufficient or appropriate in more exotic environments like
Cygwin.

Signed-off-by: Torsten Bögershausen <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 t/test-lib.sh | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index b2b2ec7..446d8d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -997,9 +997,28 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
-# When the tests are run as root, permission tests will report that
-# things are writable when they shouldn't be.
-test -w / || test_set_prereq SANITY
+# On a filesystem that lacks SANITY, a file can be deleted even if
+# the containing directory doesn't have write permissions, or a file
+# can be accessed even if the containing directory doesn't have read
+# or execute permissions, causing our tests that validate that Git
+# works sensibly in such situations.
+test_lazy_prereq SANITY '
+	mkdir SANETESTD.1 SANETESTD.2 &&
+
+	chmod +w SANETESTD.1 SANETESTD.2 &&
+	>SANETESTD.1/x 2>SANETESTD.2/x &&
+	chmod -w SANETESTD.1 &&
+	chmod -rx SANETESTD.2 ||
+	error "bug in test sript: cannot prepare SANETESTD"
+
+	! rm SANETESTD.1/x && ! test -f SANETESTD.2/x
+	status=$?
+
+	chmod +rwx SANETESTD.1 SANETESTD.2 &&
+	rm -rf SANETESTD.1 SANETESTD.2 ||
+	error "bug in test sript: cannot clean SANETESTD"
+	return $status
+'
 
 GIT_UNZIP=${GIT_UNZIP:-unzip}
 test_lazy_prereq UNZIP '

-- 
2.3.0-rc1-180-g1a69fe5

On 28.01.15 18:38, Junio C Hamano wrote:

On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen [off-list ref] wrote:

quoted

On 27.01.15 23:20, Junio C Hamano wrote:

quoted

How about extending it like this (not tested)?

Thanks, this looks good: the test is more extensive,
I can test this next week.

quoted

-- >8 --
From: Torsten Bögershausen <redacted>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need

What we wanted out of the SANITY precondition is that the filesystem
behaves sensibly with permission bits settings.

 - You should not be able to remove a file in a read-only directory,

 - You should not be able to tell if a file in a directory exists if
   the directory lacks read or execute permission bits.

Forgot one thing. I do not offhand know if tests that needs SANITY
depends on this, but we may also want to check for this:

 - You should not be able to write to a file that is marked as read-only.

by adding something like

  >sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && !
test -s sanitytest"

in the mix.

quoted

quoted

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test.  Neither test
is sufficient or appropriate in more exotic environments like
Cygwin.

How about going this direction:

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test. Neither test
is sufficient or appropriate, especially in environments like
Cygwin, Mingw or Mac OS X.

OK, but MacOS X does not have SANITY problem; "is the / writable?" test
was misdetecting and declaring a system with SANITY does not have one.

Perhaps roll Cygwin and Mingw into a single Windows category? I dunno.

The whole discussion actually started with Mac OS X,
and the conclusion was that Mac OS X should have SANITY set, but hadn't,
because  / is writable (if you install from scratch):

$gmane/262389
and especially:
$gmane/262456

The whole discussion ended up a fix for t5539, and, as a different improvement,
the lazy SANITY probing - which works for me on all systems I had the chance to test it.

The code is OK (we can add more tests, as you suggested).
The only problem I can see is to put everything into a good commit-msg.

On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen [off-list ref] wrote:

On 27.01.15 23:20, Junio C Hamano wrote:

quoted

How about extending it like this (not tested)?

Thanks, this looks good: the test is more extensive,
I can test this next week.

quoted

-- >8 --
From: Torsten Bögershausen <redacted>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need

What we wanted out of the SANITY precondition is that the filesystem
behaves sensibly with permission bits settings.

 - You should not be able to remove a file in a read-only directory,

 - You should not be able to tell if a file in a directory exists if
   the directory lacks read or execute permission bits.

Forgot one thing. I do not offhand know if tests that needs SANITY
depends on this, but we may also want to check for this:

 - You should not be able to write to a file that is marked as read-only.

by adding something like

  >sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && !
test -s sanitytest"

in the mix.

quoted

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test.  Neither test
is sufficient or appropriate in more exotic environments like
Cygwin.

How about going this direction:

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test. Neither test
is sufficient or appropriate, especially in environments like
Cygwin, Mingw or Mac OS X.

OK, but MacOS X does not have SANITY problem; "is the / writable?" test
was misdetecting and declaring a system with SANITY does not have one.

Perhaps roll Cygwin and Mingw into a single Windows category? I dunno.

On 27.01.15 23:20, Junio C Hamano wrote:

How about extending it like this (not tested)?

Thanks, this looks good: the test is more extensive,
I can test this next week.

-- >8 --
From: Torsten Bögershausen <redacted>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need

What we wanted out of the SANITY precondition is that the filesystem
behaves sensibly with permission bits settings.

 - You should not be able to remove a file in a read-only directory,

 - You should not be able to tell if a file in a directory exists if
   the directory lacks read or execute permission bits.

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test.  Neither test
is sufficient or appropriate in more exotic environments like
Cygwin.

How about going this direction:

We used to cheat by approximating that condition with "is the /
writable?" test and/or "are we running as root?" test. Neither test
is sufficient or appropriate, especially in environments like
Cygwin, Mingw or Mac OS X.