{"thread":{"id":"59885","subject":"[PATCH] packfile: delete .idx files before .pack files","startedAt":"2023-06-20T19:01:22Z","lastAt":"2023-06-20T22:03:06Z","messageCount":3,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478574","messageId":"pull.1547.git.1687287675248.gitgitgadget@gmail.com","threadId":"59885","inReplyTo":null,"subject":"[PATCH] packfile: delete .idx files before .pack files","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-20T19:01:15Z","receivedAt":"2023-06-20T19:01:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen installing a packfile, we place the .pack file before the .idx\nfile. The intention is that Git scans for .idx files in the pack\ndirectory and then loads the .pack files from that list.\n\nHowever, when we delete packfiles, we do not do this in the reverse\norder as we should. The unlink_pack_path() method deletes the .pack\nfollowed by the .idx.\n\nThis creates a window where the process could be interrupted between\nthe .pack deletion and the .idx deletion, leaving the repository in a\nstate that looks strange, but isn't actually too problematic if we\nassume the pack was safe to delete. The .idx without a .pack will cause\nsome overhead, but will not interrupt other Git processes.\n\nThis ordering was introduced into the 'git repack' builtin by\na1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), though\nwe must be careful to track history through the code move in 8434e85d5f9\n(repack: refactor pack deletion for future use, 2019-06-10) to see that.\n\nThis became more important after 73320e49add (builtin/repack.c: only\ncollect fully-formed packs, 2023-06-07) changed how 'git repack' scanned\nfor packfiles for use in the cruft pack process. It previously looked\nfor .pack files, but that was problematic due to the order that packs\nare installed: repacks between the creation of a .pack and the creation\nof its .idx would result in hard failures.\n\nThere is an independent proposal about what to do in the case of a .idx\nwithout a .pack during this 'git repack' scenario, but this change is\nfocused on deleting .pack files more safely.\n\nModify the order to delete the .idx before the .pack. The rest of the\nmodifiers on the .pack should still come after the .pack so we know all\nof the presumed properties of the packfile as long as it exists in the\nfilesystem, in case we wish to reinstate it by re-indexing the .pack\nfile.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    packfile: delete .idx files before .pack files\n    \n    I'm going to submit an RFC soon [1] that approaches this issue from\n    another angle, but this should be a straightforward fix to avoid the\n    problematic \"an .idx exists without its .pack\" case that has been seen\n    in the wild.\n    \n    [1] https://github.com/gitgitgadget/git/pull/1546\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1547%2Fderrickstolee%2Fdelete-idx-first-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1547/derrickstolee/delete-idx-first-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1547\n\n packfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex fd083c86e00..6b591ddcccf 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -381,7 +381,7 @@ void close_object_store(struct raw_object_store *o)\n \n void unlink_pack_path(const char *pack_name, int force_delete)\n {\n-\tstatic const char *exts[] = {\".pack\", \".idx\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".mtimes\"};\n+\tstatic const char *exts[] = {\".idx\", \".pack\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".mtimes\"};\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t plen;\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n-- \ngitgitgadget\n"},{"id":"478594","messageId":"xmqqwmzxlvgr.fsf@gitster.g","threadId":"59885","inReplyTo":"pull.1547.git.1687287675248.gitgitgadget@gmail.com","subject":"Re: [PATCH] packfile: delete .idx files before .pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-20T20:39:16Z","receivedAt":"2023-06-20T20:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> However, when we delete packfiles, we do not do this in the reverse\n> order as we should. The unlink_pack_path() method deletes the .pack\n> followed by the .idx.\n\nMakes sense.\n"},{"id":"478605","messageId":"ZJIiElehrukHlNy2@nand.local","threadId":"59885","inReplyTo":"pull.1547.git.1687287675248.gitgitgadget@gmail.com","subject":"Re: [PATCH] packfile: delete .idx files before .pack files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-20T22:02:58Z","receivedAt":"2023-06-20T22:03:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 20, 2023 at 07:01:15PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> When installing a packfile, we place the .pack file before the .idx\n> file. The intention is that Git scans for .idx files in the pack\n> directory and then loads the .pack files from that list.\n>\n> However, when we delete packfiles, we do not do this in the reverse\n> order as we should. The unlink_pack_path() method deletes the .pack\n> followed by the .idx.\n>\n> This creates a window where the process could be interrupted between\n> the .pack deletion and the .idx deletion, leaving the repository in a\n> state that looks strange, but isn't actually too problematic if we\n> assume the pack was safe to delete. The .idx without a .pack will cause\n> some overhead, but will not interrupt other Git processes.\n\nI think that this specific case wouldn't cause a problem since we were\nabout to delete that pack anyway, but it may be worth emphasizing that\nhaving a missing pack with an extant .idx file is not always guaranteed\nto be safe.\n\n> This ordering was introduced into the 'git repack' builtin by\n> a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), though\n> we must be careful to track history through the code move in 8434e85d5f9\n> (repack: refactor pack deletion for future use, 2019-06-10) to see that.\n\nThanks for calling this out, I agree that the \"regression\" dates back to\na1bbc6c0176, since we can see that in git-repack.sh, we do:\n\n    # Remove the \"old-\" files\n    for name in $names\n    do\n      rm -f \"$PACKDIR/old-pack-$name.idx\"\n      rm -f \"$PACKDIR/old-pack-$name.pack\"\n    done\n\n...removing the .idx first, which is the right thing to do here.\n\n> diff --git a/packfile.c b/packfile.c\n> index fd083c86e00..6b591ddcccf 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -381,7 +381,7 @@ void close_object_store(struct raw_object_store *o)\n>\n>  void unlink_pack_path(const char *pack_name, int force_delete)\n>  {\n> -\tstatic const char *exts[] = {\".pack\", \".idx\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".mtimes\"};\n> +\tstatic const char *exts[] = {\".idx\", \".pack\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".mtimes\"};\n>  \tint i;\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tsize_t plen;\n\nLooks good, thanks.\n\nThanks,\nTaylor\n"}]}