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

Matt McCutchen [off-list ref] 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.

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?

On Tue, 2008-10-14 at 15:32 -0700, Junio C Hamano wrote:

Matt McCutchen [off-list ref] writes:

quoted

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.

quoted

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.

quoted

Signed-off-by: Matt McCutchen <redacted>
---
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

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:

quoted

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

Junio C Hamano schrieb:

Matt McCutchen [off-list ref] writes:

quoted

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

On Wed, 2008-10-15 at 08:13 +0200, Johannes Sixt wrote:

Junio C Hamano schrieb:

quoted

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