threads / patch / 15903

patcht1301-shared-repo.sh: don't let a default ACL interfere with the test

Subject: [PATCH] t1301-shared-repo.sh: don't let a default ACL interfere with the test

## tl;dr

17 messages between Oct 14, 2008 and Oct 17, 2008. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Matt McCutchen· Oct 14, 2008, 22:07 UTC · lore
This test creates files with several different umasks and expects the files to
be permissioned according to the umasks, so a default ACL on the test dir causes
the test to fail.  To avoid that, remove the default ACL if possible with
setfacl(1).  (Will work on many systems.)
---
 t/t1301-shared-repo.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to t/t1301-shared-repo.sh +3 −0
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index dc85e8b..2275caa 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
 
 . ./test-lib.sh
 
+# Remove a default ACL from the test dir if possible.
+setfacl -k . 2>/dev/null
+
 # User must have read permissions to the repo -> failure on --shared=0400
 test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	mkdir sub && (

-- 
1.6.0.2.530.gb503b
Matt McCutchen· Oct 14, 2008, 22:10 UTC · re: Matt McCutchen · lore

[PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

This test creates files with several different umasks and expects the files to be permissioned according to the umasks, so a default ACL on the test dir causes the test to fail. To avoid that, remove the default ACL if possible with setfacl(1). (Will work on many systems.)

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
This time with a signoff.
 t/t1301-shared-repo.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to t/t1301-shared-repo.sh +3 −0
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index dc85e8b..2275caa 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
 
 . ./test-lib.sh
 
+# Remove a default ACL from the test dir if possible.
+setfacl -k . 2>/dev/null
+
 # User must have read permissions to the repo -> failure on --shared=0400
 test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	mkdir sub && (

-- 
1.6.0.2.530.gb503b
Junio C Hamano· Oct 14, 2008, 22:32 UTC · re: Matt McCutchen · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Matt McCutchen <matt@mattmccutchen.net> writes:
> This test creates files with several different umasks and expects the files to
> be permissioned according to the umasks, so a default ACL on the test dir causes
Is "to permission" a verb?
> the test to fail.  To avoid that, remove the default ACL if possible with
> setfacl(1).  (Will work on many systems.)

It is not clear in the comment in parentheses what provision you have made not to harm people on systems without setfacl.

I think "if possible" which you already have is a good enough description (i.e. "if setfacl fails we do not barf and if you do not have the command you probably are not running with a funky default ACL to see this issue anyway"), so I'd rather drop the comment in parentheses.

Show 18 quoted lines
> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
> ---
> This time with a signoff.
>
>  t/t1301-shared-repo.sh |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index dc85e8b..2275caa 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
>  
>  . ./test-lib.sh
>  
> +# Remove a default ACL from the test dir if possible.
> +setfacl -k . 2>/dev/null
> +

Makes me wonder why this is _not_ inside test-lib.sh where it creates the test (trash) directory. That way, you would cover future tests that wants to see a saner/simpler POSIX permission behaviour, wouldn't you?

Matt McCutchen· Oct 14, 2008, 23:00 UTC · re: Junio C Hamano · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

On Tue, 2008-10-14 at 15:32 -0700, Junio C Hamano wrote:
Show 5 quoted lines
> Matt McCutchen <matt@mattmccutchen.net> writes:
> > This test creates files with several different umasks and expects the files to
> > be permissioned according to the umasks, so a default ACL on the test dir causes
> 
> Is "to permission" a verb?
I thought it could be, but I'll reword that sentence.
Show 10 quoted lines
> > the test to fail.  To avoid that, remove the default ACL if possible with
> > setfacl(1).  (Will work on many systems.)
> 
> It is not clear in the comment in parentheses what provision you have made
> not to harm people on systems without setfacl.
> 
> I think "if possible" which you already have is a good enough description
> (i.e. "if setfacl fails we do not barf and if you do not have the command
> you probably are not running with a funky default ACL to see this issue
> anyway"), so I'd rather drop the comment in parentheses.
Sure.
Show 22 quoted lines
> > Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
> > ---
> > This time with a signoff.
> >
> >  t/t1301-shared-repo.sh |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> > index dc85e8b..2275caa 100755
> > --- a/t/t1301-shared-repo.sh
> > +++ b/t/t1301-shared-repo.sh
> > @@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
> >  
> >  . ./test-lib.sh
> >  
> > +# Remove a default ACL from the test dir if possible.
> > +setfacl -k . 2>/dev/null
> > +
> 
> Makes me wonder why this is _not_ inside test-lib.sh where it creates the
> test (trash) directory.  That way, you would cover future tests that wants
> to see a saner/simpler POSIX permission behaviour, wouldn't you?

Yes. However, I don't anticipate there being any tests specifically about file permissions other than t1301-shared-repo.sh, and if the user has set a default ACL on the git source tree, we might want to let trash directories obey that setting except in the one case where it breaks the test. What do you think?

Matt
Deskin Miller· Oct 14, 2008, 23:45 UTC · re: Matt McCutchen · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

On Tue, Oct 14, 2008 at 07:00:27PM -0400, Matt McCutchen wrote:
Show 10 quoted lines
> On Tue, 2008-10-14 at 15:32 -0700, Junio C Hamano wrote:
> > Makes me wonder why this is _not_ inside test-lib.sh where it creates the
> > test (trash) directory.  That way, you would cover future tests that wants
> > to see a saner/simpler POSIX permission behaviour, wouldn't you?
> 
> Yes.  However, I don't anticipate there being any tests specifically
> about file permissions other than t1301-shared-repo.sh, and if the user
> has set a default ACL on the git source tree, we might want to let trash
> directories obey that setting except in the one case where it breaks the
> test.  What do you think?

I'll add a shameless plug for my patch: Fix testcase failure when extended attributes are in use, available from Gmane at

http://thread.gmane.org/gmane.comp.version-control.git/98170

It's orthogonal to this patch, I think: this patch deals with ACLs overriding the umask testing we're doing, while my patch deals with parsing the permissions that ls returns, and applies to instances where extended attributes are in use which we can't get rid of, like SELinux.

Deskin Miller
Johannes Sixt· Oct 15, 2008, 06:13 UTC · re: Junio C Hamano · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Junio C Hamano schrieb:
Show 16 quoted lines
> Matt McCutchen <matt@mattmccutchen.net> writes:
>> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
>> index dc85e8b..2275caa 100755
>> --- a/t/t1301-shared-repo.sh
>> +++ b/t/t1301-shared-repo.sh
>> @@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
>>  
>>  . ./test-lib.sh
>>  
>> +# Remove a default ACL from the test dir if possible.
>> +setfacl -k . 2>/dev/null
>> +
> 
> Makes me wonder why this is _not_ inside test-lib.sh where it creates the
> test (trash) directory.  That way, you would cover future tests that wants
> to see a saner/simpler POSIX permission behaviour, wouldn't you?

But that would also paper over unanticipated bad interactions with strange ACLs that people might set, wouldn't it? By not placing this into test-lib.sh there is a higher chance that such an interaction is revealed, and we can react on it (educate users or fix the code).

-- Hannes
Junio C Hamano· Oct 15, 2008, 06:30 UTC · re: Johannes Sixt · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 14 quoted lines
> Junio C Hamano schrieb:
> ...
>>> +# Remove a default ACL from the test dir if possible.
>>> +setfacl -k . 2>/dev/null
>>> +
>> 
>> Makes me wonder why this is _not_ inside test-lib.sh where it creates the
>> test (trash) directory.  That way, you would cover future tests that wants
>> to see a saner/simpler POSIX permission behaviour, wouldn't you?
>
> But that would also paper over unanticipated bad interactions with strange
> ACLs that people might set, wouldn't it? By not placing this into
> test-lib.sh there is a higher chance that such an interaction is revealed,
> and we can react on it (educate users or fix the code).

What do you exactly mean by "educate users or fix the code"? For example, by not putting this setfacl in test-lib.sh, t1301 revealed that with a default ACL higher up, "git init --shared" would not work as expected.

Then what?
 - Do you mean, by "educate users", that we teach users not to play fun
   games with ACL in a git controled working tree?
 - Do you mean, by "fix the code", that we teach adjust_shared_perm() to
   deal with ACL?
Johannes Sixt· Oct 15, 2008, 07:18 UTC · re: Junio C Hamano · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Junio C Hamano schrieb:
Show 23 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Junio C Hamano schrieb:
>> ...
>>>> +# Remove a default ACL from the test dir if possible.
>>>> +setfacl -k . 2>/dev/null
>>>> +
>>> Makes me wonder why this is _not_ inside test-lib.sh where it creates the
>>> test (trash) directory.  That way, you would cover future tests that wants
>>> to see a saner/simpler POSIX permission behaviour, wouldn't you?
>> But that would also paper over unanticipated bad interactions with strange
>> ACLs that people might set, wouldn't it? By not placing this into
>> test-lib.sh there is a higher chance that such an interaction is revealed,
>> and we can react on it (educate users or fix the code).
> 
> What do you exactly mean by "educate users or fix the code"?  For example,
> by not putting this setfacl in test-lib.sh, t1301 revealed that with a
> default ACL higher up, "git init --shared" would not work as expected.
> 
> Then what?
> 
>  - Do you mean, by "educate users", that we teach users not to play fun
>    games with ACL in a git controled working tree?

Correct. In the case of a shared repository we can educate users not to play with ACLs.

>  - Do you mean, by "fix the code", that we teach adjust_shared_perm() to
>    deal with ACL?

Correct in principle, but we need not go this route in the case of shared repositories because we better educate users.

-- Hannes
Junio C Hamano· Oct 15, 2008, 07:57 UTC · re: Johannes Sixt · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 22 quoted lines
>>> But that would also paper over unanticipated bad interactions with strange
>>> ACLs that people might set, wouldn't it? By not placing this into
>>> test-lib.sh there is a higher chance that such an interaction is revealed,
>>> and we can react on it (educate users or fix the code).
>> 
>> What do you exactly mean by "educate users or fix the code"?  For example,
>> by not putting this setfacl in test-lib.sh, t1301 revealed that with a
>> default ACL higher up, "git init --shared" would not work as expected.
>> 
>> Then what?
>> 
>>  - Do you mean, by "educate users", that we teach users not to play fun
>>    games with ACL in a git controled working tree?
>
> Correct. In the case of a shared repository we can educate users not to
> play with ACLs.
>
>>  - Do you mean, by "fix the code", that we teach adjust_shared_perm() to
>>    deal with ACL?
>
> Correct in principle, but we need not go this route in the case of shared
> repositories because we better educate users.

If that is the case what difference does your suggestion of not putting it in test-lib.sh make? We discourage users from playing ACL games, and we protect ourselves from such by making sure the trash directory used for running tests are not contaminated with ACL. Wouldn't it make more sense to do so for all the tests, so that future test writers do not have to worry about it?

Johannes Sixt· Oct 15, 2008, 08:10 UTC · re: Junio C Hamano · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Junio C Hamano schrieb:
Show 17 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
>>>  - Do you mean, by "educate users", that we teach users not to play fun
>>>    games with ACL in a git controled working tree?
>> Correct. In the case of a shared repository we can educate users not to
>> play with ACLs.
>>
>>>  - Do you mean, by "fix the code", that we teach adjust_shared_perm() to
>>>    deal with ACL?
>> Correct in principle, but we need not go this route in the case of shared
>> repositories because we better educate users.
> 
> If that is the case what difference does your suggestion of not putting it
> in test-lib.sh make?  We discourage users from playing ACL games, and we
> protect ourselves from such by making sure the trash directory used for
> running tests are not contaminated with ACL.  Wouldn't it make more sense
> to do so for all the tests, so that future test writers do not have to
> worry about it?

We have to decide case by case. In the case of shared directories it makes sense to suggest "do not play ACL games". In other cases, however, this suggestion could not work out that well, and a workaround in the code is the better solutions. But we do not know what those other cases are, and the test suite may be a tool to uncover them.

Perhaps I'm worrying too much because a case that warrants a change in the code would either have to happen frequently or be backed with very strong arguments that ACLs are required in that particular way. (And in both cases there would still have be conflicts in the way how git handles permissions - we wouldn't care otherwise.) But no such case has turned up so far.

-- Hannes
Junio C Hamano· Oct 16, 2008, 05:23 UTC · re: Johannes Sixt · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 14 quoted lines
> Junio C Hamano schrieb:
>
>> If that is the case what difference does your suggestion of not putting it
>> in test-lib.sh make?  We discourage users from playing ACL games, and we
>> protect ourselves from such by making sure the trash directory used for
>> running tests are not contaminated with ACL.  Wouldn't it make more sense
>> to do so for all the tests, so that future test writers do not have to
>> worry about it?
>
> We have to decide case by case. In the case of shared directories it makes
> sense to suggest "do not play ACL games". In other cases, however, this
> suggestion could not work out that well, and a workaround in the code is
> the better solutions. But we do not know what those other cases are, and
> the test suite may be a tool to uncover them.

Although I am not particularly interested in hypothetical case that does not have concrete examples, I do not care deeply enough either. So let's take this patch (with updated/corrected log message) that minimally covers the parts that can be broken by ACL games.

Matt McCutchen· Oct 17, 2008, 02:28 UTC · re: Junio C Hamano · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

On Wed, 2008-10-15 at 22:23 -0700, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> > We have to decide case by case. In the case of shared directories it makes
> > sense to suggest "do not play ACL games". In other cases, however, this
> > suggestion could not work out that well, and a workaround in the code is
> > the better solutions. But we do not know what those other cases are, and
> > the test suite may be a tool to uncover them.
> 
> Although I am not particularly interested in hypothetical case that does
> not have concrete examples, I do not care deeply enough either.  So let's
> take this patch (with updated/corrected log message) that minimally covers
> the parts that can be broken by ACL games.

As I said in my other message, default ACLs do not break git, they only break the way git is being tested in t1301-shared-repo.sh . There is no cause for concern.

In fact, default ACLs obsolete core.sharedrepository as a means of setting default permissions on a repository because default ACLs apply to files created by any program while core.sharedrepository is recognized only by git. Thus, a user who has a default ACL would be unlikely to also have core.sharedrepository, so even if there were a bad interaction between the two, no one would be likely to encounter it.

Updated patch to follow.
Matt
Junio C Hamano· Oct 17, 2008, 04:30 UTC · re: Matt McCutchen · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

Matt McCutchen <matt@mattmccutchen.net> writes:
> As I said in my other message, default ACLs do not break git, they only
> break the way git is being tested in t1301-shared-repo.sh .  There is no
> cause for concern.

Is it also true if the default is too tight? Wouldn't that interfere with the attempt to loosen the permission bits by core.sharedrepository?

Just being curious.
Matt McCutchen· Oct 17, 2008, 04:33 UTC · re: Junio C Hamano · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

On Thu, 2008-10-16 at 21:30 -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Matt McCutchen <matt@mattmccutchen.net> writes:
> > As I said in my other message, default ACLs do not break git, they only
> > break the way git is being tested in t1301-shared-repo.sh .  There is no
> > cause for concern.
> 
> Is it also true if the default is too tight?  Wouldn't that interfere with
> the attempt to loosen the permission bits by core.sharedrepository?

No. adjust_shared_perm does an explicit chmod, which always sets exactly the requested permissions. A default ACL just replaces the umask in the calculation of a file's *initial* permissions.

Matt
Matt McCutchen· Oct 17, 2008, 02:28 UTC · re: Junio C Hamano · lore

[PATCH try 3] t1301-shared-repo.sh: don't let a default ACL interfere with the test

This test creates files with several different umasks and expects thei permissions to be initialized according to the umask, so a default ACL on the trash directory (which overrides the umask for files created in that directory) causes the test to fail. To avoid that, remove the default ACL if possible with setfacl(1).

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
 t/t1301-shared-repo.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to t/t1301-shared-repo.sh +3 −0
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index dc85e8b..2275caa 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
 
 . ./test-lib.sh
 
+# Remove a default ACL from the test dir if possible.
+setfacl -k . 2>/dev/null
+
 # User must have read permissions to the repo -> failure on --shared=0400
 test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	mkdir sub && (

-- 
1.6.0.2.530.gb503b
Matt McCutchen· Oct 17, 2008, 02:32 UTC · re: Junio C Hamano · lore

[PATCH try 4] t1301-shared-repo.sh: don't let a default ACL interfere with the test

This test creates files with several different umasks and expects their permissions to be initialized according to the umask, so a default ACL on the trash directory (which overrides the umask for files created in that directory) causes the test to fail. To avoid that, remove the default ACL if possible with setfacl(1).

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
Fix typo in commit message.  I'm sorry for the noise.
 t/t1301-shared-repo.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to t/t1301-shared-repo.sh +3 −0
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index dc85e8b..2275caa 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -7,6 +7,9 @@ test_description='Test shared repository initialization'
 
 . ./test-lib.sh
 
+# Remove a default ACL from the test dir if possible.
+setfacl -k . 2>/dev/null
+
 # User must have read permissions to the repo -> failure on --shared=0400
 test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	mkdir sub && (

-- 
1.6.0.2.530.gb503b
Matt McCutchen· Oct 15, 2008, 14:34 UTC · re: Johannes Sixt · lore

Re: [PATCH try 2] t1301-shared-repo.sh: don't let a default ACL interfere with the test

On Wed, 2008-10-15 at 08:13 +0200, Johannes Sixt wrote:
Show 9 quoted lines
> Junio C Hamano schrieb:
> > Makes me wonder why this is _not_ inside test-lib.sh where it creates the
> > test (trash) directory.  That way, you would cover future tests that wants
> > to see a saner/simpler POSIX permission behaviour, wouldn't you?
> 
> But that would also paper over unanticipated bad interactions with strange
> ACLs that people might set, wouldn't it? By not placing this into
> test-lib.sh there is a higher chance that such an interaction is revealed,
> and we can react on it (educate users or fix the code).

A default ACL on the working tree does not interfere with git's operation. If the repository is shared, git will explicitly set the permissions of every file as configured; otherwise, new files will simply take their permissions from the default ACL instead of the creating process's umask. This is exactly the behavior that a user who sets a default ACL would expect. There is no need to modify adjust_shared_perm or to warn users not to use default ACLs.

The only problem here is that a default ACL prevents t1301-shared-repo.sh from testing the interaction between the umask and the sharedRepository setting, since the test case expects new files to be created according to the umask it set but the default ACL is overriding the umask. Removing the trash directory's default ACL is a perfectly legitimate way for t1301-shared-repo.sh to test what it wants to test. Another option would be to modify the trash directory's default ACL instead of modifying the umask.

Other tests will not care whether test-lib.sh clears a default ACL for them because they are not specifically testing file permissions. Therefore, I thought it best to leave the default ACL alone so that the trash directories get the permissions the user has specified in the default ACL in case he/she cares about sharing the trash directories with others.

Matt

← back to recent threads