git-clone ignores umask for working tree

git-clone ignores umask for working tree

[Alex Riesen]

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?

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

Re: git-clone ignores umask for working tree

[Daniel Barkalow]

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*

[PATCH] Restore umasks influence on the permissions of work tree created by clone

[Alex Riesen]

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

Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone

[Junio C Hamano]

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);

Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone

[Alex Riesen]

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>

Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone

[Jeff King]

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

Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone

[Junio C Hamano]

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 ;-)

Re: [PATCH] Restore umasks influence on the permissions of work tree created by clone

[Junio C Hamano]

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.

[PATCH] rerere: make rr-cache fanout directory honor umask

[Junio C Hamano]

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

Re: [PATCH] rerere: make rr-cache fanout directory honor umask

[Jeff King]

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