Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:46
Torsten Bögershausen [off-list ref] writes:
In a1bbc6c0 a shell command "mv -f" was replaced with the rename() function. Use move_temp_to_file() from sha1_file.c instead of rename(). This is in line with the handling of other Git internal tmp files, and calls adjust_shared_perm()
Signed-off-by: Torsten Bögershausen <redacted> --- Thanks for all comments. I haven't been able to reproduce the problem here. Tips and information how to reproduce it are wellcome. I think this patch makes sense to support core.sharedRepository(), but I haven't made a test case for the pack/idx files. Jochen, doess this patch fix your problem, or do we need another patch on top of this?
Turning the call to rename(2) in the last hunk in your patch to move_temp_to_file() may be an improvement, but the other changes are wrong, I think. There are two big differences between the rename(2) used here and move_temp_to_file(). You need to understand what move_temp_to_file() is from its name. - It is meant to move a temporary file we create ourselves, and the most important characteristic of such a file is that we do not create it read-only. The function uses platform's rename(2) and returns a failure if the platform's rename(2) fails, but that is not a problem even on Windows because of this. - When returning a failure, it unlinks the temporary file. This unlinking is necessary to clean up after our half-done mess. The other two calls to rename(2) you changed in the patch are *not* about turning a newly created temporary file into the final one. They are about moving existing packfiles away just in case we fail in the later phases of the command so that they can be moved back to their original name to restore the state before we started, and actually moving them back to their original name. We do not want to remove these files. The platform's rename(2) must do the right thing in these two calls and there is no guarantee these existing packfiles are read-write. Earlier, I said that turning the call to rename(2) in the last hunk in your patch to move_temp_to_file() may be an improvement, because I thought that it is a good thing that the latter makes a call to adjust_shared_perm(). But after a closer inspection, I no longer think that hunk is an improvement. These new packfiles were created by pack-objects, which finishes each packfile it produces by calling finish_tmp_packfile(), and that is where adjust_shared_perm() happens already. As far as "pack-objects" that was called from "repack" is concerned, these new packfiles are not "temporary"; they are finished product. It may be OK to remove them as part of "rewind back to the original state, as a later phase of repack failed" if we saw a failure (but note that the original "git-repack.sh" didn't), but a plain vanilla rename(2) without any frills is what we want to happen to them.
quoted hunk
builtin/repack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)diff --git a/builtin/repack.c b/builtin/repack.c index ba66c6e..4b6d663 100644 --- a/builtin/repack.c +++ b/builtin/repack.c@@ -271,7 +271,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (unlink(fname_old)) failed = 1; - if (!failed && rename(fname, fname_old)) { + if (!failed && move_temp_to_file(fname, fname_old)) { free(fname); failed = 1; break;@@ -288,7 +288,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) char *fname, *fname_old; fname = mkpathdup("%s/%s", packdir, item->string); fname_old = mkpath("%s/old-%s", packdir, item->string); - if (rename(fname_old, fname)) + if (move_temp_to_file(fname_old, fname)) string_list_append(&rollback_failure, fname); free(fname); }@@ -324,7 +324,7 @@ 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 (rename(fname_old, fname)) + if (move_temp_to_file(fname_old, fname)) die_errno(_("renaming '%s' failed"), fname_old); free(fname); free(fname_old);-- 1.8.5.2 --
-- -- *** 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.