threads / discuss / 30971
Subject: git-clone ignores umask for working tree
Alex RiesenHi list,
when git-clone was built in, its treatment of umask has changed: the shell version respected umask for newly created directories by using plain mkdir(1), and the builtin version just uses mkdir(work_tree, 0755).
Is it intentional?
This Stackoverflow question is what got me interested:
http://stackoverflow.com/questions/10637416/git-clone-respects-umask-except-for-top-level-project-directory
and might provide a credible use case.
Regards, Alex
Daniel BarkalowRe: git-clone ignores umask for working tree
On Fri, 6 Jul 2012, Alex Riesen wrote:
> Hi list, > > when git-clone was built in, its treatment of umask has changed: the shell > version respected umask for newly created directories by using plain mkdir(1), > and the builtin version just uses mkdir(work_tree, 0755). > > Is it intentional?
I have the vague feeling that it was intentional, but it's entirely plausible that I just overlooked that mkdir(2) applies umask and went for the mode that you normally want. I don't think there's any particular need for this operation to be more restrictive than umask.
-Daniel *This .sig left intentionally blank*
Alex Riesen[PATCH] Restore umasks influence on the permissions of work tree created by clone
The original (shell coded) version of the git-clone just used mkdir(1) to create the working directories. The builtin changed the mode argument to mkdir(2) to 0755, which was a bit unfortunate, as there are use cases where umask-controlled creation is preferred and in any case it is a well-known behaviour for new directory/file creation. ---
On Fri, 6 Jul 2012, Daniel Barkalow wrote:
> On Fri, 6 Jul 2012, Alex Riesen wrote: >> when git-clone was built in, its treatment of umask has changed: the shell >> version respected umask for newly created directories by using plain mkdir(1), >> and the builtin version just uses mkdir(work_tree, 0755). >> >> Is it intentional? > > I have the vague feeling that it was intentional, but it's entirely > plausible that I just overlooked that mkdir(2) applies umask and went for > the mode that you normally want. I don't think there's any particular need > for this operation to be more restrictive than umask.
I didn't look hard enough, but still, I found not much of complaining either way (frankly - none, but as I said, I didn'l look hard): none before - for being too permissive, the only one in original post after building the thing in - for being too restrictive.
Maybe we should reconsider and go back to the old permission handling?
builtin/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c index d3b7fdc..e314b0b 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -708,7 +708,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (safe_create_leading_directories_const(work_tree) < 0) die_errno(_("could not create leading directories of '%s'"), work_tree); - if (!dest_exists && mkdir(work_tree, 0755)) + if (!dest_exists && mkdir(work_tree, 0777)) die_errno(_("could not create work tree dir '%s'."), work_tree); set_git_work_tree(work_tree);
-- 1.7.11.1.185.g5abe2c9
Junio C HamanoRe: [PATCH] Restore umasks influence on the permissions of work tree created by clone
Alex Riesen <raa.lkml@gmail.com> writes:
> The original (shell coded) version of the git-clone just used mkdir(1) > to create the working directories. The builtin changed the mode argument > to mkdir(2) to 0755, which was a bit unfortunate, as there are use
A much more important reason why this is a good change (I think you could even say this is a bugfix) is because directories and files in the working tree are created with entry.c::create_directories() and entry.c::create_file(), and they do honour umask settings, and the top-level of the working tree should be handled the same way, no?
> cases where umask-controlled creation is preferred and in any case > it is a well-known behaviour for new directory/file creation.
> ---
Sign-off?
>
> On Fri, 6 Jul 2012, Daniel Barkalow wrote:
>> On Fri, 6 Jul 2012, Alex Riesen wrote:
>>> when git-clone was built in, its treatment of umask has changed: the shell
>>> version respected umask for newly created directories by using plain mkdir(1),
>>> and the builtin version just uses mkdir(work_tree, 0755).
>>>
>>> Is it intentional?
>>
>> I have the vague feeling that it was intentional, but it's entirely
>> plausible that I just overlooked that mkdir(2) applies umask and went for
>> the mode that you normally want. I don't think there's any particular need
>> for this operation to be more restrictive than umask.
>
> I didn't look hard enough, but still, I found not much of complaining either
> way (frankly - none, but as I said, I didn'l look hard): none before - for
> being too permissive, the only one in original post after building the thing
> in - for being too restrictive.
>
> Maybe we should reconsider and go back to the old permission handling?
>
> builtin/clone.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index d3b7fdc..e314b0b 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -708,7 +708,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> if (safe_create_leading_directories_const(work_tree) < 0)
> die_errno(_("could not create leading directories of '%s'"),
> work_tree);
> - if (!dest_exists && mkdir(work_tree, 0755))
> + if (!dest_exists && mkdir(work_tree, 0777))
> die_errno(_("could not create work tree dir '%s'."),
> work_tree);
> set_git_work_tree(work_tree);Alex RiesenRe: [PATCH] Restore umasks influence on the permissions of work tree created by clone
On Mon, Jul 9, 2012 at 3:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Alex Riesen <raa.lkml@gmail.com> writes: > >> The original (shell coded) version of the git-clone just used mkdir(1) >> to create the working directories. The builtin changed the mode argument >> to mkdir(2) to 0755, which was a bit unfortunate, as there are use > > A much more important reason why this is a good change (I think you > could even say this is a bugfix) is because directories and files in > the working tree are created with entry.c::create_directories() and > entry.c::create_file(), and they do honour umask settings, and the > top-level of the working tree should be handled the same way, no?
Well, the top-level directories of anything are often handled specially, but yes, I agree indeed. Frankly, I wondered why the top-level wasn't created safe_create_leading_directories() or something like that.
>> cases where umask-controlled creation is preferred and in any case >> it is a well-known behaviour for new directory/file creation. > > Sign-off?
It was an RFC until now :)
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
Jeff KingRe: [PATCH] Restore umasks influence on the permissions of work tree created by clone
On Sun, Jul 08, 2012 at 06:41:39PM -0700, Junio C Hamano wrote:
> Alex Riesen <raa.lkml@gmail.com> writes: > > > The original (shell coded) version of the git-clone just used mkdir(1) > > to create the working directories. The builtin changed the mode argument > > to mkdir(2) to 0755, which was a bit unfortunate, as there are use > > A much more important reason why this is a good change (I think you > could even say this is a bugfix) is because directories and files in > the working tree are created with entry.c::create_directories() and > entry.c::create_file(), and they do honour umask settings, and the > top-level of the working tree should be handled the same way, no?
Does the mkdir of "rr-cache/*" in rerere.c make the same mistake? The rr-cache root is made with 0777, and the files inside each subdirectory are created with 0666. So it is the only thing preventing users of shared repos from using rerere.
-Peff
Junio C HamanoRe: [PATCH] Restore umasks influence on the permissions of work tree created by clone
Jeff King <peff@peff.net> writes:
> Does the mkdir of "rr-cache/*" in rerere.c make the same mistake? The > rr-cache root is made with 0777, and the files inside each subdirectory > are created with 0666. So it is the only thing preventing users of > shared repos from using rerere.
Quite possibly yes. I do not recall tightening permissions on purpose, and it was a long time ago ;-)
Junio C HamanoRe: [PATCH] Restore umasks influence on the permissions of work tree created by clone
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes: > >> Does the mkdir of "rr-cache/*" in rerere.c make the same mistake? The >> rr-cache root is made with 0777, and the files inside each subdirectory >> are created with 0666. So it is the only thing preventing users of >> shared repos from using rerere. > > Quite possibly yes. I do not recall tightening permissions on > purpose, and it was a long time ago ;-)
Yup, that's the last remaining "mkdir(.*, 755)" in the codebase, and it should be trivial to replace it with mkdir_in_gitdir() or something.
Junio C Hamano[PATCH] rerere: make rr-cache fanout directory honor umask
This is the last remaining call to mkdir(2) that restricts the permission bits by passing 0755. Just use the same mkdir_in_gitdir() used to create the leaf directories.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- rerere.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/rerere.c b/rerere.c index dcb525a..651c5de 100644 --- a/rerere.c +++ b/rerere.c @@ -524,7 +524,7 @@ static int do_plain_rerere(struct string_list *rr, int fd) continue; hex = xstrdup(sha1_to_hex(sha1)); string_list_insert(rr, path)->util = hex; - if (mkdir(git_path("rr-cache/%s", hex), 0755)) + if (mkdir_in_gitdir(git_path("rr-cache/%s", hex))) continue; handle_file(path, NULL, rerere_path(hex, "preimage")); fprintf(stderr, "Recorded preimage for '%s'\n", path);
-- 1.7.11.1.294.gf7b86df
Jeff KingRe: [PATCH] rerere: make rr-cache fanout directory honor umask
On Mon, Jul 09, 2012 at 04:28:21PM -0700, Junio C Hamano wrote:
> This is the last remaining call to mkdir(2) that restricts the permission > bits by passing 0755. Just use the same mkdir_in_gitdir() used to create > the leaf directories. > > Signed-off-by: Junio C Hamano <gitster@pobox.com>
Looks obviously correct to me.
I notice that grepping finds a few 0644 modes, too. Most of them are false-positives (e.g., we store and transmit 100644 as a shorthand for "normal non-executable permissions"). This is the only one that looked legitimate to me:
-- >8 -- Subject: [PATCH] add: create ADD_EDIT.patch with mode 0666
We should be letting the user's umask take care of restricting permissions. Even though this is a temporary file and probably nobody would notice, this brings us in line with other temporary file creations in git (e.g., choosing "e"dit from git-add--interactive).
Signed-off-by: Jeff King <peff@peff.net> --- builtin/add.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c index 41edd63..815ac4b 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -287,7 +287,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix) argc = setup_revisions(argc, argv, &rev, NULL); rev.diffopt.output_format = DIFF_FORMAT_PATCH; DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES); - out = open(file, O_CREAT | O_WRONLY, 0644); + out = open(file, O_CREAT | O_WRONLY, 0666); if (out < 0) die (_("Could not open '%s' for writing."), file); rev.diffopt.file = xfdopen(out, "w");
-- 1.7.10.5.16.ga1c6f1c