{"thread":{"id":"59843","subject":"[PATCH] builtin/repack.c: only collect fully-formed packs","startedAt":"2023-06-07T10:16:21Z","lastAt":"2023-06-07T13:51:28Z","messageCount":2,"participants":["Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478123","messageId":"dda5a34a3e879787ce8651674962db6cf913b7b2.1686132967.git.me@ttaylorr.com","threadId":"59843","inReplyTo":null,"subject":"[PATCH] builtin/repack.c: only collect fully-formed packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-07T10:16:12Z","receivedAt":"2023-06-07T10:16:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To partition the set of packs based on which ones are \"kept\" (either\nthey have a .keep file, or were otherwise marked via the `--keep-pack`\noption) and \"non-kept\" ones (anything else), `git repack` uses its\n`collect_pack_filenames()` function.\n\nOrdinarily, we would rely on a convenience function such as\n`get_all_packs()` to enumerate and partition the set of packs. But\n`collect_pack_filenames()` uses `readdir()` directly to read the\ncontents of the \"$GIT_DIR/objects/pack\" directory, and adds each entry\nending in \".pack\" to the appropriate list (either kept, or non-kept as\nabove).\n\nThis is subtly racy, since `collect_pack_filenames()` may see a pack\nthat is not fully staged (i.e., it is missing its \".idx\" file).\nOrdinarily, this doesn't cause a problem. But it can cause issues when\ngenerating a cruft pack.\n\nThis is because `git repack` feeds (among other things) the list of\nexisting kept packs down to `git pack-objects --cruft` to indicate that\nany kept packs will not be removed from the repository (so that the\ncruft pack machinery can avoid packing objects that appear in those\npacks as cruft).\n\nBut `read_cruft_objects()` lists packfiles by calling `get_all_packs()`.\nSo if a \".pack\" file exists (necessary to get that pack to appear to\n`collect_pack_filenames()`), but doesn't have a corresponding \".idx\"\nfile (necessary to get that pack to appear via `get_all_packs()`), we'll\ncomplain with:\n\n    fatal: could not find pack '.tmp-5841-pack-a6b0150558609c323c496ced21de6f4b66589260.pack'\n\nFix the above by teaching `collect_pack_filenames()` to only collect\npacks with their corresponding `*.idx` files in place, indicating that\nthose packs have been fully staged.\n\nThere are a couple of things worth noting:\n\n  - Since each entry in the `extra_keep` list (which contains the\n    `--keep-pack` names) has a `*.pack` suffix, we'll have to swap the\n    suffix from \".pack\" to \".idx\", and compare that instead.\n\n  - Since we use the the `fname_kept_list` to figure out which packs to\n    delete (with `git repack -d`), we would have previously deleted a\n    `*.pack` with no index (since the existince of a \".pack\" file is\n    necessary and sufficient to include that pack in the list of\n    existing non-kept packs).\n\n    Now we will leave it alone (since that pack won't appear in the\n    list). This is far more correct behavior, since we don't want\n    to race with a pack being staged. Deleting a partially staged pack\n    is unlikely, however, since the window of time between staging a\n    pack and moving its .idx file into place is miniscule.\n\n    Note that this window does *not* include the time it takes to\n    receive and index the pack, since the incoming data goes into\n    \"$GIT_DIR/objects/tmp_pack_XXXXXX\", which does not end in \".pack\"\n    and is thus ignored by collect_pack_filenames().\n\nIn the future, this function should probably be rewritten as a callback\nto `for_each_file_in_pack_dir()`, but this is the simplest change we\ncould do in the short-term.\n\nReported-by: Michael Haggerty <mhagger@github.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c  | 14 ++++++++++----\n t/t7700-repack.sh | 23 +++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0541c3ce15..1e21a21ea8 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -95,8 +95,8 @@ static int repack_config(const char *var, const char *value, void *cb)\n }\n \n /*\n- * Adds all packs hex strings to either fname_nonkept_list or\n- * fname_kept_list based on whether each pack has a corresponding\n+ * Adds all packs hex strings (pack-$HASH) to either fname_nonkept_list\n+ * or fname_kept_list based on whether each pack has a corresponding\n  * .keep file or not.  Packs without a .keep file are not to be kept\n  * if we are going to pack everything into one file.\n  */\n@@ -107,6 +107,7 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \tDIR *dir;\n \tstruct dirent *e;\n \tchar *fname;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (!(dir = opendir(packdir)))\n \t\treturn;\n@@ -115,11 +116,15 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\tsize_t len;\n \t\tint i;\n \n-\t\tif (!strip_suffix(e->d_name, \".pack\", &len))\n+\t\tif (!strip_suffix(e->d_name, \".idx\", &len))\n \t\t\tcontinue;\n \n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_add(&buf, e->d_name, len);\n+\t\tstrbuf_addstr(&buf, \".pack\");\n+\n \t\tfor (i = 0; i < extra_keep->nr; i++)\n-\t\t\tif (!fspathcmp(e->d_name, extra_keep->items[i].string))\n+\t\t\tif (!fspathcmp(buf.buf, extra_keep->items[i].string))\n \t\t\t\tbreak;\n \n \t\tfname = xmemdupz(e->d_name, len);\n@@ -136,6 +141,7 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\t}\n \t}\n \tclosedir(dir);\n+\tstrbuf_release(&buf);\n \n \tstring_list_sort(fname_kept_list);\n }\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex faa739eeb9..08b5ba0c15 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -10,6 +10,10 @@ test_description='git repack works correctly'\n commit_and_pack () {\n \ttest_commit \"$@\" 1>&2 &&\n \tincrpackid=$(git pack-objects --all --unpacked --incremental .git/objects/pack/pack </dev/null) &&\n+\t# Remove any loose object(s) created by test_commit, since they have\n+\t# already been packed. Leaving these around can create subtly different\n+\t# packs with `pack-objects`'s `--unpacked` option.\n+\tgit prune-packed 1>&2 &&\n \techo pack-${incrpackid}.pack\n }\n \n@@ -209,6 +213,8 @@ test_expect_success 'repack --keep-pack' '\n \ttest_create_repo keep-pack &&\n \t(\n \t\tcd keep-pack &&\n+\t\t# avoid producing difference packs to delta/base choices\n+\t\tgit config pack.window 0 &&\n \t\tP1=$(commit_and_pack 1) &&\n \t\tP2=$(commit_and_pack 2) &&\n \t\tP3=$(commit_and_pack 3) &&\n@@ -220,6 +226,23 @@ test_expect_success 'repack --keep-pack' '\n \t\tgrep -q $P1 new-counts &&\n \t\tgrep -q $P4 new-counts &&\n \t\ttest_line_count = 3 new-counts &&\n+\t\tgit fsck &&\n+\n+\t\tP5=$(commit_and_pack --no-tag 5) &&\n+\t\tgit reset --hard HEAD^ &&\n+\t\tgit reflog expire --all --expire=all &&\n+\t\trm -f \".git/objects/pack/${P5%.pack}.idx\" &&\n+\t\trm -f \".git/objects/info/commit-graph\" &&\n+\t\tfor from in $(find .git/objects/pack -type f -name \"${P5%.pack}.*\")\n+\t\tdo\n+\t\t\tto=\"$(dirname \"$from\")/.tmp-1234-$(basename \"$from\")\" &&\n+\t\t\tmv \"$from\" \"$to\" || return 1\n+\t\tdone &&\n+\n+\t\tgit repack --cruft -d --keep-pack $P1 --keep-pack $P4 &&\n+\n+\t\tls .git/objects/pack/*.pack >newer-counts &&\n+\t\ttest_cmp new-counts newer-counts &&\n \t\tgit fsck\n \t)\n '\n-- \n2.41.0.1.gcf79d13182\n"},{"id":"478144","messageId":"d3c67344-a10d-f680-4601-581138985006@github.com","threadId":"59843","inReplyTo":"dda5a34a3e879787ce8651674962db6cf913b7b2.1686132967.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: only collect fully-formed packs","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-06-07T13:51:22Z","receivedAt":"2023-06-07T13:51:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/7/2023 6:16 AM, Taylor Blau wrote:\n> To partition the set of packs based on which ones are \"kept\" (either\n> they have a .keep file, or were otherwise marked via the `--keep-pack`\n> option) and \"non-kept\" ones (anything else), `git repack` uses its\n> `collect_pack_filenames()` function.\n\n...\n\n> Fix the above by teaching `collect_pack_filenames()` to only collect\n> packs with their corresponding `*.idx` files in place, indicating that\n> those packs have been fully staged.\n\nI reviewed an internal version of this patch, so I'm happy with this\nversion. I'd be very interested if anyone has concerns with the\napproach, though.\n\nThanks,\n-Stolee\n\n"}]}