threads / patch / 35741

v2repack.c: Use move_temp_to_file() instead of rename()

Subject: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()

## tl;dr

2 messages between Jan 27, 2014 and Jan 29, 2014. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Torsten Bögershausen· Jan 27, 2014, 20:24 UTC · lore
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 <tboegi@web.de>
---
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?

 builtin/repack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin/repack.c +3 −3
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.
Junio C Hamano· Jan 29, 2014, 18:17 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()

Torsten Bögershausen <tboegi@web.de> writes:
Show 5 quoted lines
> 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()
Show 12 quoted lines
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> 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.

Show 38 quoted lines
>  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.

← back to recent threads