* Re: Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()
[not found] <201402021609.56984.tboegi@web.de>
@ 2014-02-02 15:17 ` Torsten Bögershausen
0 siblings, 0 replies; only message in thread
From: Torsten Bögershausen @ 2014-02-02 15:17 UTC (permalink / raw
To: Torsten Bögershausen, git, zwanzig12, stefanbeller,
kusmabite, Johannes.Schindelin, msysgit
(It seems as if the mail went only to Junio, sorry)
On 2014-02-02 16.09, Torsten Bögershausen wrote:
> On 2014-01-29 19.17, Junio C Hamano wrote:
>> 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.
> Thanks for deeper inspection, I now suspect the root cause to be here:
>
> -- >8 --
> Subject: [PATCH v3] repack.c: Rename and unlink pack file if it exists
>
> This comment in builtin/repack.c:
> * First see if there are packs of the same name and if so
> * if we can move them out of the way (this can happen if we
> * repacked immediately after packing fully.
> indicates that when a repo was fully repacked, and is repacked again,
> we may run into the situation that "new" packfiles have the name
> (and content) as already existing ones.
>
> The logic is to rename the existing ones into filename like "old-XXX",
> create the new ones and remove the "old-" ones.
> When something went wrong, a manual roll-back could be done be renaming
> the "old-" files.
>
> The renaming into "old-" did not work as designed, because file_exists()
> was done on the wrong file name.
> This could give problems for a repo on a network share under Windows,
> as reported by Jochen Haag <zwanzig12@googlemail.com>
>
> Solution:
> Create the right file name, like this:
> fname = mkpathdup("%s/pack-%s%s", packdir,
> and when removing the old-files:
> fname_old = mkpath("%s/old-%s%s",
>
> Rename the variables to match what they are used for:
> fname, fname_old and fname_tmp
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> builtin/repack.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 6284846..de69401 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -258,7 +258,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> for_each_string_list_item(item, &names) {
> for (ext = 0; ext < 2; ext++) {
> char *fname, *fname_old;
> - fname = mkpathdup("%s/%s%s", packdir,
> + fname = mkpathdup("%s/pack-%s%s", packdir,
> item->string, exts[ext]);
> if (!file_exists(fname)) {
> free(fname);
> @@ -314,33 +314,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> /* Now the ones with the same name are out of the way... */
> for_each_string_list_item(item, &names) {
> for (ext = 0; ext < 2; ext++) {
> - char *fname, *fname_old;
> + char *fname, *fname_tmp;
> struct stat statbuffer;
> fname = mkpathdup("%s/pack-%s%s",
> packdir, item->string, exts[ext]);
> - fname_old = mkpathdup("%s-%s%s",
> + fname_tmp = mkpathdup("%s-%s%s",
> packtmp, item->string, exts[ext]);
> - if (!stat(fname_old, &statbuffer)) {
> + if (!stat(fname_tmp, &statbuffer)) {
> statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> - chmod(fname_old, statbuffer.st_mode);
> + chmod(fname_tmp, statbuffer.st_mode);
> }
> - if (rename(fname_old, fname))
> - die_errno(_("renaming '%s' failed"), fname_old);
> + if (rename(fname_tmp, fname))
> + die_errno(_("renaming '%s' into '%s' failed"), fname_tmp, fname);
> free(fname);
> - free(fname_old);
> + free(fname_tmp);
> }
> }
>
> /* Remove the "old-" files */
> for_each_string_list_item(item, &names) {
> for (ext = 0; ext < 2; ext++) {
> - char *fname;
> - fname = mkpath("%s/old-pack-%s%s",
> + char *fname_old;
> + fname_old = mkpath("%s/old-%s%s",
> packdir,
> item->string,
> exts[ext]);
> - if (remove_path(fname))
> - warning(_("removing '%s' failed"), fname);
> + if (remove_path(fname_old))
> + warning(_("removing '%s' failed"), fname_old);
> }
> }
>
--
--
*** 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] only message in thread
only message in thread, other threads:[~2014-02-02 15:17 UTC | newest]
Thread overview: (only message) (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201402021609.56984.tboegi@web.de>
2014-02-02 15:17 ` Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename() Torsten Bögershausen
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).