{"thread":{"id":"35784","subject":"Re: Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()","startedAt":"2014-02-02T15:17:08Z","lastAt":"2014-02-02T15:17:08Z","messageCount":1,"participants":["Torsten Bögershausen"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"234049","messageId":"52EE6174.1050401@web.de","threadId":"35784","inReplyTo":"201402021609.56984.tboegi@web.de","subject":"Re: Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-02-02T15:17:08Z","receivedAt":"2014-02-02T15:17:08Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"(It seems as if the mail went only to Junio, sorry)\nOn 2014-02-02 16.09, Torsten Bögershausen wrote:\n> On 2014-01-29 19.17, Junio C Hamano wrote:\n>> But after a closer inspection, I no longer think that hunk is an\n>> improvement.  These new packfiles were created by pack-objects,\n>> which finishes each packfile it produces by calling\n>> finish_tmp_packfile(), and that is where adjust_shared_perm()\n>> happens already.  As far as \"pack-objects\" that was called from\n>> \"repack\" is concerned, these new packfiles are not \"temporary\"; they\n>> are finished product.  It may be OK to remove them as part of\n>> \"rewind back to the original state, as a later phase of repack\n>> failed\" if we saw a failure (but note that the original\n>> \"git-repack.sh\" didn't), but a plain vanilla rename(2) without any\n>> frills is what we want to happen to them.\n> Thanks for deeper inspection, I now suspect the root cause to be here:\n>\n> -- >8 --\n> Subject: [PATCH v3] repack.c: Rename and unlink pack file if it exists\n>\n> This comment in builtin/repack.c:\n>   * First see if there are packs of the same name and if so\n>   * if we can move them out of the way (this can happen if we\n>   * repacked immediately after packing fully.\n> indicates that when a repo was fully repacked, and is repacked again,\n> we may run into the situation that \"new\" packfiles have the name\n> (and content) as already existing ones.\n>\n> The logic is to rename the existing ones into filename like \"old-XXX\",\n> create the new ones and remove the \"old-\" ones.\n> When something went wrong, a manual roll-back could be done be renaming\n> the \"old-\" files.\n>\n> The renaming into \"old-\" did not work as designed, because file_exists()\n> was done on the wrong file name.\n> This could give problems for a repo on a network share under Windows,\n> as reported by Jochen Haag <zwanzig12@googlemail.com>\n>\n> Solution:\n> Create the right file name, like this:\n>   fname = mkpathdup(\"%s/pack-%s%s\", packdir,\n> and when removing the old-files:\n>   fname_old = mkpath(\"%s/old-%s%s\",\n>\n> Rename the variables to match what they are used for:\n> fname, fname_old and fname_tmp\n>\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n>  builtin/repack.c | 24 ++++++++++++------------\n>  1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 6284846..de69401 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -258,7 +258,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < 2; ext++) {\n>  \t\t\tchar *fname, *fname_old;\n> -\t\t\tfname = mkpathdup(\"%s/%s%s\", packdir,\n> +\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n>  \t\t\t\t\t\titem->string, exts[ext]);\n>  \t\t\tif (!file_exists(fname)) {\n>  \t\t\t\tfree(fname);\n> @@ -314,33 +314,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t/* Now the ones with the same name are out of the way... */\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < 2; ext++) {\n> -\t\t\tchar *fname, *fname_old;\n> +\t\t\tchar *fname, *fname_tmp;\n>  \t\t\tstruct stat statbuffer;\n>  \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n>  \t\t\t\t\tpackdir, item->string, exts[ext]);\n> -\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> +\t\t\tfname_tmp = mkpathdup(\"%s-%s%s\",\n>  \t\t\t\t\tpacktmp, item->string, exts[ext]);\n> -\t\t\tif (!stat(fname_old, &statbuffer)) {\n> +\t\t\tif (!stat(fname_tmp, &statbuffer)) {\n>  \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> -\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> +\t\t\t\tchmod(fname_tmp, statbuffer.st_mode);\n>  \t\t\t}\n> -\t\t\tif (rename(fname_old, fname))\n> -\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> +\t\t\tif (rename(fname_tmp, fname))\n> +\t\t\t\tdie_errno(_(\"renaming '%s' into '%s' failed\"), fname_tmp, fname);\n>  \t\t\tfree(fname);\n> -\t\t\tfree(fname_old);\n> +\t\t\tfree(fname_tmp);\n>  \t\t}\n>  \t}\n>  \n>  \t/* Remove the \"old-\" files */\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < 2; ext++) {\n> -\t\t\tchar *fname;\n> -\t\t\tfname = mkpath(\"%s/old-pack-%s%s\",\n> +\t\t\tchar *fname_old;\n> +\t\t\tfname_old = mkpath(\"%s/old-%s%s\",\n>  \t\t\t\t\tpackdir,\n>  \t\t\t\t\titem->string,\n>  \t\t\t\t\texts[ext]);\n> -\t\t\tif (remove_path(fname))\n> -\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n> +\t\t\tif (remove_path(fname_old))\n> +\t\t\t\twarning(_(\"removing '%s' failed\"), fname_old);\n>  \t\t}\n>  \t}\n>  \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"}]}