test-lib.sh: Dynamic test for the prerequisite SANITY
PATCH 2/2 test-lib.sh: Dynamic test for the prerequisite SANITY
The SANITY precondition was not set when running as root, but this is not 100% reliable for CYGWIN:
A file may be allowed to be deleted when the containing directory does not have write permissions.
See
https://technet.microsoft.com/en-us/library/bb463216.aspx
"...In UNIX, the write permission bit on a directory permits both the creation and removal of new files or sub-directories in the directory. On Windows, the Write_Data access right controls the creation of new sub-files and the Delete_Child access right controls the deletion. ...."
We may argue that the translation of the POSIX write permission bit into "Windows access rights" can be improved in CYGWIN.
A dynamic test handles all cases: when the sequence
$ mkdir SANETESTD && $ chmod +w SANETESTD && $ >SANETESTD/x && $ ! rm SANETESTD/x succeeds the prerequisite SANITY is true.
--- t/test-lib.sh | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
t/test-lib.sh +16 -1
@@ -1038,8 +1038,23 @@ test_lazy_prereq NOT_ROOT ' # When the tests are run as root, permission tests will report that # things are writable when they shouldn't be. +# Special check for CYGWIN (or Windows in general): +# A file can be deleted, even if the containing directory does'nt +# have write permissions test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir SANETESTD && + chmod +w SANETESTD && + >SANETESTD/x && + chmod -w SANETESTD || + error "bug in test sript: cannot prepare SANETESTD" + + ! rm SANETESTD/x + status=$? + + chmod +w SANETESTD && + rm -rf SANETESTD || + error "bug in test sript: cannot clean SANETESTD" + return $status ' GIT_UNZIP=${GIT_UNZIP:-unzip}
Signature · 1 line
2.2.0.rc1.26.g3e3a70d
Minor typo in a comment.
Quoted 34 lines · Torsten Bögershausen
The SANITY precondition was not set when running as root, but this is not 100% reliable for CYGWIN:
A file may be allowed to be deleted when the containing directory does not have write permissions.
See
https://technet.microsoft.com/en-us/library/bb463216.aspx
"...In UNIX, the write permission bit on a directory permits both the creation and removal of new files or sub-directories in the directory. On Windows, theWrite_Dataaccess right controls the creation of new sub-files and theDelete_Childaccess right controls the deletion. ...."We may argue that the translation of the POSIX write permission bit into "Windows access rights" can be improved in CYGWIN.
A dynamic test handles all cases: when the sequence
$ mkdir SANETESTD && $ chmod +w SANETESTD && $ >SANETESTD/x && $ ! rm SANETESTD/x succeeds the prerequisite SANITY is true.Helped-by Junio C Hamano <[email protected]>Signed-off-by Torsten Bögershausen <[email protected]>--- t/test-lib.sh | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)t/test-lib.sh +0
-0
t/test-lib.sh test_lazy_prereq NOT_ROOT ' · line 1042
Quoted 5 lines
t/test-lib.sh +2
-0@@ -1038,8 +1038,23 @@ test_lazy_prereq NOT_ROOT ' # When the tests are run as root, permission tests will report that # things are writable when they shouldn't be. +# Special check for CYGWIN (or Windows in general): +# A file can be deleted, even if the containing directory does'nt
s/does'nt/doesn't/
Quoted 26 lines
t/test-lib.sh +14
-2+# have write permissions test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir SANETESTD && + chmod +w SANETESTD && + >SANETESTD/x && + chmod -w SANETESTD || + error "bug in test sript: cannot prepare SANETESTD" + + ! rm SANETESTD/x + status=$? + + chmod +w SANETESTD && + rm -rf SANETESTD || + error "bug in test sript: cannot clean SANETESTD" + return $status ' GIT_UNZIP=${GIT_UNZIP:-unzip} --2.2.0.rc1.26.g3e3a70d
--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines · Torsten Bögershausen
# 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.
Quoted 1 line
+# 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".
Review
Quoted 2 lines
(quoted diff) +2
-0+# 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 <[email protected]>
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.
--- t/test-lib.sh | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
t/test-lib.sh +22 -3
@@ -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 '
Signature · 1 line
2.3.0-rc1-180-g1a69fe5
Quoted 1 line · Junio C Hamano
How about extending it like this (not tested)?
Thanks, this looks good: the test is more extensive, I can test this next week.
Quoted 11 lines
-- >8 --
From: Torsten Bögershausen <[email protected]>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH]test-lib.sh: set prerequisite SANITY by testing what we really needWhat 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,
Quoted 7 lines
- 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.
Quoted 19 lines · Torsten Bögershausen
Quoted 1 line · Junio C Hamano
How about extending it like this (not tested)?
Thanks, this looks good: the test is more extensive, I can test this next week.
Quoted 13 lines
-- >8 --
From: Torsten Bögershausen <[email protected]>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH]test-lib.sh: set prerequisite SANITY by testing what we really needWhat 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
Quoted 1 line
sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && !
test -s sanitytest"
in the mix.
Quoted 11 lines
Quoted 5 lines
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.
Quoted 37 lines · Junio C Hamano
Quoted 18 lines · Torsten Bögershausen
Quoted 1 line · Junio C Hamano
How about extending it like this (not tested)?
Thanks, this looks good: the test is more extensive, I can test this next week.
Quoted 12 lines
-- >8 --
From: Torsten Bögershausen <[email protected]>
Date: Tue, 27 Jan 2015 16:39:01 +0100
Subject: [PATCH]test-lib.sh: set prerequisite SANITY by testing what we really needWhat 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
Quoted 1 line
sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && !
test -s sanitytest"
in the mix.
Quoted 6 lines
Quoted 4 lines
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:
Quoted 8 lines
Quoted 4 lines
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.
Quoted 7 lines · Torsten Bögershausen
Quoted 4 lines
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):
Exactly.
On MacOSX running as non-root-but-can-write-to-slash-user, we should say SANITY is satisfied, but "is the / writable?" check was misdetecting and declaring the system does not have SANITY.
So we are in agreement, no?