{"thread":{"id":"30971","subject":"git-clone ignores umask for working tree","startedAt":"2012-07-06T19:27:29Z","lastAt":"2012-07-10T06:37:22Z","messageCount":10,"participants":["Alex Riesen","Daniel Barkalow","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"194748","messageId":"CALxABCZn5W-cEQ9PS+4XoqhH7L+5P3KN==_RrcruK+oKdijmWw@mail.gmail.com","threadId":"30971","inReplyTo":null,"subject":"git-clone ignores umask for working tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2012-07-06T19:27:29Z","receivedAt":"2012-07-06T19:27:29Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Hi list,\n\nwhen git-clone was built in, its treatment of umask has changed: the shell\nversion respected umask for newly created directories by using plain mkdir(1),\nand the builtin version just uses mkdir(work_tree, 0755).\n\nIs it intentional?\n\nThis Stackoverflow question is what got me interested:\n\nhttp://stackoverflow.com/questions/10637416/git-clone-respects-umask-except-for-top-level-project-directory\n\nand might provide a credible use case.\n\nRegards,\nAlex\n"},{"id":"194759","messageId":"alpine.LNX.2.00.1207061700060.2056@iabervon.org","threadId":"30971","inReplyTo":"CALxABCZn5W-cEQ9PS+4XoqhH7L+5P3KN==_RrcruK+oKdijmWw@mail.gmail.com","subject":"Re: git-clone ignores umask for working tree","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2012-07-06T21:20:15Z","receivedAt":"2012-07-06T21:20:15Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 6 Jul 2012, Alex Riesen wrote:\n\n> Hi list,\n> \n> when git-clone was built in, its treatment of umask has changed: the shell\n> version respected umask for newly created directories by using plain mkdir(1),\n> and the builtin version just uses mkdir(work_tree, 0755).\n>\n> Is it intentional?\n\nI have the vague feeling that it was intentional, but it's entirely \nplausible that I just overlooked that mkdir(2) applies umask and went for \nthe mode that you normally want. I don't think there's any particular need \nfor this operation to be more restrictive than umask.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"194776","messageId":"20120707215029.GA26819@blimp.dmz","threadId":"30971","inReplyTo":"alpine.LNX.2.00.1207061700060.2056@iabervon.org","subject":"[PATCH] Restore umasks influence on the permissions of work tree created by clone","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2012-07-07T21:50:30Z","receivedAt":"2012-07-07T21:50:30Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The original (shell coded) version of the git-clone just used mkdir(1)\nto create the working directories. The builtin changed the mode argument\nto mkdir(2) to 0755, which was a bit unfortunate, as there are use\ncases where umask-controlled creation is preferred and in any case\nit is a well-known behaviour for new directory/file creation.\n---\n\nOn Fri, 6 Jul 2012, Daniel Barkalow wrote:\n> On Fri, 6 Jul 2012, Alex Riesen wrote:\n>> when git-clone was built in, its treatment of umask has changed: the shell\n>> version respected umask for newly created directories by using plain mkdir(1),\n>> and the builtin version just uses mkdir(work_tree, 0755).\n>>\n>> Is it intentional?\n> \n> I have the vague feeling that it was intentional, but it's entirely \n> plausible that I just overlooked that mkdir(2) applies umask and went for \n> the mode that you normally want. I don't think there's any particular need \n> for this operation to be more restrictive than umask.\n\nI didn't look hard enough, but still, I found not much of complaining either\nway (frankly - none, but as I said, I didn'l look hard): none before - for\nbeing too permissive, the only one in original post after building the thing\nin - for being too restrictive.\n\nMaybe we should reconsider and go back to the old permission handling?\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex d3b7fdc..e314b0b 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -708,7 +708,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n \t\t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t\t work_tree);\n-\t\tif (!dest_exists && mkdir(work_tree, 0755))\n+\t\tif (!dest_exists && mkdir(work_tree, 0777))\n \t\t\tdie_errno(_(\"could not create work tree dir '%s'.\"),\n \t\t\t\t work_tree);\n \t\tset_git_work_tree(work_tree);\n-- \n1.7.11.1.185.g5abe2c9\n"},{"id":"194794","messageId":"7vobnpn224.fsf@alter.siamese.dyndns.org","threadId":"30971","inReplyTo":"20120707215029.GA26819@blimp.dmz","subject":"Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T01:41:39Z","receivedAt":"2012-07-09T01:41:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> The original (shell coded) version of the git-clone just used mkdir(1)\n> to create the working directories. The builtin changed the mode argument\n> to mkdir(2) to 0755, which was a bit unfortunate, as there are use\n\nA much more important reason why this is a good change (I think you\ncould even say this is a bugfix) is because directories and files in\nthe working tree are created with entry.c::create_directories() and\nentry.c::create_file(), and they do honour umask settings, and the\ntop-level of the working tree should be handled the same way, no?\n\n> cases where umask-controlled creation is preferred and in any case\n> it is a well-known behaviour for new directory/file creation.\n\n> ---\n\nSign-off?\n\n>\n> On Fri, 6 Jul 2012, Daniel Barkalow wrote:\n>> On Fri, 6 Jul 2012, Alex Riesen wrote:\n>>> when git-clone was built in, its treatment of umask has changed: the shell\n>>> version respected umask for newly created directories by using plain mkdir(1),\n>>> and the builtin version just uses mkdir(work_tree, 0755).\n>>>\n>>> Is it intentional?\n>> \n>> I have the vague feeling that it was intentional, but it's entirely \n>> plausible that I just overlooked that mkdir(2) applies umask and went for \n>> the mode that you normally want. I don't think there's any particular need \n>> for this operation to be more restrictive than umask.\n>\n> I didn't look hard enough, but still, I found not much of complaining either\n> way (frankly - none, but as I said, I didn'l look hard): none before - for\n> being too permissive, the only one in original post after building the thing\n> in - for being too restrictive.\n>\n> Maybe we should reconsider and go back to the old permission handling?\n>\n> builtin/clone.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index d3b7fdc..e314b0b 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -708,7 +708,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n> \t\t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n> \t\t\t\t work_tree);\n> -\t\tif (!dest_exists && mkdir(work_tree, 0755))\n> +\t\tif (!dest_exists && mkdir(work_tree, 0777))\n> \t\t\tdie_errno(_(\"could not create work tree dir '%s'.\"),\n> \t\t\t\t work_tree);\n> \t\tset_git_work_tree(work_tree);\n"},{"id":"194822","messageId":"CALxABCY=0J6FN7MHLst4mf3PBV729U=wpVB3XNR-wWopdQ23nA@mail.gmail.com","threadId":"30971","inReplyTo":"7vobnpn224.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2012-07-09T18:21:11Z","receivedAt":"2012-07-09T18:21:11Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Mon, Jul 9, 2012 at 3:41 AM, Junio C Hamano wrote:\n> Alex Riesen writes:\n>\n>> The original (shell coded) version of the git-clone just used mkdir(1)\n>> to create the working directories. The builtin changed the mode argument\n>> to mkdir(2) to 0755, which was a bit unfortunate, as there are use\n>\n> A much more important reason why this is a good change (I think you\n> could even say this is a bugfix) is because directories and files in\n> the working tree are created with entry.c::create_directories() and\n> entry.c::create_file(), and they do honour umask settings, and the\n> top-level of the working tree should be handled the same way, no?\n\nWell, the top-level directories of anything are often handled specially,\nbut yes, I agree indeed. Frankly, I wondered why the top-level wasn't\ncreated safe_create_leading_directories() or something like that.\n\n>> cases where umask-controlled creation is preferred and in any case\n>> it is a well-known behaviour for new directory/file creation.\n>\n> Sign-off?\n\nIt was an RFC until now :)\n\nSigned-off-by: Alex Riesen \n"},{"id":"194836","messageId":"20120709225829.GA8397@sigill.intra.peff.net","threadId":"30971","inReplyTo":"7vobnpn224.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-09T22:58:29Z","receivedAt":"2012-07-09T22:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 08, 2012 at 06:41:39PM -0700, Junio C Hamano wrote:\n\n> Alex Riesen writes:\n> \n> > The original (shell coded) version of the git-clone just used mkdir(1)\n> > to create the working directories. The builtin changed the mode argument\n> > to mkdir(2) to 0755, which was a bit unfortunate, as there are use\n> \n> A much more important reason why this is a good change (I think you\n> could even say this is a bugfix) is because directories and files in\n> the working tree are created with entry.c::create_directories() and\n> entry.c::create_file(), and they do honour umask settings, and the\n> top-level of the working tree should be handled the same way, no?\n\nDoes the mkdir of \"rr-cache/*\" in rerere.c make the same mistake? The\nrr-cache root is made with 0777, and the files inside each subdirectory\nare created with 0666. So it is the only thing preventing users of\nshared repos from using rerere.\n\n-Peff\n"},{"id":"194837","messageId":"7v1ukkjzyz.fsf@alter.siamese.dyndns.org","threadId":"30971","inReplyTo":"20120709225829.GA8397@sigill.intra.peff.net","subject":"Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T23:07:16Z","receivedAt":"2012-07-09T23:07:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Does the mkdir of \"rr-cache/*\" in rerere.c make the same mistake? The\n> rr-cache root is made with 0777, and the files inside each subdirectory\n> are created with 0666. So it is the only thing preventing users of\n> shared repos from using rerere.\n\nQuite possibly yes. I do not recall tightening permissions on\npurpose, and it was a long time ago ;-)\n"},{"id":"194838","messageId":"7vwr2cikwa.fsf@alter.siamese.dyndns.org","threadId":"30971","inReplyTo":"7v1ukkjzyz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T23:18:13Z","receivedAt":"2012-07-09T23:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Does the mkdir of \"rr-cache/*\" in rerere.c make the same mistake? The\n>> rr-cache root is made with 0777, and the files inside each subdirectory\n>> are created with 0666. So it is the only thing preventing users of\n>> shared repos from using rerere.\n>\n> Quite possibly yes. I do not recall tightening permissions on\n> purpose, and it was a long time ago ;-)\n\nYup, that's the last remaining \"mkdir(.*, 755)\" in the codebase, and\nit should be trivial to replace it with mkdir_in_gitdir() or\nsomething.\n"},{"id":"194839","messageId":"7vsjd0ikfe.fsf_-_@alter.siamese.dyndns.org","threadId":"30971","inReplyTo":"7vwr2cikwa.fsf@alter.siamese.dyndns.org","subject":"[PATCH] rerere: make rr-cache fanout directory honor umask","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T23:28:21Z","receivedAt":"2012-07-09T23:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is the last remaining call to mkdir(2) that restricts the permission\nbits by passing 0755. Just use the same mkdir_in_gitdir() used to create\nthe leaf directories.\n\nSigned-off-by: Junio C Hamano \n---\n rerere.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex dcb525a..651c5de 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -524,7 +524,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)\n \t\t\t\tcontinue;\n \t\t\thex = xstrdup(sha1_to_hex(sha1));\n \t\t\tstring_list_insert(rr, path)->util = hex;\n-\t\t\tif (mkdir(git_path(\"rr-cache/%s\", hex), 0755))\n+\t\t\tif (mkdir_in_gitdir(git_path(\"rr-cache/%s\", hex)))\n \t\t\t\tcontinue;\n \t\t\thandle_file(path, NULL, rerere_path(hex, \"preimage\"));\n \t\t\tfprintf(stderr, \"Recorded preimage for '%s'\\n\", path);\n-- \n1.7.11.1.294.gf7b86df\n"},{"id":"194841","messageId":"20120710063721.GA6347@sigill.intra.peff.net","threadId":"30971","inReplyTo":"7vsjd0ikfe.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] rerere: make rr-cache fanout directory honor umask","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-10T06:37:22Z","receivedAt":"2012-07-10T06:37:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 09, 2012 at 04:28:21PM -0700, Junio C Hamano wrote:\n\n> This is the last remaining call to mkdir(2) that restricts the permission\n> bits by passing 0755. Just use the same mkdir_in_gitdir() used to create\n> the leaf directories.\n> \n> Signed-off-by: Junio C Hamano \n\nLooks obviously correct to me.\n\nI notice that grepping finds a few 0644 modes, too. Most of them are\nfalse-positives (e.g., we store and transmit 100644 as a shorthand for\n\"normal non-executable permissions\"). This is the only one that looked\nlegitimate to me:\n\n-- >8 --\nSubject: [PATCH] add: create ADD_EDIT.patch with mode 0666\n\nWe should be letting the user's umask take care of\nrestricting permissions. Even though this is a temporary\nfile and probably nobody would notice, this brings us in\nline with other temporary file creations in git (e.g.,\nchoosing \"e\"dit from git-add--interactive).\n\nSigned-off-by: Jeff King \n---\n builtin/add.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 41edd63..815ac4b 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -287,7 +287,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \targc = setup_revisions(argc, argv, &rev, NULL);\n \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n \tDIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);\n-\tout = open(file, O_CREAT | O_WRONLY, 0644);\n+\tout = open(file, O_CREAT | O_WRONLY, 0666);\n \tif (out < 0)\n \t\tdie (_(\"Could not open '%s' for writing.\"), file);\n \trev.diffopt.file = xfdopen(out, \"w\");\n-- \n1.7.10.5.16.ga1c6f1c\n"}]}