repack: don't move existing packs out of the way

This behavior dates all the way back to 2ad47d6 (git-repack: Be careful when updating the same pack as an existing one., 2006-06-25). 2ad47d6 is mainly concerned about a case where a newly written pack would have a different structure than its index. This used to be possible when the pack name was a hash of the set of objects. Under this naming scheme, two packs that store the same set of objects could differ in delta selection, object positioning, or both. If this happened, then any such packs would be unreadable in the instant between copying the new pack and new index (i.e., either the index or pack will be stale depending on the order that they were copied).

This patch is mostly limited to removing code paths that deal with the 'old' prefixing, with the exception of pack metadata.

t7700.14 ensures
that 'git repack' will, for example, remove existing bitmaps even if the pack written is verbatim the same (when repacking without '-b' in a repository unchanged from the time 'git repack -b' was run). So, we have to handle metadata that we didn't write, by unlinking any existing metadata that our invocation of pack-objects didn't generate itself.

builtin/repack.c +0 -0

@@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Ok we have prepared all new packfiles.
 	 */
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
 			char *fname, *fname_old;
 
 			fname = mkpathdup("%s/pack-%s%s",
 					packdir, item->string, exts[ext].name);
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);

builtin/repack.c +0 -0

 			free(fname);
 			free(fname_old);
 		}
 	}
 	/* End of pack replacement. */
 
 	reprepare_packed_git(the_repository);

Quoted 6 lines

t7700.14 ensures
that 'git repack' will, for example, remove existing bitmaps even if the pack written is verbatim the same (when repacking without '-b' in a repository unchanged from the time 'git repack -b' was run). So, we have to handle metadata that we didn't write, by unlinking any existing metadata that our invocation of pack-objects didn't generate itself.

Hmph, t7700.14 wants it that way because?

If we were told to generate a packfile, and we ended up regenerating the exactly the same one, it appears to me that we can just pretend nothing happened and leave things as they were? Puzzled...

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 484

Quoted 2 lines

builtin/repack.c +0 -0

 				if (rename(fname_old, fname))
 					die_errno(_("renaming '%s' failed"), fname_old);

OK, so this is where the previous step matters. We do the same as before (i.e. stat+chmod and rename) only for paths we have created.

We don't reuse the old one because we have already written a new file so we won't save anything by doing so, and checking if the target of rename exists already before deciding not to rename cannot be made atomic, so just relying on rename() to do the right thing is a good idea anyway.

I think you could make an argument either way:

- we have an existing bitmap for free, and bitmaps make things faster, so why not keep it?

- the user did not ask for bitmaps, so we should make the outcome consistent whether a pack of the exact name existed before or not

The second one seems less surprising to me. But I think if we did the first, the code would be shorter (it would not need any of this "keep track of what pack-objects generated" stuff at all, but would just copy whatever files we see into place).

Quoted 11 lines

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 484

Quoted 2 lines

builtin/repack.c +0 -0

 				if (rename(fname_old, fname))
 					die_errno(_("renaming '%s' failed"), fname_old);

OK, so this is where the previous step matters. We do the same as before (i.e. stat+chmod and rename) only for paths we have created.

We don't reuse the old one because we have already written a new file so we won't save anything by doing so, and checking if the target of rename exists already before deciding not to rename cannot be made atomic, so just relying on rename() to do the right thing is a good idea anyway.

Even though the pack is byte-for-byte identical, the new .idx, etc, might not be. And those could be affected by options. E.g.:

git -c pack.writeBitmapHashCache=false repack -adb git -c pack.writeBitmapHashCache=true repack -adb

should probably end up with a bitmap file that contains a hash cache.

-Peff

Quoted 10 lines · Taylor Blau

This behavior dates all the way back to 2ad47d6 (git-repack: Be careful when updating the same pack as an existing one., 2006-06-25). 2ad47d6 is mainly concerned about a case where a newly written pack would have a different structure than its index. This used to be possible when the pack name was a hash of the set of objects. Under this naming scheme, two packs that store the same set of objects could differ in delta selection, object positioning, or both. If this happened, then any such packs would be unreadable in the instant between copying the new pack and new index (i.e., either the index or pack will be stale depending on the order that they were copied).

