{"thread":{"id":"35741","subject":"[PATCH v2] repack.c: Use move_temp_to_file() instead of rename()","startedAt":"2014-01-27T20:24:42Z","lastAt":"2014-01-29T18:17:43Z","messageCount":2,"participants":["Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"233836","messageId":"201401272124.42978.tboegi@web.de","threadId":"35741","inReplyTo":null,"subject":"[PATCH v2] repack.c: Use move_temp_to_file() instead of rename()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-01-27T20:24:42Z","receivedAt":"2014-01-27T20:24:42Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"In a1bbc6c0 a shell command \"mv -f\" was replaced with the rename() function.\n\nUse move_temp_to_file() from sha1_file.c instead of rename().\nThis is in line with the handling of other Git internal tmp files,\nand calls adjust_shared_perm()\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\nThanks for all comments.\nI haven't been able to reproduce the problem here.\nTips and information how to reproduce it are wellcome.\n\nI think this patch makes sense to support core.sharedRepository(),\nbut I haven't made a test case for the pack/idx files.\n\nJochen, doess this patch fix your problem, or do we need\nanother patch on top of this?\n\n builtin/repack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ba66c6e..4b6d663 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -271,7 +271,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tif (unlink(fname_old))\n \t\t\t\t\tfailed = 1;\n \n-\t\t\tif (!failed && rename(fname, fname_old)) {\n+\t\t\tif (!failed && move_temp_to_file(fname, fname_old)) {\n \t\t\t\tfree(fname);\n \t\t\t\tfailed = 1;\n \t\t\t\tbreak;\n@@ -288,7 +288,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tchar *fname, *fname_old;\n \t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n \t\t\tfname_old = mkpath(\"%s/old-%s\", packdir, item->string);\n-\t\t\tif (rename(fname_old, fname))\n+\t\t\tif (move_temp_to_file(fname_old, fname))\n \t\t\t\tstring_list_append(&rollback_failure, fname);\n \t\t\tfree(fname);\n \t\t}\n@@ -324,7 +324,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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}\n-\t\t\tif (rename(fname_old, fname))\n+\t\t\tif (move_temp_to_file(fname_old, fname))\n \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n \t\t\tfree(fname);\n \t\t\tfree(fname_old);\n-- \n1.8.5.2\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"},{"id":"233894","messageId":"xmqqiot2k5yw.fsf@gitster.dls.corp.google.com","threadId":"35741","inReplyTo":"201401272124.42978.tboegi@web.de","subject":"Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-29T18:17:43Z","receivedAt":"2014-01-29T18:17:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> In a1bbc6c0 a shell command \"mv -f\" was replaced with the rename() function.\n>\n> Use move_temp_to_file() from sha1_file.c instead of rename().\n> This is in line with the handling of other Git internal tmp files,\n> and calls adjust_shared_perm()\n\n>\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n> Thanks for all comments.\n> I haven't been able to reproduce the problem here.\n> Tips and information how to reproduce it are wellcome.\n>\n> I think this patch makes sense to support core.sharedRepository(),\n> but I haven't made a test case for the pack/idx files.\n>\n> Jochen, doess this patch fix your problem, or do we need\n> another patch on top of this?\n\nTurning the call to rename(2) in the last hunk in your patch to\nmove_temp_to_file() may be an improvement, but the other changes are\nwrong, I think.\n\nThere are two big differences between the rename(2) used here and\nmove_temp_to_file().  You need to understand what move_temp_to_file()\nis from its name.\n\n - It is meant to move a temporary file we create ourselves, and the\n   most important characteristic of such a file is that we do not\n   create it read-only.  The function uses platform's rename(2) and\n   returns a failure if the platform's rename(2) fails, but that is\n   not a problem even on Windows because of this.\n\n - When returning a failure, it unlinks the temporary file.  This\n   unlinking is necessary to clean up after our half-done mess.\n\nThe other two calls to rename(2) you changed in the patch are *not*\nabout turning a newly created temporary file into the final one.\nThey are about moving existing packfiles away just in case we fail\nin the later phases of the command so that they can be moved back to\ntheir original name to restore the state before we started, and\nactually moving them back to their original name.  We do not want to\nremove these files.  The platform's rename(2) must do the right\nthing in these two calls and there is no guarantee these existing\npackfiles are read-write.\n\nEarlier, I said that turning the call to rename(2) in the last hunk\nin your patch to move_temp_to_file() may be an improvement, because\nI thought that it is a good thing that the latter makes a call to\nadjust_shared_perm().\n\nBut after a closer inspection, I no longer think that hunk is an\nimprovement.  These new packfiles were created by pack-objects,\nwhich finishes each packfile it produces by calling\nfinish_tmp_packfile(), and that is where adjust_shared_perm()\nhappens already.  As far as \"pack-objects\" that was called from\n\"repack\" is concerned, these new packfiles are not \"temporary\"; they\nare 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\nfailed\" if we saw a failure (but note that the original\n\"git-repack.sh\" didn't), but a plain vanilla rename(2) without any\nfrills is what we want to happen to them.\n\n>  builtin/repack.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index ba66c6e..4b6d663 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -271,7 +271,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tif (unlink(fname_old))\n>  \t\t\t\t\tfailed = 1;\n>  \n> -\t\t\tif (!failed && rename(fname, fname_old)) {\n> +\t\t\tif (!failed && move_temp_to_file(fname, fname_old)) {\n>  \t\t\t\tfree(fname);\n>  \t\t\t\tfailed = 1;\n>  \t\t\t\tbreak;\n> @@ -288,7 +288,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\tchar *fname, *fname_old;\n>  \t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n>  \t\t\tfname_old = mkpath(\"%s/old-%s\", packdir, item->string);\n> -\t\t\tif (rename(fname_old, fname))\n> +\t\t\tif (move_temp_to_file(fname_old, fname))\n>  \t\t\t\tstring_list_append(&rollback_failure, fname);\n>  \t\t\tfree(fname);\n>  \t\t}\n> @@ -324,7 +324,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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}\n> -\t\t\tif (rename(fname_old, fname))\n> +\t\t\tif (move_temp_to_file(fname_old, fname))\n>  \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n>  \t\t\tfree(fname);\n>  \t\t\tfree(fname_old);\n> -- \n> 1.8.5.2\n>\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"}]}