Torsten Bögershausent5539 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>
Junio C HamanoRe: t5539 broken under Mac OS X
Torsten Bögershausen <tboegi@web.de> writes:
> 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"
fiand 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 SANITYIt appears that the check in lib-httpd.sh thinks you lack SANITY; is the root directory of your system somehow writable by you?
> > > 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>
Torsten BögershausenRe: t5539 broken under Mac OS X
On 2015-01-14 19.37, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes: > >> 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? >
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)
Jeff KingRe: t5539 broken under Mac OS X
On Wed, Jan 14, 2015 at 08:50:47PM +0100, Torsten Bögershausen wrote:
> 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
Kyle J. McKayRe: t5539 broken under Mac OS X
On Jan 14, 2015, at 13:17, Jeff King wrote:
> On Wed, Jan 14, 2015 at 08:50:47PM +0100, Torsten Bögershausen wrote: > >> 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).
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
Junio C HamanoRe: t5539 broken under Mac OS X
"Kyle J. McKay" <mackyle@gmail.com> writes:
> 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(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh index bb1402d..cdafab5 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -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 '
Jeff KingRe: t5539 broken under Mac OS X
On Thu, Jan 15, 2015 at 12:29:39PM -0800, Junio C Hamano wrote:
> 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(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index bb1402d..cdafab5 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -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
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
Junio C HamanoRe: t5539 broken under Mac OS X
Jeff King <peff@peff.net> writes:
> 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.
> 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)?
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.
Jeff KingRe: t5539 broken under Mac OS X
On Thu, Jan 15, 2015 at 02:39:56PM -0800, Junio C Hamano wrote:
> 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.
> 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.
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
Junio C HamanoRe: t5539 broken under Mac OS X
Jeff King <peff@peff.net> writes:
> 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.
Jeff King[PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On Thu, Jan 15, 2015 at 04:04:24PM -0800, Junio C Hamano wrote:
> 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.
Signed-off-by: Jeff King <peff@peff.net> --- t/lib-httpd.sh | 2 +- t/test-lib.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index fd53b57..d154d1e 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index bb1402d..60020ca 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -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 +'
-- 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Kyle J. McKayRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On Jan 15, 2015, at 17:32, Jeff King wrote:
> On Thu, Jan 15, 2015 at 04:04:24PM -0800, Junio C Hamano wrote: > >> 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. >
[...]
> 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).
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).
> + > +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
Jeff KingRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On Thu, Jan 15, 2015 at 07:27:34PM -0800, Kyle J. McKay wrote:
> >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). > > 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).
> "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)?
> >+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.
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.
> 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
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Jeff KingRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On Thu, Jan 15, 2015 at 10:34:46PM -0500, Jeff King wrote:
> > "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.
Helped-by: Kyle J. McKay <mackyle@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- t/lib-httpd.sh | 2 +- t/test-lib.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index fd53b57..d154d1e 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index bb1402d..be50c77 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -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 +'
-- 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Junio C HamanoRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Jeff King <peff@peff.net> writes:
> 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....
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(-)
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 17e969d..9acf628 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -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" && diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh index 335420f..b6977d4 100755 --- a/t/t1509-root-worktree.sh +++ b/t/t1509-root-worktree.sh @@ -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
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Junio C HamanoRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Junio C Hamano <gitster@pobox.com> writes:
> 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.
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.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- 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(-)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh index e62c0ff..4aa8660 100755 --- a/t/t0001-init.sh +++ b/t/t0001-init.sh @@ -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 && diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh index e3137d6..d5729d4 100755 --- a/t/t0004-unwritable.sh +++ b/t/t0004-unwritable.sh @@ -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/?? && diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 9acf628..52722ee 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -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" && diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 5ed69a6..ccd88e2 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -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" && diff --git a/t/t3700-add.sh b/t/t3700-add.sh index fe274e2..2bc2bcc 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -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 && diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh index c0460bb..b7abfb2 100755 --- a/t/t4056-diff-order.sh +++ b/t/t4056-diff-order.sh @@ -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 diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index a980574..56bead1 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -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" && diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 8ed5788..6037415 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index b2b2ec7..37d1b0e 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -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 '
-- 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Torsten BögershausenRe: 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.
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Junio C HamanoRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Torsten Bögershausen <tboegi@web.de> writes:
> 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)?
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Torsten BögershausenRe: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On 2015-01-21 23.33, Junio C Hamano wrote: > 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:
diff --git a/t/test-lib.sh b/t/test-lib.sh --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -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 '
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Junio C HamanoRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Torsten Bögershausen <tboegi@web.de> writes:
> 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"
Good (or is that Sad?).
> diff --git a/t/test-lib.sh b/t/test-lib.sh > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -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 > '
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.
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Torsten BögershausenRe: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On 2015-01-22 23.07, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes: > >> 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" > > 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.
> >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -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 >> ' > > 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. >
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 here test_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 ?
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Junio C HamanoRe: [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?
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Torsten BögershausenRe: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On 2015-02-12 23.36, Junio C Hamano wrote:
> 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? >
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//
Junio C HamanoRe: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Torsten Bögershausen <tboegi@web.de> writes:
> 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).
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Torsten BögershausenRe: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On 2015-01-22 23.07, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes: > >> 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" > > Good (or is that Sad?). > >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -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 >> ' > > 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. >
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.
Signed-off-by: Torsten Bögershausen <tboegi@web.de> --- t/test-lib.sh | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh index 93f7cad..b8f736f 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -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}
-- 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 <tboegi@web.de> --- t/t2026-prune-linked-checkouts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh index 170aefe..2936d52 100755 --- a/t/t2026-prune-linked-checkouts.sh +++ b/t/t2026-prune-linked-checkouts.sh @@ -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 &&
Junio C HamanoRe: Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Torsten Bögershausen <tboegi@web.de> writes:
> 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 $status
or something along that line?
>
> -- >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.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> t/test-lib.sh | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 93f7cad..b8f736f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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}-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Johannes SchindelinRe: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On 2015-01-23 22:24, Torsten Bögershausen wrote:
> [...] either to always switch off SANITY for CYGWIN (or Windows in general).
Nice one! You gave me the chuckle for the day ;-)
Ciao, Dscho
Kyle J. McKayRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On Jan 16, 2015, at 01:16, Jeff King wrote:
> Subject: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
[...]
> 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.
[...]
> + > +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
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Kyle J. McKayRe: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On Jan 15, 2015, at 19:34, Jeff King wrote:
> On Thu, Jan 15, 2015 at 07:27:34PM -0800, Kyle J. McKay wrote: > >> "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
-- -- *** 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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Kyle J. McKay writes:
>>> "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.
> 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.
> 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).
> 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.
-- +<[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 msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com 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 msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Erik Faye-LundRe: t5539 broken under Mac OS X
On Fri, Jan 16, 2015 at 1:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes: > >> 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.
$ perl -e 'print $>' 500
Junio C HamanoRe: t5539 broken under Mac OS X
Erik Faye-Lund <kusmabite@gmail.com> writes:
> On Fri, Jan 16, 2015 at 1:04 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Jeff King <peff@peff.net> writes: >> >>> 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. > > $ 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?".
Erik Faye-LundRe: t5539 broken under Mac OS X
On Tue, Jan 27, 2015 at 3:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes: > >> On Fri, Jan 16, 2015 at 1:04 AM, Junio C Hamano <gitster@pobox.com> wrote: >>> Jeff King <peff@peff.net> writes: >>> >>>> 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. >> >> $ 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).
It's pretty much the same thing:
$ id -u 500
> 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.