* Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()
2014-01-27 20:24 [PATCH v2] repack.c: Use move_temp_to_file() instead of rename() Torsten Bögershausen
@ 2014-01-29 18:17 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2014-01-29 18:17 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: git, zwanzig12, stefanbeller, kusmabite, Johannes.Schindelin,
msysgit
Torsten Bögershausen <tboegi@web.de> 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 <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.
> 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.
^ permalink raw reply [flat|nested] 2+ messages in thread