{"thread":{"id":"15903","subject":"[PATCH] t1301-shared-repo.sh: don't let a default ACL interfere with the test","startedAt":"2008-10-14T22:07:00Z","lastAt":"2008-10-17T04:33:14Z","messageCount":17,"participants":["Matt McCutchen","Junio C Hamano","Deskin Miller","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93036","messageId":"1224022020.2699.4.camel@mattlaptop2.local","threadId":"15903","inReplyTo":null,"subject":"[PATCH] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-14T22:07:00Z","receivedAt":"2008-10-14T22:07:00Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"This test creates files with several different umasks and expects the files to\nbe permissioned according to the umasks, so a default ACL on the test dir causes\nthe test to fail. To avoid that, remove the default ACL if possible with\nsetfacl(1). (Will work on many systems.)\n---\n t/t1301-shared-repo.sh | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex dc85e8b..2275caa 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n \n . ./test-lib.sh\n \n+# Remove a default ACL from the test dir if possible.\n+setfacl -k . 2>/dev/null\n+\n # User must have read permissions to the repo -> failure on --shared=0400\n test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \tmkdir sub && (\n-- \n1.6.0.2.530.gb503b\n"},{"id":"93037","messageId":"1224022216.2699.5.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"1224022020.2699.4.camel@mattlaptop2.local","subject":"[PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-14T22:10:16Z","receivedAt":"2008-10-14T22:10:16Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"This test creates files with several different umasks and expects the files to\nbe permissioned according to the umasks, so a default ACL on the test dir causes\nthe test to fail. To avoid that, remove the default ACL if possible with\nsetfacl(1). (Will work on many systems.)\n\nSigned-off-by: Matt McCutchen \n---\nThis time with a signoff.\n\n t/t1301-shared-repo.sh | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex dc85e8b..2275caa 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n \n . ./test-lib.sh\n \n+# Remove a default ACL from the test dir if possible.\n+setfacl -k . 2>/dev/null\n+\n # User must have read permissions to the repo -> failure on --shared=0400\n test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \tmkdir sub && (\n-- \n1.6.0.2.530.gb503b\n"},{"id":"93039","messageId":"7vzll66c5u.fsf@gitster.siamese.dyndns.org","threadId":"15903","inReplyTo":"1224022216.2699.5.camel@mattlaptop2.local","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-14T22:32:13Z","receivedAt":"2008-10-14T22:32:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McCutchen writes:\n\n> This test creates files with several different umasks and expects the files to\n> be permissioned according to the umasks, so a default ACL on the test dir causes\n\nIs \"to permission\" a verb?\n\n> the test to fail. To avoid that, remove the default ACL if possible with\n> setfacl(1). (Will work on many systems.)\n\nIt is not clear in the comment in parentheses what provision you have made\nnot to harm people on systems without setfacl.\n\nI think \"if possible\" which you already have is a good enough description\n(i.e. \"if setfacl fails we do not barf and if you do not have the command\nyou probably are not running with a funky default ACL to see this issue\nanyway\"), so I'd rather drop the comment in parentheses.\n\n> Signed-off-by: Matt McCutchen \n> ---\n> This time with a signoff.\n>\n> t/t1301-shared-repo.sh | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\n> index dc85e8b..2275caa 100755\n> --- a/t/t1301-shared-repo.sh\n> +++ b/t/t1301-shared-repo.sh\n> @@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n> \n> . ./test-lib.sh\n> \n> +# Remove a default ACL from the test dir if possible.\n> +setfacl -k . 2>/dev/null\n> +\n\nMakes me wonder why this is _not_ inside test-lib.sh where it creates the\ntest (trash) directory. That way, you would cover future tests that wants\nto see a saner/simpler POSIX permission behaviour, wouldn't you?\n"},{"id":"93041","messageId":"1224025227.2699.13.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"7vzll66c5u.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-14T23:00:27Z","receivedAt":"2008-10-14T23:00:27Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Tue, 2008-10-14 at 15:32 -0700, Junio C Hamano wrote:\n> Matt McCutchen writes:\n> > This test creates files with several different umasks and expects the files to\n> > be permissioned according to the umasks, so a default ACL on the test dir causes\n> \n> Is \"to permission\" a verb?\n\nI thought it could be, but I'll reword that sentence.\n\n> > the test to fail. To avoid that, remove the default ACL if possible with\n> > setfacl(1). (Will work on many systems.)\n> \n> It is not clear in the comment in parentheses what provision you have made\n> not to harm people on systems without setfacl.\n> \n> I think \"if possible\" which you already have is a good enough description\n> (i.e. \"if setfacl fails we do not barf and if you do not have the command\n> you probably are not running with a funky default ACL to see this issue\n> anyway\"), so I'd rather drop the comment in parentheses.\n\nSure.\n\n> > Signed-off-by: Matt McCutchen \n> > ---\n> > This time with a signoff.\n> >\n> > t/t1301-shared-repo.sh | 3 +++\n> > 1 files changed, 3 insertions(+), 0 deletions(-)\n> >\n> > diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\n> > index dc85e8b..2275caa 100755\n> > --- a/t/t1301-shared-repo.sh\n> > +++ b/t/t1301-shared-repo.sh\n> > @@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n> > \n> > . ./test-lib.sh\n> > \n> > +# Remove a default ACL from the test dir if possible.\n> > +setfacl -k . 2>/dev/null\n> > +\n> \n> Makes me wonder why this is _not_ inside test-lib.sh where it creates the\n> test (trash) directory. That way, you would cover future tests that wants\n> to see a saner/simpler POSIX permission behaviour, wouldn't you?\n\nYes. However, I don't anticipate there being any tests specifically\nabout file permissions other than t1301-shared-repo.sh, and if the user\nhas set a default ACL on the git source tree, we might want to let trash\ndirectories obey that setting except in the one case where it breaks the\ntest. What do you think?\n\nMatt\n"},{"id":"93045","messageId":"20081014234512.GC14994@riemann.deskinm.fdns.net","threadId":"15903","inReplyTo":"1224025227.2699.13.camel@mattlaptop2.local","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Deskin Miller","fromEmail":"deskinm@umich.edu","sentAt":"2008-10-14T23:45:12Z","receivedAt":"2008-10-14T23:45:12Z","isPatch":true,"sender":{"key":"deskinm@umich.edu","avatar":"https://gravatar.com/avatar/d340a0e612cdf0a79535c71863c0b4c535e9aba63b42032226ae903e638b64f9?d=mp&s=160"},"body":"On Tue, Oct 14, 2008 at 07:00:27PM -0400, Matt McCutchen wrote:\n> On Tue, 2008-10-14 at 15:32 -0700, Junio C Hamano wrote:\n> > Makes me wonder why this is _not_ inside test-lib.sh where it creates the\n> > test (trash) directory. That way, you would cover future tests that wants\n> > to see a saner/simpler POSIX permission behaviour, wouldn't you?\n> \n> Yes. However, I don't anticipate there being any tests specifically\n> about file permissions other than t1301-shared-repo.sh, and if the user\n> has set a default ACL on the git source tree, we might want to let trash\n> directories obey that setting except in the one case where it breaks the\n> test. What do you think?\n\nI'll add a shameless plug for my patch: Fix testcase failure when extended\nattributes are in use, available from Gmane at\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/98170\n\nIt's orthogonal to this patch, I think: this patch deals with ACLs overriding\nthe umask testing we're doing, while my patch deals with parsing the\npermissions that ls returns, and applies to instances where extended attributes\nare in use which we can't get rid of, like SELinux.\n\nDeskin Miller\n"},{"id":"93053","messageId":"48F589EC.6050307@viscovery.net","threadId":"15903","inReplyTo":"7vzll66c5u.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-15T06:13:00Z","receivedAt":"2008-10-15T06:13:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Matt McCutchen writes:\n>> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\n>> index dc85e8b..2275caa 100755\n>> --- a/t/t1301-shared-repo.sh\n>> +++ b/t/t1301-shared-repo.sh\n>> @@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n>> \n>> . ./test-lib.sh\n>> \n>> +# Remove a default ACL from the test dir if possible.\n>> +setfacl -k . 2>/dev/null\n>> +\n> \n> Makes me wonder why this is _not_ inside test-lib.sh where it creates the\n> test (trash) directory. That way, you would cover future tests that wants\n> to see a saner/simpler POSIX permission behaviour, wouldn't you?\n\nBut that would also paper over unanticipated bad interactions with strange\nACLs that people might set, wouldn't it? By not placing this into\ntest-lib.sh there is a higher chance that such an interaction is revealed,\nand we can react on it (educate users or fix the code).\n\n-- Hannes\n"},{"id":"93057","messageId":"7vmyh64bgy.fsf@gitster.siamese.dyndns.org","threadId":"15903","inReplyTo":"48F589EC.6050307@viscovery.net","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-15T06:30:05Z","receivedAt":"2008-10-15T06:30:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Junio C Hamano schrieb:\n> ...\n>>> +# Remove a default ACL from the test dir if possible.\n>>> +setfacl -k . 2>/dev/null\n>>> +\n>> \n>> Makes me wonder why this is _not_ inside test-lib.sh where it creates the\n>> test (trash) directory. That way, you would cover future tests that wants\n>> to see a saner/simpler POSIX permission behaviour, wouldn't you?\n>\n> But that would also paper over unanticipated bad interactions with strange\n> ACLs that people might set, wouldn't it? By not placing this into\n> test-lib.sh there is a higher chance that such an interaction is revealed,\n> and we can react on it (educate users or fix the code).\n\nWhat do you exactly mean by \"educate users or fix the code\"? For example,\nby not putting this setfacl in test-lib.sh, t1301 revealed that with a\ndefault ACL higher up, \"git init --shared\" would not work as expected.\n\nThen what?\n\n - Do you mean, by \"educate users\", that we teach users not to play fun\n games with ACL in a git controled working tree?\n\n - Do you mean, by \"fix the code\", that we teach adjust_shared_perm() to\n deal with ACL?\n"},{"id":"93063","messageId":"48F59928.5040502@viscovery.net","threadId":"15903","inReplyTo":"7vmyh64bgy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-15T07:18:00Z","receivedAt":"2008-10-15T07:18:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n> \n>> Junio C Hamano schrieb:\n>> ...\n>>>> +# Remove a default ACL from the test dir if possible.\n>>>> +setfacl -k . 2>/dev/null\n>>>> +\n>>> Makes me wonder why this is _not_ inside test-lib.sh where it creates the\n>>> test (trash) directory. That way, you would cover future tests that wants\n>>> to see a saner/simpler POSIX permission behaviour, wouldn't you?\n>> But that would also paper over unanticipated bad interactions with strange\n>> ACLs that people might set, wouldn't it? By not placing this into\n>> test-lib.sh there is a higher chance that such an interaction is revealed,\n>> and we can react on it (educate users or fix the code).\n> \n> What do you exactly mean by \"educate users or fix the code\"? For example,\n> by not putting this setfacl in test-lib.sh, t1301 revealed that with a\n> default ACL higher up, \"git init --shared\" would not work as expected.\n> \n> Then what?\n> \n> - Do you mean, by \"educate users\", that we teach users not to play fun\n> games with ACL in a git controled working tree?\n\nCorrect. In the case of a shared repository we can educate users not to\nplay with ACLs.\n\n> - Do you mean, by \"fix the code\", that we teach adjust_shared_perm() to\n> deal with ACL?\n\nCorrect in principle, but we need not go this route in the case of shared\nrepositories because we better educate users.\n\n-- Hannes\n"},{"id":"93075","messageId":"7v7i8a47f6.fsf@gitster.siamese.dyndns.org","threadId":"15903","inReplyTo":"48F59928.5040502@viscovery.net","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-15T07:57:33Z","receivedAt":"2008-10-15T07:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>>> But that would also paper over unanticipated bad interactions with strange\n>>> ACLs that people might set, wouldn't it? By not placing this into\n>>> test-lib.sh there is a higher chance that such an interaction is revealed,\n>>> and we can react on it (educate users or fix the code).\n>> \n>> What do you exactly mean by \"educate users or fix the code\"? For example,\n>> by not putting this setfacl in test-lib.sh, t1301 revealed that with a\n>> default ACL higher up, \"git init --shared\" would not work as expected.\n>> \n>> Then what?\n>> \n>> - Do you mean, by \"educate users\", that we teach users not to play fun\n>> games with ACL in a git controled working tree?\n>\n> Correct. In the case of a shared repository we can educate users not to\n> play with ACLs.\n>\n>> - Do you mean, by \"fix the code\", that we teach adjust_shared_perm() to\n>> deal with ACL?\n>\n> Correct in principle, but we need not go this route in the case of shared\n> repositories because we better educate users.\n\nIf that is the case what difference does your suggestion of not putting it\nin test-lib.sh make? We discourage users from playing ACL games, and we\nprotect ourselves from such by making sure the trash directory used for\nrunning tests are not contaminated with ACL. Wouldn't it make more sense\nto do so for all the tests, so that future test writers do not have to\nworry about it?\n"},{"id":"93077","messageId":"48F5A590.3050905@viscovery.net","threadId":"15903","inReplyTo":"7v7i8a47f6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-15T08:10:56Z","receivedAt":"2008-10-15T08:10:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n>>> - Do you mean, by \"educate users\", that we teach users not to play fun\n>>> games with ACL in a git controled working tree?\n>> Correct. In the case of a shared repository we can educate users not to\n>> play with ACLs.\n>>\n>>> - Do you mean, by \"fix the code\", that we teach adjust_shared_perm() to\n>>> deal with ACL?\n>> Correct in principle, but we need not go this route in the case of shared\n>> repositories because we better educate users.\n> \n> If that is the case what difference does your suggestion of not putting it\n> in test-lib.sh make? We discourage users from playing ACL games, and we\n> protect ourselves from such by making sure the trash directory used for\n> running tests are not contaminated with ACL. Wouldn't it make more sense\n> to do so for all the tests, so that future test writers do not have to\n> worry about it?\n\nWe have to decide case by case. In the case of shared directories it makes\nsense to suggest \"do not play ACL games\". In other cases, however, this\nsuggestion could not work out that well, and a workaround in the code is\nthe better solutions. But we do not know what those other cases are, and\nthe test suite may be a tool to uncover them.\n\nPerhaps I'm worrying too much because a case that warrants a change in the\ncode would either have to happen frequently or be backed with very strong\narguments that ACLs are required in that particular way. (And in both\ncases there would still have be conflicts in the way how git handles\npermissions - we wouldn't care otherwise.) But no such case has turned up\nso far.\n\n-- Hannes\n"},{"id":"93093","messageId":"1224081242.2935.13.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"48F589EC.6050307@viscovery.net","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-15T14:34:02Z","receivedAt":"2008-10-15T14:34:02Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Wed, 2008-10-15 at 08:13 +0200, Johannes Sixt wrote:\n> Junio C Hamano schrieb:\n> > Makes me wonder why this is _not_ inside test-lib.sh where it creates the\n> > test (trash) directory. That way, you would cover future tests that wants\n> > to see a saner/simpler POSIX permission behaviour, wouldn't you?\n> \n> But that would also paper over unanticipated bad interactions with strange\n> ACLs that people might set, wouldn't it? By not placing this into\n> test-lib.sh there is a higher chance that such an interaction is revealed,\n> and we can react on it (educate users or fix the code).\n\nA default ACL on the working tree does not interfere with git's\noperation. If the repository is shared, git will explicitly set the\npermissions of every file as configured; otherwise, new files will\nsimply take their permissions from the default ACL instead of the\ncreating process's umask. This is exactly the behavior that a user who\nsets a default ACL would expect. There is no need to modify\nadjust_shared_perm or to warn users not to use default ACLs.\n\nThe only problem here is that a default ACL prevents\nt1301-shared-repo.sh from testing the interaction between the umask and\nthe sharedRepository setting, since the test case expects new files to\nbe created according to the umask it set but the default ACL is\noverriding the umask. Removing the trash directory's default ACL is a\nperfectly legitimate way for t1301-shared-repo.sh to test what it wants\nto test. Another option would be to modify the trash directory's\ndefault ACL instead of modifying the umask.\n\nOther tests will not care whether test-lib.sh clears a default ACL for\nthem because they are not specifically testing file permissions.\nTherefore, I thought it best to leave the default ACL alone so that the\ntrash directories get the permissions the user has specified in the\ndefault ACL in case he/she cares about sharing the trash directories\nwith others.\n\nMatt\n"},{"id":"93163","messageId":"7vzll5jepj.fsf@gitster.siamese.dyndns.org","threadId":"15903","inReplyTo":"48F5A590.3050905@viscovery.net","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-16T05:23:20Z","receivedAt":"2008-10-16T05:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Junio C Hamano schrieb:\n>\n>> If that is the case what difference does your suggestion of not putting it\n>> in test-lib.sh make? We discourage users from playing ACL games, and we\n>> protect ourselves from such by making sure the trash directory used for\n>> running tests are not contaminated with ACL. Wouldn't it make more sense\n>> to do so for all the tests, so that future test writers do not have to\n>> worry about it?\n>\n> We have to decide case by case. In the case of shared directories it makes\n> sense to suggest \"do not play ACL games\". In other cases, however, this\n> suggestion could not work out that well, and a workaround in the code is\n> the better solutions. But we do not know what those other cases are, and\n> the test suite may be a tool to uncover them.\n\nAlthough I am not particularly interested in hypothetical case that does\nnot have concrete examples, I do not care deeply enough either. So let's\ntake this patch (with updated/corrected log message) that minimally covers\nthe parts that can be broken by ACL games.\n"},{"id":"93261","messageId":"1224210520.2952.39.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"7vzll5jepj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-17T02:28:40Z","receivedAt":"2008-10-17T02:28:40Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Wed, 2008-10-15 at 22:23 -0700, Junio C Hamano wrote:\n> Johannes Sixt writes:\n> > We have to decide case by case. In the case of shared directories it makes\n> > sense to suggest \"do not play ACL games\". In other cases, however, this\n> > suggestion could not work out that well, and a workaround in the code is\n> > the better solutions. But we do not know what those other cases are, and\n> > the test suite may be a tool to uncover them.\n> \n> Although I am not particularly interested in hypothetical case that does\n> not have concrete examples, I do not care deeply enough either. So let's\n> take this patch (with updated/corrected log message) that minimally covers\n> the parts that can be broken by ACL games.\n\nAs I said in my other message, default ACLs do not break git, they only\nbreak the way git is being tested in t1301-shared-repo.sh . There is no\ncause for concern.\n\nIn fact, default ACLs obsolete core.sharedrepository as a means of\nsetting default permissions on a repository because default ACLs apply\nto files created by any program while core.sharedrepository is\nrecognized only by git. Thus, a user who has a default ACL would be\nunlikely to also have core.sharedrepository, so even if there were a bad\ninteraction between the two, no one would be likely to encounter it.\n\nUpdated patch to follow.\n\nMatt\n"},{"id":"93262","messageId":"1224210527.2952.41.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"7vzll5jepj.fsf@gitster.siamese.dyndns.org","subject":"[PATCH try 3] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-17T02:28:47Z","receivedAt":"2008-10-17T02:28:47Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"This test creates files with several different umasks and expects thei\npermissions to be initialized according to the umask, so a default ACL on the\ntrash directory (which overrides the umask for files created in that directory)\ncauses the test to fail. To avoid that, remove the default ACL if possible with\nsetfacl(1).\n\nSigned-off-by: Matt McCutchen \n---\n t/t1301-shared-repo.sh | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex dc85e8b..2275caa 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n \n . ./test-lib.sh\n \n+# Remove a default ACL from the test dir if possible.\n+setfacl -k . 2>/dev/null\n+\n # User must have read permissions to the repo -> failure on --shared=0400\n test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \tmkdir sub && (\n-- \n1.6.0.2.530.gb503b\n"},{"id":"93263","messageId":"1224210734.2952.42.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"7vzll5jepj.fsf@gitster.siamese.dyndns.org","subject":"[PATCH try 4] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-17T02:32:14Z","receivedAt":"2008-10-17T02:32:14Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"This test creates files with several different umasks and expects their\npermissions to be initialized according to the umask, so a default ACL on the\ntrash directory (which overrides the umask for files created in that directory)\ncauses the test to fail. To avoid that, remove the default ACL if possible with\nsetfacl(1).\n\nSigned-off-by: Matt McCutchen \n---\nFix typo in commit message. I'm sorry for the noise.\n\n t/t1301-shared-repo.sh | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex dc85e8b..2275caa 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'\n \n . ./test-lib.sh\n \n+# Remove a default ACL from the test dir if possible.\n+setfacl -k . 2>/dev/null\n+\n # User must have read permissions to the repo -> failure on --shared=0400\n test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \tmkdir sub && (\n-- \n1.6.0.2.530.gb503b\n"},{"id":"93264","messageId":"7viqrrq1wz.fsf@gitster.siamese.dyndns.org","threadId":"15903","inReplyTo":"1224210520.2952.39.camel@mattlaptop2.local","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-17T04:30:04Z","receivedAt":"2008-10-17T04:30:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McCutchen writes:\n\n> As I said in my other message, default ACLs do not break git, they only\n> break the way git is being tested in t1301-shared-repo.sh . There is no\n> cause for concern.\n\nIs it also true if the default is too tight? Wouldn't that interfere with\nthe attempt to loosen the permission bits by core.sharedrepository?\n\nJust being curious.\n"},{"id":"93265","messageId":"1224217994.10291.2.camel@mattlaptop2.local","threadId":"15903","inReplyTo":"7viqrrq1wz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-17T04:33:14Z","receivedAt":"2008-10-17T04:33:14Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Thu, 2008-10-16 at 21:30 -0700, Junio C Hamano wrote:\n> Matt McCutchen writes:\n> > As I said in my other message, default ACLs do not break git, they only\n> > break the way git is being tested in t1301-shared-repo.sh . There is no\n> > cause for concern.\n> \n> Is it also true if the default is too tight? Wouldn't that interfere with\n> the attempt to loosen the permission bits by core.sharedrepository?\n\nNo. adjust_shared_perm does an explicit chmod, which always sets\nexactly the requested permissions. A default ACL just replaces the\numask in the calculation of a file's *initial* permissions.\n\nMatt\n"}]}