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

> 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.

> 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.

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

> 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