Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()

[Johan Herland <johan@herland.net>]

On Saturday 28 March 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > -	if (chmod(filename, 0444) || adjust_shared_perm(filename))
> > +	if (chmod(filename, get_shared_perm(0444)))
>
> Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would
> most likely say "no" and cause real harm, but there is no guarantee that
> we won't start checking S_ISREG() or other things in get_shared_perm()
> later.  I do not like this.

You are right.

> How about doing it this way instead?
>
> One thing to note is that we seem to have been passing what we read from
> st.st_mode, together with S_IFMT bits, to chmod(2); I do not think I've
> seen any breakage reports on exotic systems (glibc on Linux seems to
> ignore the higher bits), but from my reading of POSIX, I would not be
> surprised if somebody's chmod(2) returned EINVAL.

Agreed.

> -- >8 --
> set_shared_perm(): sometimes we know what the final mode bits should look
> like
>
> adjust_shared_perm() first obtains the mode bits from lstat(2), expecting
> to find what the result of applying user's umask is, and then tweaked it

s/tweaked/tweaks/

> as necessary.  When the file to be adjusted is created with mkstemp(3),
> however, the mode thusly obtained does not have anything to do with
> usre's umask, and we would need to start from 0444 in such a case and

s/usre/user/

> there is no point running lstat(2) for such a path.
>
> This introduces a new API set_shared_perm() to bypass the lstat(2) and
> instead force setting the mode bits to the desired value directly.
> adjust_shared_perm() becomes a thin wrapper to the function.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

[...]

> diff --git a/sha1_file.c b/sha1_file.c
> index 8869488..5bfc36c 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const
> char *filename) *
>  	 * The same holds for FAT formatted media.
>  	 *
> -	 * When this succeeds, we just return 0. We have nothing
> +	 * When this succeeds, we just return; we have nothing

Small nit: This belongs in the previous patch, doesn't it?


All in all, this looks very good. Please drop my second patch, and use this 
instead.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net