True. So the idea is that we can now pretend that we never wrote the new packfile and leave the existing one as-is?

Quoted 2 lines

This patch is mostly limited to removing code paths that deal with the 'old' prefixing, with the exception of pack metadata.

... "pack metadata" meaning? We do not remove and replace the file, but we update their mtime to keep these packfiles more fresh than other packfiles, or something?

Quoted 13 lines

builtin/repack.c +0 -0

@@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Ok we have prepared all new packfiles.
 	 */
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
 			char *fname, *fname_old;
 
 			fname = mkpathdup("%s/pack-%s%s",
 					packdir, item->string, exts[ext].name);
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 484

Quoted 10 lines

builtin/repack.c +8 -0

+
+			if (((uintptr_t)item->util) & (1 << ext)) {
+				struct stat statbuffer;
+				if (!stat(fname_old, &statbuffer)) {
+					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
+					chmod(fname_old, statbuffer.st_mode);
+				}
+
 				if (rename(fname_old, fname))
 					die_errno(_("renaming '%s' failed"), fname_old);

OK, so this is where the previous step matters. We do the same as before (i.e. stat+chmod and rename) only for paths we have created.

We don't reuse the old one because we have already written a new file so we won't save anything by doing so, and checking if the target of rename exists already before deciding not to rename cannot be made atomic, so just relying on rename() to do the right thing is a good idea anyway.

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 486

Quoted 2 lines

builtin/repack.c +2 -0

+			} else if (!exts[ext].optional)
+				die(_("missing required file: %s"), fname_old);

If one branch of if/else if.../else requires multi-statement block, use {} on all of them, for consistency.

So, if we wrote a few .$ext for a packfile but not some .$ext, if the one we didn't write is among the necessary one, we are in trouble? OK.

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 488

Quoted 2 lines

builtin/repack.c +2 -0

+			else if (unlink(fname) < 0 && errno != ENOENT)
+				die_errno(_("could not unlink: %s"), fname);

And if we wrote .pack and .idx but not .bitmap, the old .bitmpa that has the same pack hash may be stale and we discard it for safety? That sounds "prudent" but it is not immdiately clear from what danger we are protecting ourselves.

In any case, much of what I speculated while reading the proposed log message turned out to be false, which may be a sign that the log message did not explain the approach clearly enough. I thought that a newly created file that happened to be identical to existing ones would be discarded without getting renamed to their final location, but the code does not do such special casing. I thought the 'metadata' it talks about were to compensate for side effects of reusing the old files, but that was not what the 'metadata' was even about.

