Is git-am expected to honor core.sharedRepository?

Is git-am expected to honor core.sharedRepository?

[Matheus Tavares Bernardino]

Hi, everyone

I'm not very familiar with this setting, but to my understanding it
should only affect files in $GIT_DIR not $GIT_WORK_TREE, is that
correct? Nevertheless, apply and am (which uses apply) end up
adjusting the permissions of created directories based on the setting.
To give an example:

We first commit the directory 'd':
$ mkdir d
$ touch d/f
$ git add d
$ git commit -m d
$ ls -l
drwxr-xr-x 2 matheus matheus 60 dez  1 11:29 d

Then we create a patch and use am to apply it:
$ git format-patch -1
$ git reset --hard HEAD~
$ git config core.sharedRepository 0700
$ git am *.patch

The setting was honored by am:
$ ls -l
drwx--S--- 2 matheus matheus 60 dez  1 11:30 d

And if we delete 'd' and check it out again, the setting is ignored:
$ rm -rf d
$ git checkout d
$ ls -l
drwxr-xr-x 2 matheus matheus 60 dez  1 11:31 d

Is this expected?

If not, the place to be changed is probably the
safe_create_leading_directories() call in apply.c. This function
internally calls adjust_shared_perm() to modify the permissions
according to core.sharedRepository, so we could probably pass a flag
to skip this step. But this function has at least 15 callers, so
should we introduce a wrapper for the non-shared case, instead?

(For some background, I stumbled across this while considering using
safe_create_leading_directories() for a parallel-checkout patch. But
then I noticed it adjusts the directories' permissions based on the
setting and I was worried whether it could be user for checkout.)

Thanks,
Matheus

Re: Is git-am expected to honor core.sharedRepository?

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> If not, the place to be changed is probably the
> safe_create_leading_directories() call in apply.c.

https://lore.kernel.org/git/xmqqziglaxj4.fsf@gitster.mtv.corp.google.com/

Calling adjust_shared_perm() on a path outside .git/ is a potential
bug, as you found out, and definitely a bug if used on working tree
files.  We may want to share with only selected users in a group the
contents of the repository (e.g. allow local cloning from us), while
allowing daemon-ish tools to scan what is in the working tree files
without letting them touch what is in the repository, for example;
adjust_shared_perm() is meant for .git/ repository files and
tightening working tree files' permissions using it would break such
arrangement.

I think bugreport uses scld, but it may be used to drop cruft inside
the working tree, but the files created are *not* to be "git add"ed,
so the use case does not count as "if used on working tree files".

> $ git commit -m d
> $ ls -l
> drwxr-xr-x 2 matheus matheus 60 dez  1 11:29 d
> ...
> Then we create a patch and use am to apply it:
> The setting was honored by am:
> $ ls -l
> drwx--S--- 2 matheus matheus 60 dez  1 11:30 d

Having said that, I know it can be argued both ways.  If we want to
protect .git/ contents in a certain way, it may be worth protecting
the files in the working tree in the same way as well.  But at least
that is not the current rule is (even though as you found out we do
have bugs).

Thanks.

[PATCH] apply: don't use core.sharedRepository to create working tree files

[Matheus Tavares]

core.sharedRepository defines which permissions Git should set when
creating files in $GIT_DIR, so that the repository may be shared with
other users. But (in its current form) the setting shouldn't affect how
files are created in the working tree. This is not respected by apply
and am (which uses apply), when creating leading directories:

$ cat d.patch
 diff --git a/d/f b/d/f
 new file mode 100644
 index 0000000..e69de29

Apply without the setting:
$ umask 0077
$ git apply d.patch
$ ls -ld d
 drwx------

Apply with the setting:
$ umask 0077
$ git -c core.sharedRepository=0770 apply d.patch
$ ls -ld d
 drwxrws---

Only the leading directories are affected. That's because they are
created with safe_create_leading_directories(), which calls
adjust_shared_perm() to set the directories' permissions based on
core.sharedRepository. To fix that, let's introduce a variant of this
function that ignores the setting, and use it in apply. Also add a
regression test and a note in the function documentation about the use
of each variant according to the destination (working tree or git
dir).

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 apply.c                   |  2 +-
 cache.h                   |  7 ++++++-
 sha1-file.c               | 14 ++++++++++++--
 t/t4129-apply-samemode.sh | 26 ++++++++++++++++++++++++++
 t/test-lib-functions.sh   |  4 ++--
 5 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/apply.c b/apply.c
