Re: [PATCH] repack.c: chmod +w before rename()

[Junio C Hamano <gitster@pobox.com>]

Torsten Bögershausen <tboegi@web.de> writes:

> Another solution could be to do the "chmod +x" in mingw_rename().
> This may be done in another commit, because
> a) It improves git gc only when Git for Windows is used
>    on the client machine
> b) Windows refuses to delete a file when the file is read-only.
>    So setting a file to read-only under Windows is a way for a user
>    to protect it from being deleted.
>    Changing the behaviour of rename() globally may not be what we want.

But doesn't this affect non Windows platforms in negative ways?  We
unconditionally run stat(2) and chmod(2) unnecessarily, and we leave
the resulting file writable when it originally may have been marked
read-only on purpose.

Also, it feels to me that doing this in mingw_rename() the right
approach in the first place.  If the user wants "git mv a b" to
rename "a" in the working tree and if that path is read-only, what
happens, and what should happen?  Does "chmod -w" on a file in the
working tree mean "I want to make sure nobody accidentally writes to
it, and also I want to protect it from being renamed or deleted"?

So perhaps mingw_rename() can be taught to

 - First try to just do rename(), and if it succeeds, happily return
   without doing anything extra.

 - If it fails, then

   - check if the failure is due to read-only-ness (optional: if you
     cannot reliably tell this from the error code, do not bother),
     and if not, return failure.

   - otherwise, stat() to grab the current permission bits, do the
     chmod(), retry the rename, then restore the permission bits
     with another chmod().  Return failure if any of these steps
     fail.

That way, it can cover any call to rename(), be it for packfiles or
a path in the working tree, and you would need to pay the overhead
of stat/chmod only when it matters, no?

> Reported-by: Jochen Haag <zwanzig12@googlemail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/repack.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ba66c6e..033b4c2 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>  				chmod(fname_old, statbuffer.st_mode);
>  			}
> +			if (!stat(fname, &statbuffer)) {
> +				statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
> +				chmod(fname, statbuffer.st_mode);
> +			}
>  			if (rename(fname_old, fname))
>  				die_errno(_("renaming '%s' failed"), fname_old);
>  			free(fname);
> -- 
> 1.9.rc0.143.g6fd479e
>
> -- 

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.