{"thread":{"id":"38461","subject":"[PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","startedAt":"2015-01-27T15:39:01Z","lastAt":"2015-01-28T20:38:52Z","messageCount":7,"participants":["Torsten Bögershausen","Chris Packham","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"255329","messageId":"54C7B115.7020405@web.de","threadId":"38461","inReplyTo":null,"subject":"[PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-01-27T15:39:01Z","receivedAt":"2015-01-27T15:39:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"The SANITY precondition was not set when running as root,\nbut this is not 100% reliable for CYGWIN:\n\nA file may be allowed to be deleted when the containing\ndirectory does not have write permissions.\n\nSee\nhttps://technet.microsoft.com/en-us/library/bb463216.aspx\n\"...In UNIX, the write permission bit on a directory permits both\nthe creation and removal of new files or sub-directories in the directory.\nOn Windows, the Write_Data access right controls the creation of new\nsub-files and the Delete_Child access right controls the deletion. ....\"\n\nWe may argue that the translation of the POSIX write permission bit\ninto \"Windows access rights\" can be improved in CYGWIN.\n\nA dynamic test handles all cases: when the sequence\n$ mkdir SANETESTD &&\n$ chmod +w SANETESTD &&\n$ >SANETESTD/x &&\n$ ! rm SANETESTD/x\nsucceeds the prerequisite SANITY is true.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Torsten Bögershausen \n---\n t/test-lib.sh | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 93f7cad..887e986 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1038,8 +1038,23 @@ test_lazy_prereq NOT_ROOT '\n \n # When the tests are run as root, permission tests will report that\n # things are writable when they shouldn't be.\n+# Special check for CYGWIN (or Windows in general):\n+# A file can be deleted, even if the containing directory does'nt\n+# have write permissions\n test_lazy_prereq SANITY '\n-\ttest_have_prereq POSIXPERM,NOT_ROOT\n+\tmkdir SANETESTD &&\n+\tchmod +w SANETESTD &&\n+\t>SANETESTD/x &&\n+\tchmod -w SANETESTD ||\n+\terror \"bug in test sript: cannot prepare SANETESTD\"\n+\n+\t! rm SANETESTD/x\n+\tstatus=$?\n+\n+\tchmod +w SANETESTD &&\n+\trm -rf SANETESTD ||\n+\terror \"bug in test sript: cannot clean SANETESTD\"\n+\treturn $status\n '\n \n GIT_UNZIP=${GIT_UNZIP:-unzip}\n-- \n2.2.0.rc1.26.g3e3a70d\n"},{"id":"255342","messageId":"CAFOYHZDcvBvF8rgyax_4xMc88sZJFTVX-fXwV1iZa8veTaFixQ@mail.gmail.com","threadId":"38461","inReplyTo":"54C7B115.7020405@web.de","subject":"Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2015-01-27T19:53:46Z","receivedAt":"2015-01-27T19:53:46Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Minor typo in a comment.\n\nOn Wed, Jan 28, 2015 at 4:39 AM, Torsten Bögershausen wrote:\n> The SANITY precondition was not set when running as root,\n> but this is not 100% reliable for CYGWIN:\n>\n> A file may be allowed to be deleted when the containing\n> directory does not have write permissions.\n>\n> See\n> https://technet.microsoft.com/en-us/library/bb463216.aspx\n> \"...In UNIX, the write permission bit on a directory permits both\n> the creation and removal of new files or sub-directories in the directory.\n> On Windows, the Write_Data access right controls the creation of new\n> sub-files and the Delete_Child access right controls the deletion. ....\"\n>\n> We may argue that the translation of the POSIX write permission bit\n> into \"Windows access rights\" can be improved in CYGWIN.\n>\n> A dynamic test handles all cases: when the sequence\n> $ mkdir SANETESTD &&\n> $ chmod +w SANETESTD &&\n> $ >SANETESTD/x &&\n> $ ! rm SANETESTD/x\n> succeeds the prerequisite SANITY is true.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Torsten Bögershausen \n> ---\n> t/test-lib.sh | 17 ++++++++++++++++-\n> 1 file changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 93f7cad..887e986 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1038,8 +1038,23 @@ test_lazy_prereq NOT_ROOT '\n>\n> # When the tests are run as root, permission tests will report that\n> # things are writable when they shouldn't be.\n> +# Special check for CYGWIN (or Windows in general):\n> +# A file can be deleted, even if the containing directory does'nt\n\ns/does'nt/doesn't/\n\n> +# have write permissions\n> test_lazy_prereq SANITY '\n> - test_have_prereq POSIXPERM,NOT_ROOT\n> + mkdir SANETESTD &&\n> + chmod +w SANETESTD &&\n> + >SANETESTD/x &&\n> + chmod -w SANETESTD ||\n> + error \"bug in test sript: cannot prepare SANETESTD\"\n> +\n> + ! rm SANETESTD/x\n> + status=$?\n> +\n> + chmod +w SANETESTD &&\n> + rm -rf SANETESTD ||\n> + error \"bug in test sript: cannot clean SANETESTD\"\n> + return $status\n> '\n>\n> GIT_UNZIP=${GIT_UNZIP:-unzip}\n> --\n> 2.2.0.rc1.26.g3e3a70d\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"255351","messageId":"xmqqh9vbkgrg.fsf@gitster.dls.corp.google.com","threadId":"38461","inReplyTo":"54C7B115.7020405@web.de","subject":"Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T22:20:19Z","receivedAt":"2015-01-27T22:20:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> # When the tests are run as root, permission tests will report that\n> # things are writable when they shouldn't be.\n\nThis no longer is relevant, I think.\n\n> +# Special check for CYGWIN (or Windows in general):\n\nMisleading comment in the end result, as your new test drops SANITY\ncorrectly on POSIX for the root user, too. In a commit log message\nit is correct to say \"This adds special check for Cygwin\", but the\nresulting code is sensible with or without Cygwin, I would think,\nwith the justification to \"test by checking what we really want, not\nby inferring from the result of indirectly testing something else\".\n\n> +# A file can be deleted, even if the containing directory does'nt\n> +# have write permissions\n\nWe also rely on SANITY to make sure that \"chmod -rx directory\" makes\n\"directory/file\" undiscoverable.\n\nHow about extending it like this (not tested)?\n\n-- >8 --\nFrom: Torsten Bögershausen \nDate: Tue, 27 Jan 2015 16:39:01 +0100\nSubject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need\n\nWhat we wanted out of the SANITY precondition is that the filesystem\nbehaves sensibly with permission bits settings.\n\n - You should not be able to remove a file in a read-only directory,\n\n - You should not be able to tell if a file in a directory exists if\n the directory lacks read or execute permission bits.\n\nWe used to cheat by approximating that condition with \"is the /\nwritable?\" test and/or \"are we running as root?\" test. Neither test\nis sufficient or appropriate in more exotic environments like\nCygwin.\n\nSigned-off-by: Torsten Bögershausen \nSigned-off-by: Junio C Hamano \n---\n t/test-lib.sh | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b2b2ec7..446d8d5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -997,9 +997,28 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n-# When the tests are run as root, permission tests will report that\n-# things are writable when they shouldn't be.\n-test -w / || test_set_prereq SANITY\n+# On a filesystem that lacks SANITY, a file can be deleted even if\n+# the containing directory doesn't have write permissions, or a file\n+# can be accessed even if the containing directory doesn't have read\n+# or execute permissions, causing our tests that validate that Git\n+# works sensibly in such situations.\n+test_lazy_prereq SANITY '\n+\tmkdir SANETESTD.1 SANETESTD.2 &&\n+\n+\tchmod +w SANETESTD.1 SANETESTD.2 &&\n+\t>SANETESTD.1/x 2>SANETESTD.2/x &&\n+\tchmod -w SANETESTD.1 &&\n+\tchmod -rx SANETESTD.2 ||\n+\terror \"bug in test sript: cannot prepare SANETESTD\"\n+\n+\t! rm SANETESTD.1/x && ! test -f SANETESTD.2/x\n+\tstatus=$?\n+\n+\tchmod +rwx SANETESTD.1 SANETESTD.2 &&\n+\trm -rf SANETESTD.1 SANETESTD.2 ||\n+\terror \"bug in test sript: cannot clean SANETESTD\"\n+\treturn $status\n+'\n \n GIT_UNZIP=${GIT_UNZIP:-unzip}\n test_lazy_prereq UNZIP '\n-- \n2.3.0-rc1-180-g1a69fe5\n"},{"id":"255390","messageId":"54C89DC8.1050805@web.de","threadId":"38461","inReplyTo":"xmqqh9vbkgrg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-01-28T08:28:56Z","receivedAt":"2015-01-28T08:28:56Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 27.01.15 23:20, Junio C Hamano wrote:\n\n> How about extending it like this (not tested)?\nThanks, this looks good: the test is more extensive,\nI can test this next week.\n\n> \n> -- >8 --\n> From: Torsten Bögershausen \n> Date: Tue, 27 Jan 2015 16:39:01 +0100\n> Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need\n> \n> What we wanted out of the SANITY precondition is that the filesystem\n> behaves sensibly with permission bits settings.\n> \n> - You should not be able to remove a file in a read-only directory,\n> \n> - You should not be able to tell if a file in a directory exists if\n> the directory lacks read or execute permission bits.\n> \n> We used to cheat by approximating that condition with \"is the /\n> writable?\" test and/or \"are we running as root?\" test. Neither test\n> is sufficient or appropriate in more exotic environments like\n> Cygwin.\nHow about going this direction:\n\nWe used to cheat by approximating that condition with \"is the /\nwritable?\" test and/or \"are we running as root?\" test. Neither test\nis sufficient or appropriate, especially in environments like\nCygwin, Mingw or Mac OS X.\n"},{"id":"255369","messageId":"CAPc5daVAGOnu3pe_r=GuDXp1kNkJgbCqP-qD38kXmdEJa3ZSFQ@mail.gmail.com","threadId":"38461","inReplyTo":"54C89DC8.1050805@web.de","subject":"Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-28T17:38:19Z","receivedAt":"2015-01-28T17:38:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen wrote:\n> On 27.01.15 23:20, Junio C Hamano wrote:\n>\n>> How about extending it like this (not tested)?\n> Thanks, this looks good: the test is more extensive,\n> I can test this next week.\n>\n>>\n>> -- >8 --\n>> From: Torsten Bögershausen \n>> Date: Tue, 27 Jan 2015 16:39:01 +0100\n>> Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need\n>>\n>> What we wanted out of the SANITY precondition is that the filesystem\n>> behaves sensibly with permission bits settings.\n>>\n>> - You should not be able to remove a file in a read-only directory,\n>>\n>> - You should not be able to tell if a file in a directory exists if\n>> the directory lacks read or execute permission bits.\n\nForgot one thing. I do not offhand know if tests that needs SANITY\ndepends on this, but we may also want to check for this:\n\n - You should not be able to write to a file that is marked as read-only.\n\nby adding something like\n\n >sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && !\ntest -s sanitytest\"\n\nin the mix.\n\n>>\n>> We used to cheat by approximating that condition with \"is the /\n>> writable?\" test and/or \"are we running as root?\" test. Neither test\n>> is sufficient or appropriate in more exotic environments like\n>> Cygwin.\n> How about going this direction:\n>\n> We used to cheat by approximating that condition with \"is the /\n> writable?\" test and/or \"are we running as root?\" test. Neither test\n> is sufficient or appropriate, especially in environments like\n> Cygwin, Mingw or Mac OS X.\n\nOK, but MacOS X does not have SANITY problem; \"is the / writable?\" test\nwas misdetecting and declaring a system with SANITY does not have one.\n\nPerhaps roll Cygwin and Mingw into a single Windows category? I dunno.\n"},{"id":"255365","messageId":"54C9362E.2000705@web.de","threadId":"38461","inReplyTo":"CAPc5daVAGOnu3pe_r=GuDXp1kNkJgbCqP-qD38kXmdEJa3ZSFQ@mail.gmail.com","subject":"Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-01-28T19:19:10Z","receivedAt":"2015-01-28T19:19:10Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\nOn 28.01.15 18:38, Junio C Hamano wrote:\n> On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen wrote:\n>> On 27.01.15 23:20, Junio C Hamano wrote:\n>>\n>>> How about extending it like this (not tested)?\n>> Thanks, this looks good: the test is more extensive,\n>> I can test this next week.\n>>\n>>> -- >8 --\n>>> From: Torsten Bögershausen \n>>> Date: Tue, 27 Jan 2015 16:39:01 +0100\n>>> Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need\n>>>\n>>> What we wanted out of the SANITY precondition is that the filesystem\n>>> behaves sensibly with permission bits settings.\n>>>\n>>> - You should not be able to remove a file in a read-only directory,\n>>>\n>>> - You should not be able to tell if a file in a directory exists if\n>>> the directory lacks read or execute permission bits.\n> Forgot one thing. I do not offhand know if tests that needs SANITY\n> depends on this, but we may also want to check for this:\n>\n> - You should not be able to write to a file that is marked as read-only.\n>\n> by adding something like\n>\n> >sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && !\n> test -s sanitytest\"\n>\n> in the mix.\n>\n>>> We used to cheat by approximating that condition with \"is the /\n>>> writable?\" test and/or \"are we running as root?\" test. Neither test\n>>> is sufficient or appropriate in more exotic environments like\n>>> Cygwin.\n>> How about going this direction:\n>>\n>> We used to cheat by approximating that condition with \"is the /\n>> writable?\" test and/or \"are we running as root?\" test. Neither test\n>> is sufficient or appropriate, especially in environments like\n>> Cygwin, Mingw or Mac OS X.\n> OK, but MacOS X does not have SANITY problem; \"is the / writable?\" test\n> was misdetecting and declaring a system with SANITY does not have one.\n>\n> Perhaps roll Cygwin and Mingw into a single Windows category? I dunno.\nThe whole discussion actually started with Mac OS X,\nand the conclusion was that Mac OS X should have SANITY set, but hadn't,\nbecause / is writable (if you install from scratch):\n\n$gmane/262389\nand especially:\n$gmane/262456\n\nThe whole discussion ended up a fix for t5539, and, as a different improvement,\nthe lazy SANITY probing - which works for me on all systems I had the chance to test it.\n\nThe code is OK (we can add more tests, as you suggested).\nThe only problem I can see is to put everything into a good commit-msg.\n"},{"id":"255371","messageId":"xmqq7fw6hc83.fsf@gitster.dls.corp.google.com","threadId":"38461","inReplyTo":"54C9362E.2000705@web.de","subject":"Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-28T20:38:52Z","receivedAt":"2015-01-28T20:38:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> OK, but MacOS X does not have SANITY problem; \"is the / writable?\" test\n>> was misdetecting and declaring a system with SANITY does not have one.\n>>\n>> Perhaps roll Cygwin and Mingw into a single Windows category? I dunno.\n> The whole discussion actually started with Mac OS X,\n> and the conclusion was that Mac OS X should have SANITY set, but hadn't,\n> because / is writable (if you install from scratch):\n\nExactly. \n\nOn MacOSX running as non-root-but-can-write-to-slash-user, we should\nsay SANITY is satisfied, but \"is the / writable?\" check was\nmisdetecting and declaring the system does not have SANITY.\n\nSo we are in agreement, no?\n"}]}