* Is git-am expected to honor core.sharedRepository? @ 2020-12-01 15:23 Matheus Tavares Bernardino 2020-12-01 17:58 ` Junio C Hamano 2020-12-02 22:06 ` Is git-am expected to honor core.sharedRepository? Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Matheus Tavares Bernardino @ 2020-12-01 15:23 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Is git-am expected to honor core.sharedRepository? 2020-12-01 15:23 Is git-am expected to honor core.sharedRepository? Matheus Tavares Bernardino @ 2020-12-01 17:58 ` Junio C Hamano 2020-12-01 23:45 ` [PATCH] apply: don't use core.sharedRepository to create working tree files Matheus Tavares 2020-12-02 22:06 ` Is git-am expected to honor core.sharedRepository? Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-12-01 17:58 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: git 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-01 17:58 ` Junio C Hamano @ 2020-12-01 23:45 ` Matheus Tavares 2020-12-02 0:21 ` Junio C Hamano 2020-12-19 17:51 ` Adam Dinwoodie 0 siblings, 2 replies; 17+ messages in thread From: Matheus Tavares @ 2020-12-01 23:45 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-01 23:45 ` [PATCH] apply: don't use core.sharedRepository to create working tree files Matheus Tavares @ 2020-12-02 0:21 ` Junio C Hamano 2020-12-19 17:51 ` Adam Dinwoodie 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2020-12-02 0:21 UTC (permalink / raw) To: Matheus Tavares; +Cc: git 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-01 23:45 ` [PATCH] apply: don't use core.sharedRepository to create working tree files Matheus Tavares 2020-12-02 0:21 ` Junio C Hamano @ 2020-12-19 17:51 ` Adam Dinwoodie 2020-12-19 18:12 ` Junio C Hamano 2020-12-19 18:32 ` Achim Gratz 1 sibling, 2 replies; 17+ messages in thread From: Adam Dinwoodie @ 2020-12-19 17:51 UTC (permalink / raw) To: Matheus Tavares; +Cc: Git Mailing List, Junio C Hamano 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 ++ : ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-19 17:51 ` Adam Dinwoodie @ 2020-12-19 18:12 ` Junio C Hamano 2020-12-19 18:59 ` Adam Dinwoodie 2020-12-19 18:32 ` Achim Gratz 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-12-19 18:12 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: Matheus Tavares, Git Mailing List 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-19 18:12 ` Junio C Hamano @ 2020-12-19 18:59 ` Adam Dinwoodie 0 siblings, 0 replies; 17+ messages in thread From: Adam Dinwoodie @ 2020-12-19 18:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matheus Tavares, Git Mailing List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-19 17:51 ` Adam Dinwoodie 2020-12-19 18:12 ` Junio C Hamano @ 2020-12-19 18:32 ` Achim Gratz 2020-12-19 19:57 ` Adam Dinwoodie 1 sibling, 1 reply; 17+ messages in thread From: Achim Gratz @ 2020-12-19 18:32 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-19 18:32 ` Achim Gratz @ 2020-12-19 19:57 ` Adam Dinwoodie 2020-12-19 21:01 ` Achim Gratz 0 siblings, 1 reply; 17+ messages in thread From: Adam Dinwoodie @ 2020-12-19 19:57 UTC (permalink / raw) To: Achim Gratz; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-19 19:57 ` Adam Dinwoodie @ 2020-12-19 21:01 ` Achim Gratz 2020-12-22 22:24 ` Adam Dinwoodie 0 siblings, 1 reply; 17+ messages in thread From: Achim Gratz @ 2020-12-19 21:01 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-19 21:01 ` Achim Gratz @ 2020-12-22 22:24 ` Adam Dinwoodie 2020-12-22 22:49 ` Matheus Tavares Bernardino 0 siblings, 1 reply; 17+ messages in thread From: Adam Dinwoodie @ 2020-12-22 22:24 UTC (permalink / raw) To: Matheus Tavares, Git Mailing List; +Cc: Achim Gratz, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: don't use core.sharedRepository to create working tree files 2020-12-22 22:24 ` Adam Dinwoodie @ 2020-12-22 22:49 ` Matheus Tavares Bernardino 2020-12-23 11:44 ` [PATCH] t4129: fix setfacl-related permissions failure Adam Dinwoodie 0 siblings, 1 reply; 17+ messages in thread From: Matheus Tavares Bernardino @ 2020-12-22 22:49 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: Git Mailing List, Achim Gratz, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] t4129: fix setfacl-related permissions failure 2020-12-22 22:49 ` Matheus Tavares Bernardino @ 2020-12-23 11:44 ` Adam Dinwoodie 2021-01-09 15:06 ` Matheus Tavares Bernardino 0 siblings, 1 reply; 17+ messages in thread From: Adam Dinwoodie @ 2020-12-23 11:44 UTC (permalink / raw) To: git; +Cc: Matheus Tavares Bernardino, Achim Gratz 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] t4129: fix setfacl-related permissions failure 2020-12-23 11:44 ` [PATCH] t4129: fix setfacl-related permissions failure Adam Dinwoodie @ 2021-01-09 15:06 ` Matheus Tavares Bernardino 2021-01-09 22:43 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Matheus Tavares Bernardino @ 2021-01-09 15:06 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: git, Achim Gratz 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t4129: fix setfacl-related permissions failure 2021-01-09 15:06 ` Matheus Tavares Bernardino @ 2021-01-09 22:43 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2021-01-09 22:43 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: Adam Dinwoodie, git, Achim Gratz 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Is git-am expected to honor core.sharedRepository? 2020-12-01 15:23 Is git-am expected to honor core.sharedRepository? Matheus Tavares Bernardino 2020-12-01 17:58 ` Junio C Hamano @ 2020-12-02 22:06 ` Junio C Hamano 2020-12-03 1:44 ` Matheus Tavares Bernardino 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-12-02 22:06 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: git 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(). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Is git-am expected to honor core.sharedRepository? 2020-12-02 22:06 ` Is git-am expected to honor core.sharedRepository? Junio C Hamano @ 2020-12-03 1:44 ` Matheus Tavares Bernardino 0 siblings, 0 replies; 17+ messages in thread From: Matheus Tavares Bernardino @ 2020-12-03 1:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-01-09 22:44 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-01 15:23 Is git-am expected to honor core.sharedRepository? Matheus Tavares Bernardino 2020-12-01 17:58 ` Junio C Hamano 2020-12-01 23:45 ` [PATCH] apply: don't use core.sharedRepository to create working tree files Matheus Tavares 2020-12-02 0:21 ` Junio C Hamano 2020-12-19 17:51 ` Adam Dinwoodie 2020-12-19 18:12 ` Junio C Hamano 2020-12-19 18:59 ` Adam Dinwoodie 2020-12-19 18:32 ` Achim Gratz 2020-12-19 19:57 ` Adam Dinwoodie 2020-12-19 21:01 ` Achim Gratz 2020-12-22 22:24 ` Adam Dinwoodie 2020-12-22 22:49 ` Matheus Tavares Bernardino 2020-12-23 11:44 ` [PATCH] t4129: fix setfacl-related permissions failure Adam Dinwoodie 2021-01-09 15:06 ` Matheus Tavares Bernardino 2021-01-09 22:43 ` Junio C Hamano 2020-12-02 22:06 ` Is git-am expected to honor core.sharedRepository? Junio C Hamano 2020-12-03 1:44 ` Matheus Tavares Bernardino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).