{"thread":{"id":"23547","subject":"[PATCH 0/3] running tests as fakeroot","startedAt":"2010-04-21T13:12:55Z","lastAt":"2010-04-27T01:25:11Z","messageCount":8,"participants":["Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"140026","messageId":"20100421131255.GA2750@progeny.tock","threadId":"23547","inReplyTo":null,"subject":"[PATCH 0/3] running tests as fakeroot","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-04-21T13:12:55Z","receivedAt":"2010-04-21T13:12:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThe Debian build system makes it very tempting to build as\nfakeroot: basically, one has a choice of\n\n debian/rules build\n fakeroot debian/rules binary\n\nor just\n\n fakeroot debian/rules binary\n\nto build a .deb from the current sources. I hadn’t succumbed\nto temptation until today; but now that I have, I want to fix it.\nI’ll discuss whether this is a good idea more in patch 3.\n\nPatches 1 and 2 are a bit simpler. Patch 1 allows tests to have\nmultiple prerequisites, which seems like a good idea generally;\npatch 2 adds a missing POSIXPERM prerequisite to an existing test.\n\nI hope you like the patches. Sorry for the huge cc list; I did a\nquick search to find people interested one way or another, and\naparently there are many. I am sending patches 1 and 2 only to the\ngit list to avoid making the problem worse.\n\nThoughts?\nJonathan Nieder (3):\n test-lib: tests can have multiple prerequisites\n t1004 (read-tree): the unremovable symlink test requires POSIXPERM\n Permit tests to be run as a (fake) root user\n\n t/t1004-read-tree-m-u-wf.sh | 2 +-\n t/test-lib.sh | 22 ++++++++++++++--------\n 2 files changed, 15 insertions(+), 9 deletions(-)\n"},{"id":"140027","messageId":"20100421133806.GA5595@progeny.tock","threadId":"23547","inReplyTo":"20100421131255.GA2750@progeny.tock","subject":"[PATCH 1/3] test-lib: tests can have multiple prerequisites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-04-21T13:38:06Z","receivedAt":"2010-04-21T13:38:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Treat the prereq argument to test_expect_success and\ntest_expect_failure as a space-separated list of prerequisites.\n\nSigned-off-by: Jonathan Nieder \n---\nAlas, I didn’t notice the thread with the almost identical patch [1]\nbefore writing this one.\n\nSince I am painting it, I prefer this way. I find it intuitive and\ncan’t really see where the fuss about using some other coding style\ncame from. So I would be very happy if someone who does understand\nthe fuss could come up with a comment to add to the commit message\nabout this. :)\n\n t/test-lib.sh | 17 ++++++++++-------\n 1 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c582964..a53b6cf 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -315,12 +315,15 @@ test_set_prereq () {\n satisfied=\" \"\n \n test_have_prereq () {\n-\tcase $satisfied in\n-\t*\" $1 \"*)\n-\t\t: yes, have it ;;\n-\t*)\n-\t\t! : nope ;;\n-\tesac\n+\tfor prereq_to_test\n+\tdo\n+\t\tcase $satisfied in\n+\t\t*\" $prereq_to_test \"*)\n+\t\t\t: yes, have it ;;\n+\t\t*)\n+\t\t\treturn 1 ;;\n+\t\tesac\n+\tdone\n }\n \n # You are not expected to call test_ok_ and test_failure_ directly, use\n@@ -370,7 +373,7 @@ test_skip () {\n \t\tesac\n \tdone\n \tif test -z \"$to_skip\" && test -n \"$prereq\" &&\n-\t ! test_have_prereq \"$prereq\"\n+\t ! test_have_prereq $prereq\n \tthen\n \t\tto_skip=t\n \tfi\n-- \n1.7.1.rc1\n"},{"id":"140028","messageId":"20100421134457.GB5595@progeny.tock","threadId":"23547","inReplyTo":"20100421131255.GA2750@progeny.tock","subject":"[PATCH 2/3] t1004 (read-tree): the unremovable symlink test requires POSIXPERM","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-04-21T13:44:57Z","receivedAt":"2010-04-21T13:44:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Now that it is possible to express this, declare the test from\n8a785dc (Add tests to catch problems with un-unlinkable symlinks,\n2008-03-18) to require both POSIXPERM and SYMLINKS.\n\nThis test would fail on Windows on filesystems that do support\nsymbolic links and in other situations where symlinks are usable but\nUnix-style permissions are not.\n\nSigned-off-by: Jonathan Nieder \n---\nIt seems that an almost identical patch was submitted while the\nprerequisite mechanism was in its infancy.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385\n\n t/t1004-read-tree-m-u-wf.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh\nindex f19b4a2..62854a7 100755\n--- a/t/t1004-read-tree-m-u-wf.sh\n+++ b/t/t1004-read-tree-m-u-wf.sh\n@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '\n \n '\n \n-test_expect_success SYMLINKS 'funny symlink in work tree, un-unlink-able' '\n+test_expect_success 'POSIXPERM SYMLINKS' 'funny symlink in work tree, un-unlink-able' '\n \n \trm -fr a b &&\n \tgit reset --hard &&\n-- \n1.7.1.rc1\n"},{"id":"140031","messageId":"20100421142058.GC5595@progeny.tock","threadId":"23547","inReplyTo":"20100421131255.GA2750@progeny.tock","subject":"[PATCH 3/3] Permit tests to be run as a (fake) root user","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-04-21T14:20:59Z","receivedAt":"2010-04-21T14:20:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Skip tests that require sane behavior wrt permissions if the current\nuid is 0.\n\nSo now if you run\n\n fakeroot sh -c '\n\tmake all test install install-doc prefix=/usr/local \\\n\t\tDESTDIR=wherever >packaged-git.log 2>&1 &&\n\tcd wherever &&\n\ttar -cf - . |\n\tgzip\n ' >packaged-git.tar.gz\n\nthen this should successfully build a binary tarball for git, running\nsome tests while at it.\n\nSigned-off-by: Jonathan Nieder \n---\n Goals\n -----\n Before the POSIXPERM mechanism existed (which makes this change too\n easy to avoid considering), I think many people wanted to discourage\n running tests as root to avoid hosing people’s machines. Which makes\n some sense.\n \n But it is possible and may even be valuable to run tests in a\n sandbox. [1] [8] If that were the only reason to run tests as root, I\n would suggest requiring some particular environment variable to be\n set as a safety.\n \n In Debian, I think it might be policy to allow building as “root”. In\n this context, that almost always happens through some sort of ptrace or\n LD_PRELOAD hack like fakeroot. If a user knows to, it is easy to fall\n back to building as an unprivileged user, but really I think a user\n should not have to know to. [2] [3] If some non-POSIXPERM test fails\n when run as (fake) root, that is something that would be nice to\n know. [4]\n \n Which prerequisite?\n -------------------\n It is possible to use autoconf-style tests to check for each\n capability we would like to lack. [5] Certainly, POSIXPERM does not\n actually describe the exact set of abilities one gains by running as\n an unprivileged user. [6] But unsetting POSIXPERM does successfully\n convey a simple truth: if you run tests as root, you have lost the ability\n to pay attention to permissions and keep your sanity. [7]\n \n [1] http://thread.gmane.org/gmane.comp.version-control.git/86885/focus=87664\n [2] http://thread.gmane.org/gmane.comp.version-control.git/140128\n [3] http://thread.gmane.org/gmane.comp.version-control.git/17904/focus=17910\n [4] http://thread.gmane.org/gmane.comp.version-control.git/75561/focus=75568\n [5] http://thread.gmane.org/gmane.comp.version-control.git/18667\n [6] http://thread.gmane.org/gmane.comp.version-control.git/121502/focus=121531\n [7] http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385\n [8] http://thread.gmane.org/gmane.comp.version-control.git/52654/focus=52689\n Holy cow, that Solaris bug is still not fixed [9]. Maybe it\n would be worthwhile to make an unlink() wrapper in compat/ to\n avoid new uses leaving users on such operating systems in danger.\n [9] http://bugs.opensolaris.org/view_bug.do?bug_id=6581318\n\nThanks for reading. I hope the story was not too dull.\n\n t/test-lib.sh | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex a53b6cf..0f51757 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -782,7 +782,10 @@ case $(uname -s) in\n \t# exec does not inherit the PID\n \t;;\n *)\n-\ttest_set_prereq POSIXPERM\n+\tif test $(id -u) != 0\n+\tthen\n+\t\ttest_set_prereq POSIXPERM\n+\tfi\n \ttest_set_prereq BSLASHPSPEC\n \ttest_set_prereq EXECKEEPSPID\n \t;;\n-- \n1.7.1.rc1\n"},{"id":"140036","messageId":"20100421145721.GA8726@progeny.tock","threadId":"23547","inReplyTo":"20100421133806.GA5595@progeny.tock","subject":"Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-04-21T14:57:21Z","receivedAt":"2010-04-21T14:57:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Treat the prereq argument to test_expect_success and\n> test_expect_failure as a space-separated list of prerequisites.\n> \n> Signed-off-by: Jonathan Nieder \n> ---\n> Alas, I didn’t notice the thread with the almost identical patch [1]\n> before writing this one.\n> \n> Since I am painting it, I prefer this way. I find it intuitive and\n> can’t really see where the fuss about using some other coding style\n> came from.\n\nWhoops, forgot to link. Maybe this would be a good entry point:\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118595\n\nSorry about that.\nJonathan\n"},{"id":"140448","messageId":"20100426191703.GC10772@coredump.intra.peff.net","threadId":"23547","inReplyTo":"20100421133806.GA5595@progeny.tock","subject":"Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-26T19:17:03Z","receivedAt":"2010-04-26T19:17:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 21, 2010 at 08:38:06AM -0500, Jonathan Nieder wrote:\n\n> Alas, I didn’t notice the thread with the almost identical patch [1]\n> before writing this one.\n> \n> Since I am painting it, I prefer this way. I find it intuitive and\n> can’t really see where the fuss about using some other coding style\n> came from. So I would be very happy if someone who does understand\n> the fuss could come up with a comment to add to the commit message\n> about this. :)\n\nI liked commas better, but since you are doing the work of writing the\npatches, I will let it go. :)\n\nI do wonder if it might be less error-prone to have:\n\n test_expect_success PREREQ1 PREREQ2 'desc' 'test'\n\ninstead of\n\n test_expect_success 'PREREQ1 PREREQ2' 'desc' 'test'\n\nwhich should just be something like\n\n while test $# -gt 2; do\n test_have_prereq \"$1\" || skip=t\n shift\n done\n\nBut if you think that's dumb, just ignore it. It is really not worth\nspending that much time on.\n\n-Peff\n"},{"id":"140463","messageId":"7vpr1liwrk.fsf@alter.siamese.dyndns.org","threadId":"23547","inReplyTo":"20100426191703.GC10772@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-27T00:06:55Z","receivedAt":"2010-04-27T00:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do wonder if it might be less error-prone to have:\n>\n> test_expect_success PREREQ1 PREREQ2 'desc' 'test'\n>\n> instead of\n>\n> test_expect_success 'PREREQ1 PREREQ2' 'desc' 'test'\n\nSpot what is wrong with this:\n\n test_expect_success make sure we have the repo '\n test -d .git\n '\n\nI think \"we may or may not have a single prereq word\" would be less error\nprone. (you could make sure \"test_have_prereq\" gets no lowercase letter\nto mitigate the issue, though).\n\nI tend to like the comma myself, but quoted, space separated list is fine\nby me.\n"},{"id":"140465","messageId":"20100427012511.GA1844@coredump.intra.peff.net","threadId":"23547","inReplyTo":"7vpr1liwrk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-27T01:25:11Z","receivedAt":"2010-04-27T01:25:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 26, 2010 at 05:06:55PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I do wonder if it might be less error-prone to have:\n> >\n> > test_expect_success PREREQ1 PREREQ2 'desc' 'test'\n> >\n> > instead of\n> >\n> > test_expect_success 'PREREQ1 PREREQ2' 'desc' 'test'\n> \n> Spot what is wrong with this:\n> \n> test_expect_success make sure we have the repo '\n> test -d .git\n> '\n\nTo me it's easy to spot, but there is nothing wrong with it that the\nshell will spot, so that is a downside. Whatever we go with, it would\nperhaps be less error prone to set \"NOPREREQ1\" when we don't have\nPREREQ1, and check that either $1 or NO$1 is set when checking\nprerequisites. But that is probably just over-engineering. This is not\nan error we have seen a lot of, and it is not that hard to spot with\ncode review.\n\n-Peff\n"}]}