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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2012, 01:41 UTC
Message-ID: <7vobnpn224.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120707215029.GA26819@blimp.dmz>

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?

Show 37 quoted lines

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