Torsten Bögershausen <tboegi@xxxxxx> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Torsten Bögershausen <tboegi@xxxxxx>
> ---
> 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
>
> --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html