t5539 broken under Mac OS X
t5539 doesn't seem to work as expected under Mac OX X 10.6 (10.9 is OK)
I am not root.
Are there any ideas how we can improve the situation, or how to debug ?
t>
t> ./t5539-fetch-http-shallow.sh ; echo $?
1..0 # SKIP Cannot run httpd tests as root
0
t>
t> GIT_TEST_HTTPD=t ./t5539-fetch-http-shallow.sh ; echo $?
error: Cannot run httpd tests as root
1
t>
Quoted 5 lines · Torsten Bögershausen
t5539 doesn't seem to work as expected under Mac OX X 10.6 (10.9 is OK)
I am not root.
Are there any ideas how we can improve the situation, or how to debug ?
As to "how to debug", the first step is to grep for that message and notice that it comes from here:
t/lib-httpd.sh:
if ! test_have_prereq SANITY; then
test_skip_or_die $GIT_TEST_HTTPD \
"Cannot run httpd tests as root"
fi
and then grep for SANITY to find:
t/test-lib.sh:
# 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
It appears that the check in lib-httpd.sh thinks you lack SANITY; is the root directory of your system somehow writable by you?
Quoted 12 lines
t>
t> ./t5539-fetch-http-shallow.sh ; echo $?1..0 # SKIP Cannot run httpd tests as root
0
t>
t> GIT_TEST_HTTPD=t ./t5539-fetch-http-shallow.sh ; echo $?
error: Cannot run httpd tests as root
1
t>
Quoted 20 lines · Junio C Hamano
Quoted 5 lines · Torsten Bögershausen
t5539 doesn't seem to work as expected under Mac OX X 10.6 (10.9 is OK)
I am not root.
Are there any ideas how we can improve the situation, or how to debug ?As to "how to debug", the first step is to grep for that message and notice that it comes from here:
t/lib-httpd.sh:if !
test_have_prereqSANITY; then
test_skip_or_die$GIT_TEST_HTTPD \
"Cannot run httpd tests as root"
fiand then grep for SANITY to find:
Quoted 9 lines
t/test-lib.sh:# When the tests are run as root, permission tests will report that # things are writable when they shouldn't be.
test -w / ||test_set_prereqSANITYIt appears that the check in
lib-httpd.shthinks you lack SANITY; is the root directory of your system somehow writable by you?
Yes, that was a good hint, thanks.
The "problem" is that I am Admin on one machine, but not on the other, and / was writable for the admin group for some reasons, and only on this machine.
But, why does e.g. t0004 behave more gracefully (and skips) and t5539 just dies ?
./t0004-unwritable.sh
ok 1 - setup
ok 2 # skip write-tree should notice unwritable repository (missing SANITY of POSIXPERM,SANITY)
(And after changing the group of / t5539 passes, and so does t0004)
Quoted 5 lines · Torsten Bögershausen
But, why does e.g. t0004 behave more gracefully (and skips) and t5539 just dies ?
./t0004-unwritable.sh
ok 1 - setup
ok 2 # skip write-tree should notice unwritable repository (missing SANITY of POSIXPERM,SANITY)
The http code uses test_skip_or_die when it runs into setup errors. The intent there is that the user has either:
1. Told us explicitly that they want http tests by setting GIT_TEST_HTTPD=true.
2. Wants to run http tests if they can by setting GIT_TEST_HTTPD=auto (or leaving it unset, as that is the default).
In case (1), we treat this as a test failure. They asked for httpd tests, and we could not run them. In case (2), we would just skip all of the tests.
You may want to loosen your GIT_TEST_HTTPD setting (pre-83d842dc, you had to set it to true to run the tests at all, but nowadays we have auto).
-Peff
Quoted 20 lines · Jeff King
Quoted 7 lines · Torsten Bögershausen
But, why does e.g. t0004 behave more gracefully (and skips) and t5539 just dies ?
./t0004-unwritable.sh
ok 1 - setup
ok 2 # skip write-tree should notice unwritable repository (missing SANITY of POSIXPERM,SANITY)The http code uses
test_skip_or_diewhen it runs into setup errors. The
intent there is that the user has either:1. Told us explicitly that they want http tests by setting GIT_TEST_HTTPD=true.
2. Wants to run http tests if they can by setting GIT_TEST_HTTPD=auto (or leaving it unset, as that is the default).
Quoted 8 lines
In case (1), we treat this as a test failure. They asked for httpd tests, and we could not run them. In case (2), we would just skip all of
the tests.You may want to loosen your
GIT_TEST_HTTPDsetting (pre-83d842dc, you had to set it to true to run the tests at all, but nowadays we have auto).
I ran into this problem. It seems like (at least on older Mac OS X) that the root directory is created like so:
drwxrwxr-t 39 root admin /
And since the first (and likely only user) on Mac OS X is a member of the admin group, the SANITY test fails and complains even though you're not running as root (the failure message is misleading).
I ended up removing group write permission from / (which happened to find a bug in another script of mine) and then it was happy.
-Kyle
Quoted 8 lines · Kyle J. McKay
I ran into this problem. It seems like (at least on older Mac OS X) that the root directory is created like so:
drwxrwxr-t 39 root admin /
And since the first (and likely only user) on Mac OS X is a member of the admin group, the SANITY test fails and complains even though you're not running as root (the failure message is misleading).
The design choice Mac OS X makes around filesystems may deserve the !SANITY label ;-) but we may want to tighten the check for SANITY, or better yet, rethink the interaction between POSIXPERM and SANITY.
What we want to express with SANITY is:
On this system, if the user who is running the test does not have write permission to a file, write to such a file would fail.
So running our tests as a non-root admin user should be labeled as being sane. We just use a more expedient "if you can write into the root directory, you must be root, asit is crazy to allow non-root user to 'mv /etc /foo && mkdir /etc && write /etc/passwd'" heuristics which is old-school.
This should not be the final patch (I think it should become a lazy prereq as it does a lot more), but just for testing, how does this look?
t/test-lib.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
t/test-lib.sh +10 -1
@@ -1033,7 +1033,16 @@ test_lazy_prereq USR_BIN_TIME ' # 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 +if test_have_prereq POSIXPERM && + ! test -w / && + >sanitytest && + chmod a= sanitytest && + ! (>sanitytest) 2>/dev/null && + chmod +w sanitytest && + rm -f sanitytest +then + test_set_prereq SANITY +fi GIT_UNZIP=${GIT_UNZIP:-unzip} test_lazy_prereq UNZIP '
Quoted 16 lines · Junio C Hamano
This should not be the final patch (I think it should become a lazy prereq as it does a lot more), but just for testing, how does this look?
t/test-lib.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)t/test-lib.sh +0
-1@@ -1033,7 +1033,16 @@ test_lazy_prereq USR_BIN_TIME ' # 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
t/test-lib.sh test_lazy_prereq USR_BIN_TIME ' · line 1045
Quoted 10 lines
t/test-lib.sh +10
-0+if test_have_prereq POSIXPERM && + ! test -w / && + >sanitytest && + chmod a= sanitytest && + ! (>sanitytest) 2>/dev/null && + chmod +w sanitytest && + rm -f sanitytest +then + test_set_prereq SANITY +fi
The current scheme does not require POSIXPERM. Would this mean that some platforms no longer runs SANITY tests (e.g., Windows)?
Many of the SANITY-marked tests already require both, but not all. And certainly lib-httpd actually cares whether you are _truly_ root, not about weird filesystem permissions. Should lib-httpd literally be checking the output of id (though I can imagine that is anything but portable)?
-Peff
Quoted 4 lines · Jeff King
The current scheme does not require POSIXPERM. Would this mean that some platforms no longer runs SANITY tests (e.g., Windows)?
Many of the SANITY-marked tests already require both, but not all.
Before writing that patchlet, I briefly looked at grep output and thought that many that are protected only by SANITY lacked POSIXPERM by mistake:
t/t1004-read-tree-m-u-wf.sh:test_expect_success SANITY 'funny symlink in... t/t3600-rm.sh 'Test that "git rm -f" fails if its rm fails' t/t7300-clean.sh:test_expect_success SANITY 'removal failure' ' t/t7300-clean.sh:test_expect_success SANITY 'git clean -d with an...
All of the above relies on a working chmod as far as I can tell, so they should require POSIXPERM,SANITY, not just SANITY.
Quoted 5 lines
And
certainly lib-httpd actually cares whether you are _truly_ root, not about weird filesystem permissions. Should lib-httpd literally be checking the output ofid(though I can imagine that is anything but portable)?
Even though t/README describes SANITY to require:
Test is not run by root user, and an attempt to write to an unwritable file is expected to fail correctly.
and it has been that way from day one, c91cfd19 (tests: A SANITY test prereq for testing if we're root, 2010-08-06) is clear that this is about "'chmod -w' is a good way to test unwritable files"
lib-httpd should, if it cares about the root-ness, be checking that in a more direct way, "test_have_prereq RUNNING_AS_ROOT". Making the implementation of that portable is another matter, though.
Quoted 11 lines · Junio C Hamano
Before writing that patchlet, I briefly looked at grep output and thought that many that are protected only by SANITY lacked POSIXPERM by mistake:
t/t1004-read-tree-m-u-wf.sh:test_expect_success SANITY 'funny symlink in...
t/t3600-rm.sh'Test that "git rm -f" fails if its rm fails' t/t7300-clean.sh:test_expect_success SANITY 'removal failure' ' t/t7300-clean.sh:test_expect_success SANITY 'git clean -d with an...All of the above relies on a working chmod as far as I can tell, so they should require POSIXPERM,SANITY, not just SANITY.
Yeah, skimming the grep output, I had the same feeling. But I did not investigate closely.
Quoted 3 lines
lib-httpd should, if it cares about the root-ness, be checking that in a more direct way, "
test_have_prereqRUNNING_AS_ROOT". Making the implementation of that portable is another matter, though.
Exactly. I am happy to submit a patch, but I cannot think of any mechanisms besides:
1. Calling id, which I suspect is very not portable.
2. Writing a C program to check getuid(). That's portable for most Unixes. It looks like we already have a hacky wrapper on mingw that will always return "1".
Is (2) too gross?
-Peff
Quoted 10 lines · Jeff King
Exactly. I am happy to submit a patch, but I cannot think of any mechanisms besides:
1. Calling
id, which I suspect is very not portable.2. Writing a C program to check
getuid(). That's portable for most Unixes. It looks like we already have a hacky wrapper on mingw that will always return "1".Is (2) too gross?
Not overly gross compared to some existing test-*.c files, I would say.
I wondered what 'perl -e 'print $>' would say in mingw, and if that is portable enough, though.
PATCH 1/1 t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 2 lines · Junio C Hamano
I wondered what 'perl -e 'print $>' would say in mingw, and if that is portable enough, though.
Good thinking. I guess the best way to find out is to convince somebody from msysgit to try this patch. :)
We may simply find that nobody there even has apache installed on their box, and they do not run the http tests at all.
-- >8 --
The SANITY prerequisite is really about whether the filesystem will respect the permissions we set, and being root is only one part of that. But the httpd tests really just care about not being root, as they are trying to avoid weirdness in apache (see a1a3011 for details).
Let's switch out SANITY for a new NOT_ROOT prerequisite, which will let us tweak SANITY more freely.
We implement NOT_ROOT by checking perl's "$>" variable, since we cannot rely on the "id" program being available everywhere (and we would rather avoid writing a custom C program to run geteuid if we can).
Note that we cannot just call this "ROOT" and ask for "!ROOT". The possible outcomes are:
1. we know we are root
2. we know we are not root
3. we could not tell, because perl was not installed or barfed showing us $>
We should conservatively treat (3) as "does not have the prerequisite", which means that a naive negation would not work.
--- t/lib-httpd.sh | 2 +- t/test-lib.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
t/lib-httpd.sh +1 -1
@@ -37,7 +37,7 @@ then test_done fi -if ! test_have_prereq SANITY; then +if ! test_have_prereq NOT_ROOT; then test_skip_or_die $GIT_TEST_HTTPD \ "Cannot run httpd tests as root" fi
t/test-lib.sh +5 -0
@@ -1040,3 +1040,8 @@ test_lazy_prereq UNZIP ' "$GIT_UNZIP" -v test $? -ne 127 ' + +test_lazy_prereq NOT_ROOT ' + uid=$(perl -e "print \$<") && + test "$uid" != 0 +'
Signature · 21 lines
2.2.1.425.g441bb3c -- -- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 11 lines · Jeff King
Quoted 2 lines · Junio C Hamano
I wondered what 'perl -e 'print $>' would say in mingw, and if that is portable enough, though.
Good thinking. I guess the best way to find out is to convince somebody from msysgit to try this patch. :)
We may simply find that nobody there even has apache installed on their box, and they do not run the http tests at all.
[...]
Quoted 4 lines
We implement
NOT_ROOTby checking perl's "$>" variable, since we cannot rely on the "id" program being available everywhere (and we would rather avoid writing a custom C program to run geteuid if we can).
Does it make a difference that id is POSIX [1]?
So the test "if [ $(id -u) = 0 ]" or similar ought to work.
"id -u" works for me in MSYS and cygwin (each appears to have it's own id.exe).
t/test-lib.sh test_lazy_prereq UNZIP ' · line 1047
Quoted 5 lines
t/test-lib.sh +5
-0+ +test_lazy_prereq NOT_ROOT ' + uid=$(perl -e "print \$<") && + test "$uid" != 0 +'
Does NO_PERL affect this? Or is Perl always required to run the tests.
Also "$<" is real user id. Don't you want effective user id ("$>"), that's what the comment says...
Both "$<" and "$>" work for me in MSYS and cygwin although if I run it from cmd.exe using strawberry perl, both "$<" and "$>" give 0. (There's no id.exe for cmd.exe unless it finds the cygwin/msys one.)
As long as NO_PERL is not also intended to affect "make test" either the perl or id version seems fine to me (as long as it's Perl's "$>") since I doubt the tests would run with just cmd.exe. :)
-Kyle
[1] http://pubs.opengroup.org/onlinepubs/009604499/utilities/id.html
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 6 lines · Kyle J. McKay
Quoted 4 lines
We implement
NOT_ROOTby checking perl's "$>" variable, since we cannot rely on the "id" program being available everywhere (and we would rather avoid writing a custom C program to run geteuid if we can).Does it make a difference that id is POSIX [1]?
I don't know. Do all of the platforms where we run http tests have it (and conforming to POSIX-ish options or output)? It may be OK to guess yes and see if anybody complains (the worst case is skipping http tests).
Quoted 2 lines
"id -u" works for me in MSYS and cygwin (each appears to have it's own id.exe).
That's comforting. MSYS was the one I was most worried about. What UID do they report? I.e., do they correctly tell us if we are root (or more accurately, if we are not root)?
Quoted 6 lines
t/test-lib.sh test_lazy_prereq UNZIP ' · line 1047
Quoted 4 lines
t/test-lib.sh +4
-0+test_lazy_prereq NOT_ROOT ' + uid=$(perl -e "print \$<") && + test "$uid" != 0 +'Does
NO_PERLaffect this? Or is Perl always required to run the tests.
No, we use a very limited subset of perl in our tests when necessary (basic enough that any perl5 will do), regardless of the NO_PERL setting.
Quoted 2 lines
Also "$<" is real user id. Don't you want effective user id ("$>"), that's what the comment says...
Yeah, I bungled this initially and thought I fixed it, but clearly not. :-/
I'll re-roll, but if we can get away with "id -u" I think that's preferable.
-Peff
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 8 lines · Jeff King
Quoted 2 lines · Kyle J. McKay
"id -u" works for me in MSYS and cygwin (each appears to have it's own id.exe).
That's comforting. MSYS was the one I was most worried about. What UID do they report? I.e., do they correctly tell us if we are root (or more accurately, if we are not root)?
It's funny, really. The MSYS version gives a different answer than the cygwin version although both are non-zero. The MSYS perl gives the same answer as the MSYS id and the cygwin perl gives the same answer as the cygwin id.
I'm not even sure what it would mean to "be root" on one of those systems.
The closest I can think of would be to run as the "SYSTEM" user. And that's not nearly as simple as just "sudo -s". [1].
I haven't tested that. I will try to remember to give that a try next time I'm feeling the need for some frustration. ;)
-Kyle
[1] http://cygwin.com/ml/cygwin/2010-04/msg00651.html
Signature · 17 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en ---You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 6 lines · Kyle J. McKay
Quoted 6 lines
Quoted 2 lines
"id -u" works for me in MSYS and cygwin (each appears to have it's own id.exe).
That's comforting. MSYS was the one I was most worried about. What UID do they report? I.e., do they correctly tell us if we are root (or more accurately, if we are not root)?
Checking for UID 0 won't work on Cygwin in the general case. That fools literally dozens of Perl module tests that find out the user can actually do something they think (s)he should be unable to.
Quoted 4 lines
It's funny, really. The MSYS version gives a different answer than the cygwin version although both are non-zero. The MSYS perl gives the same answer as the MSYS id and the cygwin perl gives the same answer as the cygwin id.
That result changes depending on the content /etc/passwd (which arguably is a either a bug or a feature depending on which way you look at it). But Windows itself doesn't have the notion of a root user at all, so looking for one isn't going to be helpful.
Quoted 2 lines
I'm not even sure what it would mean to "be root" on one of those systems.
It means you have the capabilities that a root user would be expected to have. For most intents and purposes on Windows this would mean the user running the command is in group 544 ("Administrators" in an english version of Windows).
Quoted 2 lines
The closest I can think of would be to run as the "SYSTEM" user. And that's not nearly as simple as just "sudo -s". [1].
The SYSTEM user isn't a good approximation of root under UN*X for reasonably modern Windows versions.
http://support.microsoft.com/kb/120929
For more discussion on the UID 0 topic from a Cygwin perspective, see
http://thread.gmane.org/gmane.os.cygwin.applications/28129
http://thread.gmane.org/gmane.os.cygwin.applications/28203
Regards,
Achim.
Signature · 24 lines
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ SD adaptation for Waldorf microQ V2.22R2: http://Synth.Stromeko.net/Downloads.html#WaldorfSDada -- -- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
In reply to Jeff KingQuoted 6 lines · Jeff King
Quoted 2 lines
"id -u" works for me in MSYS and cygwin (each appears to have it's own id.exe).
That's comforting. MSYS was the one I was most worried about. What UID do they report? I.e., do they correctly tell us if we are root (or more accurately, if we are not root)?
So here's a re-roll with id -u, as that may be the simplest way to get people to test (with the patch applied, running t5550 as a normal user should work, and as root should skip the tests).
-- >8 --
Subject: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
The SANITY prerequisite is really about whether the filesystem will respect the permissions we set, and being root is only one part of that. But the httpd tests really just care about not being root, as they are trying to avoid weirdness in apache (see a1a3011 for details).
Let's switch out SANITY for a new NOT_ROOT prerequisite, which will let us tweak SANITY more freely.
We implement NOT_ROOT by checking id -u, which is in POSIX and seems to be available even on MSYS. Note that we cannot just call this "ROOT" and ask for "!ROOT". The possible outcomes are:
1. we know we are root
2. we know we are not root
3. we could not tell, because id was not available
We should conservatively treat (3) as "does not have the prerequisite", which means that a naive negation would not work.
--- t/lib-httpd.sh | 2 +- t/test-lib.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
t/lib-httpd.sh +1 -1
@@ -37,7 +37,7 @@ then test_done fi -if ! test_have_prereq SANITY; then +if ! test_have_prereq NOT_ROOT; then test_skip_or_die $GIT_TEST_HTTPD \ "Cannot run httpd tests as root" fi
t/test-lib.sh +5 -0
@@ -1040,3 +1040,8 @@ test_lazy_prereq UNZIP ' "$GIT_UNZIP" -v test $? -ne 127 ' + +test_lazy_prereq NOT_ROOT ' + uid=$(id -u) && + test "$uid" != 0 +'
Signature · 21 lines
2.2.1.425.g441bb3c -- -- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 1 line · Jeff King
Subject: [PATCH] t/lib-httpd: switch SANITY check for
NOT_ROOT
[...]
Quoted 14 lines
We implement
NOT_ROOTby checkingid -u, which is in POSIX and seems to be available even on MSYS. Note that we cannot just call this "ROOT" and ask for "!ROOT". The possible outcomes are:1. we know we are root
2. we know we are not root
3. we could not tell, because
idwas not availableWe should conservatively treat (3) as "does not have the prerequisite", which means that a naive negation would not work.
[...]
t/test-lib.sh test_lazy_prereq UNZIP ' · line 1047
Quoted 5 lines
t/test-lib.sh +5
-0+ +test_lazy_prereq NOT_ROOT ' + uid=$(id -u) && + test "$uid" != 0 +'
That looks good to me and worked as expected when I tried it.
-Kyle
Signature · 17 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
In reply to Jeff KingQuoted 10 lines · Jeff King
So here's a re-roll with
id -u, as that may be the simplest way to get people to test (with the patch applied, running t5550 as a normal user should work, and as root should skip the tests).-- >8 --
Subject: [PATCH] t/lib-httpd: switch SANITY check forNOT_ROOTThe SANITY prerequisite is really about whether the filesystem will respect the permissions we set, and being root is only one part of that....
I checked the use of POSIXPERM that is not tied to SANITY and found a few questionable ones (this is orthogonal from the earlier list of glitches I mentioned, which is SANITY without POSIXPERM).
I think we will later make SANITY to require NOT_ROOT and POSIXPERM, at which point many existing tests that require POSIXPERM,SANITY can be simplified to require only SANITY, but that will be a follow-up change to this fix.
-- >8 --
Subject: tests: correct misuses of POSIXPERM
POSIXPERM requires that a later call to stat(2) (hence "ls -l") faithfully reproduces what an earlier chmod(2) did. Some filesystems cannot satisify this.
SANITY requires that a file or a directory is indeed accessible (or inaccessible) when its permission bits would say it ought to be accessible (or inaccessible). Running tests as root would lose this prerequisite for obvious reasons.
Fix a few tests that misuse POSIXPERM.
t0061-run-command.sh has two uses of POSIXPERM.
- One checks that an attempt to execute a file that is marked as unexecutable results in a failure with EACCES; I do not think having root-ness or any other capability that busts the filesystem permission mode bits will make you run an unexecutable file, so this should be left as-is. The test does not have anything to do with SANITY.
- The other one expects 'git nitfol' runs the alias when an alias.nitfol is defined and a directory on the PATH is marked as unreadable and unsearchable. I _think_ the test tries to reject the alternative expectation that we want to refuse to run the alias because it would break "no alias may mask a command" rule if a file 'git-nitfol' exists in the unreadable directory but we cannot even determine if that is the case. Under !SANITY that busts the permission bits, this test no longer checks that, so it must be protected with SANITY.
t1509-root-worktree.sh expects to be run on a / that is writable by the user and sees if Git behaves "sensibly" when /.git is the repository to govern a worktree that is the whole filesystem, and also if Git behaves "sensibly" when / itself is a bare repository with refs, objects, and friends (I find the definition of "behaves sensibly" under these conditions hard to fathom, but it is a different matter).
The implementation of the test is very much problematic.
- It requires POSIXPERM, but it does not do chmod or checks modes in any way.
- It runs "rm /*" and "rm -fr /refs /objects ..." in one of the tests, and also does "cd / && git init --bare". If done on a live system that takes advantages of the "feature" being tested, these obviously will clobber the system. But there is no guard against such a breakage.
- It uses "test $UID = 0" to see rootness, which now should be spelled "! test_have_prereq NOT_ROOT"
t/t0061-run-command.sh | 2 +- t/t1509-root-worktree.sh | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-)
t/t0061-run-command.sh +1 -1
@@ -34,7 +34,7 @@ test_expect_success POSIXPERM 'run_command reports EACCES' ' grep "fatal: cannot exec.*hello.sh" err ' -test_expect_success POSIXPERM 'unreadable directory in PATH' ' +test_expect_success POSIXPERM,SANITY 'unreadable directory in PATH' ' mkdir local-command && test_when_finished "chmod u+rwx local-command && rm -fr local-command" && git config alias.nitfol "!echo frotz" &&
t/t1509-root-worktree.sh +13 -4
@@ -98,8 +98,16 @@ test_foobar_foobar() { ' } -if ! test_have_prereq POSIXPERM || ! [ -w / ]; then - skip_all="Dangerous test skipped. Read this test if you want to execute it" +if ! test -w / +then + skip_all="Test requiring writable / skipped. Read this test if you want to run it" + test_done +fi + +if test -e /refs || test -e /objects || test -e /info || test -e /hooks || + test -e /.git || test -e /foo || test -e /me +then + skip_all="Skip test that clobbers existing files in /" test_done fi
@@ -108,8 +116,9 @@ if [ "$IKNOWWHATIAMDOING" != "YES" ]; then test_done fi -if [ "$UID" = 0 ]; then - skip_all="No you can't run this with root" +if ! test_have_prereq NOT_ROOT +then + skip_all="No you can't run this as root" test_done fi
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 4 lines · Junio C Hamano
I think we will later make SANITY to require
NOT_ROOTand POSIXPERM, at which point many existing tests that require POSIXPERM,SANITY can be simplified to require only SANITY, but that will be a follow-up change to this fix.
And here is such a follow-up.
-- >8 --
Subject: [PATCH] tests: SANITY requires POSIXPERM
SANITY requires that a file or a directory is indeed accessible (or inaccessible) when its permission bits would say it ought to be accessible (or inaccessible). Running tests as root would lose this prerequisite for obvious reasons, and a test that requires SANITY implies it needs POSIXPERM working.
Redefine SANITY in terms of POSIXPERM and NOT_ROOT and simplify tests that require both to only require SANITY.
--- t/t0001-init.sh | 2 +- t/t0004-unwritable.sh | 8 ++++---- t/t0061-run-command.sh | 2 +- t/t0070-fundamental.sh | 2 +- t/t3700-add.sh | 10 +++++----- t/t4056-diff-order.sh | 2 +- t/t5537-fetch-shallow.sh | 2 +- t/t7508-status.sh | 2 +- t/test-lib.sh | 4 +++- 9 files changed, 18 insertions(+), 16 deletions(-)
t/t0001-init.sh +1 -1
@@ -261,7 +261,7 @@ test_expect_success 'init notices EEXIST (2)' ' test_path_is_file newdir/a ' -test_expect_success POSIXPERM,SANITY 'init notices EPERM' ' +test_expect_success SANITY 'init notices EPERM' ' rm -fr newdir && mkdir newdir && chmod -w newdir &&
t/t0004-unwritable.sh +4 -4
@@ -15,26 +15,26 @@ test_expect_success setup ' ' -test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable repository' ' +test_expect_success SANITY 'write-tree should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && test_must_fail git write-tree ' -test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' ' +test_expect_success SANITY 'commit should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && test_must_fail git commit -m second ' -test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' ' +test_expect_success SANITY 'update-index should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && echo 6O >file && chmod a-w .git/objects .git/objects/?? && test_must_fail git update-index file ' -test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' ' +test_expect_success SANITY 'add should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && echo b >file && chmod a-w .git/objects .git/objects/?? &&
t/t0061-run-command.sh +1 -1
@@ -34,7 +34,7 @@ test_expect_success POSIXPERM 'run_command reports EACCES' ' grep "fatal: cannot exec.*hello.sh" err ' -test_expect_success POSIXPERM,SANITY 'unreadable directory in PATH' ' +test_expect_success SANITY 'unreadable directory in PATH' ' mkdir local-command && test_when_finished "chmod u+rwx local-command && rm -fr local-command" && git config alias.nitfol "!echo frotz" &&
t/t0070-fundamental.sh +1 -1
@@ -17,7 +17,7 @@ test_expect_success 'mktemp to nonexistent directory prints filename' ' grep "doesnotexist/test" err ' -test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints filename' ' +test_expect_success SANITY 'mktemp to unwritable directory prints filename' ' mkdir cannotwrite && chmod -w cannotwrite && test_when_finished "chmod +w cannotwrite" &&
t/t3700-add.sh +5 -5
@@ -191,7 +191,7 @@ test_expect_success 'git add --refresh with pathspec' ' grep baz actual ' -test_expect_success POSIXPERM,SANITY 'git add should fail atomically upon an unreadable file' ' +test_expect_success SANITY 'git add should fail atomically upon an unreadable file' ' git reset --hard && date >foo1 && date >foo2 &&
@@ -202,7 +202,7 @@ test_expect_success POSIXPERM,SANITY 'git add should fail atomically upon an unr rm -f foo2 -test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' ' +test_expect_success SANITY 'git add --ignore-errors' ' git reset --hard && date >foo1 && date >foo2 &&
@@ -213,7 +213,7 @@ test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' ' rm -f foo2 -test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors)' ' +test_expect_success SANITY 'git add (add.ignore-errors)' ' git config add.ignore-errors 1 && git reset --hard && date >foo1 &&
@@ -224,7 +224,7 @@ test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors)' ' ' rm -f foo2 -test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors = false)' ' +test_expect_success SANITY 'git add (add.ignore-errors = false)' ' git config add.ignore-errors 0 && git reset --hard && date >foo1 &&
@@ -235,7 +235,7 @@ test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors = false)' ' ' rm -f foo2 -test_expect_success POSIXPERM,SANITY '--no-ignore-errors overrides config' ' +test_expect_success SANITY '--no-ignore-errors overrides config' ' git config add.ignore-errors 1 && git reset --hard && date >foo1 &&
t/t4056-diff-order.sh +1 -1
@@ -62,7 +62,7 @@ test_expect_success 'missing orderfile' ' test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD ' -test_expect_success POSIXPERM,SANITY 'unreadable orderfile' ' +test_expect_success SANITY 'unreadable orderfile' ' >unreadable_file && chmod -r unreadable_file && test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
t/t5537-fetch-shallow.sh +1 -1
@@ -173,7 +173,7 @@ EOF ) ' -test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' +test_expect_success 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" &&
t/t7508-status.sh +1 -1
@@ -1035,7 +1035,7 @@ EOF test_i18ncmp expect output ' -test_expect_success POSIXPERM,SANITY 'status succeeds in a read-only repository' ' +test_expect_success SANITY 'status succeeds in a read-only repository' ' ( chmod a-w .git && # make dir1/tracked stat-dirty
t/test-lib.sh +3 -1
@@ -999,7 +999,9 @@ 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. -test -w / || test_set_prereq SANITY +test_lazy_prereq SANITY ' + test_have_prereq POSIXPERM,NOT_ROOT +' GIT_UNZIP=${GIT_UNZIP:-unzip} test_lazy_prereq UNZIP '
Signature · 21 lines
2.3.0-rc0-149-g0286818 -- -- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Hm, being one day offline and there are lots of ideas and
new patches, I like that.
I run these test under msys and cygwin on latest pu (a3dc223ff234481356c):
./t0001-init.sh
./t0004-unwritable.sh
./t0061-run-command.sh
./t0070-fundamental.sh
./t1004-read-tree-m-u-wf.sh
./t1300-repo-config.sh
./t1301-shared-repo.sh
./t1308-config-set.sh
./t2026-prune-linked-checkouts.sh
./t3600-rm.sh
./t3700-add.sh
./t4039-diff-assume-unchanged.sh
./t4056-diff-order.sh
./t5537-fetch-shallow.sh
./t7300-clean.sh
./t7503-pre-commit-hook.sh
./t7504-commit-msg-hook.sh
./t7508-status.sh
(msys passes or skips all)
Without digging further, these fail on my cygwin:
$ grep "not ok" p.txt not ok 29 - init notices EPERM not ok 2 - write-tree should notice unwritable repository not ok 3 - commit should notice unwritable repository not ok 4 - update-index should notice unwritable repository not ok 5 - add should notice unwritable repository not ok 3 - mktemp to unwritable directory prints filename not ok 13 - funny symlink in work tree, un-unlink-able not ok 23 - proper error on non-accessible files not ok 4 - prune directories with unreadable gitdir not ok 15 - Test that "git rm -f" fails if its rm fails not ok 16 - When the rm in "git rm -f" fails, it should not remove the file from the index not ok 20 - Re-add foo and baz not ok 21 - Modify foo -- rm should refuse not ok 22 - Modified foo -- rm -f should work not ok 23 - Re-add foo and baz for HEAD tests not ok 24 - foo is different in index from HEAD -- rm should refuse not ok 23 - git add should fail atomically upon an unreadable file not ok 24 - git add --ignore-errors not ok 25 - git add (add.ignore-errors) not ok 26 - git add (add.ignore-errors = false) not ok 27 - --no-ignore-errors overrides config not ok 4 - unreadable orderfile not ok 28 - removal failure not ok 61 - status succeeds in a read-only repository
If we remove POSIXPERM from CYGWIN, all tests pass ;-) but some are skipped :
< ok 26 - init creates a new deep directory (umask vs. shared) < ok 3 - run_command reports EACCES
< ok 4 - unreadable directory in PATH
< ok 113 - preserves existing permissions
< ok 2 - shared=1 does not clear bits preset by umask 002 < ok 3 - shared=1 does not clear bits preset by umask 022 < ok 5 - update-server-info honors core.sharedRepository < ok 6 - shared = 0660 (r--r-----) ro
< ok 7 - shared = 0660 (rw-rw----) rw
< ok 8 - shared = 0640 (r--r-----) ro
< ok 9 - shared = 0640 (rw-r-----) rw
< ok 10 - shared = 0600 (r--------) ro
< ok 11 - shared = 0600 (rw-------) rw
< ok 12 - shared = 0666 (r--r--r--) ro
< ok 13 - shared = 0666 (rw-rw-rw-) rw
< ok 14 - shared = 0664 (r--r--r--) ro
< ok 15 - shared = 0664 (rw-rw-r--) rw
< ok 16 - info/refs respects umask in unshared repo < ok 17 - git reflog expire honors core.sharedRepository < ok 18 - forced modes
< ok 4 - find-copies-harder is not confused by mode bits < ok 10 - shallow fetch from a read-only repo
< ok 32 - git clean -d with an unreadable empty directory < ok 7 - with non-executable hook
< ok 8 - --no-verify with non-executable hook
< ok 13 - with non-executable hook
< ok 14 - with non-executable hook (editor)
< ok 15 - --no-verify with non-executable hook
< ok 16 - --no-verify with non-executable hook (editor)
I'm not sure what is the best way forward, it seems as if CYGIN is "half POSIX" now.
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 9 lines · Torsten Bögershausen
Hm, being one day offline and there are lots of ideas and
new patches, I like that.
I run these test under msys and cygwin on latest pu (a3dc223ff234481356c):
...
(msys passes or skips all)Without digging further, these fail on my cygwin:
...
I'm not sure what is the best way forward, it seems as if CYGIN is "half POSIX" now.
Are you reporting differences between the state before these patches and after, or just the fact that with these patches the named tests break (which may or may not be broken before the patches)?
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 4 lines · Junio C Hamano
Are you reporting differences between the state before these patches and after, or just the fact that with these patches the named tests break (which may or may not be broken before the patches)?
The intention was to report what is now breaking.
One example is this one:
---------------------
git.git/master:
ok 15 # skip Test that "git rm -f" fails if its rm fails (missing SANITY)
git.git/pu:
not ok 15 - Test that "git rm -f" fails if its rm fails #
# chmod a-w . && # test_must_fail git rm -f baz && # chmod 775 .
#
The next step could be to dig further:
If I run that sequence manually:
chmod 755 .
touch x
chmod a-w .
rm x
touch y
x is gone, (but shoudn't according to POSIX)
y is not created, "access denied"
--------------
I can see that there are 3 groups of OS/FS combinations:
Group 1:
File access bits are not maintained, and not obeyed.
Typical: VFAT, Git for Windows, (and some network protocols like SAMBA,
depending on the OS/FS involved and/or the mount options)
Typically core.filemode is false after "git init"
Group 2:
File access bits are maintained and obeyed:
POSIX/Unix/Linux/Mac OS and CYGWIN Typically core.filemode is true after "git init"
Group 3 :
File access bits are maintained, but not (fully) obeyed
running as root under Linux/Unix...
Or Windows, when a file is allowed to be deleted from a directory without write permissions.
-----------------
In short, the following seems to be an improvement:
t/test-lib.sh +11 -2
@@ -1039,7 +1039,17 @@ 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. test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir ds && + touch ds/x && + chmod -w ds && + if rm ds/x + then + chmod +w ds + false + else + chmod +w ds + true + fi ' --
Signature · 17 lines
*** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 9 lines · Torsten Bögershausen
If I run that sequence manually:
chmod 755 .
touch x
chmod a-w .
rm x
touch yx is gone, (but shoudn't according to POSIX)
y is not created, "access denied"
Good (or is that Sad?).
Quoted 10 lines
t/test-lib.sh +2
-1@@ -1039,7 +1039,17 @@ 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. test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir ds && + touch ds/x &&
t/test-lib.sh test_lazy_prereq NOT_ROOT ' · line 1053
Quoted 10 lines
t/test-lib.sh +9
-0+ chmod -w ds && + if rm ds/x + then + chmod +w ds + false + else + chmod +w ds + true + fi '
It looks like a better approach overall.
Because we cannot know where $(pwd) is when lazy prereq is evaluated (it typically is at the root of the trash hierarchy, but not always) and we would not want to add, leave or remove random files in the working tree that are not expected by the tests proper (e.g. a test that counts untracked paths are not expecting ds/ to be there), your actual "fix" may need to be a bit more careful, though.
Thanks.
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Torsten Bögershausen
If I run that sequence manually:
chmod 755 .
touch x
chmod a-w .
rm x
touch y
Quoted 4 lines
Quoted 2 lines
x is gone, (but shoudn't according to POSIX)
y is not created, "access denied"Good (or is that Sad?).
It feels that this is by design:
In old days under MS/DOS the only way to hinder people from deleting a file was to make it "read only" with help of the ATTRIB command.
https://en.wikipedia.org/wiki/ATTRIB
Later NTFS introduced the (ACL like) secutity information, and (unless I am completely wrong) the "delete file" is part of the "modify" permission, not write.
Quoted 24 lines
Quoted 11 lines
t/test-lib.sh +2
-1@@ -1039,7 +1039,17 @@ 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. test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir ds && + touch ds/x &&t/test-lib.sh test_lazy_prereq NOT_ROOT ' · line 1053
Quoted 10 lines
t/test-lib.sh +9
-0+ chmod -w ds && + if rm ds/x + then + chmod +w ds + false + else + chmod +w ds + true + fi 'It looks like a better approach overall.
Quoted 9 lines
Because we cannot know where $(pwd) is when lazy prereq is evaluated (it typically is at the root of the trash hierarchy, but not always) and we would not want to add, leave or remove random files in the working tree that are not expected by the tests proper (e.g. a test that counts untracked paths are not expecting ds/ to be there), your actual "fix" may need to be a bit more careful, though.
Thanks.
So true, what is a better place or way to run the test ? Can we use /tmp (Which may be a different file system)? Or can we use $HOME/$$ds (Which is an artificial HOME)
We already "pollute" the $PWD heretest_lazy_prereq CASE_INSENSITIVE_FS '
echo good >CamelCase &&
echo bad >camelcase && test "$(cat CamelCase)" != good
'
and here:test_lazy_prereq UTF8_NFD_TO_NFC '
....
Would
mkdir $HOME/ds
be a better approach then ?
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
So after discussing this one and queuing the resulting three-patch series jk/sanity that consists of the three patches:
* jk/sanity (2015-01-27) 3 commits
- test-lib.sh: set prerequisite SANITY by testing what we really need
- tests: correct misuses of POSIXPERM
- t/lib-httpd: switch SANITY check for NOT_ROOT
Waiting for ack or counter-proposal from Torsten. Otherwise looking good.
Do we want to proceed with these, or do we want any more work done on them?
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 8 lines · Junio C Hamano
So after discussing this one and queuing the resulting three-patch series jk/sanity that consists of the three patches:
* jk/sanity (2015-01-27) 3 commits
-test-lib.sh: set prerequisite SANITY by testing what we really need
- tests: correct misuses of POSIXPERM
- t/lib-httpd: switch SANITY check forNOT_ROOT
Quoted 6 lines
Waiting for ack or counter-proposal from Torsten. Otherwise looking good.
Do we want to proceed with these, or do we want any more work done on them?
I managed to run the tests with POSIXPERM and/or SANITY under Cygwin, Msysgit, Linux, root@linux, Mac and root@Mac. All passed.
The work to "be done", what I can see: please amend the commit message:
s/more exotic//
Re: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 2 lines · Torsten Bögershausen
The work to "be done", what I can see: please amend the commit message:
s/more exotic//
Thanks for reminding; I thought this was excised already but apparently hasn't (yet).
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
In reply to Junio C HamanoQuoted 37 lines · Junio C Hamano
Quoted 9 lines · Torsten Bögershausen
If I run that sequence manually:
chmod 755 .
touch x
chmod a-w .
rm x
touch yx is gone, (but shoudn't according to POSIX)
y is not created, "access denied"Good (or is that Sad?).
Quoted 10 lines
t/test-lib.sh +2
-1@@ -1039,7 +1039,17 @@ 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. test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir ds && + touch ds/x &&t/test-lib.sh test_lazy_prereq NOT_ROOT ' · line 1053
Quoted 10 lines
t/test-lib.sh +9
-0+ chmod -w ds && + if rm ds/x + then + chmod +w ds + false + else + chmod +w ds + true + fi 'It looks like a better approach overall.
Quoted 9 lines
Because we cannot know where $(pwd) is when lazy prereq is evaluated (it typically is at the root of the trash hierarchy, but not always) and we would not want to add, leave or remove random files in the working tree that are not expected by the tests proper (e.g. a test that counts untracked paths are not expecting ds/ to be there), your actual "fix" may need to be a bit more careful, though.
Thanks.
Hm,
I think there are 2 different possiblities to go further, either to always switch off SANITY for CYGWIN (or Windows in general). I haven't tested anything, the idea came up while writing this email.
The other way is to go away from the hard coded "we know we are root, so SANITY must be false, or we know that Windows is not 100% POSIX", and probe the OS/FS dynamically.
The following probably deserves the price for the most clumsy prerequisite ever written.
(Copy&Paste of a real patch into the mailer, not sure if it applies)
It has been tested under Mac OS, root@Mac OS, Cygwin / Msysgit What do you think ?
-- >8 --
Subject: [PATCH 1/2] test-lib.sh: Improve SANITY
SANITY was not set when running as root,
but this is not 100% reliable for CYGWIN:
A file is allowed to be deleted when the containing directory does not have write permissions.
--- t/test-lib.sh | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
t/test-lib.sh +19 -1
@@ -1038,8 +1038,26 @@ 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 + dsdir=$$ds + mkdir $dsdir && + touch $dsdir/x && + chmod -w $dsdir && + if rm $dsdir/x + then + chmod +w $dsdir + rm -rf $dsdir + echo >&2 SANITY=false + false + else + chmod +w $dsdir + rm -rf $dsdir + echo >&2 SANITY=true + true + fi ' GIT_UNZIP=${GIT_UNZIP:-unzip}
Signature · 9 lines
Subject: [PATCH 2/2] t2026 needs SANITY When running as root 'prune directories with unreadable gitdir' in t2026 fails. Protect this TC with SANITY Signed-off-by: Torsten Bögershausen <[email protected]> --- t/t2026-prune-linked-checkouts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t2026-prune-linked-checkouts.sh +1 -1
@@ -33,7 +33,7 @@ EOF ! test -d .git/worktrees ' -test_expect_success POSIXPERM 'prune directories with unreadable gitdir' ' +test_expect_success SANITY 'prune directories with unreadable gitdir' ' mkdir -p .git/worktrees/def/abc && : >.git/worktrees/def/def && : >.git/worktrees/def/gitdir &&
Re: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Quoted 1 line · Torsten Bögershausen
[...] either to always switch off SANITY for CYGWIN (or Windows in general).
Nice one! You gave me the chuckle for the day ;-)
Ciao,
Dscho
Re: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
In reply to Torsten BögershausenQuoted 2 lines · Torsten Bögershausen
It has been tested under Mac OS, root@Mac OS, Cygwin / Msysgit What do you think ?
Except that we may want to be more careful to detect errors from the initial mkdir and clean-up part (which should abort the test, not just declare !SANITY), I think the basic idea is sound.
test_dir=$TRASH_DIRECTORY/.sanity-test-dir
! mkdir "$test_dir" &&
>"$test_dir/x" &&
chmod -w "$test_dir" ||
error "bug in test sript: cannot prepare .sanity-test-dir"
rm "$test_dir/x"
status=$?
chmod +w "$test_dir" &&
rm -r "$test_dir" ||
error "bug in test sript: cannot clean .sanity-test-dir"
return $statusor something along that line?
Quoted 47 lines
-- >8 --
Subject: [PATCH 1/2]test-lib.sh: Improve SANITYSANITY was not set when running as root,
but this is not 100% reliable for CYGWIN:A file is allowed to be deleted when the containing directory does not have write permissions.
Signed-off-by Torsten Bögershausen <[email protected]>--- t/test-lib.sh | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)t/test-lib.sh +19
-1@@ -1038,8 +1038,26 @@ 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 + dsdir=$$ds + mkdir $dsdir && + touch $dsdir/x && + chmod -w $dsdir && + if rm $dsdir/x + then + chmod +w $dsdir + rm -rf $dsdir + echo >&2 SANITY=false + false + else + chmod +w $dsdir + rm -rf $dsdir + echo >&2 SANITY=true + true + fi ' GIT_UNZIP=${GIT_UNZIP:-unzip}
Signature · 18 lines
-- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "Git for Windows" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
Quoted 11 lines · Junio C Hamano
Quoted 9 lines · Jeff King
Exactly. I am happy to submit a patch, but I cannot think of any mechanisms besides:
1. Calling
id, which I suspect is very not portable.2. Writing a C program to check
getuid(). That's portable for most Unixes. It looks like we already have a hacky wrapper on mingw that will always return "1".
Quoted 7 lines
Quoted 1 line
Is (2) too gross?
Not overly gross compared to some existing test-*.c files, I would say.
I wondered what 'perl -e 'print $>' would say in mingw, and if that is portable enough, though.
$ perl -e 'print $>' 500
Quoted 14 lines · Erik Faye-Lund
Quoted 13 lines · Junio C Hamano
Quoted 10 lines · Jeff King
Exactly. I am happy to submit a patch, but I cannot think of any mechanisms besides:
1. Calling
id, which I suspect is very not portable.2. Writing a C program to check
getuid(). That's portable for most Unixes. It looks like we already have a hacky wrapper on mingw that will always return "1".Is (2) too gross?
Quoted 8 lines
Quoted 5 lines
Not overly gross compared to some existing test-*.c files, I would say.
I wondered what 'perl -e 'print $>' would say in mingw, and if that is portable enough, though.
$ perl -e 'print $>' 500
Thanks for a follow-up.
Is "id -u" not useful over there? I ask because that is what is used in the version tentatively queued on 'pu' for NOT_ROOT prerequisite (the jk/sanity topic).
The SANITY prerequisite in that topic needs to be replaced with the one from Torsten that attempts to check what we want to know in a more direct way; i.e. "after making a directory or a file read-only, does the filesystem really honours that, or lets us clobber?" is what we need to know to skip some tests, and we should check that, instead of "is / writable by us?" or "are we root?".
Quoted 22 lines · Junio C Hamano
Quoted 20 lines · Erik Faye-Lund
Quoted 18 lines · Junio C Hamano
Quoted 10 lines · Jeff King
Exactly. I am happy to submit a patch, but I cannot think of any mechanisms besides:
1. Calling
id, which I suspect is very not portable.2. Writing a C program to check
getuid(). That's portable for most Unixes. It looks like we already have a hacky wrapper on mingw that will always return "1".Is (2) too gross?
Not overly gross compared to some existing test-*.c files, I would say.
I wondered what 'perl -e 'print $>' would say in mingw, and if that is portable enough, though.
Quoted 8 lines
Quoted 2 lines
$ perl -e 'print $>' 500Thanks for a follow-up.
Is "id -u" not useful over there? I ask because that is what is used in the version tentatively queued on 'pu' for
NOT_ROOTprerequisite (the jk/sanity topic).
It's pretty much the same thing:
$ id -u 500
Quoted 6 lines
The SANITY prerequisite in that topic needs to be replaced with the one from Torsten that attempts to check what we want to know in a more direct way; i.e. "after making a directory or a file read-only, does the filesystem really honours that, or lets us clobber?" is what we need to know to skip some tests, and we should check that, instead of "is / writable by us?" or "are we root?".
$ test -w / && echo yes yes
$ mkdir foo && chmod a= foo $ test -w w && echo yes $ rm -r foo rm: directory `foo' is write protected; descend into it anyway? n $ rm -r foo < /dev/null $ ls -la foo ls: foo: No such file or directory $
So, Windows does only kind-of respect read-only flags. Dunno if this tells you something useful, though.