When 'git repack' creates a pack with the same name as any existing
pack, it moves the existing one to 'old-pack-xxx.{pack,idx,...}' and
then renames the new one into place.
Eventually, it would be nice to have 'git repack' allow for writing a
multi-pack index at the critical time (after the new packs have been
written / moved into place, but before the old ones have been deleted).
Guessing that this option might be called '--write-midx', this makes the
following situation (where repacks are issued back-to-back without any
new objects) impossible:
$ git repack -adb
$ git repack -adb --write-midx
In the second repack, the existing packs are overwritten verbatim with
the same rename-to-old sequence. At that point, the current MIDX is
invalidated, since it refers to now-missing packs. So that code wants to
be run after the MIDX is re-written. But (prior to this patch) the new
MIDX can't be written until the new packs are moved into place. So, we
have a circular dependency.
This is all hypothetical, since no code currently exists to write a MIDX
safely during a 'git repack' (the 'GIT_TEST_MULTI_PACK_INDEX' does so
unsafely). Putting hypothetical aside, though: why do we need to rename
existing packs to be prefixed with 'old-' anyway?
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).
But since 1190a1a (pack-objects: name pack files after trailer hash,
2013-12-05), this is no longer possible, since pack files are named not
after their logical contents (i.e., the set of objects), but by the
actual checksum of their contents. So, this old- behavior can safely go,
which allows us to avoid our circular dependency above.
In addition to avoiding the circular dependency, this patch also makes
'git repack' a lot simpler, since we don't have to deal with failures
encountered when renaming existing packs to be prefixed with 'old-'.
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.
Helped-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
builtin/repack.c | 103 +++++++----------------------------------------
1 file changed, 14 insertions(+), 89 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bb839180da..279be11a16 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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
