{"thread":{"id":"54742","subject":"Is git-am expected to honor core.sharedRepository?","startedAt":"2020-12-01T15:24:57Z","lastAt":"2021-01-09T22:44:47Z","messageCount":17,"participants":["Matheus Tavares Bernardino","Junio C Hamano","Matheus Tavares","Adam Dinwoodie","Achim Gratz"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"411072","messageId":"CAHd-oW4yHSTYr0Gwn60tu2c7VY=PJbSbg23Z5Bd_11Do-+juGA@mail.gmail.com","threadId":"54742","inReplyTo":null,"subject":"Is git-am expected to honor core.sharedRepository?","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-12-01T15:23:55Z","receivedAt":"2020-12-01T15:24:57Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, everyone\n\nI'm not very familiar with this setting, but to my understanding it\nshould only affect files in $GIT_DIR not $GIT_WORK_TREE, is that\ncorrect? Nevertheless, apply and am (which uses apply) end up\nadjusting the permissions of created directories based on the setting.\nTo give an example:\n\nWe first commit the directory 'd':\n$ mkdir d\n$ touch d/f\n$ git add d\n$ git commit -m d\n$ ls -l\ndrwxr-xr-x 2 matheus matheus 60 dez 1 11:29 d\n\nThen we create a patch and use am to apply it:\n$ git format-patch -1\n$ git reset --hard HEAD~\n$ git config core.sharedRepository 0700\n$ git am *.patch\n\nThe setting was honored by am:\n$ ls -l\ndrwx--S--- 2 matheus matheus 60 dez 1 11:30 d\n\nAnd if we delete 'd' and check it out again, the setting is ignored:\n$ rm -rf d\n$ git checkout d\n$ ls -l\ndrwxr-xr-x 2 matheus matheus 60 dez 1 11:31 d\n\nIs this expected?\n\nIf not, the place to be changed is probably the\nsafe_create_leading_directories() call in apply.c. This function\ninternally calls adjust_shared_perm() to modify the permissions\naccording to core.sharedRepository, so we could probably pass a flag\nto skip this step. But this function has at least 15 callers, so\nshould we introduce a wrapper for the non-shared case, instead?\n\n(For some background, I stumbled across this while considering using\nsafe_create_leading_directories() for a parallel-checkout patch. But\nthen I noticed it adjusts the directories' permissions based on the\nsetting and I was worried whether it could be user for checkout.)\n\nThanks,\nMatheus\n"},{"id":"411074","messageId":"xmqqpn3tqugm.fsf@gitster.c.googlers.com","threadId":"54742","inReplyTo":"CAHd-oW4yHSTYr0Gwn60tu2c7VY=PJbSbg23Z5Bd_11Do-+juGA@mail.gmail.com","subject":"Re: Is git-am expected to honor core.sharedRepository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T17:58:33Z","receivedAt":"2020-12-01T17:59:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> If not, the place to be changed is probably the\n> safe_create_leading_directories() call in apply.c.\n\nhttps://lore.kernel.org/git/xmqqziglaxj4.fsf@gitster.mtv.corp.google.com/\n\nCalling adjust_shared_perm() on a path outside .git/ is a potential\nbug, as you found out, and definitely a bug if used on working tree\nfiles. We may want to share with only selected users in a group the\ncontents of the repository (e.g. allow local cloning from us), while\nallowing daemon-ish tools to scan what is in the working tree files\nwithout letting them touch what is in the repository, for example;\nadjust_shared_perm() is meant for .git/ repository files and\ntightening working tree files' permissions using it would break such\narrangement.\n\nI think bugreport uses scld, but it may be used to drop cruft inside\nthe working tree, but the files created are *not* to be \"git add\"ed,\nso the use case does not count as \"if used on working tree files\".\n\n> $ git commit -m d\n> $ ls -l\n> drwxr-xr-x 2 matheus matheus 60 dez 1 11:29 d\n> ...\n> Then we create a patch and use am to apply it:\n> The setting was honored by am:\n> $ ls -l\n> drwx--S--- 2 matheus matheus 60 dez 1 11:30 d\n\nHaving said that, I know it can be argued both ways. If we want to\nprotect .git/ contents in a certain way, it may be worth protecting\nthe files in the working tree in the same way as well. But at least\nthat is not the current rule is (even though as you found out we do\nhave bugs).\n\nThanks.\n\n\n"},{"id":"411124","messageId":"3f0403b84ab06b9deb7c5c189792bebe1db586a7.1606866276.git.matheus.bernardino@usp.br","threadId":"54742","inReplyTo":"xmqqpn3tqugm.fsf@gitster.c.googlers.com","subject":"[PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-12-01T23:45:04Z","receivedAt":"2020-12-01T23:45:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"core.sharedRepository defines which permissions Git should set when\ncreating files in $GIT_DIR, so that the repository may be shared with\nother users. But (in its current form) the setting shouldn't affect how\nfiles are created in the working tree. This is not respected by apply\nand am (which uses apply), when creating leading directories:\n\n$ cat d.patch\n diff --git a/d/f b/d/f\n new file mode 100644\n index 0000000..e69de29\n\nApply without the setting:\n$ umask 0077\n$ git apply d.patch\n$ ls -ld d\n drwx------\n\nApply with the setting:\n$ umask 0077\n$ git -c core.sharedRepository=0770 apply d.patch\n$ ls -ld d\n drwxrws---\n\nOnly the leading directories are affected. That's because they are\ncreated with safe_create_leading_directories(), which calls\nadjust_shared_perm() to set the directories' permissions based on\ncore.sharedRepository. To fix that, let's introduce a variant of this\nfunction that ignores the setting, and use it in apply. Also add a\nregression test and a note in the function documentation about the use\nof each variant according to the destination (working tree or git\ndir).\n\nSigned-off-by: Matheus Tavares \n---\n apply.c | 2 +-\n cache.h | 7 ++++++-\n sha1-file.c | 14 ++++++++++++--\n t/t4129-apply-samemode.sh | 26 ++++++++++++++++++++++++++\n t/test-lib-functions.sh | 4 ++--\n 5 files changed, 47 insertions(+), 6 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 359ceb632c..4a4e9a0158 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4409,7 +4409,7 @@ static int create_one_file(struct apply_state *state,\n \t\treturn 0;\n \n \tif (errno == ENOENT) {\n-\t\tif (safe_create_leading_directories(path))\n+\t\tif (safe_create_leading_directories_no_share(path))\n \t\t\treturn 0;\n \t\tres = try_create_file(state, path, mode, buf, size);\n \t\tif (res < 0)\ndiff --git a/cache.h b/cache.h\nindex e986cf4ea9..8d279bc110 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1255,7 +1255,11 @@ int adjust_shared_perm(const char *path);\n * safe_create_leading_directories() temporarily changes path while it\n * is working but restores it before returning.\n * safe_create_leading_directories_const() doesn't modify path, even\n- * temporarily.\n+ * temporarily. Both these variants adjust the permissions of the\n+ * created directories to honor core.sharedRepository, so they are best\n+ * suited for files inside the git dir. For working tree files, use\n+ * safe_create_leading_directories_no_share() instead, as it ignores\n+ * the core.sharedRepository setting.\n */\n enum scld_error {\n \tSCLD_OK = 0,\n@@ -1266,6 +1270,7 @@ enum scld_error {\n };\n enum scld_error safe_create_leading_directories(char *path);\n enum scld_error safe_create_leading_directories_const(const char *path);\n+enum scld_error safe_create_leading_directories_no_share(char *path);\n \n /*\n * Callback function for raceproof_create_file(). This function is\ndiff --git a/sha1-file.c b/sha1-file.c\nindex dd65bd5c68..c3c49d2fa5 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -291,7 +291,7 @@ int mkdir_in_gitdir(const char *path)\n \treturn adjust_shared_perm(path);\n }\n \n-enum scld_error safe_create_leading_directories(char *path)\n+static enum scld_error safe_create_leading_directories_1(char *path, int share)\n {\n \tchar *next_component = path + offset_1st_component(path);\n \tenum scld_error ret = SCLD_OK;\n@@ -337,7 +337,7 @@ enum scld_error safe_create_leading_directories(char *path)\n \t\t\t\tret = SCLD_VANISHED;\n \t\t\telse\n \t\t\t\tret = SCLD_FAILED;\n-\t\t} else if (adjust_shared_perm(path)) {\n+\t\t} else if (share && adjust_shared_perm(path)) {\n \t\t\tret = SCLD_PERMS;\n \t\t}\n \t\t*slash = slash_character;\n@@ -345,6 +345,16 @@ enum scld_error safe_create_leading_directories(char *path)\n \treturn ret;\n }\n \n+enum scld_error safe_create_leading_directories(char *path)\n+{\n+\treturn safe_create_leading_directories_1(path, 1);\n+}\n+\n+enum scld_error safe_create_leading_directories_no_share(char *path)\n+{\n+\treturn safe_create_leading_directories_1(path, 0);\n+}\n+\n enum scld_error safe_create_leading_directories_const(const char *path)\n {\n \tint save_errno;\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 5cdd76dfa7..41818d8315 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -73,4 +73,30 @@ test_expect_success FILEMODE 'bogus mode is rejected' '\n \ttest_i18ngrep \"invalid mode\" err\n '\n \n+test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree files' '\n+\tgit reset --hard &&\n+\ttest_config core.sharedRepository 0666 &&\n+\t(\n+\t\t# Remove a default ACL if possible.\n+\t\t(setfacl -k newdir 2>/dev/null || true) &&\n+\t\tumask 0077 &&\n+\n+\t\t# Test both files (f1) and leading dirs (d)\n+\t\tmkdir d &&\n+\t\ttouch f1 d/f2 &&\n+\t\tgit add f1 d/f2 &&\n+\t\tgit diff --staged >patch-f1-and-f2.txt &&\n+\n+\t\trm -rf d f1 &&\n+\t\tgit apply patch-f1-and-f2.txt &&\n+\n+\t\techo \"-rw-------\" >f1_mode.expected &&\n+\t\techo \"drwx------\" >d_mode.expected &&\n+\t\ttest_modebits f1 >f1_mode.actual &&\n+\t\ttest_modebits d >d_mode.actual &&\n+\t\ttest_cmp f1_mode.expected f1_mode.actual &&\n+\t\ttest_cmp d_mode.expected d_mode.actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 7ba3011b90..0f7f247159 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -367,9 +367,9 @@ test_chmod () {\n \tgit update-index --add \"--chmod=$@\"\n }\n \n-# Get the modebits from a file.\n+# Get the modebits from a file or directory.\n test_modebits () {\n-\tls -l \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n+\tls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n }\n \n # Unset a configuration variable, but don't fail if it doesn't exist.\n-- \n2.29.2\n\n"},{"id":"411125","messageId":"xmqqmtyxm51h.fsf@gitster.c.googlers.com","threadId":"54742","inReplyTo":"3f0403b84ab06b9deb7c5c189792bebe1db586a7.1606866276.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-02T00:21:14Z","receivedAt":"2020-12-02T00:21:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"8361e1d4 (Use sha1_file.c's mkdir-like routine in apply.c.,\n2006-02-03) is the ancient source of this behaviour change, it\nseems.\n"},{"id":"411177","messageId":"xmqqim9jn9rn.fsf@gitster.c.googlers.com","threadId":"54742","inReplyTo":"CAHd-oW4yHSTYr0Gwn60tu2c7VY=PJbSbg23Z5Bd_11Do-+juGA@mail.gmail.com","subject":"Re: Is git-am expected to honor core.sharedRepository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-02T22:06:04Z","receivedAt":"2020-12-02T22:06:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> (For some background, I stumbled across this while considering using\n> safe_create_leading_directories() for a parallel-checkout patch. But\n> then I noticed it adjusts the directories' permissions based on the\n> setting and I was worried whether it could be user for checkout.)\n\nForgot to respond to this part, but you are correct. \n\nAnything that tries to replace what is in entry.c must not trigger\nadjust_shared_perm() on files and directories in the working tree,\nand it is a no-no to call safe_create_leading_directories().\n\n"},{"id":"411196","messageId":"CAHd-oW4bkQ6uDxY87D-8r0E+756unTzmY8eFv_99z=oN2nu16A@mail.gmail.com","threadId":"54742","inReplyTo":"xmqqim9jn9rn.fsf@gitster.c.googlers.com","subject":"Re: Is git-am expected to honor core.sharedRepository?","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-12-03T01:44:44Z","receivedAt":"2020-12-03T01:46:00Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Dec 2, 2020 at 7:06 PM Junio C Hamano wrote:\n>\n> Matheus Tavares Bernardino writes:\n>\n> > (For some background, I stumbled across this while considering using\n> > safe_create_leading_directories() for a parallel-checkout patch. But\n> > then I noticed it adjusts the directories' permissions based on the\n> > setting and I was worried whether it could be user for checkout.)\n>\n> Forgot to respond to this part, but you are correct.\n>\n> Anything that tries to replace what is in entry.c must not trigger\n> adjust_shared_perm() on files and directories in the working tree,\n> and it is a no-no to call safe_create_leading_directories().\n\nGot it, thanks. I've adjusted the parallel-checkout patch to use the\n_no_share() scld variant from mt/do-not-use-scld-in-working-tree.\n"},{"id":"412658","messageId":"CA+kUOamDD_SDNLk3sPSwNAojrAAP+g38MjkfG4JMPRTGOVAKAQ@mail.gmail.com","threadId":"54742","inReplyTo":"3f0403b84ab06b9deb7c5c189792bebe1db586a7.1606866276.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2020-12-19T17:51:39Z","receivedAt":"2020-12-19T17:53:12Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"This commit – eb3c027e17 (\"apply: don't use core.sharedRepository to\ncreate working tree files\", 2020-12-01) – seems to have introduced a\nnew test failure in the Cygwin builds for v2.30.0-rc0, and which is\nstill present in rc1. I'm not quite sure I understand what the\nexpected behaviour here is, but I expect the issue is down to Cygwin's\nslightly odd file permission handling.\n\nTo my surprise, the test fails if the worktree is under \"/cygdrive\",\nbut not if it's under \"/home\" within the Cygwin filesystem. I expect\nthis is some complexity with Cygwin's mount handling, but it's not a\nfailure mode I've seen before. I'm also going to follow up on the\nCygwin mailing list to see if the folk with a better understanding of\nCygwin's filesystem wrangling have a better understanding of what's\ngoing on.\n\nExtract from the relevant section of ./t4129-apply-samemode.sh -x\noutput, when run with that commit checked out, below:\n\nexpecting success of 4129.10 'do not use core.sharedRepository for\nworking tree files':\n git reset --hard &&\n test_config core.sharedRepository 0666 &&\n (\n # Remove a default ACL if possible.\n (setfacl -k newdir 2>/dev/null || true) &&\n umask 0077 &&\n\n # Test both files (f1) and leading dirs (d)\n mkdir d &&\n touch f1 d/f2 &&\n git add f1 d/f2 &&\n git diff --staged >patch-f1-and-f2.txt &&\n\n rm -rf d f1 &&\n git apply patch-f1-and-f2.txt &&\n\n echo \"-rw-------\" >f1_mode.expected &&\n echo \"drwx------\" >d_mode.expected &&\n test_modebits f1 >f1_mode.actual &&\n test_modebits d >d_mode.actual &&\n test_cmp f1_mode.expected f1_mode.actual &&\n test_cmp d_mode.expected d_mode.actual\n )\n\n++ git reset --hard\nHEAD is now at e950771 initial\n++ test_config core.sharedRepository 0666\n++ config_dir=\n++ test core.sharedRepository = -C\n++ test_when_finished 'test_unconfig '\\''core.sharedRepository'\\'''\n++ test 0 = 0\n++ test_cleanup='{ test_unconfig '\\''core.sharedRepository'\\''\n } && (exit \"$eval_ret\"); eval_ret=$?; :'\n++ git config core.sharedRepository 0666\n++ setfacl -k newdir\n++ true\n++ umask 0077\n++ mkdir d\n++ touch f1 d/f2\n++ git add f1 d/f2\n++ git diff --staged\n++ rm -rf d f1\n++ git apply patch-f1-and-f2.txt\n++ echo -rw-------\n++ echo drwx------\n++ test_modebits f1\n++ ls -ld f1\n++ sed -e 's|^\\(..........\\).*|\\1|'\n++ test_modebits d\n++ ls -ld d\n++ sed -e 's|^\\(..........\\).*|\\1|'\n++ test_cmp f1_mode.expected f1_mode.actual\n++ test 2 -eq 2\n++ eval 'diff -u' '\"$@\"'\n+++ diff -u f1_mode.expected f1_mode.actual\n--- f1_mode.expected 2020-12-19 16:50:20.169378700 +0000\n+++ f1_mode.actual 2020-12-19 16:50:20.249126000 +0000\n@@ -1 +1 @@\n--rw-------\n+-rw-rw-r--\n++ test xf1_mode.expected = x-\n++ test -e f1_mode.expected\n++ test xf1_mode.actual = x-\n++ test -e f1_mode.actual\n++ return 1\nerror: last command exited with $?=1\n++ test_unconfig core.sharedRepository\n++ config_dir=\n++ test core.sharedRepository = -C\n++ git config --unset-all core.sharedRepository\n++ config_status=0\n++ case \"$config_status\" in\n++ return 0\n++ exit 1\n++ eval_ret=1\n++ :\n"},{"id":"412660","messageId":"xmqqtushoeaf.fsf@gitster.c.googlers.com","threadId":"54742","inReplyTo":"CA+kUOamDD_SDNLk3sPSwNAojrAAP+g38MjkfG4JMPRTGOVAKAQ@mail.gmail.com","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T18:12:56Z","receivedAt":"2020-12-19T18:13:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie writes:\n\n> Extract from the relevant section of ./t4129-apply-samemode.sh -x\n> output, when run with that commit checked out, below:\n>\n> expecting success of 4129.10 'do not use core.sharedRepository for\n> working tree files':\n> git reset --hard &&\n> test_config core.sharedRepository 0666 &&\n> (\n> # Remove a default ACL if possible.\n> (setfacl -k newdir 2>/dev/null || true) &&\n> umask 0077 &&\n>\n> # Test both files (f1) and leading dirs (d)\n> mkdir d &&\n> touch f1 d/f2 &&\n> git add f1 d/f2 &&\n> git diff --staged >patch-f1-and-f2.txt &&\n\n... \"point X\" (see below) ...\n\n>\n> rm -rf d f1 &&\n> git apply patch-f1-and-f2.txt &&\n>\n> echo \"-rw-------\" >f1_mode.expected &&\n> echo \"drwx------\" >d_mode.expected &&\n> test_modebits f1 >f1_mode.actual &&\n> test_modebits d >d_mode.actual &&\n> test_cmp f1_mode.expected f1_mode.actual &&\n> test_cmp d_mode.expected d_mode.actual\n> )\n> ...\n> +++ diff -u f1_mode.expected f1_mode.actual\n> --- f1_mode.expected 2020-12-19 16:50:20.169378700 +0000\n> +++ f1_mode.actual 2020-12-19 16:50:20.249126000 +0000\n> @@ -1 +1 @@\n> --rw-------\n> +-rw-rw-r--\n\nThis tells us that we are getting the umask (set to \"me only\")\nignored by \"git apply\".\n\nWhat would we see in the \"t4129-*.sh -x\" output if we inserted\n\n\t\tls -ld f1 d d/f2 &&\n\nat \"point X\" above?\n\nTHanks.\n"},{"id":"412662","messageId":"87y2ht4pfr.fsf@Rainer.invalid","threadId":"54742","inReplyTo":"CA+kUOamDD_SDNLk3sPSwNAojrAAP+g38MjkfG4JMPRTGOVAKAQ@mail.gmail.com","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Achim Gratz","fromEmail":"stromeko@nexgo.de","sentAt":"2020-12-19T18:32:24Z","receivedAt":"2020-12-19T18:33:11Z","isPatch":true,"sender":{"key":"stromeko@nexgo.de","avatar":null},"body":"Adam Dinwoodie writes:\n> To my surprise, the test fails if the worktree is under \"/cygdrive\",\n\n/cygdrive is normally mounted with \"posix=0\", which only affects case\nsensitivity, so that isn't the reason for this particular fail. You\nshould anyway not build a Cygwin package with that option in effect,\ninstead create your own mount point for that directory (with\n\"binary,user\" options).\n\n> +++ diff -u f1_mode.expected f1_mode.actual\n> --- f1_mode.expected 2020-12-19 16:50:20.169378700 +0000\n> +++ f1_mode.actual 2020-12-19 16:50:20.249126000 +0000\n> @@ -1 +1 @@\n> --rw-------\n> +-rw-rw-r--\n\nYou seemingly can't change the ACL and/or several mode bits and see the\neffective access that your euid / egid has instead. It is possible to\nset up the (default) ACL in a way that removes the permission to change\nthem while otherwise still giving you what is effectively full access,\nin which case the test fail is the result of an inability to remove the\ndefault ACL from the directory. I suspect your build directory is owned\nby a different user than the one you're building with and/or has been\nmoved or re-used from another Windows installation that has different\nSID.\n\n\nRegards,\nAchim.\n-- \n+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+\n\nFactory and User Sound Singles for Waldorf Blofeld:\nhttp://Synth.Stromeko.net/Downloads.html#WaldorfSounds\n\n"},{"id":"412664","messageId":"CA+kUOanL3Kix4iH8dvsj1sf75y_3+v4qYwDWseMtaRFBqKkNwg@mail.gmail.com","threadId":"54742","inReplyTo":"xmqqtushoeaf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2020-12-19T18:59:50Z","receivedAt":"2020-12-19T19:01:27Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Sat, 19 Dec 2020 at 18:13, Junio C Hamano wrote:\n> Adam Dinwoodie writes:\n> > Extract from the relevant section of ./t4129-apply-samemode.sh -x\n> > output, when run with that commit checked out, below:\n> >\n> > expecting success of 4129.10 'do not use core.sharedRepository for\n> > working tree files':\n> > git reset --hard &&\n> > test_config core.sharedRepository 0666 &&\n> > (\n> > # Remove a default ACL if possible.\n> > (setfacl -k newdir 2>/dev/null || true) &&\n> > umask 0077 &&\n> >\n> > # Test both files (f1) and leading dirs (d)\n> > mkdir d &&\n> > touch f1 d/f2 &&\n> > git add f1 d/f2 &&\n> > git diff --staged >patch-f1-and-f2.txt &&\n>\n> ... \"point X\" (see below) ...\n>\n> >\n> > rm -rf d f1 &&\n> > git apply patch-f1-and-f2.txt &&\n> >\n> > echo \"-rw-------\" >f1_mode.expected &&\n> > echo \"drwx------\" >d_mode.expected &&\n> > test_modebits f1 >f1_mode.actual &&\n> > test_modebits d >d_mode.actual &&\n> > test_cmp f1_mode.expected f1_mode.actual &&\n> > test_cmp d_mode.expected d_mode.actual\n> > )\n> > ...\n> > +++ diff -u f1_mode.expected f1_mode.actual\n> > --- f1_mode.expected 2020-12-19 16:50:20.169378700 +0000\n> > +++ f1_mode.actual 2020-12-19 16:50:20.249126000 +0000\n> > @@ -1 +1 @@\n> > --rw-------\n> > +-rw-rw-r--\n>\n> This tells us that we are getting the umask (set to \"me only\")\n> ignored by \"git apply\".\n>\n> What would we see in the \"t4129-*.sh -x\" output if we inserted\n>\n> ls -ld f1 d d/f2 &&\n>\n> at \"point X\" above?\n\nAdditional output as below:\n\n++ ls -ld f1 d d/f2\ndrwxrwxr-x+ 1 Adam None 0 Dec 19 18:57 d\n-rw-rw-r--+ 1 Adam None 0 Dec 19 18:57 d/f2\n-rw-rw-r--+ 1 Adam None 0 Dec 19 18:57 f1\n"},{"id":"412665","messageId":"CA+kUOam3h859kK76QuS9OFojeavXO15JNpinUQ0vPrAXrcsCoA@mail.gmail.com","threadId":"54742","inReplyTo":"87y2ht4pfr.fsf@Rainer.invalid","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2020-12-19T19:57:14Z","receivedAt":"2020-12-19T19:58:56Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Sat, 19 Dec 2020 at 18:34, Achim Gratz wrote:\n>\n> Adam Dinwoodie writes:\n> > To my surprise, the test fails if the worktree is under \"/cygdrive\",\n>\n> /cygdrive is normally mounted with \"posix=0\", which only affects case\n> sensitivity, so that isn't the reason for this particular fail. You\n> should anyway not build a Cygwin package with that option in effect,\n> instead create your own mount point for that directory (with\n> \"binary,user\" options).\n>\n> > +++ diff -u f1_mode.expected f1_mode.actual\n> > --- f1_mode.expected 2020-12-19 16:50:20.169378700 +0000\n> > +++ f1_mode.actual 2020-12-19 16:50:20.249126000 +0000\n> > @@ -1 +1 @@\n> > --rw-------\n> > +-rw-rw-r--\n>\n> You seemingly can't change the ACL and/or several mode bits and see the\n> effective access that your euid / egid has instead. It is possible to\n> set up the (default) ACL in a way that removes the permission to change\n> them while otherwise still giving you what is effectively full access,\n> in which case the test fail is the result of an inability to remove the\n> default ACL from the directory. I suspect your build directory is owned\n> by a different user than the one you're building with and/or has been\n> moved or re-used from another Windows installation that has different\n> SID.\n\nHaving done a bit more digging, you're (unsurprisingly) right that\nthis seems to be about permissions rather than mount points per se. I\nsee the same failure with a build in\n/cygdrive/c/Users/Adam/Documents/git, though, where that directory was\ncreated solely using Git commands with the installed version of Cygwin\nGit (v2.29.2-1). I'm using a test VM here that was created from\nscratch solely to run these tests, and where there has only ever been\na single login user account, so the permissions setup should be about\nas straightforward as they possibly could be.\n\nThis seems like a scenario that Cygwin should be able to handle, but I\ndon't have a clear enough grasp of how Windows ACLs work in normal\ncircumstances, let alone when Cygwin is handling them in its\nnon-standard ways, to know what an appropriate solution here is. \"Only\never build things within the Cygwin home directory\" seems like a\ndecidedly suboptimal workaround, though.\n"},{"id":"412668","messageId":"87pn354ijn.fsf@Rainer.invalid","threadId":"54742","inReplyTo":"CA+kUOam3h859kK76QuS9OFojeavXO15JNpinUQ0vPrAXrcsCoA@mail.gmail.com","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Achim Gratz","fromEmail":"stromeko@nexgo.de","sentAt":"2020-12-19T21:01:16Z","receivedAt":"2020-12-19T21:02:20Z","isPatch":true,"sender":{"key":"stromeko@nexgo.de","avatar":null},"body":"Adam Dinwoodie writes:\n> Having done a bit more digging, you're (unsurprisingly) right that\n> this seems to be about permissions rather than mount points per se. I\n> see the same failure with a build in\n> /cygdrive/c/Users/Adam/Documents/git, though, where that directory was\n> created solely using Git commands with the installed version of Cygwin\n> Git (v2.29.2-1).\n\nWindows is \"protecting\" various directories and that can get in the way\nas well.\n\n> I'm using a test VM here that was created from\n> scratch solely to run these tests, and where there has only ever been\n> a single login user account, so the permissions setup should be about\n> as straightforward as they possibly could be.\n\nYou haven't shown what these are in detail, though. Use getfacl to see\nwhat Cygwin thinks the permissions are and icacls to get the Windows\nview. Once you know what the ACL look like it usually becomes clear\nwhat you need to do to get what you want. In your particular case I'd\ntry to recursively do a 'setfacl -kb' to remove all ACL and inheritable\ndefaults. Again, it's possible that your user has insufficient\npermisions to do that (which will then result in some ACL still present,\ni.e. a '+' sign after the permission bits in 'ls -l' output).\n\nKeep in mind that running things as a member of the Administrator group\nusually confers some extra permissions on top of that, like\nBackup/Restore privileges.\n\n> This seems like a scenario that Cygwin should be able to handle, but I\n> don't have a clear enough grasp of how Windows ACLs work in normal\n> circumstances, let alone when Cygwin is handling them in its\n> non-standard ways, to know what an appropriate solution here is. \"Only\n> ever build things within the Cygwin home directory\" seems like a\n> decidedly suboptimal workaround, though.\n\nI have a dedicated build directory outside anything that Windows cares\nabout and mount that under /mnt/share from Cygwin. I usually remove all\ninheritable and default ACL on the toplevel directory before populating\nit.\n\n\nRegards,\nAchim.\n-- \n+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+\n\nFactory and User Sound Singles for Waldorf Blofeld:\nhttp://Synth.Stromeko.net/Downloads.html#WaldorfSounds\n\n"},{"id":"412866","messageId":"CA+kUOamSd_3z8LbYt8QRx==HauYXoCe95B5hAW1W-LdnwGP-xw@mail.gmail.com","threadId":"54742","inReplyTo":"87pn354ijn.fsf@Rainer.invalid","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2020-12-22T22:24:08Z","receivedAt":"2020-12-22T22:25:55Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"Cracked it, and it's a simple error in the test script. It wasn't\nreadily obvious because the error gets silently swallowed, and\npresumably because the command isn't necessary on most *nix systems\nthat have different behaviour for inheriting permissions, but the\nentire problem is fixed with the following diff:\n\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -78,7 +78,7 @@\n test_config core.sharedRepository 0666 &&\n (\n # Remove a default ACL if possible.\n- (setfacl -k newdir 2>/dev/null || true) &&\n+ (setfacl -k . 2>/dev/null || true) &&\n umask 0077 &&\n\n # Test both files (f1) and leading dirs (d)\n\nIt looks like the erroneous line was copied from t0001-init.sh, but\nthat's a test where \"newdir\" is actually an existent directory, where\nwe never use a directory of that name in this test script. A more\nlikely candidate in the circumstances would have been\nt1301-shared-repo.sh, which does call `setfacl -k .` as part of its\nsetup.\n\nI'm assuming this is a simple and obvious enough fix that it can just\nget squashed into the original commit, but I don't know if that breaks\nthings given the original commit is now included in rc tags. Let me\nknow if I need to format and submit this as a full patch?\n\nAdam\n"},{"id":"412869","messageId":"CAHd-oW7XJL_a1zMAUetHzvrh8DrLT4g2awv-fjbTdeLVLKVsew@mail.gmail.com","threadId":"54742","inReplyTo":"CA+kUOamSd_3z8LbYt8QRx==HauYXoCe95B5hAW1W-LdnwGP-xw@mail.gmail.com","subject":"Re: [PATCH] apply: don't use core.sharedRepository to create working tree files","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-12-22T22:49:26Z","receivedAt":"2020-12-22T22:50:21Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Dec 22, 2020 at 7:24 PM Adam Dinwoodie wrote:\n>\n> Cracked it, and it's a simple error in the test script. It wasn't\n> readily obvious because the error gets silently swallowed, and\n> presumably because the command isn't necessary on most *nix systems\n> that have different behaviour for inheriting permissions, but the\n> entire problem is fixed with the following diff:\n>\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -78,7 +78,7 @@\n> test_config core.sharedRepository 0666 &&\n> (\n> # Remove a default ACL if possible.\n> - (setfacl -k newdir 2>/dev/null || true) &&\n> + (setfacl -k . 2>/dev/null || true) &&\n> umask 0077 &&\n>\n> # Test both files (f1) and leading dirs (d)\n>\n> It looks like the erroneous line was copied from t0001-init.sh, but\n> that's a test where \"newdir\" is actually an existent directory, where\n> we never use a directory of that name in this test script.\n\nMy bad, I should have been more careful here. Thanks for finding the problem!\n\n> I'm assuming this is a simple and obvious enough fix that it can just\n> get squashed into the original commit, but I don't know if that breaks\n> things given the original commit is now included in rc tags. Let me\n> know if I need to format and submit this as a full patch?\n\nYeah, since the original patch is already merged into `master`, I\nthink a new patch fixing the problem would be more appropriate.\n\nThanks,\nMatheus\n"},{"id":"412917","messageId":"20201223114431.4595-1-adam@dinwoodie.org","threadId":"54742","inReplyTo":"CAHd-oW7XJL_a1zMAUetHzvrh8DrLT4g2awv-fjbTdeLVLKVsew@mail.gmail.com","subject":"[PATCH] t4129: fix setfacl-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2020-12-23T11:44:31Z","receivedAt":"2020-12-23T11:45:38Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"When running this test in Cygwin, it's necessary to remove the inherited\naccess control lists from the Git working directory in order for later\npermissions tests to work as expected.\n\nAs such, fix an error in the test script so that the ACLs are set for\nthe working directory, not a nonexistent subdirectory.\n\nSigned-off-by: Adam Dinwoodie \n---\n t/t4129-apply-samemode.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 41818d8315..576632f868 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -78,7 +78,7 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \ttest_config core.sharedRepository 0666 &&\n \t(\n \t\t# Remove a default ACL if possible.\n-\t\t(setfacl -k newdir 2>/dev/null || true) &&\n+\t\t(setfacl -k . 2>/dev/null || true) &&\n \t\tumask 0077 &&\n \n \t\t# Test both files (f1) and leading dirs (d)\n-- \n2.30.0.rc1\n\n"},{"id":"413914","messageId":"CAHd-oW7r9D09F7=3JLTiQcbRDXyXhkYY3FFuRLbR9vH=p4M92w@mail.gmail.com","threadId":"54742","inReplyTo":"20201223114431.4595-1-adam@dinwoodie.org","subject":"Re: [PATCH] t4129: fix setfacl-related permissions failure","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-01-09T15:06:55Z","receivedAt":"2021-01-09T15:08:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Adam\n\nApologies for the late reply.\n\nOn Wed, Dec 23, 2020 at 8:44 AM Adam Dinwoodie wrote:\n>\n> When running this test in Cygwin, it's necessary to remove the inherited\n> access control lists from the Git working directory in order for later\n> permissions tests to work as expected.\n\nNit: Although this sentence is correct and the bug was first found on\nCygwin, the test may fail in any other environment which has a default\nACL set. In this sense, I think we could perhaps rephrase the commit\nmessage to something like this:\n\nThis test creates a couple files and expects their permissions to be\nbased on the umask. However, if the test's directory has a default ACL\nset, it will be inherited by the created files, overriding the umask.\nTo work around that, the test attempts to remove the default ACL, but\nit erroneously passes a nonexistent path to the setfacl command. Fix\nthat by passing the working directory.\n\n> ---\n> t/t4129-apply-samemode.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index 41818d8315..576632f868 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -78,7 +78,7 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n> test_config core.sharedRepository 0666 &&\n> (\n> # Remove a default ACL if possible.\n> - (setfacl -k newdir 2>/dev/null || true) &&\n> + (setfacl -k . 2>/dev/null || true) &&\n\nThe change is obviously correct, thanks!\n\nReviewed-by: Matheus Tavares \n"},{"id":"413940","messageId":"xmqqk0slenof.fsf@gitster.c.googlers.com","threadId":"54742","inReplyTo":"CAHd-oW7r9D09F7=3JLTiQcbRDXyXhkYY3FFuRLbR9vH=p4M92w@mail.gmail.com","subject":"Re: [PATCH] t4129: fix setfacl-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-09T22:43:44Z","receivedAt":"2021-01-09T22:44:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> Hi, Adam\n>\n> Apologies for the late reply.\n>\n> On Wed, Dec 23, 2020 at 8:44 AM Adam Dinwoodie wrote:\n>>\n>> When running this test in Cygwin, it's necessary to remove the inherited\n>> access control lists from the Git working directory in order for later\n>> permissions tests to work as expected.\n>\n> Nit: Although this sentence is correct and the bug was first found on\n> Cygwin, the test may fail in any other environment which has a default\n> ACL set. In this sense, I think we could perhaps rephrase the commit\n> message to something like this:\n>\n> This test creates a couple files and expects their permissions to be\n> based on the umask. However, if the test's directory has a default ACL\n> set, it will be inherited by the created files, overriding the umask.\n> To work around that, the test attempts to remove the default ACL, but\n> it erroneously passes a nonexistent path to the setfacl command. Fix\n> that by passing the working directory.\n>\n>> ---\n>> t/t4129-apply-samemode.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n>> index 41818d8315..576632f868 100755\n>> --- a/t/t4129-apply-samemode.sh\n>> +++ b/t/t4129-apply-samemode.sh\n>> @@ -78,7 +78,7 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n>> test_config core.sharedRepository 0666 &&\n>> (\n>> # Remove a default ACL if possible.\n>> - (setfacl -k newdir 2>/dev/null || true) &&\n>> + (setfacl -k . 2>/dev/null || true) &&\n>\n> The change is obviously correct, thanks!\n>\n> Reviewed-by: Matheus Tavares \n\nThanks, both. Will queue.\n"}]}