* [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
@ 2015-01-27 15:39 Torsten Bögershausen
2015-01-27 19:53 ` Chris Packham
2015-01-27 22:20 ` Junio C Hamano
0 siblings, 2 replies; 7+ messages in thread
From: Torsten Bögershausen @ 2015-01-27 15:39 UTC (permalink / raw)
To: git; +Cc: tboegi
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.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
t/test-lib.sh | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 93f7cad..887e986 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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}
--
2.2.0.rc1.26.g3e3a70d
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
2015-01-27 15:39 [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Torsten Bögershausen
@ 2015-01-27 19:53 ` Chris Packham
2015-01-27 22:20 ` Junio C Hamano
1 sibling, 0 replies; 7+ messages in thread
From: Chris Packham @ 2015-01-27 19:53 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: GIT
Minor typo in a comment.
On Wed, Jan 28, 2015 at 4:39 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> 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.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> t/test-lib.sh | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 93f7cad..887e986 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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/
> +# 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
2015-01-27 15:39 [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Torsten Bögershausen
2015-01-27 19:53 ` Chris Packham
@ 2015-01-27 22:20 ` Junio C Hamano
2015-01-28 8:28 ` Torsten Bögershausen
1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2015-01-27 22:20 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
Torsten Bögershausen <tboegi@web.de> 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 <tboegi@web.de>
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 <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
2015-01-27 22:20 ` Junio C Hamano
@ 2015-01-28 8:28 ` Torsten Bögershausen
2015-01-28 17:38 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Torsten Bögershausen @ 2015-01-28 8:28 UTC (permalink / raw)
To: Junio C Hamano, Torsten Bögershausen; +Cc: git
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 <tboegi@web.de>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
2015-01-28 8:28 ` Torsten Bögershausen
@ 2015-01-28 17:38 ` Junio C Hamano
2015-01-28 19:19 ` Torsten Bögershausen
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2015-01-28 17:38 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> 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 <tboegi@web.de>
>> 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.
>>
>> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
2015-01-28 17:38 ` Junio C Hamano
@ 2015-01-28 19:19 ` Torsten Bögershausen
2015-01-28 20:38 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Torsten Bögershausen @ 2015-01-28 19:19 UTC (permalink / raw)
To: Junio C Hamano, Torsten Bögershausen; +Cc: Git Mailing List
On 28.01.15 18:38, Junio C Hamano wrote:
> On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen <tboegi@web.de> wrote:
>> 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 <tboegi@web.de>
>>> 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.
>
>>> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY
2015-01-28 19:19 ` Torsten Bögershausen
@ 2015-01-28 20:38 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2015-01-28 20:38 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
Torsten Bögershausen <tboegi@web.de> writes:
>> 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?
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-01-29 2:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 15:39 [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Torsten Bögershausen
2015-01-27 19:53 ` Chris Packham
2015-01-27 22:20 ` Junio C Hamano
2015-01-28 8:28 ` Torsten Bögershausen
2015-01-28 17:38 ` Junio C Hamano
2015-01-28 19:19 ` Torsten Bögershausen
2015-01-28 20:38 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.