{"thread":{"id":"54654","subject":"[PATCH 0/3] repack: don't move existing packs out of the way","startedAt":"2020-11-16T18:41:29Z","lastAt":"2020-11-17T21:29:01Z","messageCount":11,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"410029","messageId":"cover.1605552016.git.me@ttaylorr.com","threadId":"54654","inReplyTo":null,"subject":"[PATCH 0/3] repack: don't move existing packs out of the way","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-16T18:41:04Z","receivedAt":"2020-11-16T18:41:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere are a few patches that GitHub has been running to get rid of the\nrename-to-old behavior that 'git repack' has when its invocation of\n'pack-objects' produced a pack that already exists.\n\nThis was developed in the context of a circular dependency involving\nwriting a multi-pack index during 'git repack' (we have some patches\nthat do so by adding a '--write-midx' in the repack builtin), but it\nshould be generally useful.\n\nThe idea (which is explained in detail in the final patch) is that prior\nto 1190a1acf8 (pack-objects: name pack files after trailer hash,\n2013-12-05), 'git repack' had to move existing packs out of the way for\nsafety, but after 1190a1acf8 no longer needs to do so.\n\nThis makes 'git repack' a little simpler since it no longer has to deal\nwith any failures encountered during this rename-to-old behavior. It\nalso paves the way for sending the MIDX-during-repack patches.\n\nThanks,\nTaylor\n\nJeff King (1):\n  repack: make \"exts\" array available outside cmd_repack()\n\nTaylor Blau (2):\n  builtin/repack.c: keep track of what pack-objects wrote\n  builtin/repack.c: don't move existing packs out of the way\n\n builtin/repack.c | 153 +++++++++++++++++------------------------------\n 1 file changed, 54 insertions(+), 99 deletions(-)\n\n--\n2.29.2.312.gabc4d358d8\n"},{"id":"410030","messageId":"a235722df4e373cded3fa161124924cc0a301c0c.1605552016.git.me@ttaylorr.com","threadId":"54654","inReplyTo":"cover.1605552016.git.me@ttaylorr.com","subject":"[PATCH 1/3] repack: make \"exts\" array available outside cmd_repack()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-16T18:41:12Z","receivedAt":"2020-11-16T18:41:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWe'll use it in a helper function soon.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 01e7767c79..03e2c2c44b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -202,6 +202,16 @@ static int write_oid(const struct object_id *oid, struct packed_git *pack,\n \treturn 0;\n }\n \n+static struct {\n+\tconst char *name;\n+\tunsigned optional:1;\n+} exts[] = {\n+\t{\".pack\"},\n+\t{\".idx\"},\n+\t{\".bitmap\", 1},\n+\t{\".promisor\", 1},\n+};\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t    struct string_list *names)\n {\n@@ -265,15 +275,6 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \n int cmd_repack(int argc, const char **argv, const char *prefix)\n {\n-\tstruct {\n-\t\tconst char *name;\n-\t\tunsigned optional:1;\n-\t} exts[] = {\n-\t\t{\".pack\"},\n-\t\t{\".idx\"},\n-\t\t{\".bitmap\", 1},\n-\t\t{\".promisor\", 1},\n-\t};\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-- \n2.29.2.312.gabc4d358d8\n\n"},{"id":"410031","messageId":"34e5a9deb5d02a3a349b2b662220dfc1e0d9075f.1605552016.git.me@ttaylorr.com","threadId":"54654","inReplyTo":"cover.1605552016.git.me@ttaylorr.com","subject":"[PATCH 2/3] builtin/repack.c: keep track of what pack-objects wrote","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-16T18:41:17Z","receivedAt":"2020-11-16T18:41:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the subsequent commit, it will become useful to keep track of which\nmetadata files were written by pack-objects. We already do this to an\nextent with the 'exts' array, which only is used in the context of\nexisting packs.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 31 ++++++++++++++++++++++++++++++-\n 1 file changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 03e2c2c44b..bb839180da 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -212,6 +212,27 @@ static struct {\n \t{\".promisor\", 1},\n };\n \n+static unsigned populate_pack_exts(char *name)\n+{\n+\tstruct stat statbuf;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tunsigned ret = 0;\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_addf(&path, \"%s-%s%s\", packtmp, name, exts[i].name);\n+\n+\t\tif (stat(path.buf, &statbuf))\n+\t\t\tcontinue;\n+\n+\t\tret |= (1 << i);\n+\t}\n+\n+\tstrbuf_release(&path);\n+\treturn ret;\n+}\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t    struct string_list *names)\n {\n@@ -240,11 +261,12 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tstruct string_list_item *item;\n \t\tchar *promisor_name;\n \t\tint fd;\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n-\t\tstring_list_append(names, line.buf);\n+\t\titem = string_list_append(names, line.buf);\n \n \t\t/*\n \t\t * pack-objects creates the .pack and .idx files, but not the\n@@ -263,6 +285,9 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\tif (fd < 0)\n \t\t\tdie_errno(_(\"unable to create '%s'\"), promisor_name);\n \t\tclose(fd);\n+\n+\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n+\n \t\tfree(promisor_name);\n \t}\n \tfclose(out);\n@@ -430,6 +455,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (!names.nr && !po_args.quiet)\n \t\tprintf_ln(_(\"Nothing new to pack.\"));\n \n+\tfor_each_string_list_item(item, &names) {\n+\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n+\t}\n+\n \tclose_object_store(the_repository->objects);\n \n \t/*\n-- \n2.29.2.312.gabc4d358d8\n\n"},{"id":"410032","messageId":"d4e73aad14189a00621da9e000d2c6a5fe776d84.1605552016.git.me@ttaylorr.com","threadId":"54654","inReplyTo":"cover.1605552016.git.me@ttaylorr.com","subject":"[PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-16T18:41:21Z","receivedAt":"2020-11-16T18:41:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When 'git repack' creates a pack with the same name as any existing\npack, it moves the existing one to 'old-pack-xxx.{pack,idx,...}' and\nthen renames the new one into place.\n\nEventually, it would be nice to have 'git repack' allow for writing a\nmulti-pack index at the critical time (after the new packs have been\nwritten / moved into place, but before the old ones have been deleted).\nGuessing that this option might be called '--write-midx', this makes the\nfollowing situation (where repacks are issued back-to-back without any\nnew objects) impossible:\n\n    $ git repack -adb\n    $ git repack -adb --write-midx\n\nIn the second repack, the existing packs are overwritten verbatim with\nthe same rename-to-old sequence. At that point, the current MIDX is\ninvalidated, since it refers to now-missing packs. So that code wants to\nbe run after the MIDX is re-written. But (prior to this patch) the new\nMIDX can't be written until the new packs are moved into place. So, we\nhave a circular dependency.\n\nThis is all hypothetical, since no code currently exists to write a MIDX\nsafely during a 'git repack' (the 'GIT_TEST_MULTI_PACK_INDEX' does so\nunsafely). Putting hypothetical aside, though: why do we need to rename\nexisting packs to be prefixed with 'old-' anyway?\n\nThis behavior dates all the way back to 2ad47d6 (git-repack: Be\ncareful when updating the same pack as an existing one., 2006-06-25).\n2ad47d6 is mainly concerned about a case where a newly written pack\nwould have a different structure than its index. This used to be\npossible when the pack name was a hash of the set of objects. Under this\nnaming scheme, two packs that store the same set of objects could differ\nin delta selection, object positioning, or both. If this happened, then\nany such packs would be unreadable in the instant between copying the\nnew pack and new index (i.e., either the index or pack will be stale\ndepending on the order that they were copied).\n\nBut since 1190a1a (pack-objects: name pack files after trailer hash,\n2013-12-05), this is no longer possible, since pack files are named not\nafter their logical contents (i.e., the set of objects), but by the\nactual checksum of their contents. So, this old- behavior can safely go,\nwhich allows us to avoid our circular dependency above.\n\nIn addition to avoiding the circular dependency, this patch also makes\n'git repack' a lot simpler, since we don't have to deal with failures\nencountered when renaming existing packs to be prefixed with 'old-'.\n\nThis patch is mostly limited to removing code paths that deal with the\n'old' prefixing, with the exception of pack metadata. t7700.14 ensures\nthat 'git repack' will, for example, remove existing bitmaps even if the\npack written is verbatim the same (when repacking without '-b' in a\nrepository unchanged from the time 'git repack -b' was run). So, we have\nto handle metadata that we didn't write, by unlinking any existing\nmetadata that our invocation of pack-objects didn't generate itself.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 103 +++++++----------------------------------------\n 1 file changed, 14 insertions(+), 89 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex bb839180da..279be11a16 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -306,7 +306,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n \tstruct strbuf line = STRBUF_INIT;\n-\tint i, ext, ret, failed;\n+\tint i, ext, ret;\n \tFILE *out;\n \n \t/* variables to be filled by option parsing */\n@@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t/*\n \t * Ok we have prepared all new packfiles.\n-\t * First see if there are packs of the same name and if so\n-\t * if we can move them out of the way (this can happen if we\n-\t * repacked immediately after packing fully.\n \t */\n-\tfailed = 0;\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n \t\t\tchar *fname, *fname_old;\n \n-\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n-\t\t\t\t\t\titem->string, exts[ext].name);\n-\t\t\tif (!file_exists(fname)) {\n-\t\t\t\tfree(fname);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tfname_old = mkpathdup(\"%s/old-%s%s\", packdir,\n-\t\t\t\t\t\titem->string, exts[ext].name);\n-\t\t\tif (file_exists(fname_old))\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\t\tfree(fname);\n-\t\t\t\tfree(fname_old);\n-\t\t\t\tfailed = 1;\n-\t\t\t\tbreak;\n-\t\t\t} else {\n-\t\t\t\tstring_list_append(&rollback, fname);\n-\t\t\t\tfree(fname_old);\n-\t\t\t}\n-\t\t}\n-\t\tif (failed)\n-\t\t\tbreak;\n-\t}\n-\tif (failed) {\n-\t\tstruct string_list rollback_failure = STRING_LIST_INIT_DUP;\n-\t\tfor_each_string_list_item(item, &rollback) {\n-\t\t\tchar *fname, *fname_old;\n-\t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n-\t\t\tfname_old = mkpathdup(\"%s/old-%s\", packdir, item->string);\n-\t\t\tif (rename(fname_old, fname))\n-\t\t\t\tstring_list_append(&rollback_failure, fname);\n-\t\t\tfree(fname);\n-\t\t\tfree(fname_old);\n-\t\t}\n-\n-\t\tif (rollback_failure.nr) {\n-\t\t\tint i;\n-\t\t\tfprintf(stderr,\n-\t\t\t\t_(\"WARNING: Some packs in use have been renamed by\\n\"\n-\t\t\t\t  \"WARNING: prefixing old- to their name, in order to\\n\"\n-\t\t\t\t  \"WARNING: replace them with the new version of the\\n\"\n-\t\t\t\t  \"WARNING: file.  But the operation failed, and the\\n\"\n-\t\t\t\t  \"WARNING: attempt to rename them back to their\\n\"\n-\t\t\t\t  \"WARNING: original names also failed.\\n\"\n-\t\t\t\t  \"WARNING: Please rename them in %s manually:\\n\"), packdir);\n-\t\t\tfor (i = 0; i < rollback_failure.nr; i++)\n-\t\t\t\tfprintf(stderr, \"WARNING:   old-%s -> %s\\n\",\n-\t\t\t\t\trollback_failure.items[i].string,\n-\t\t\t\t\trollback_failure.items[i].string);\n-\t\t}\n-\t\texit(1);\n-\t}\n-\n-\t/* Now the ones with the same name are out of the way... */\n-\tfor_each_string_list_item(item, &names) {\n-\t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tchar *fname, *fname_old;\n-\t\t\tstruct stat statbuffer;\n-\t\t\tint exists = 0;\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n \t\t\t\t\tpackdir, item->string, exts[ext].name);\n \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n-\t\t\tif (!stat(fname_old, &statbuffer)) {\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\texists = 1;\n-\t\t\t}\n-\t\t\tif (exists || !exts[ext].optional) {\n+\n+\t\t\tif (((uintptr_t)item->util) & (1 << ext)) {\n+\t\t\t\tstruct stat statbuffer;\n+\t\t\t\tif (!stat(fname_old, &statbuffer)) {\n+\t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n+\t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n+\t\t\t\t}\n+\n \t\t\t\tif (rename(fname_old, fname))\n \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n-\t\t\t}\n+\t\t\t} else if (!exts[ext].optional)\n+\t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n+\t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n+\t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n+\n \t\t\tfree(fname);\n \t\t\tfree(fname_old);\n \t\t}\n \t}\n-\n-\t/* Remove the \"old-\" files */\n-\tfor_each_string_list_item(item, &names) {\n-\t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tchar *fname;\n-\t\t\tfname = mkpathdup(\"%s/old-%s%s\",\n-\t\t\t\t\t  packdir,\n-\t\t\t\t\t  item->string,\n-\t\t\t\t\t  exts[ext].name);\n-\t\t\tif (remove_path(fname))\n-\t\t\t\twarning(_(\"failed to remove '%s'\"), fname);\n-\t\t\tfree(fname);\n-\t\t}\n-\t}\n-\n \t/* End of pack replacement. */\n \n \treprepare_packed_git(the_repository);\n-- \n2.29.2.312.gabc4d358d8\n"},{"id":"410046","messageId":"xmqqpn4c7uf2.fsf@gitster.c.googlers.com","threadId":"54654","inReplyTo":"d4e73aad14189a00621da9e000d2c6a5fe776d84.1605552016.git.me@ttaylorr.com","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-16T23:29:05Z","receivedAt":"2020-11-16T23:29:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This behavior dates all the way back to 2ad47d6 (git-repack: Be\n> careful when updating the same pack as an existing one., 2006-06-25).\n> 2ad47d6 is mainly concerned about a case where a newly written pack\n> would have a different structure than its index. This used to be\n> possible when the pack name was a hash of the set of objects. Under this\n> naming scheme, two packs that store the same set of objects could differ\n> in delta selection, object positioning, or both. If this happened, then\n> any such packs would be unreadable in the instant between copying the\n> new pack and new index (i.e., either the index or pack will be stale\n> depending on the order that they were copied).\n\nTrue.  So the idea is that we can now pretend that we never wrote\nthe new packfile and leave the existing one as-is?\n\n> This patch is mostly limited to removing code paths that deal with the\n> 'old' prefixing, with the exception of pack metadata.\n\n... \"pack metadata\" meaning?  We do not remove and replace the file,\nbut we update their mtime to keep these packfiles more fresh than\nother packfiles, or something?\n\n> t7700.14 ensures\n> that 'git repack' will, for example, remove existing bitmaps even if the\n> pack written is verbatim the same (when repacking without '-b' in a\n> repository unchanged from the time 'git repack -b' was run). So, we have\n> to handle metadata that we didn't write, by unlinking any existing\n> metadata that our invocation of pack-objects didn't generate itself.\n\nHmph, t7700.14 wants it that way because?\n\nIf we were told to generate a packfile, and we ended up regenerating\nthe exactly the same one, it appears to me that we can just pretend\nnothing happened and leave things as they were?  Puzzled...\n\n> @@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \t/*\n>  \t * Ok we have prepared all new packfiles.\n>  \t */\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n>  \t\t\tchar *fname, *fname_old;\n>  \n>  \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n>  \t\t\t\t\tpackdir, item->string, exts[ext].name);\n>  \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n>  \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> +\n> +\t\t\tif (((uintptr_t)item->util) & (1 << ext)) {\n> +\t\t\t\tstruct stat statbuffer;\n> +\t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> +\t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> +\t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> +\t\t\t\t}\n> +\n>  \t\t\t\tif (rename(fname_old, fname))\n>  \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n\nOK, so this is where the previous step matters.  We do the same as\nbefore (i.e. stat+chmod and rename) only for paths we have created.\n\nWe don't reuse the old one because we have already written a new\nfile so we won't save anything by doing so, and checking if the\ntarget of rename exists already before deciding not to rename cannot\nbe made atomic, so just relying on rename() to do the right thing is\na good idea anyway.\n\n> +\t\t\t} else if (!exts[ext].optional)\n> +\t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n\nIf one branch of if/else if.../else requires multi-statement block,\nuse {} on all of them, for consistency.\n\nSo, if we wrote a few .$ext for a packfile but not some .$ext, if\nthe one we didn't write is among the necessary one, we are in\ntrouble?  OK.\n\n> +\t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n> +\t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n\nAnd if we wrote .pack and .idx but not .bitmap, the old .bitmpa that\nhas the same pack hash may be stale and we discard it for safety?\nThat sounds \"prudent\" but it is not immdiately clear from what\ndanger we are protecting ourselves.\n\nIn any case, much of what I speculated while reading the proposed\nlog message turned out to be false, which may be a sign that the log\nmessage did not explain the approach clearly enough.  I thought that\na newly created file that happened to be identical to existing ones\nwould be discarded without getting renamed to their final location,\nbut the code does not do such special casing.  I thought the\n'metadata' it talks about were to compensate for side effects of\nreusing the old files, but that was not what the 'metadata' was even\nabout.\n\nOther than that, the change in [2/3] and [3/3] look quite sensible\n(I am not saying [1/3] is bad---I haven't looked at it yet).\n\nThanks.\n\n>  \t\t\tfree(fname);\n>  \t\t\tfree(fname_old);\n>  \t\t}\n>  \t}\n>  \t/* End of pack replacement. */\n>  \n>  \treprepare_packed_git(the_repository);\n"},{"id":"410050","messageId":"20201117000252.GB12322@coredump.intra.peff.net","threadId":"54654","inReplyTo":"xmqqpn4c7uf2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T00:02:52Z","receivedAt":"2020-11-17T00:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2020 at 03:29:05PM -0800, Junio C Hamano wrote:\n\n> > t7700.14 ensures\n> > that 'git repack' will, for example, remove existing bitmaps even if the\n> > pack written is verbatim the same (when repacking without '-b' in a\n> > repository unchanged from the time 'git repack -b' was run). So, we have\n> > to handle metadata that we didn't write, by unlinking any existing\n> > metadata that our invocation of pack-objects didn't generate itself.\n> \n> Hmph, t7700.14 wants it that way because?\n> \n> If we were told to generate a packfile, and we ended up regenerating\n> the exactly the same one, it appears to me that we can just pretend\n> nothing happened and leave things as they were?  Puzzled...\n\nWe definitely could, and the outcome would be correct in the sense that\nthe bitmap would still work. I'm not sure that the test in t7700 cares\nparticularly about this \"making the same pack\" case. It only wants to\nknow that if we disable bitmaps and repack, we end up without a bitmap.\nAnd normally, if you added in even a single extra object, you'd get a\nnew pack and that would happen.\n\nBut of course the test just repacks back-to-back, so it does trigger\nthe \"making the same pack\" case.\n\nI think you could make an argument either way:\n\n  - we have an existing bitmap for free, and bitmaps make things faster,\n    so why not keep it?\n\n  - the user did not ask for bitmaps, so we should make the outcome\n    consistent whether a pack of the exact name existed before or not\n\nThe second one seems less surprising to me. But I think if we did the\nfirst, the code would be shorter (it would not need any of this \"keep\ntrack of what pack-objects generated\" stuff at all, but would just copy\nwhatever files we see into place).\n\n> >  \t\t\t\tif (rename(fname_old, fname))\n> >  \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> \n> OK, so this is where the previous step matters.  We do the same as\n> before (i.e. stat+chmod and rename) only for paths we have created.\n> \n> We don't reuse the old one because we have already written a new\n> file so we won't save anything by doing so, and checking if the\n> target of rename exists already before deciding not to rename cannot\n> be made atomic, so just relying on rename() to do the right thing is\n> a good idea anyway.\n\nEven though the pack is byte-for-byte identical, the new .idx, etc,\nmight not be. And those could be affected by options. E.g.:\n\n  git -c pack.writeBitmapHashCache=false repack -adb\n  git -c pack.writeBitmapHashCache=true  repack -adb\n\nshould probably end up with a bitmap file that contains a hash cache.\n\n-Peff\n"},{"id":"410052","messageId":"X7MYifflg7SaTRXm@nand.local","threadId":"54654","inReplyTo":"xmqqpn4c7uf2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-17T00:25:45Z","receivedAt":"2020-11-17T00:25:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 16, 2020 at 03:29:05PM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > This behavior dates all the way back to 2ad47d6 (git-repack: Be\n> > careful when updating the same pack as an existing one., 2006-06-25).\n> > 2ad47d6 is mainly concerned about a case where a newly written pack\n> > would have a different structure than its index. This used to be\n> > possible when the pack name was a hash of the set of objects. Under this\n> > naming scheme, two packs that store the same set of objects could differ\n> > in delta selection, object positioning, or both. If this happened, then\n> > any such packs would be unreadable in the instant between copying the\n> > new pack and new index (i.e., either the index or pack will be stale\n> > depending on the order that they were copied).\n>\n> True.  So the idea is that we can now pretend that we never wrote\n> the new packfile and leave the existing one as-is?\n\nTrue for packs, which are guaranteed to be unchanged by having the same\nchecksum in their filename. For other files that use their pack's\nchecksum in their filename, we want to overwrite the existing copy with\nthe one we just wrote.\n\n> > This patch is mostly limited to removing code paths that deal with the\n> > 'old' prefixing, with the exception of pack metadata.\n>\n> ... \"pack metadata\" meaning?  We do not remove and replace the file,\n> but we update their mtime to keep these packfiles more fresh than\n> other packfiles, or something?\n\nMeaning: .idx, .bitmap, .promisor files. I probably ought to be more\nclear in what \"metadata\" means, since it could easily be confused with\nmtime and others.\n\n> > @@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> >\n> >  \t/*\n> >  \t * Ok we have prepared all new packfiles.\n> >  \t */\n> >  \tfor_each_string_list_item(item, &names) {\n> >  \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n> >  \t\t\tchar *fname, *fname_old;\n> >\n> >  \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n> >  \t\t\t\t\tpackdir, item->string, exts[ext].name);\n> >  \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> >  \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> > +\n> > +\t\t\tif (((uintptr_t)item->util) & (1 << ext)) {\n> > +\t\t\t\tstruct stat statbuffer;\n> > +\t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> > +\t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> > +\t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> > +\t\t\t\t}\n> > +\n> >  \t\t\t\tif (rename(fname_old, fname))\n> >  \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n>\n> OK, so this is where the previous step matters.  We do the same as\n> before (i.e. stat+chmod and rename) only for paths we have created.\n>\n> We don't reuse the old one because we have already written a new\n> file so we won't save anything by doing so, and checking if the\n> target of rename exists already before deciding not to rename cannot\n> be made atomic, so just relying on rename() to do the right thing is\n> a good idea anyway.\n>\n> > +\t\t\t} else if (!exts[ext].optional)\n> > +\t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n>\n> If one branch of if/else if.../else requires multi-statement block,\n> use {} on all of them, for consistency.\n\nThanks.\n\n> So, if we wrote a few .$ext for a packfile but not some .$ext, if\n> the one we didn't write is among the necessary one, we are in\n> trouble?  OK.\n\nYes, exactly.\n\n> > +\t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n> > +\t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n>\n> And if we wrote .pack and .idx but not .bitmap, the old .bitmpa that\n> has the same pack hash may be stale and we discard it for safety?\n> That sounds \"prudent\" but it is not immdiately clear from what\n> danger we are protecting ourselves.\n>\n> In any case, much of what I speculated while reading the proposed\n> log message turned out to be false, which may be a sign that the log\n> message did not explain the approach clearly enough.  I thought that\n> a newly created file that happened to be identical to existing ones\n> would be discarded without getting renamed to their final location,\n> but the code does not do such special casing.  I thought the\n> 'metadata' it talks about were to compensate for side effects of\n> reusing the old files, but that was not what the 'metadata' was even\n> about.\n\nIt's more about: we trust what pack-objects wrote to be the current\nstate of things. So, if a bitmap already exists, but the caller did a\nback-to-back repack and this time generates their bitmap with the\nhash cache extension, we prefer the newer bitmap to the one that already\nexisted.\n\nThat goes both ways: if pack-objects _didn't_ write a file that already\nexists, we want to drop the existing file. Peff seems to talk more about\nthis in his email, which I'll respond to now...\n\n> Other than that, the change in [2/3] and [3/3] look quite sensible\n> (I am not saying [1/3] is bad---I haven't looked at it yet).\n>\n> Thanks.\n\nThanks,\nTaylor\n"},{"id":"410053","messageId":"X7MYtjLQmmhfyHA5@nand.local","threadId":"54654","inReplyTo":"20201117000252.GB12322@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-17T00:26:30Z","receivedAt":"2020-11-17T00:26:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 16, 2020 at 07:02:52PM -0500, Jeff King wrote:\n> I think you could make an argument either way:\n>\n>   - we have an existing bitmap for free, and bitmaps make things faster,\n>     so why not keep it?\n>\n>   - the user did not ask for bitmaps, so we should make the outcome\n>     consistent whether a pack of the exact name existed before or not\n>\n> The second one seems less surprising to me. But I think if we did the\n> first, the code would be shorter (it would not need any of this \"keep\n> track of what pack-objects generated\" stuff at all, but would just copy\n> whatever files we see into place).\n\nI think that the second one is _far_ less surprising to me, so I'd\nprefer that we did that instead of keeping the .bitmap from the last run\nregardless of whether or not the user asked for it.\n\n> > >  \t\t\t\tif (rename(fname_old, fname))\n> > >  \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> >\n> > OK, so this is where the previous step matters.  We do the same as\n> > before (i.e. stat+chmod and rename) only for paths we have created.\n> >\n> > We don't reuse the old one because we have already written a new\n> > file so we won't save anything by doing so, and checking if the\n> > target of rename exists already before deciding not to rename cannot\n> > be made atomic, so just relying on rename() to do the right thing is\n> > a good idea anyway.\n>\n> Even though the pack is byte-for-byte identical, the new .idx, etc,\n> might not be. And those could be affected by options. E.g.:\n>\n>   git -c pack.writeBitmapHashCache=false repack -adb\n>   git -c pack.writeBitmapHashCache=true  repack -adb\n>\n> should probably end up with a bitmap file that contains a hash cache.\n\nAgreed completely.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"410057","messageId":"xmqqd00c7qup.fsf@gitster.c.googlers.com","threadId":"54654","inReplyTo":"X7MYifflg7SaTRXm@nand.local","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-17T00:46:06Z","receivedAt":"2020-11-17T00:46:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> In any case, much of what I speculated while reading the proposed\n>> log message turned out to be false, which may be a sign that the log\n>> message did not explain the approach clearly enough.  I thought that\n>> a newly created file that happened to be identical to existing ones\n>> would be discarded without getting renamed to their final location,\n>> but the code does not do such special casing.  I thought the\n>> 'metadata' it talks about were to compensate for side effects of\n>> reusing the old files, but that was not what the 'metadata' was even\n>> about.\n>\n> It's more about: ...\n\nYou do not have to explain that to me here.  Instead explain that to\nfuture readers of our history in the commit log message.\n\nThanks.\n"},{"id":"410126","messageId":"X7QvVB8GjI866a8z@nand.local","threadId":"54654","inReplyTo":"xmqqd00c7qup.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-17T20:15:16Z","receivedAt":"2020-11-17T20:15:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 16, 2020 at 04:46:06PM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> > It's more about: ...\n>\n> You do not have to explain that to me here.  Instead explain that to\n> future readers of our history in the commit log message.\n>\n> Thanks.\n\nUnderstood. Here's a replacement for the final patch (the log message is\nupdated, but the patch contents are not):\n\n--- >8 ---\n\nSubject: [PATCH] builtin/repack.c: don't move existing packs out of the way\n\nWhen 'git repack' creates a pack with the same name as any existing\npack, it moves the existing one to 'old-pack-xxx.{pack,idx,...}' and\nthen renames the new one into place.\n\nEventually, it would be nice to have 'git repack' allow for writing a\nmulti-pack index at the critical time (after the new packs have been\nwritten / moved into place, but before the old ones have been deleted).\nGuessing that this option might be called '--write-midx', this makes the\nfollowing situation (where repacks are issued back-to-back without any\nnew objects) impossible:\n\n    $ git repack -adb\n    $ git repack -adb --write-midx\n\nIn the second repack, the existing packs are overwritten verbatim with\nthe same rename-to-old sequence. At that point, the current MIDX is\ninvalidated, since it refers to now-missing packs. So that code wants to\nbe run after the MIDX is re-written. But (prior to this patch) the new\nMIDX can't be written until the new packs are moved into place. So, we\nhave a circular dependency.\n\nThis is all hypothetical, since no code currently exists to write a MIDX\nsafely during a 'git repack' (the 'GIT_TEST_MULTI_PACK_INDEX' does so\nunsafely). Putting hypothetical aside, though: why do we need to rename\nexisting packs to be prefixed with 'old-' anyway?\n\nThis behavior dates all the way back to 2ad47d6 (git-repack: Be\ncareful when updating the same pack as an existing one., 2006-06-25).\n2ad47d6 is mainly concerned about a case where a newly written pack\nwould have a different structure than its index. This used to be\npossible when the pack name was a hash of the set of objects. Under this\nnaming scheme, two packs that store the same set of objects could differ\nin delta selection, object positioning, or both. If this happened, then\nany such packs would be unreadable in the instant between copying the\nnew pack and new index (i.e., either the index or pack will be stale\ndepending on the order that they were copied).\n\nBut since 1190a1a (pack-objects: name pack files after trailer hash,\n2013-12-05), this is no longer possible, since pack files are named not\nafter their logical contents (i.e., the set of objects), but by the\nactual checksum of their contents. So, this old- behavior can safely go,\nwhich allows us to avoid our circular dependency above.\n\nIn addition to avoiding the circular dependency, this patch also makes\n'git repack' a lot simpler, since we don't have to deal with failures\nencountered when renaming existing packs to be prefixed with 'old-'.\n\nThis patch is mostly limited to removing code paths that deal with the\n'old' prefixing, with the exception of files that include the pack's\nname in their own filename, like .idx, .bitmap, and related files. The\nexception is that we want to continue to trust what pack-objects wrote.\nThat is, it is not the case that we pretend as if pack-objects didn't\nwrite files identical to ones that already exist, but rather that we\nrespect what pack-objects wrote as the source of truth. That cuts two\nways:\n\n  - If pack-objects produced an identical pack to one that already\n    exists with a bitmap, but did not produce a bitmap, we remove the\n    bitmap that already exists. (This behavior is codified in t7700.14).\n\n  - If pack-objects produced an identical pack to one that already\n    exists, we trust the just-written version of the coresponding .idx,\n    .promisor, and other files over the ones that already exist. This\n    ensures that we use the most up-to-date versions of this files,\n    which is safe even in the face of format changes in, say, the .idx\n    file (which would not be reflected in the .idx file's name).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 103 +++++++----------------------------------------\n 1 file changed, 14 insertions(+), 89 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex bb839180da..279be11a16 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -306,7 +306,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n \tstruct strbuf line = STRBUF_INIT;\n-\tint i, ext, ret, failed;\n+\tint i, ext, ret;\n \tFILE *out;\n\n \t/* variables to be filled by option parsing */\n@@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n\n \t/*\n \t * Ok we have prepared all new packfiles.\n-\t * First see if there are packs of the same name and if so\n-\t * if we can move them out of the way (this can happen if we\n-\t * repacked immediately after packing fully.\n \t */\n-\tfailed = 0;\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n \t\t\tchar *fname, *fname_old;\n\n-\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n-\t\t\t\t\t\titem->string, exts[ext].name);\n-\t\t\tif (!file_exists(fname)) {\n-\t\t\t\tfree(fname);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tfname_old = mkpathdup(\"%s/old-%s%s\", packdir,\n-\t\t\t\t\t\titem->string, exts[ext].name);\n-\t\t\tif (file_exists(fname_old))\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\t\tfree(fname);\n-\t\t\t\tfree(fname_old);\n-\t\t\t\tfailed = 1;\n-\t\t\t\tbreak;\n-\t\t\t} else {\n-\t\t\t\tstring_list_append(&rollback, fname);\n-\t\t\t\tfree(fname_old);\n-\t\t\t}\n-\t\t}\n-\t\tif (failed)\n-\t\t\tbreak;\n-\t}\n-\tif (failed) {\n-\t\tstruct string_list rollback_failure = STRING_LIST_INIT_DUP;\n-\t\tfor_each_string_list_item(item, &rollback) {\n-\t\t\tchar *fname, *fname_old;\n-\t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n-\t\t\tfname_old = mkpathdup(\"%s/old-%s\", packdir, item->string);\n-\t\t\tif (rename(fname_old, fname))\n-\t\t\t\tstring_list_append(&rollback_failure, fname);\n-\t\t\tfree(fname);\n-\t\t\tfree(fname_old);\n-\t\t}\n-\n-\t\tif (rollback_failure.nr) {\n-\t\t\tint i;\n-\t\t\tfprintf(stderr,\n-\t\t\t\t_(\"WARNING: Some packs in use have been renamed by\\n\"\n-\t\t\t\t  \"WARNING: prefixing old- to their name, in order to\\n\"\n-\t\t\t\t  \"WARNING: replace them with the new version of the\\n\"\n-\t\t\t\t  \"WARNING: file.  But the operation failed, and the\\n\"\n-\t\t\t\t  \"WARNING: attempt to rename them back to their\\n\"\n-\t\t\t\t  \"WARNING: original names also failed.\\n\"\n-\t\t\t\t  \"WARNING: Please rename them in %s manually:\\n\"), packdir);\n-\t\t\tfor (i = 0; i < rollback_failure.nr; i++)\n-\t\t\t\tfprintf(stderr, \"WARNING:   old-%s -> %s\\n\",\n-\t\t\t\t\trollback_failure.items[i].string,\n-\t\t\t\t\trollback_failure.items[i].string);\n-\t\t}\n-\t\texit(1);\n-\t}\n-\n-\t/* Now the ones with the same name are out of the way... */\n-\tfor_each_string_list_item(item, &names) {\n-\t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tchar *fname, *fname_old;\n-\t\t\tstruct stat statbuffer;\n-\t\t\tint exists = 0;\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n \t\t\t\t\tpackdir, item->string, exts[ext].name);\n \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n-\t\t\tif (!stat(fname_old, &statbuffer)) {\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\texists = 1;\n-\t\t\t}\n-\t\t\tif (exists || !exts[ext].optional) {\n+\n+\t\t\tif (((uintptr_t)item->util) & (1 << ext)) {\n+\t\t\t\tstruct stat statbuffer;\n+\t\t\t\tif (!stat(fname_old, &statbuffer)) {\n+\t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n+\t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n+\t\t\t\t}\n+\n \t\t\t\tif (rename(fname_old, fname))\n \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n-\t\t\t}\n+\t\t\t} else if (!exts[ext].optional)\n+\t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n+\t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n+\t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n+\n \t\t\tfree(fname);\n \t\t\tfree(fname_old);\n \t\t}\n \t}\n-\n-\t/* Remove the \"old-\" files */\n-\tfor_each_string_list_item(item, &names) {\n-\t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tchar *fname;\n-\t\t\tfname = mkpathdup(\"%s/old-%s%s\",\n-\t\t\t\t\t  packdir,\n-\t\t\t\t\t  item->string,\n-\t\t\t\t\t  exts[ext].name);\n-\t\t\tif (remove_path(fname))\n-\t\t\t\twarning(_(\"failed to remove '%s'\"), fname);\n-\t\t\tfree(fname);\n-\t\t}\n-\t}\n-\n \t/* End of pack replacement. */\n\n \treprepare_packed_git(the_repository);\n--\n2.29.2.312.gabc4d358d8\n\n"},{"id":"410136","messageId":"xmqqzh3f4qqy.fsf@gitster.c.googlers.com","threadId":"54654","inReplyTo":"X7QvVB8GjI866a8z@nand.local","subject":"Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-17T21:28:53Z","receivedAt":"2020-11-17T21:29:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> That is, it is not the case that we pretend as if pack-objects didn't\n> write files identical to ones that already exist, but rather that we\n> respect what pack-objects wrote as the source of truth. That cuts two\n> ways:\n>\n>   - If pack-objects produced an identical pack to one that already\n>     exists with a bitmap, but did not produce a bitmap, we remove the\n>     bitmap that already exists. (This behavior is codified in t7700.14).\n>\n>   - If pack-objects produced an identical pack to one that already\n>     exists, we trust the just-written version of the coresponding .idx,\n>     .promisor, and other files over the ones that already exist. This\n>     ensures that we use the most up-to-date versions of this files,\n>     which is safe even in the face of format changes in, say, the .idx\n>     file (which would not be reflected in the .idx file's name).\n\nVery clearly written.  I see no room for confusion, unlike the\noriginal one.\n\nThanks, will replace.\n\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  builtin/repack.c | 103 +++++++----------------------------------------\n>  1 file changed, 14 insertions(+), 89 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index bb839180da..279be11a16 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -306,7 +306,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n>  \tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n>  \tstruct strbuf line = STRBUF_INIT;\n> -\tint i, ext, ret, failed;\n> +\tint i, ext, ret;\n>  \tFILE *out;\n>\n>  \t/* variables to be filled by option parsing */\n> @@ -463,109 +463,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>\n>  \t/*\n>  \t * Ok we have prepared all new packfiles.\n> -\t * First see if there are packs of the same name and if so\n> -\t * if we can move them out of the way (this can happen if we\n> -\t * repacked immediately after packing fully.\n>  \t */\n> -\tfailed = 0;\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n>  \t\t\tchar *fname, *fname_old;\n>\n> -\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n> -\t\t\t\t\t\titem->string, exts[ext].name);\n> -\t\t\tif (!file_exists(fname)) {\n> -\t\t\t\tfree(fname);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\n> -\t\t\tfname_old = mkpathdup(\"%s/old-%s%s\", packdir,\n> -\t\t\t\t\t\titem->string, exts[ext].name);\n> -\t\t\tif (file_exists(fname_old))\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\t\tfree(fname);\n> -\t\t\t\tfree(fname_old);\n> -\t\t\t\tfailed = 1;\n> -\t\t\t\tbreak;\n> -\t\t\t} else {\n> -\t\t\t\tstring_list_append(&rollback, fname);\n> -\t\t\t\tfree(fname_old);\n> -\t\t\t}\n> -\t\t}\n> -\t\tif (failed)\n> -\t\t\tbreak;\n> -\t}\n> -\tif (failed) {\n> -\t\tstruct string_list rollback_failure = STRING_LIST_INIT_DUP;\n> -\t\tfor_each_string_list_item(item, &rollback) {\n> -\t\t\tchar *fname, *fname_old;\n> -\t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n> -\t\t\tfname_old = mkpathdup(\"%s/old-%s\", packdir, item->string);\n> -\t\t\tif (rename(fname_old, fname))\n> -\t\t\t\tstring_list_append(&rollback_failure, fname);\n> -\t\t\tfree(fname);\n> -\t\t\tfree(fname_old);\n> -\t\t}\n> -\n> -\t\tif (rollback_failure.nr) {\n> -\t\t\tint i;\n> -\t\t\tfprintf(stderr,\n> -\t\t\t\t_(\"WARNING: Some packs in use have been renamed by\\n\"\n> -\t\t\t\t  \"WARNING: prefixing old- to their name, in order to\\n\"\n> -\t\t\t\t  \"WARNING: replace them with the new version of the\\n\"\n> -\t\t\t\t  \"WARNING: file.  But the operation failed, and the\\n\"\n> -\t\t\t\t  \"WARNING: attempt to rename them back to their\\n\"\n> -\t\t\t\t  \"WARNING: original names also failed.\\n\"\n> -\t\t\t\t  \"WARNING: Please rename them in %s manually:\\n\"), packdir);\n> -\t\t\tfor (i = 0; i < rollback_failure.nr; i++)\n> -\t\t\t\tfprintf(stderr, \"WARNING:   old-%s -> %s\\n\",\n> -\t\t\t\t\trollback_failure.items[i].string,\n> -\t\t\t\t\trollback_failure.items[i].string);\n> -\t\t}\n> -\t\texit(1);\n> -\t}\n> -\n> -\t/* Now the ones with the same name are out of the way... */\n> -\tfor_each_string_list_item(item, &names) {\n> -\t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n> -\t\t\tchar *fname, *fname_old;\n> -\t\t\tstruct stat statbuffer;\n> -\t\t\tint exists = 0;\n>  \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n>  \t\t\t\t\tpackdir, item->string, exts[ext].name);\n>  \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n>  \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> -\t\t\tif (!stat(fname_old, &statbuffer)) {\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\texists = 1;\n> -\t\t\t}\n> -\t\t\tif (exists || !exts[ext].optional) {\n> +\n> +\t\t\tif (((uintptr_t)item->util) & (1 << ext)) {\n> +\t\t\t\tstruct stat statbuffer;\n> +\t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> +\t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> +\t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> +\t\t\t\t}\n> +\n>  \t\t\t\tif (rename(fname_old, fname))\n>  \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> -\t\t\t}\n> +\t\t\t} else if (!exts[ext].optional)\n> +\t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n> +\t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n> +\t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n> +\n>  \t\t\tfree(fname);\n>  \t\t\tfree(fname_old);\n>  \t\t}\n>  \t}\n> -\n> -\t/* Remove the \"old-\" files */\n> -\tfor_each_string_list_item(item, &names) {\n> -\t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n> -\t\t\tchar *fname;\n> -\t\t\tfname = mkpathdup(\"%s/old-%s%s\",\n> -\t\t\t\t\t  packdir,\n> -\t\t\t\t\t  item->string,\n> -\t\t\t\t\t  exts[ext].name);\n> -\t\t\tif (remove_path(fname))\n> -\t\t\t\twarning(_(\"failed to remove '%s'\"), fname);\n> -\t\t\tfree(fname);\n> -\t\t}\n> -\t}\n> -\n>  \t/* End of pack replacement. */\n>\n>  \treprepare_packed_git(the_repository);\n> --\n> 2.29.2.312.gabc4d358d8\n"}]}