index 359ceb632c..4a4e9a0158 100644
--- a/apply.c
+++ b/apply.c
@@ -4409,7 +4409,7 @@ static int create_one_file(struct apply_state *state,
 		return 0;
 
 	if (errno == ENOENT) {
-		if (safe_create_leading_directories(path))
+		if (safe_create_leading_directories_no_share(path))
 			return 0;
 		res = try_create_file(state, path, mode, buf, size);
 		if (res < 0)
diff --git a/cache.h b/cache.h
index e986cf4ea9..8d279bc110 100644
--- a/cache.h
+++ b/cache.h
@@ -1255,7 +1255,11 @@ int adjust_shared_perm(const char *path);
  * safe_create_leading_directories() temporarily changes path while it
  * is working but restores it before returning.
  * safe_create_leading_directories_const() doesn't modify path, even
- * temporarily.
+ * temporarily. Both these variants adjust the permissions of the
+ * created directories to honor core.sharedRepository, so they are best
+ * suited for files inside the git dir. For working tree files, use
+ * safe_create_leading_directories_no_share() instead, as it ignores
+ * the core.sharedRepository setting.
  */
 enum scld_error {
 	SCLD_OK = 0,
@@ -1266,6 +1270,7 @@ enum scld_error {
 };
 enum scld_error safe_create_leading_directories(char *path);
 enum scld_error safe_create_leading_directories_const(const char *path);
+enum scld_error safe_create_leading_directories_no_share(char *path);
 
 /*
  * Callback function for raceproof_create_file(). This function is
diff --git a/sha1-file.c b/sha1-file.c
index dd65bd5c68..c3c49d2fa5 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -291,7 +291,7 @@ int mkdir_in_gitdir(const char *path)
 	return adjust_shared_perm(path);
 }
 
-enum scld_error safe_create_leading_directories(char *path)
+static enum scld_error safe_create_leading_directories_1(char *path, int share)
 {
 	char *next_component = path + offset_1st_component(path);
 	enum scld_error ret = SCLD_OK;
@@ -337,7 +337,7 @@ enum scld_error safe_create_leading_directories(char *path)
 				ret = SCLD_VANISHED;
 			else
 				ret = SCLD_FAILED;
-		} else if (adjust_shared_perm(path)) {
+		} else if (share && adjust_shared_perm(path)) {
 			ret = SCLD_PERMS;
 		}
 		*slash = slash_character;
@@ -345,6 +345,16 @@ enum scld_error safe_create_leading_directories(char *path)
 	return ret;
 }
 
+enum scld_error safe_create_leading_directories(char *path)
+{
+	return safe_create_leading_directories_1(path, 1);
+}
+
+enum scld_error safe_create_leading_directories_no_share(char *path)
+{
+	return safe_create_leading_directories_1(path, 0);
+}
+
 enum scld_error safe_create_leading_directories_const(const char *path)
 {
 	int save_errno;
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 5cdd76dfa7..41818d8315 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -73,4 +73,30 @@ test_expect_success FILEMODE 'bogus mode is rejected' '
 	test_i18ngrep "invalid mode" err
 '
 
+test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree files' '
+	git reset --hard &&
+	test_config core.sharedRepository 0666 &&
+	(
+		# Remove a default ACL if possible.
+		(setfacl -k newdir 2>/dev/null || true) &&
+		umask 0077 &&
+
+		# Test both files (f1) and leading dirs (d)
+		mkdir d &&
+		touch f1 d/f2 &&
+		git add f1 d/f2 &&
+		git diff --staged >patch-f1-and-f2.txt &&
+
+		rm -rf d f1 &&
+		git apply patch-f1-and-f2.txt &&
+
+		echo "-rw-------" >f1_mode.expected &&
+		echo "drwx------" >d_mode.expected &&
+		test_modebits f1 >f1_mode.actual &&
+		test_modebits d >d_mode.actual &&
+		test_cmp f1_mode.expected f1_mode.actual &&
+		test_cmp d_mode.expected d_mode.actual
+	)
+'
+
 test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 7ba3011b90..0f7f247159 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -367,9 +367,9 @@ test_chmod () {
 	git update-index --add "--chmod=$@"
 }
 
-# Get the modebits from a file.
+# Get the modebits from a file or directory.
 test_modebits () {
-	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+	ls -ld "$1" | sed -e 's|^\(..........\).*|\1|'
 }
 
 # Unset a configuration variable, but don't fail if it doesn't exist.
-- 
2.29.2

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Junio C Hamano]

8361e1d4 (Use sha1_file.c's mkdir-like routine in apply.c.,
2006-02-03) is the ancient source of this behaviour change, it
seems.

Re: Is git-am expected to honor core.sharedRepository?

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> (For some background, I stumbled across this while considering using
> safe_create_leading_directories() for a parallel-checkout patch. But
> then I noticed it adjusts the directories' permissions based on the
> setting and I was worried whether it could be user for checkout.)

Forgot to respond to this part, but you are correct.  

Anything that tries to replace what is in entry.c must not trigger
adjust_shared_perm() on files and directories in the working tree,
and it is a no-no to call safe_create_leading_directories().

Re: Is git-am expected to honor core.sharedRepository?

[Matheus Tavares Bernardino]

On Wed, Dec 2, 2020 at 7:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:
>
> > (For some background, I stumbled across this while considering using
> > safe_create_leading_directories() for a parallel-checkout patch. But
> > then I noticed it adjusts the directories' permissions based on the
> > setting and I was worried whether it could be user for checkout.)
>
> Forgot to respond to this part, but you are correct.
>
> Anything that tries to replace what is in entry.c must not trigger
> adjust_shared_perm() on files and directories in the working tree,
> and it is a no-no to call safe_create_leading_directories().

Got it, thanks. I've adjusted the parallel-checkout patch to use the
_no_share() scld variant from mt/do-not-use-scld-in-working-tree.

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Adam Dinwoodie]

This commit – eb3c027e17 ("apply: don't use core.sharedRepository to
create working tree files", 2020-12-01) – seems to have introduced a
new test failure in the Cygwin builds for v2.30.0-rc0, and which is
still present in rc1. I'm not quite sure I understand what the
expected behaviour here is, but I expect the issue is down to Cygwin's
slightly odd file permission handling.

To my surprise, the test fails if the worktree is under "/cygdrive",
but not if it's under "/home" within the Cygwin filesystem. I expect
this is some complexity with Cygwin's mount handling, but it's not a
failure mode I've seen before. I'm also going to follow up on the
Cygwin mailing list to see if the folk with a better understanding of
Cygwin's filesystem wrangling have a better understanding of what's
going on.

Extract from the relevant section of ./t4129-apply-samemode.sh -x
output, when run with that commit checked out, below:

expecting success of 4129.10 'do not use core.sharedRepository for
working tree files':
        git reset --hard &&
        test_config core.sharedRepository 0666 &&
        (
                # Remove a default ACL if possible.
                (setfacl -k newdir 2>/dev/null || true) &&
                umask 0077 &&

                # Test both files (f1) and leading dirs (d)
                mkdir d &&
                touch f1 d/f2 &&
                git add f1 d/f2 &&
                git diff --staged >patch-f1-and-f2.txt &&

                rm -rf d f1 &&
                git apply patch-f1-and-f2.txt &&

                echo "-rw-------" >f1_mode.expected &&
                echo "drwx------" >d_mode.expected &&
                test_modebits f1 >f1_mode.actual &&
                test_modebits d >d_mode.actual &&
                test_cmp f1_mode.expected f1_mode.actual &&
                test_cmp d_mode.expected d_mode.actual
        )

++ git reset --hard
HEAD is now at e950771 initial
++ test_config core.sharedRepository 0666
++ config_dir=
++ test core.sharedRepository = -C
++ test_when_finished 'test_unconfig  '\''core.sharedRepository'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig  '\''core.sharedRepository'\''
                } && (exit "$eval_ret"); eval_ret=$?; :'
++ git config core.sharedRepository 0666
++ setfacl -k newdir
++ true
++ umask 0077
++ mkdir d
++ touch f1 d/f2
++ git add f1 d/f2
++ git diff --staged
++ rm -rf d f1
++ git apply patch-f1-and-f2.txt
++ echo -rw-------
++ echo drwx------
++ test_modebits f1
++ ls -ld f1
++ sed -e 's|^\(..........\).*|\1|'
++ test_modebits d
++ ls -ld d
++ sed -e 's|^\(..........\).*|\1|'
++ test_cmp f1_mode.expected f1_mode.actual
++ test 2 -eq 2
++ eval 'diff -u' '"$@"'
+++ diff -u f1_mode.expected f1_mode.actual
--- f1_mode.expected    2020-12-19 16:50:20.169378700 +0000
+++ f1_mode.actual      2020-12-19 16:50:20.249126000 +0000
@@ -1 +1 @@
--rw-------
+-rw-rw-r--
++ test xf1_mode.expected = x-
++ test -e f1_mode.expected
++ test xf1_mode.actual = x-
++ test -e f1_mode.actual
++ return 1
error: last command exited with $?=1
++ test_unconfig core.sharedRepository
++ config_dir=
++ test core.sharedRepository = -C
++ git config --unset-all core.sharedRepository
++ config_status=0
++ case "$config_status" in
++ return 0
++ exit 1
++ eval_ret=1
++ :

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

> Extract from the relevant section of ./t4129-apply-samemode.sh -x
> output, when run with that commit checked out, below:
>
> expecting success of 4129.10 'do not use core.sharedRepository for
> working tree files':
>         git reset --hard &&
>         test_config core.sharedRepository 0666 &&
>         (
>                 # Remove a default ACL if possible.
>                 (setfacl -k newdir 2>/dev/null || true) &&
>                 umask 0077 &&
>
>                 # Test both files (f1) and leading dirs (d)
>                 mkdir d &&
>                 touch f1 d/f2 &&
>                 git add f1 d/f2 &&
>                 git diff --staged >patch-f1-and-f2.txt &&

... "point X" (see below) ...

>
>                 rm -rf d f1 &&
>                 git apply patch-f1-and-f2.txt &&
>
>                 echo "-rw-------" >f1_mode.expected &&
>                 echo "drwx------" >d_mode.expected &&
>                 test_modebits f1 >f1_mode.actual &&
>                 test_modebits d >d_mode.actual &&
>                 test_cmp f1_mode.expected f1_mode.actual &&
>                 test_cmp d_mode.expected d_mode.actual
>         )
> ...
> +++ diff -u f1_mode.expected f1_mode.actual
> --- f1_mode.expected    2020-12-19 16:50:20.169378700 +0000
> +++ f1_mode.actual      2020-12-19 16:50:20.249126000 +0000
> @@ -1 +1 @@
> --rw-------
> +-rw-rw-r--

This tells us that we are getting the umask (set to "me only")
ignored by "git apply".

What would we see in the "t4129-*.sh -x" output if we inserted

		ls -ld f1 d d/f2 &&

at "point X" above?

THanks.

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Achim Gratz]

Adam Dinwoodie writes:
> To my surprise, the test fails if the worktree is under "/cygdrive",

/cygdrive is normally mounted with "posix=0", which only affects case
sensitivity, so that isn't the reason for this particular fail.  You
should anyway not build a Cygwin package with that option in effect,
instead create your own mount point for that directory (with
"binary,user" options).

> +++ diff -u f1_mode.expected f1_mode.actual
> --- f1_mode.expected    2020-12-19 16:50:20.169378700 +0000
> +++ f1_mode.actual      2020-12-19 16:50:20.249126000 +0000
> @@ -1 +1 @@
> --rw-------
> +-rw-rw-r--

You seemingly can't change the ACL and/or several mode bits and see the
effective access that your euid / egid has instead.  It is possible to
set up the (default) ACL in a way that removes the permission to change
them while otherwise still giving you what is effectively full access,
in which case the test fail is the result of an inability to remove the
default ACL from the directory.  I suspect your build directory is owned
by a different user than the one you're building with and/or has been
moved or re-used from another Windows installation that has different
SID.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Adam Dinwoodie]

On Sat, 19 Dec 2020 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Dinwoodie <adam@dinwoodie.org> writes:
> > Extract from the relevant section of ./t4129-apply-samemode.sh -x
> > output, when run with that commit checked out, below:
> >
> > expecting success of 4129.10 'do not use core.sharedRepository for
> > working tree files':
> >         git reset --hard &&
> >         test_config core.sharedRepository 0666 &&
> >         (
> >                 # Remove a default ACL if possible.
> >                 (setfacl -k newdir 2>/dev/null || true) &&
> >                 umask 0077 &&
> >
> >                 # Test both files (f1) and leading dirs (d)
> >                 mkdir d &&
> >                 touch f1 d/f2 &&
> >                 git add f1 d/f2 &&
> >                 git diff --staged >patch-f1-and-f2.txt &&
>
> ... "point X" (see below) ...
>
> >
> >                 rm -rf d f1 &&
> >                 git apply patch-f1-and-f2.txt &&
> >
> >                 echo "-rw-------" >f1_mode.expected &&
> >                 echo "drwx------" >d_mode.expected &&
> >                 test_modebits f1 >f1_mode.actual &&
> >                 test_modebits d >d_mode.actual &&
> >                 test_cmp f1_mode.expected f1_mode.actual &&
> >                 test_cmp d_mode.expected d_mode.actual
> >         )
> > ...
> > +++ diff -u f1_mode.expected f1_mode.actual
> > --- f1_mode.expected    2020-12-19 16:50:20.169378700 +0000
> > +++ f1_mode.actual      2020-12-19 16:50:20.249126000 +0000
> > @@ -1 +1 @@
> > --rw-------
> > +-rw-rw-r--
>
> This tells us that we are getting the umask (set to "me only")
> ignored by "git apply".
>
> What would we see in the "t4129-*.sh -x" output if we inserted
>
>                 ls -ld f1 d d/f2 &&
>
> at "point X" above?

Additional output as below:

++ ls -ld f1 d d/f2
drwxrwxr-x+ 1 Adam None 0 Dec 19 18:57 d
-rw-rw-r--+ 1 Adam None 0 Dec 19 18:57 d/f2
-rw-rw-r--+ 1 Adam None 0 Dec 19 18:57 f1

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Adam Dinwoodie]

On Sat, 19 Dec 2020 at 18:34, Achim Gratz <Stromeko@nexgo.de> wrote:
>
> Adam Dinwoodie writes:
> > To my surprise, the test fails if the worktree is under "/cygdrive",
>
> /cygdrive is normally mounted with "posix=0", which only affects case
> sensitivity, so that isn't the reason for this particular fail.  You
> should anyway not build a Cygwin package with that option in effect,
> instead create your own mount point for that directory (with
> "binary,user" options).
>
> > +++ diff -u f1_mode.expected f1_mode.actual
> > --- f1_mode.expected    2020-12-19 16:50:20.169378700 +0000
> > +++ f1_mode.actual      2020-12-19 16:50:20.249126000 +0000
> > @@ -1 +1 @@
> > --rw-------
> > +-rw-rw-r--
>
> You seemingly can't change the ACL and/or several mode bits and see the
> effective access that your euid / egid has instead.  It is possible to
> set up the (default) ACL in a way that removes the permission to change
> them while otherwise still giving you what is effectively full access,
> in which case the test fail is the result of an inability to remove the
> default ACL from the directory.  I suspect your build directory is owned
> by a different user than the one you're building with and/or has been
> moved or re-used from another Windows installation that has different
> SID.

Having done a bit more digging, you're (unsurprisingly) right that
this seems to be about permissions rather than mount points per se. I
see the same failure with a build in
/cygdrive/c/Users/Adam/Documents/git, though, where that directory was
created solely using Git commands with the installed version of Cygwin
Git (v2.29.2-1). I'm using a test VM here that was created from
scratch solely to run these tests, and where there has only ever been
a single login user account, so the permissions setup should be about
as straightforward as they possibly could be.

This seems like a scenario that Cygwin should be able to handle, but I
don't have a clear enough grasp of how Windows ACLs work in normal
circumstances, let alone when Cygwin is handling them in its
non-standard ways, to know what an appropriate solution here is. "Only
ever build things within the Cygwin home directory" seems like a
decidedly suboptimal workaround, though.

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Achim Gratz]

Adam Dinwoodie writes:
> Having done a bit more digging, you're (unsurprisingly) right that
> this seems to be about permissions rather than mount points per se. I
> see the same failure with a build in
> /cygdrive/c/Users/Adam/Documents/git, though, where that directory was
> created solely using Git commands with the installed version of Cygwin
> Git (v2.29.2-1).

Windows is "protecting" various directories and that can get in the way
as well.

> I'm using a test VM here that was created from
> scratch solely to run these tests, and where there has only ever been
> a single login user account, so the permissions setup should be about
> as straightforward as they possibly could be.

You haven't shown what these are in detail, though.  Use getfacl to see
what Cygwin thinks the permissions are and icacls to get the Windows
view.  Once you know what the ACL look like it usually becomes clear
what you need to do to get what you want.  In your particular case I'd
try to recursively do a 'setfacl -kb' to remove all ACL and inheritable
defaults.  Again, it's possible that your user has insufficient
permisions to do that (which will then result in some ACL still present,
i.e. a '+' sign after the permission bits in 'ls -l' output).

Keep in mind that running things as a member of the Administrator group
usually confers some extra permissions on top of that, like
Backup/Restore privileges.

> This seems like a scenario that Cygwin should be able to handle, but I
> don't have a clear enough grasp of how Windows ACLs work in normal
> circumstances, let alone when Cygwin is handling them in its
> non-standard ways, to know what an appropriate solution here is. "Only
> ever build things within the Cygwin home directory" seems like a
> decidedly suboptimal workaround, though.

I have a dedicated build directory outside anything that Windows cares
about and mount that under /mnt/share from Cygwin.  I usually remove all
inheritable and default ACL on the toplevel directory before populating
it.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Adam Dinwoodie]

Cracked it, and it's a simple error in the test script. It wasn't
readily obvious because the error gets silently swallowed, and
presumably because the command isn't necessary on most *nix systems
that have different behaviour for inheriting permissions, but the
entire problem is fixed with the following diff:

--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -78,7 +78,7 @@
        test_config core.sharedRepository 0666 &&
        (
                # Remove a default ACL if possible.
-               (setfacl -k newdir 2>/dev/null || true) &&
+               (setfacl -k . 2>/dev/null || true) &&
                umask 0077 &&

                # Test both files (f1) and leading dirs (d)

It looks like the erroneous line was copied from t0001-init.sh, but
that's a test where "newdir" is actually an existent directory, where
we never use a directory of that name in this test script. A more
likely candidate in the circumstances would have been
t1301-shared-repo.sh, which does call `setfacl -k .` as part of its
setup.

I'm assuming this is a simple and obvious enough fix that it can just
get squashed into the original commit, but I don't know if that breaks
things given the original commit is now included in rc tags. Let me
know if I need to format and submit this as a full patch?

Adam

Re: [PATCH] apply: don't use core.sharedRepository to create working tree files

[Matheus Tavares Bernardino]

On Tue, Dec 22, 2020 at 7:24 PM Adam Dinwoodie <adam@dinwoodie.org> wrote:
>
> Cracked it, and it's a simple error in the test script. It wasn't
> readily obvious because the error gets silently swallowed, and
> presumably because the command isn't necessary on most *nix systems
> that have different behaviour for inheriting permissions, but the
> entire problem is fixed with the following diff:
>
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -78,7 +78,7 @@
>         test_config core.sharedRepository 0666 &&
>         (
>                 # Remove a default ACL if possible.
> -               (setfacl -k newdir 2>/dev/null || true) &&
> +               (setfacl -k . 2>/dev/null || true) &&
>                 umask 0077 &&
>
>                 # Test both files (f1) and leading dirs (d)
>
> It looks like the erroneous line was copied from t0001-init.sh, but
> that's a test where "newdir" is actually an existent directory, where
> we never use a directory of that name in this test script.

My bad, I should have been more careful here. Thanks for finding the problem!

> I'm assuming this is a simple and obvious enough fix that it can just
> get squashed into the original commit, but I don't know if that breaks
> things given the original commit is now included in rc tags. Let me
> know if I need to format and submit this as a full patch?

Yeah, since the original patch is already merged into `master`, I
think a new patch fixing the problem would be more appropriate.

Thanks,
Matheus

[PATCH] t4129: fix setfacl-related permissions failure

[Adam Dinwoodie]

When running this test in Cygwin, it's necessary to remove the inherited
access control lists from the Git working directory in order for later
permissions tests to work as expected.

As such, fix an error in the test script so that the ACLs are set for
the working directory, not a nonexistent subdirectory.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---
 t/t4129-apply-samemode.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 41818d8315..576632f868 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -78,7 +78,7 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
 	test_config core.sharedRepository 0666 &&
 	(
 		# Remove a default ACL if possible.
-		(setfacl -k newdir 2>/dev/null || true) &&
+		(setfacl -k . 2>/dev/null || true) &&
 		umask 0077 &&
 
 		# Test both files (f1) and leading dirs (d)
-- 
2.30.0.rc1

Re: [PATCH] t4129: fix setfacl-related permissions failure

[Matheus Tavares Bernardino]

Hi, Adam

Apologies for the late reply.

On Wed, Dec 23, 2020 at 8:44 AM Adam Dinwoodie <adam@dinwoodie.org> wrote:
>
> When running this test in Cygwin, it's necessary to remove the inherited
> access control lists from the Git working directory in order for later
> permissions tests to work as expected.

Nit: Although this sentence is correct and the bug was first found on
Cygwin, the test may fail in any other environment which has a default
ACL set. In this sense, I think we could perhaps rephrase the commit
message to something like this:

This test creates a couple files and expects their permissions to be
based on the umask. However, if the test's directory has a default ACL
set, it will be inherited by the created files, overriding the umask.
To work around that, the test attempts to remove the default ACL, but
it erroneously passes a nonexistent path to the setfacl command. Fix
that by passing the working directory.

> ---
>  t/t4129-apply-samemode.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index 41818d8315..576632f868 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -78,7 +78,7 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
>         test_config core.sharedRepository 0666 &&
>         (
>                 # Remove a default ACL if possible.
> -               (setfacl -k newdir 2>/dev/null || true) &&
> +               (setfacl -k . 2>/dev/null || true) &&

The change is obviously correct, thanks!

Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br>

Re: [PATCH] t4129: fix setfacl-related permissions failure

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> Hi, Adam
>
> Apologies for the late reply.
>
> On Wed, Dec 23, 2020 at 8:44 AM Adam Dinwoodie <adam@dinwoodie.org> wrote:
>>
>> When running this test in Cygwin, it's necessary to remove the inherited
>> access control lists from the Git working directory in order for later
>> permissions tests to work as expected.
>
> Nit: Although this sentence is correct and the bug was first found on
> Cygwin, the test may fail in any other environment which has a default
> ACL set. In this sense, I think we could perhaps rephrase the commit
> message to something like this:
>
> This test creates a couple files and expects their permissions to be
> based on the umask. However, if the test's directory has a default ACL
> set, it will be inherited by the created files, overriding the umask.
> To work around that, the test attempts to remove the default ACL, but
> it erroneously passes a nonexistent path to the setfacl command. Fix
> that by passing the working directory.
>
>> ---
>>  t/t4129-apply-samemode.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
>> index 41818d8315..576632f868 100755
>> --- a/t/t4129-apply-samemode.sh
>> +++ b/t/t4129-apply-samemode.sh
>> @@ -78,7 +78,7 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
>>         test_config core.sharedRepository 0666 &&
>>         (
>>                 # Remove a default ACL if possible.
>> -               (setfacl -k newdir 2>/dev/null || true) &&
>> +               (setfacl -k . 2>/dev/null || true) &&
>
> The change is obviously correct, thanks!
>
> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br>

Thanks, both.  Will queue.