{"thread":{"id":"37980","subject":"[PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","startedAt":"2014-11-16T07:21:46Z","lastAt":"2014-11-17T16:43:59Z","messageCount":14,"participants":["Michael Haggerty","Johannes Sixt","Junio C Hamano","Eric Sunshine","Torsten Bögershausen"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"251955","messageId":"1416122508-30654-1-git-send-email-mhagger@alum.mit.edu","threadId":"37980","inReplyTo":null,"subject":"[PATCH v2 0/2] Don't make $GIT_DIR executable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-16T07:21:46Z","receivedAt":"2014-11-16T07:21:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Changes since v1 [1]:\n\n* Check the chmod() return result in create_default_files(), as\n suggested by Torsten Bögershausen.\n\n* Fix a comment typo found by Stefan Beller.\n\n* Extend patch 2/2 to also clearing the executable bits when \"git\n config --edit\" is run.\n\n* Add test cases to patch 2/2 that the executable bits really are\n cleaned up when they should be.\n\nThanks to Stefan Beller, Torsten Bögershausen, and Eric Wong for their\nfeedback about v1.\n\nI have also pushed this series to my GitHub fork [2].\n\nPlease remember that this patch series applies to maint. This version\nhas a couple of conflicts with master; I have pushed my proposed\nconflict resolution to GitHub [3], including a preparatory commit that\nI recommend for master.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/259620/focus=259620\n[2] https://github.com/mhagger/git branch \"config-non-executable\"\n[3] https://github.com/mhagger/git branch \"config-non-executable-merge\"\n\nMichael Haggerty (2):\n create_default_files(): don't set u+x bit on $GIT_DIR/config\n config: clear the executable bits (if any) on $GIT_DIR/config\n\n builtin/config.c | 21 ++++++++++++++++++---\n builtin/init-db.c | 1 +\n config.c | 12 ++++++++++--\n t/t1300-repo-config.sh | 13 +++++++++++++\n 4 files changed, 42 insertions(+), 5 deletions(-)\n\n-- \n2.1.1\n"},{"id":"251954","messageId":"1416122508-30654-2-git-send-email-mhagger@alum.mit.edu","threadId":"37980","inReplyTo":"1416122508-30654-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-16T07:21:47Z","receivedAt":"2014-11-16T07:21:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since time immemorial, the test of whether to set \"core.filemode\" has\nbeen done by trying to toggle the u+x bit on $GIT_DIR/config and then\ntesting whether the change \"took\". It is somewhat odd to use the\nconfig file for this test, but whatever.\n\nThe test code didn't set the u+x bit back to its original state\nitself, instead relying on the subsequent call to git_config_set() to\nre-write the config file with correct permissions.\n\nBut ever since\n\n daa22c6f8d config: preserve config file permissions on edits (2014-05-06)\n\ngit_config_set() copies the permissions from the old config file to\nthe new one. This is a good change in and of itself, but it interacts\nbadly with create_default_files()'s sloppiness, causing \"git init\" to\nleave the executable bit set on $GIT_DIR/config.\n\nSo change create_default_files() to reset the permissions on\n$GIT_DIR/config after its test.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/init-db.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 56f85e2..4c8021d 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)\n \t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n \t\t\t\t!lstat(path, &st2) &&\n \t\t\t\tst1.st_mode != st2.st_mode);\n+\t\tfilemode &= !chmod(path, st1.st_mode);\n \t}\n \tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n \n-- \n2.1.1\n"},{"id":"251956","messageId":"1416122508-30654-3-git-send-email-mhagger@alum.mit.edu","threadId":"37980","inReplyTo":"1416122508-30654-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-16T07:21:48Z","receivedAt":"2014-11-16T07:21:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no reason for $GIT_DIR/config to be executable, plus there\nused to be a bug (fixed by the previous commit) that caused \"git init\"\nto set the u+x bit on that file. So whenever rewriting the config\nfile, take the opportunity to remove any executable bits that the file\nmight have.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/config.c | 21 ++++++++++++++++++---\n config.c | 12 ++++++++++--\n t/t1300-repo-config.sh | 13 +++++++++++++\n 3 files changed, 41 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 7bba516..1a7c17e 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -551,6 +551,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \telse if (actions == ACTION_EDIT) {\n+\t\tchar *config_file;\n+\t\tstruct stat st;\n+\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (!given_config_source.file && nongit)\n \t\t\tdie(\"not in a git directory\");\n@@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (given_config_source.blob)\n \t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tlaunch_editor(given_config_source.file ?\n-\t\t\t given_config_source.file : git_path(\"config\"),\n-\t\t\t NULL, NULL);\n+\t\tconfig_file = xstrdup(given_config_source.file ?\n+\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\t\tlaunch_editor(config_file, NULL, NULL);\n+\n+\t\t/*\n+\t\t * In git 2.1, there was a bug in \"git init\" that left\n+\t\t * the u+x bit set on the config file. To clean up any\n+\t\t * repositories affected by that bug, and just because\n+\t\t * it doesn't make sense for a config file to be\n+\t\t * executable anyway, clear any executable bits from\n+\t\t * the file (on a \"best effort\" basis):\n+\t\t */\n+\t\tif (!lstat(config_file, &st) && (st.st_mode & 0111))\n+\t\t\tchmod(config_file, st.st_mode & 07666);\n+\t\tfree(config_file);\n \t}\n \telse if (actions == ACTION_SET) {\n \t\tint ret;\ndiff --git a/config.c b/config.c\nindex 9e42d38..47eaef4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1653,7 +1653,15 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\tMAP_PRIVATE, in_fd, 0);\n \t\tclose(in_fd);\n \n-\t\tif (chmod(lock->filename, st.st_mode & 07777) < 0) {\n+\t\t/*\n+\t\t * We mask off the executable bits because (a) it\n+\t\t * doesn't make sense to have executable bits set on\n+\t\t * the config file, and (b) there was a bug in git 2.1\n+\t\t * which caused the config file to be created with u+x\n+\t\t * set, so this will help repair repositories created\n+\t\t * with that version.\n+\t\t */\n+\t\tif (chmod(lock->filename, st.st_mode & 07666) < 0) {\n \t\t\terror(\"chmod on %s failed: %s\",\n \t\t\t\tlock->filename, strerror(errno));\n \t\t\tret = CONFIG_NO_WRITE;\n@@ -1832,7 +1840,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \n \tfstat(fileno(config_file), &st);\n \n-\tif (chmod(lock->filename, st.st_mode & 07777) < 0) {\n+\tif (chmod(lock->filename, st.st_mode & 07666) < 0) {\n \t\tret = error(\"chmod on %s failed: %s\",\n \t\t\t\tlock->filename, strerror(errno));\n \t\tgoto out;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 46f6ae2..7637701 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -7,6 +7,12 @@ test_description='Test git config in different settings'\n \n . ./test-lib.sh\n \n+test_expect_success POSIXPERM 'any executable bits cleared' '\n+\tchmod u+x .git/config &&\n+\tgit config test.me foo &&\n+\ttest ! -x .git/config\n+'\n+\n test_expect_success 'clear default config' '\n \trm -f .git/config\n '\n@@ -1078,6 +1084,13 @@ test_expect_success 'git config --edit respects core.editor' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success POSIXPERM 'git config --edit clears executable bits' '\n+\tgit config -f tmp test.value no &&\n+\tchmod u+x tmp &&\n+\tGIT_EDITOR=\"echo [test]value=yes >\" git config -f tmp --edit &&\n+\ttest ! -x tmp\n+'\n+\n # malformed configuration files\n test_expect_success 'barf on syntax error' '\n \tcat >.git/config <<-\\EOF &&\n-- \n2.1.1\n"},{"id":"251958","messageId":"54685B05.90007@kdbg.org","threadId":"37980","inReplyTo":"1416122508-30654-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-11-16T08:06:29Z","receivedAt":"2014-11-16T08:06:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.11.2014 um 08:21 schrieb Michael Haggerty:\n> @@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> \t\tif (given_config_source.blob)\n> \t\t\tdie(\"editing blobs is not supported\");\n> \t\tgit_config(git_default_config, NULL);\n> -\t\tlaunch_editor(given_config_source.file ?\n> -\t\t\t given_config_source.file : git_path(\"config\"),\n> -\t\t\t NULL, NULL);\n> +\t\tconfig_file = xstrdup(given_config_source.file ?\n> +\t\t\t\t given_config_source.file : git_path(\"config\"));\n> +\t\tlaunch_editor(config_file, NULL, NULL);\n> +\n> +\t\t/*\n> +\t\t * In git 2.1, there was a bug in \"git init\" that left\n> +\t\t * the u+x bit set on the config file. To clean up any\n> +\t\t * repositories affected by that bug, and just because\n> +\t\t * it doesn't make sense for a config file to be\n> +\t\t * executable anyway, clear any executable bits from\n> +\t\t * the file (on a \"best effort\" basis):\n> +\t\t */\n> +\t\tif (!lstat(config_file, &st) && (st.st_mode & 0111))\n\nAt this point we cannot be sure that config_file is a regular file, can\nwe? It could also be a symbolic link. Wouldn't plain stat() be more\ncorrect then?\n\n> +\t\t\tchmod(config_file, st.st_mode & 07666);\n> +\t\tfree(config_file);\n> \t}\n> \telse if (actions == ACTION_SET) {\n> \t\tint ret;\n\n-- Hannes\n"},{"id":"251968","messageId":"xmqqk32vyndg.fsf@gitster.dls.corp.google.com","threadId":"37980","inReplyTo":"1416122508-30654-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-16T19:08:27Z","receivedAt":"2014-11-16T19:08:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Since time immemorial, the test of whether to set \"core.filemode\" has\n> been done by trying to toggle the u+x bit on $GIT_DIR/config and then\n> testing whether the change \"took\". It is somewhat odd to use the\n> config file for this test, but whatever.\n\nThe last sentence should read \"We could create a test file and use\nit for this purpose and then remove it, but config is a file we know\nexists at this point in the code (and it is the only file we know\nthat exists), so it was a very sensible trick\".\n\nOr remove it altogether. In other words, do not sound as if you do\nnot know what you are doing in your log message. That would rob\nconfidence in the change from the person who is reading \"git log\"\noutput later.\n\n> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)\n> \t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n> \t\t\t\t!lstat(path, &st2) &&\n> \t\t\t\tst1.st_mode != st2.st_mode);\n> +\t\tfilemode &= !chmod(path, st1.st_mode);\n\nSounds good.\n\nYou could also &&-chain this \"flip it back\" to the above statement.\nIf filemode is not trustable on a filesytem, doing one extra chmod()\nto correct would not help us anyway, no?\n\n\n> \t}\n> \tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n"},{"id":"251974","messageId":"CAPig+cQ6j-3_Ng8DVT3FYk8T6DippEbYDhQq5v3DTJhGgBhPDQ@mail.gmail.com","threadId":"37980","inReplyTo":"1416122508-30654-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-17T01:40:12Z","receivedAt":"2014-11-17T01:40:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty wrote:\n> Since time immemorial, the test of whether to set \"core.filemode\" has\n> been done by trying to toggle the u+x bit on $GIT_DIR/config and then\n> testing whether the change \"took\". It is somewhat odd to use the\n> config file for this test, but whatever.\n>\n> The test code didn't set the u+x bit back to its original state\n> itself, instead relying on the subsequent call to git_config_set() to\n> re-write the config file with correct permissions.\n>\n> But ever since\n>\n> daa22c6f8d config: preserve config file permissions on edits (2014-05-06)\n>\n> git_config_set() copies the permissions from the old config file to\n> the new one. This is a good change in and of itself, but it interacts\n> badly with create_default_files()'s sloppiness, causing \"git init\" to\n> leave the executable bit set on $GIT_DIR/config.\n>\n> So change create_default_files() to reset the permissions on\n> $GIT_DIR/config after its test.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nShould this patch include a test in t1300 to ensure that this bug does\nnot resurface (and to prove that this patch indeed fixes it)?\n\n> builtin/init-db.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 56f85e2..4c8021d 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)\n> filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n> !lstat(path, &st2) &&\n> st1.st_mode != st2.st_mode);\n> + filemode &= !chmod(path, st1.st_mode);\n> }\n> git_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n>\n> --\n> 2.1.1\n"},{"id":"251977","messageId":"5469B9FE.5070001@alum.mit.edu","threadId":"37980","inReplyTo":"54685B05.90007@kdbg.org","subject":"Re: [PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-17T09:03:58Z","receivedAt":"2014-11-17T09:03:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/16/2014 09:06 AM, Johannes Sixt wrote:\n> Am 16.11.2014 um 08:21 schrieb Michael Haggerty:\n>> @@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>> \t\tif (given_config_source.blob)\n>> \t\t\tdie(\"editing blobs is not supported\");\n>> \t\tgit_config(git_default_config, NULL);\n>> -\t\tlaunch_editor(given_config_source.file ?\n>> -\t\t\t given_config_source.file : git_path(\"config\"),\n>> -\t\t\t NULL, NULL);\n>> +\t\tconfig_file = xstrdup(given_config_source.file ?\n>> +\t\t\t\t given_config_source.file : git_path(\"config\"));\n>> +\t\tlaunch_editor(config_file, NULL, NULL);\n>> +\n>> +\t\t/*\n>> +\t\t * In git 2.1, there was a bug in \"git init\" that left\n>> +\t\t * the u+x bit set on the config file. To clean up any\n>> +\t\t * repositories affected by that bug, and just because\n>> +\t\t * it doesn't make sense for a config file to be\n>> +\t\t * executable anyway, clear any executable bits from\n>> +\t\t * the file (on a \"best effort\" basis):\n>> +\t\t */\n>> +\t\tif (!lstat(config_file, &st) && (st.st_mode & 0111))\n> \n> At this point we cannot be sure that config_file is a regular file, can\n> we? It could also be a symbolic link. Wouldn't plain stat() be more\n> correct then?\n\nYou make a good point. But I'm a little nervous about following symlinks\nand changing permissions on some distant file. Also, the bug that we are\ntrying clean up after would not have created a symlink in this place, so\nI think the cleanup is not so important if \"config\" is a symlink.\n\nSo I suggest that we stick with lstat(), but add S_ISREG(st.st_mode) to\nthe && chain above. Does that sound reasonable?\n\n>> +\t\t\tchmod(config_file, st.st_mode & 07666);\n>> +\t\tfree(config_file);\n>> \t}\n>> \telse if (actions == ACTION_SET) {\n>> \t\tint ret;\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"251978","messageId":"5469BAFA.7070709@web.de","threadId":"37980","inReplyTo":"CAPig+cQ6j-3_Ng8DVT3FYk8T6DippEbYDhQq5v3DTJhGgBhPDQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-11-17T09:08:10Z","receivedAt":"2014-11-17T09:08:10Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 11/17/2014 02:40 AM, Eric Sunshine wrote:\n> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty wrote:\n>> Since time immemorial, the test of whether to set \"core.filemode\" has\n>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then\n>> testing whether the change \"took\". It is somewhat odd to use the\n>> config file for this test, but whatever.\n>>\n>> The test code didn't set the u+x bit back to its original state\n>> itself, instead relying on the subsequent call to git_config_set() to\n>> re-write the config file with correct permissions.\n>>\n>> But ever since\n>>\n>> daa22c6f8d config: preserve config file permissions on edits (2014-05-06)\n>>\n>> git_config_set() copies the permissions from the old config file to\n>> the new one. This is a good change in and of itself, but it interacts\n>> badly with create_default_files()'s sloppiness, causing \"git init\" to\n>> leave the executable bit set on $GIT_DIR/config.\n>>\n>> So change create_default_files() to reset the permissions on\n>> $GIT_DIR/config after its test.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> Should this patch include a test in t1300 to ensure that this bug does\n> not resurface (and to prove that this patch indeed fixes it)?\n>\n>> builtin/init-db.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/builtin/init-db.c b/builtin/init-db.c\n>> index 56f85e2..4c8021d 100644\n>> --- a/builtin/init-db.c\n>> +++ b/builtin/init-db.c\n>> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)\n>> filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n>> !lstat(path, &st2) &&\n>> st1.st_mode != st2.st_mode);\n>> + filemode &= !chmod(path, st1.st_mode);\n>> }\n>> git_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n>>\n>> --\nSorry for the late reply, I actually had prepared a complete different patch\nfor a different problem, but it touches the very same lines of code.\n\n(And we may want to add a\n!chmod(path, st1.st_mode & ~S_IXUSR)\nat the end of the operation)\n\nThere are systems when a file created with 666 have 766, and we\ndo not handle this situation yet.\n\nMichael, if there is a chance that you integrate my small code changes \nin your patch ?\n\n-------------\n\ncommit 3228bedef6d45bfaf8986b6367f9388738476345\nAuthor: Torsten Bögershausen \nDate: Sun Oct 19 00:15:00 2014 +0200\n\n Improve the filemode trustability check\n\n Some file systems do not fully support the executable bit:\n a) The user executable bit is always 0\n b) The user executable bit is always 1\n c) Is similar to b):\n When a file is created with mode 0666 the file mode on disc is 766\n and the user executable bit is 1 even if it should be 0 like b)\n\n There are smbfs implementations where the file mode can be \nmaintained\n locally and chmod -x changes the file mode from 0766 to 0666.\n When the file system is unmounted and remounted,\n the file mode is 0766 and executable bit is 1 again.\n\n A typical example for a) is a VFAT drive mounted with -onoexec,\n or cifs with -ofile_mode=0644.\n b) is VFAT mounted with -oexec or cifs is mounted with -ofile_mode=0755\n\n The behaviour described in c) has been observed when a Windows \nmachine with\n NTFS exports a share via smb (or afp) to Mac OS X.\n (CIFS is an enhanced version of SMB\n The command to mount on the command line may differ,\n e.g mount.cifs under Linux or mount_smbfs under Mac OS X)\n\n Case a) and b) are detected by the current code.\n Case c) qualifies as \"non trustable executable bit\",\n and core.filemode should be false by default.\n\n Solution:\n Detect when \".git/config\" has the user executable bit set after\n creat(\".git/config\", 0666) and set core.filemode to false.\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 587a505..d3e4fb3 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -249,13 +249,11 @@ static int create_default_files(const char \n*template_path)\n strcpy(path + len, \"config\");\n\n /* Check filemode trustability */\n- filemode = TEST_FILEMODE;\n- if (TEST_FILEMODE && !lstat(path, &st1)) {\n- struct stat st2;\n- filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n- !lstat(path, &st2) &&\n- st1.st_mode != st2.st_mode);\n- }\n+ filemode = TEST_FILEMODE &&\n+ !lstat(path, &st1) && !(st1.st_mode & S_IXUSR) &&\n+ !chmod(path, st1.st_mode | S_IXUSR) &&\n+ !lstat(path, &st1) && (st1.st_mode & S_IXUSR);\n+\n git_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n\n if (is_bare_repository())\n"},{"id":"251979","messageId":"5469C156.7000907@alum.mit.edu","threadId":"37980","inReplyTo":"xmqqk32vyndg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-17T09:35:18Z","receivedAt":"2014-11-17T09:35:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/16/2014 08:08 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Since time immemorial, the test of whether to set \"core.filemode\" has\n>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then\n>> testing whether the change \"took\". It is somewhat odd to use the\n>> config file for this test, but whatever.\n> \n> The last sentence should read \"We could create a test file and use\n> it for this purpose and then remove it, but config is a file we know\n> exists at this point in the code (and it is the only file we know\n> that exists), so it was a very sensible trick\".\n> \n> Or remove it altogether. In other words, do not sound as if you do\n> not know what you are doing in your log message. That would rob\n> confidence in the change from the person who is reading \"git log\"\n> output later.\n\nThe sentence is not meant to rob confidence in this change, but rather\nto stimulate the reader's critical thinking about nearby code that I am\n*not* changing.\n\nBy making this change without changing the function to use a temporary\nfile for its chmod experiments, I might otherwise give future readers\nthe impression that I like this shortcut, which I do not. For example,\nif the original code had used a temporary file rather than \"config\",\nthen we would never have had the bug that I'm fixing. The \"but whatever\"\nis meant to indicate that I don't disagree so strongly with the choice\nof tradeoffs made by the original author that I think it is worth changing.\n\nSo maybe I am a coward (or lazy) for not proposing to change to using a\ntemporary file instead. But since this patch is suggested for maint, I\nwanted to make the smallest change that would fix the bug.\n\nFeel free to delete the controversial sentence if you prefer.\n\n>> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)\n>> \t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n>> \t\t\t\t!lstat(path, &st2) &&\n>> \t\t\t\tst1.st_mode != st2.st_mode);\n>> +\t\tfilemode &= !chmod(path, st1.st_mode);\n> \n> Sounds good.\n> \n> You could also &&-chain this \"flip it back\" to the above statement.\n> If filemode is not trustable on a filesytem, doing one extra chmod()\n> to correct would not help us anyway, no?\n\nYes, that would be better. I will fix it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"251980","messageId":"5469C40B.4080601@alum.mit.edu","threadId":"37980","inReplyTo":"CAPig+cQ6j-3_Ng8DVT3FYk8T6DippEbYDhQq5v3DTJhGgBhPDQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-17T09:46:51Z","receivedAt":"2014-11-17T09:46:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/17/2014 02:40 AM, Eric Sunshine wrote:\n> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty wrote:\n>> Since time immemorial, the test of whether to set \"core.filemode\" has\n>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then\n>> testing whether the change \"took\". It is somewhat odd to use the\n>> config file for this test, but whatever.\n>>\n>> The test code didn't set the u+x bit back to its original state\n>> itself, instead relying on the subsequent call to git_config_set() to\n>> re-write the config file with correct permissions.\n>>\n>> But ever since\n>>\n>> daa22c6f8d config: preserve config file permissions on edits (2014-05-06)\n>>\n>> git_config_set() copies the permissions from the old config file to\n>> the new one. This is a good change in and of itself, but it interacts\n>> badly with create_default_files()'s sloppiness, causing \"git init\" to\n>> leave the executable bit set on $GIT_DIR/config.\n>>\n>> So change create_default_files() to reset the permissions on\n>> $GIT_DIR/config after its test.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n> Should this patch include a test in t1300 to ensure that this bug does\n> not resurface (and to prove that this patch indeed fixes it)?\n\nThis seems like a one-off bug caused by a specific instance of odd code.\nIt could only recur if somebody were to remove the line that I added,\nwhich would be a *very* odd mistake to make given that its purpose is\npretty obvious.\n\nI tested manually that this patch fixes the problem. Admittedly, my\nmanual testing was only on one particular version of Linux. But it seems\nto me that the function being used is sufficiently portable to be\ntrusted, and the fact that the same function is being used a few lines\nearlier suggests that any portability problems would have wider\nramifications anyway.\n\nSo in summary, I think the chance that such a test would ever catch a\nnew problem is too small to justify the effort of writing and\nmaintaining it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"251981","messageId":"5469DCCE.8040801@alum.mit.edu","threadId":"37980","inReplyTo":"5469BAFA.7070709@web.de","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-17T11:32:30Z","receivedAt":"2014-11-17T11:32:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/17/2014 10:08 AM, Torsten Bögershausen wrote:\n> On 11/17/2014 02:40 AM, Eric Sunshine wrote:\n>> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty\n>> wrote:\n>>> [...]\n> Sorry for the late reply, I actually had prepared a complete different\n> patch\n> for a different problem, but it touches the very same lines of code.\n> \n> (And we may want to add a\n> !chmod(path, st1.st_mode & ~S_IXUSR)\n> at the end of the operation)\n> \n> There are systems when a file created with 666 have 766, and we\n> do not handle this situation yet.\n> \n> Michael, if there is a chance that you integrate my small code changes\n> in your patch ?\n\nYour change seems like a good idea, but is is logically separate from\nmine and I think it would be awkward to yoke them together. Both changes\nare short enough that conflict resolution shouldn't be a big problem.\n\nIt will be easier for you to follow up on your patch if you submit it\nseparately. The first question you get might be: \"Should it be applied\nto maint or is it more suitable for master?\", which also hints at one of\nthe reasons that yoking the patches together might be awkward.\n\nRegarding your patch itself:\n\n> [...]\n> b) The user executable bit is always 1\n> c) Is similar to b):\n> When a file is created with mode 0666 the file mode on disc is 766\n> and the user executable bit is 1 even if it should be 0 like b)\n> \n> There are smbfs implementations where the file mode can be maintained\n> locally and chmod -x changes the file mode from 0766 to 0666.\n> When the file system is unmounted and remounted,\n> the file mode is 0766 and executable bit is 1 again.\n\nIt seems surprising that there isn't also a case (d) like (c) except\nthat a file created with mode 0666 initially has the correct mode, and\nthe executable bits can be maintained locally while the filesystem is\nmounted, but where executable bits are lost whenever the filesystem is\nunmounted and remounted. Does that case not arise, or is it simply that\nit is impossible to test for it in create_default_files()?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"251983","messageId":"xmqqbno5zvdm.fsf@gitster.dls.corp.google.com","threadId":"37980","inReplyTo":"5469C40B.4080601@alum.mit.edu","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-17T15:42:29Z","receivedAt":"2014-11-17T15:42:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This seems like a one-off bug caused by a specific instance of odd code.\n> It could only recur if somebody were to remove the line that I added,\n> which would be a *very* odd mistake to make given that its purpose is\n> pretty obvious.\n\nOr some other code that comes _after_ your new code touches the\nfile.\n\nYou cannot anticipate what mistake others make.\n\nShouldn't something like this be sufficient?\n\n t/t0001-init.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex e62c0ff..acdc1df 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -12,6 +12,13 @@ check_config () {\n \t\techo \"expected a directory $1, a file $1/config and $1/refs\"\n \t\treturn 1\n \tfi\n+\n+\tif ! test_have_prereq POSIXPERM || test -x \"$1/config\"\n+\tthen\n+\t\techo \"$1/config is executable?\"\n+\t\treturn 1\n+\tfi\n+\n \tbare=$(cd \"$1\" && git config --bool core.bare)\n \tworktree=$(cd \"$1\" && git config core.worktree) ||\n \tworktree=unset\n"},{"id":"251985","messageId":"546A1FFC.60605@alum.mit.edu","threadId":"37980","inReplyTo":"xmqqbno5zvdm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-17T16:19:08Z","receivedAt":"2014-11-17T16:19:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/17/2014 04:42 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This seems like a one-off bug caused by a specific instance of odd code.\n>> It could only recur if somebody were to remove the line that I added,\n>> which would be a *very* odd mistake to make given that its purpose is\n>> pretty obvious.\n> \n> Or some other code that comes _after_ your new code touches the\n> file.\n> \n> You cannot anticipate what mistake others make.\n\nAnd yet we do so all the time, adding tests for the things we consider\nmost likely to break in the future and omitting them for breakages that\nseem unlikely. Otherwise, what frees us from the obligation to add a '!\ntest -x \"$GIT_DIR/config\"' following every single git operation?\n\nBut it's OK with me, we can add a test.\n\n> Shouldn't something like this be sufficient?\n> \n> t/t0001-init.sh | 7 +++++++\n> 1 file changed, 7 insertions(+)\n> \n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index e62c0ff..acdc1df 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -12,6 +12,13 @@ check_config () {\n> \t\techo \"expected a directory $1, a file $1/config and $1/refs\"\n> \t\treturn 1\n> \tfi\n> +\n> +\tif ! test_have_prereq POSIXPERM || test -x \"$1/config\"\n> +\tthen\n> +\t\techo \"$1/config is executable?\"\n> +\t\treturn 1\n> +\tfi\n> +\n> \tbare=$(cd \"$1\" && git config --bool core.bare)\n> \tworktree=$(cd \"$1\" && git config core.worktree) ||\n> \tworktree=unset\n> \n\nI think the logic should be\n\n\tif test_have_prereq POSIXPERM && test -x \"$1/config\"\n\n, right? If the system doesn't have POSIXPERM, then aren't all bets off\nregarding file permissions?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"251988","messageId":"xmqq389hzsj4.fsf@gitster.dls.corp.google.com","threadId":"37980","inReplyTo":"546A1FFC.60605@alum.mit.edu","subject":"Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-17T16:43:59Z","receivedAt":"2014-11-17T16:43:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I think the logic should be\n>\n> \tif test_have_prereq POSIXPERM && test -x \"$1/config\"\n>\n> , right?\n\nYeah ;-)\n"}]}