Other than that, the change in [2/3] and [3/3] look quite sensible (I am not saying [1/3] is bad---I haven't looked at it yet).

Thanks.

Quoted 9 lines

In any case, much of what I speculated while reading the proposed log message turned out to be false, which may be a sign that the log message did not explain the approach clearly enough. I thought that a newly created file that happened to be identical to existing ones would be discarded without getting renamed to their final location, but the code does not do such special casing. I thought the 'metadata' it talks about were to compensate for side effects of reusing the old files, but that was not what the 'metadata' was even about.

It's more about: ...

Quoted 1 line · Taylor Blau

It's more about: ...

You do not have to explain that to me here. Instead explain that to future readers of our history in the commit log message.

Thanks.

That is, it is not the case that we pretend as if pack-objects didn't write files identical to ones that already exist, but rather that we respect what pack-objects wrote as the source of truth. That cuts two ways:

- If pack-objects produced an identical pack to one that already exists with a bitmap, but did not produce a bitmap, we remove the bitmap that already exists. (This behavior is codified in t7700.14).

- If pack-objects produced an identical pack to one that already exists, we trust the just-written version of the coresponding .idx, .promisor, and other files over the ones that already exist. This ensures that we use the most up-to-date versions of this files, which is safe even in the face of format changes in, say, the .idx file (which would not be reflected in the .idx file's name).

Helped-by Jeff King <[email protected]>

Signed-off-by Taylor Blau <[email protected]>

---
 builtin/repack.c | 103 +++++++----------------------------------------
 1 file changed, 14 insertions(+), 89 deletions(-)

builtin/repack.c +14 -90

index bb839180da..279be11a16 100644
@@ -306,7 +306,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct string_list rollback = STRING_LIST_INIT_NODUP;
 	struct string_list existing_packs = STRING_LIST_INIT_DUP;
 	struct strbuf line = STRBUF_INIT;
-	int i, ext, ret, failed;
+	int i, ext, ret;
 	FILE *out;
 
 	/* variables to be filled by option parsing */

@@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Ok we have prepared all new packfiles.
-	 * 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.
 	 */
-	failed = 0;
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
 			char *fname, *fname_old;
 
-			fname = mkpathdup("%s/pack-%s%s", packdir,
-						item->string, exts[ext].name);
-			if (!file_exists(fname)) {
-				free(fname);
-				continue;
-			}
-
-			fname_old = mkpathdup("%s/old-%s%s", packdir,
-						item->string, exts[ext].name);
-			if (file_exists(fname_old))
-				if (unlink(fname_old))
-					failed = 1;
-
-			if (!failed && rename(fname, fname_old)) {
-				free(fname);
-				free(fname_old);
-				failed = 1;
-				break;
-			} else {
-				string_list_append(&rollback, fname);
-				free(fname_old);
-			}
-		}
-		if (failed)
-			break;
-	}
-	if (failed) {
-		struct string_list rollback_failure = STRING_LIST_INIT_DUP;
-		for_each_string_list_item(item, &rollback) {
-			char *fname, *fname_old;
-			fname = mkpathdup("%s/%s", packdir, item->string);
-			fname_old = mkpathdup("%s/old-%s", packdir, item->string);
-			if (rename(fname_old, fname))
-				string_list_append(&rollback_failure, fname);
-			free(fname);
-			free(fname_old);
-		}
-
-		if (rollback_failure.nr) {
-			int i;
-			fprintf(stderr,
-				_("WARNING: Some packs in use have been renamed by\n"
-				  "WARNING: prefixing old- to their name, in order to\n"
-				  "WARNING: replace them with the new version of the\n"
-				  "WARNING: file.  But the operation failed, and the\n"
-				  "WARNING: attempt to rename them back to their\n"
-				  "WARNING: original names also failed.\n"
-				  "WARNING: Please rename them in %s manually:\n"), packdir);
-			for (i = 0; i < rollback_failure.nr; i++)
-				fprintf(stderr, "WARNING:   old-%s -> %s\n",
-					rollback_failure.items[i].string,
-					rollback_failure.items[i].string);
-		}
-		exit(1);
-	}
-
-	/* Now the ones with the same name are out of the way... */
-	for_each_string_list_item(item, &names) {
-		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
-			char *fname, *fname_old;
-			struct stat statbuffer;
-			int exists = 0;
 			fname = mkpathdup("%s/pack-%s%s",
 					packdir, item->string, exts[ext].name);
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);
-			if (!stat(fname_old, &statbuffer)) {
-				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
-				chmod(fname_old, statbuffer.st_mode);
-				exists = 1;
-			}
-			if (exists || !exts[ext].optional) {
+
+			if (((uintptr_t)item->util) & (1 << ext)) {
+				struct stat statbuffer;
+				if (!stat(fname_old, &statbuffer)) {
+					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
+					chmod(fname_old, statbuffer.st_mode);
+				}
+
 				if (rename(fname_old, fname))
 					die_errno(_("renaming '%s' failed"), fname_old);
-			}
+			} else if (!exts[ext].optional)
+				die(_("missing required file: %s"), fname_old);
+			else if (unlink(fname) < 0 && errno != ENOENT)
+				die_errno(_("could not unlink: %s"), fname);
+
 			free(fname);
 			free(fname_old);
 		}
 	}
-
-	/* Remove the "old-" files */
-	for_each_string_list_item(item, &names) {
-		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
-			char *fname;
-			fname = mkpathdup("%s/old-%s%s",
-					  packdir,
-					  item->string,
-					  exts[ext].name);
-			if (remove_path(fname))
-				warning(_("failed to remove '%s'"), fname);
-			free(fname);
-		}
-	}
-
 	/* End of pack replacement. */
 
 	reprepare_packed_git(the_repository);
--

2.29.2.312.gabc4d358d8