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

--

[ message continues ]

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

--

[ message continues ]

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

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

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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?

--

[ message continues ]

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

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

-- Hannes
--

[ message continues ]

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?

--

[ message continues ]

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

--

[ message continues ]

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.

--

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]