[PATCH] repack.c: rename and unlink pack file if it exists

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2014, 23:40 UTC
Message-ID: <xmqqwqha5twg.fsf@gitster.dls.corp.google.com>

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).

When a repo was fully repacked, and is repacked again, we may run into the situation that "new" packfiles have the same name as already existing ones (traditionally packfiles have been named after the list of names of objects in them, so repacking all the objects in a single pack would have produced a packfile with the same name).

The logic is to rename the existing ones into filename like "old-XXX", create the new ones and then remove the "old-" ones. When something went wrong in the middle, this sequence is rolled back by renaming the "old-" files back.

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>.

Formulate the filenames objects/pack/pack-XXXX.{pack,idx} correctly (the code originally lacked "pack-" prefix when checking if the file exists).

Also rename the variables to match what they are used for: fname for the final name of the new packfile, fname_old for the name of the existing one, and fname_tmp for the temporary name pack-objects created the new packfile in.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Somehow this came to my private mailbox without Cc to list, so I
   am forwarding it.

   I think with 1190a1ac (pack-objects: name pack files after
   trailer hash, 2013-12-05), repacking the same set of objects may
   have less chance of producing colliding names, especially if you
   are on a box with more than one core, but it still would be a
   good idea to get this part right in the upcoming release.

   The bug in the first hunk will cause us not to find any existing
   file, even when there is a name clash.  And then we try to
   immediately the final rename without any provision for rolling
   back.  The other two hunks are pure noise renaming variables.

   I think the patch looks correct, but I'd appreciate a second set
   of eyeballs.

   Thanks.

 builtin/repack.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index bca7710..3b01353 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -260,7 +260,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);
@@ -316,33 +316,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);
 		}
 	}

-- 
1.9-rc2-217-g24a8b2e