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

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

[Matt McCutchen]

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(-)

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

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

[Matt McCutchen]

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(-)

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

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

[Junio C Hamano]

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.

> 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?

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

[Matt McCutchen]

On Tue, 2008-10-14 at 15:32 -0700, Junio C Hamano wrote:
> 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.

> > 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.

> > 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

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

[Deskin Miller]

On Tue, Oct 14, 2008 at 07:00:27PM -0400, Matt McCutchen wrote:
> 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

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

[Johannes Sixt]

Junio C Hamano schrieb:
> 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

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

[Junio C Hamano]

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?

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

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

[Johannes Sixt]

Junio C Hamano schrieb:
> 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

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

>>> 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?

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

[Johannes Sixt]

Junio C Hamano schrieb:
> 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

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

[Matt McCutchen]

On Wed, 2008-10-15 at 08:13 +0200, Johannes Sixt wrote:
> 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

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> 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.

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

[Matt McCutchen]

On Wed, 2008-10-15 at 22:23 -0700, Junio C Hamano wrote:
> 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

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

[Matt McCutchen]

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(-)

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

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

[Matt McCutchen]

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(-)

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

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

[Junio C Hamano]

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.

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

[Matt McCutchen]

On Thu, 2008-10-16 at 21:30 -0700, Junio C Hamano wrote:
> 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