Re: [PATCH] repack.c: chmod +w before rename()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 24, 2014, 23:31 UTC
- Message-ID
- <xmqq1tzxq7ml.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <201401242205.16313.tboegi@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
Show 8 quoted lines
> 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?
Show 25 quoted lines
> 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.