Message ID | 3f0403b84ab06b9deb7c5c189792bebe1db586a7.1606866276.git.matheus.bernardino@usp.br (mailing list archive) |
---|---|
State | Accepted |
Commit | c1c0a6c31e61807b776784fbfc1eb01571e788ee |
Headers | show |
Series | apply: don't use core.sharedRepository to create working tree files | expand |
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.
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 ++ :
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.
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.
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
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.
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.
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
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
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.
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(-)