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

On 2014-01-29 19.17, Junio C Hamano wrote:

quoted

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 [off-list ref]

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 <redacted>
---
 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);
 		}
 	}