{"thread":{"id":"59968","subject":"[PATCH 0/2] repack: make `collect_pack_filenames()` consistent","startedAt":"2023-07-10T19:37:19Z","lastAt":"2023-07-11T17:32:43Z","messageCount":10,"participants":["Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"479345","messageId":"cover.1689017830.git.me@ttaylorr.com","threadId":"59968","inReplyTo":null,"subject":"[PATCH 0/2] repack: make `collect_pack_filenames()` consistent","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-10T19:37:12Z","receivedAt":"2023-07-10T19:37:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series combines a patch that Stolee wrote[1] as an RFC, along with\none that I added as a follow-up on top[2].\n\nThe details are spelled out in the commit messages, but the gist is that\nthe first patch restores behavior from prior to 73320e49ad\n(builtin/repack.c: only collect fully-formed packs, 2023-06-07).\n\nThe second patch does not change any behavior, but reimplements\n`collect_pack_filenames()` in terms of `get_all_packs()`, to make `git\nrepack`'s notion of which packs exist in a usable state in the\nrepository is consistent with `add_packed_git()`,\n`install_packed_git()`, etc.\n\n[1]: https://lore.kernel.org/git/pull.1546.git.1687287782439.gitgitgadget@gmail.com/\n[2]: https://lore.kernel.org/git/ZJ1N2I6sDfxhrJo8@nand.local/\n\nDerrick Stolee (1):\n  builtin/repack.c: only repack `.pack`s that exist\n\nTaylor Blau (1):\n  builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`\n\n builtin/repack.c  | 38 +++++++++++++++-----------------------\n t/t7700-repack.sh | 47 ++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 57 insertions(+), 28 deletions(-)\n\n-- \n2.41.0.320.gb3d0d9308ef\n"},{"id":"479346","messageId":"f14a88f1075093c870cd1d53b4e0cea10d5ab67d.1689017830.git.me@ttaylorr.com","threadId":"59968","inReplyTo":"cover.1689017830.git.me@ttaylorr.com","subject":"[PATCH 1/2] builtin/repack.c: only repack `.pack`s that exist","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-10T19:37:15Z","receivedAt":"2023-07-10T19:37:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nIn 73320e49add (builtin/repack.c: only collect fully-formed packs,\n2023-06-07), we switched the check for which packs to collect by\nstarting at the .idx files and looking for matching .pack files. This\navoids trying to repack pack-files that have not had their pack-indexes\ninstalled yet.\n\nHowever, it does cause maintenance to halt if we find the (problematic,\nbut not insurmountable) case of a .idx file without a corresponding\n.pack file. In an environment where packfile maintenance is a critical\nfunction, such a hard stop is costly and requires human intervention to\nresolve (by deleting the .idx file).\n\nThis was not the case before. We successfully repacked through this\nscenario until the recent change to scan for .idx files.\n\nFurther, if we are actually in a case where objects are missing, we\ndetect this at a different point during the reachability walk.\n\nIn other cases, Git prepares its list of packfiles by scanning .idx\nfiles and then only adds it to the packfile list if the corresponding\n.pack file exists. It even does so without a warning! (See\nadd_packed_git() in packfile.c for details.)\n\nThis case is much less likely to occur than the failures seen before\n73320e49add. Packfiles are \"installed\" by writing the .pack file before\nthe .idx and that process can be interrupted. Packfiles _should_ be\ndeleted by deleting the .idx first, followed by the .pack file, but\nunlink_pack_path() does not do this: it deletes the .pack _first_,\nallowing a window where this process could be interrupted. We leave the\nconsideration of changing this order as a separate concern. Knowing that\nthis condition is possible from interrupted Git processes and not other\ntools lends some weight that Git should be more flexible around this\nscenario.\n\nAdd a check to see if the .pack file exists before adding it to the list\nfor repacking. This will stop a number of maintenance failures seen in\nproduction but fixed by deleting the .idx files.\n\nThis brings us closer to the case before 73320e49add in that 'git\nrepack' will not fail when there is an orphaned .idx file, at least, not\ndue to the way we scan for packfiles. In the case that the .pack file\nwas erroneously deleted without copies of its objects in other installed\npackfiles, then 'git repack' will fail due to the reachable object walk.\n\nThis does resolve the case where automated repacks will no longer be\nhalted on this case. The tests in t7700 show both these successful\nscenarios and the case of failing if the .pack was truly required.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c  |  3 +++\n t/t7700-repack.sh | 47 ++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 51698e3c68..724e09536e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -125,6 +125,9 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\tstrbuf_add(&buf, e->d_name, len);\n \t\tstrbuf_addstr(&buf, \".pack\");\n \n+\t\tif (!file_exists(mkpath(\"%s/%s\", packdir, buf.buf)))\n+\t\t\tcontinue;\n+\n \t\tfor (i = 0; i < extra_keep->nr; i++)\n \t\t\tif (!fspathcmp(buf.buf, extra_keep->items[i].string))\n \t\t\t\tbreak;\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex af79266c58..284954791d 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -213,16 +213,16 @@ 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\t# avoid producing different packs due 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 \t\tP4=$(commit_and_pack 4) &&\n-\t\tls .git/objects/pack/*.pack >old-counts &&\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n \t\ttest_line_count = 4 old-counts &&\n \t\tgit repack -a -d --keep-pack $P1 --keep-pack $P4 &&\n-\t\tls .git/objects/pack/*.pack >new-counts &&\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >new-counts &&\n \t\tgrep -q $P1 new-counts &&\n \t\tgrep -q $P4 new-counts &&\n \t\ttest_line_count = 3 new-counts &&\n@@ -239,14 +239,51 @@ test_expect_success 'repack --keep-pack' '\n \t\t\tmv \"$from\" \"$to\" || return 1\n \t\tdone &&\n \n+\t\t# A .idx file without a .pack should not stop us from\n+\t\t# repacking what we can.\n+\t\ttouch .git/objects/pack/pack-does-not-exist.idx &&\n+\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >packs.before &&\n \t\tgit repack --cruft -d --keep-pack $P1 --keep-pack $P4 &&\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >packs.after &&\n \n-\t\tls .git/objects/pack/*.pack >newer-counts &&\n-\t\ttest_cmp new-counts newer-counts &&\n+\t\ttest_cmp packs.before packs.after &&\n \t\tgit fsck\n \t)\n '\n \n+test_expect_success 'repacking fails when missing .pack actually means missing objects' '\n+\ttest_create_repo idx-without-pack &&\n+\t(\n+\t\tcd idx-without-pack &&\n+\n+\t\t# Avoid producing different packs due 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+\t\tP4=$(commit_and_pack 4) &&\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n+\t\ttest_line_count = 4 old-counts &&\n+\n+\t\t# Remove one .pack file\n+\t\trm .git/objects/pack/$P2 &&\n+\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f |\n+\t\t\tsort >before-pack-dir &&\n+\n+\t\ttest_must_fail git fsck &&\n+\t\ttest_must_fail git repack --cruft -d 2>err &&\n+\t\tgrep \"bad object\" err &&\n+\n+\t\t# Before failing, the repack did not modify the\n+\t\t# pack directory.\n+\t\tfind .git/objects/pack -name \"*.pack\" -type f |\n+\t\t\tsort >after-pack-dir &&\n+\t\ttest_cmp before-pack-dir after-pack-dir\n+\t)\n+'\n+\n test_expect_success 'bitmaps are created by default in bare repos' '\n \tgit clone --bare .git bare.git &&\n \trm -f bare.git/objects/pack/*.bitmap &&\n-- \n2.41.0.320.gb3d0d9308ef\n\n"},{"id":"479347","messageId":"b3d0d9308ef1a0aab8dd5a65354044568c65388b.1689017830.git.me@ttaylorr.com","threadId":"59968","inReplyTo":"cover.1689017830.git.me@ttaylorr.com","subject":"[PATCH 2/2] builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-10T19:37:18Z","receivedAt":"2023-07-10T19:37:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When repacking, the function `collect_pack_filenames()` is responsible\nfor collecting the set of existing packs in the repository, and\npartitioning them into \"kept\" (if the pack has a \".keep\" file or was\ngiven via `--keep-pack`) and \"nonkept\" (otherwise) lists.\n\nThis function comes from the original C port of git-repack.sh from back\nin a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15),\nwhere it first appears as `get_non_kept_pack_filenames()`. At the time,\nthe implementation was a fairly direct translation from the relevant\nportion of git-repack.sh, which looped over the results of\n\n    find \"$PACKDIR\" -type f -name '*.pack'\n\neither ignoring the pack as kept, or adding it to the list of existing\npacks.\n\nSo the choice to directly translate this function in terms of\n`readdir()` in a1bbc6c0176 made sense. At the time, it was possible to\nrefine the C version in terms of packed_git structs, but was never done.\n\nHowever, manually enumerating a repository's packs via `readdir()` is\nconfusing and error-prone. It leads to frustrating inconsistencies\nbetween which packs Git considers to be part of a repository (i.e.,\ncould be found in the list of packs from `get_all_packs()`), and which\npacks `collect_pack_filenames()` considers to meet the same criteria.\n\nThis bit us in 73320e49ad (builtin/repack.c: only collect fully-formed\npacks, 2023-06-07), and again in the previous commit.\n\nPrevent these issues from biting us in the future by implementing the\n`collect_pack_filenames()` function by looping over an array of pointers\nto `packed_git` structs, ensuring that we use the same criteria to\ndetermine the set of available packs.\n\nOne gotcha here is that we have to ignore non-local packs, since the\noriginal version of `collect_pack_filenames()` only looks at the local\npack directory to collect existing packs.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 41 +++++++++++++++--------------------------\n 1 file changed, 15 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 724e09536e..ac9fc61d2e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -106,49 +106,38 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\t\t\t   struct string_list *fname_kept_list,\n \t\t\t\t   const struct string_list *extra_keep)\n {\n-\tDIR *dir;\n-\tstruct dirent *e;\n-\tchar *fname;\n+\tstruct packed_git *p;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (!(dir = opendir(packdir)))\n-\t\treturn;\n-\n-\twhile ((e = readdir(dir)) != NULL) {\n-\t\tsize_t len;\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tint i;\n+\t\tconst char *base;\n \n-\t\tif (!strip_suffix(e->d_name, \".idx\", &len))\n+\t\tif (!p->pack_local)\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\tif (!file_exists(mkpath(\"%s/%s\", packdir, buf.buf)))\n-\t\t\tcontinue;\n+\t\tbase = pack_basename(p);\n \n \t\tfor (i = 0; i < extra_keep->nr; i++)\n-\t\t\tif (!fspathcmp(buf.buf, extra_keep->items[i].string))\n+\t\t\tif (!fspathcmp(base, extra_keep->items[i].string))\n \t\t\t\tbreak;\n \n-\t\tfname = xmemdupz(e->d_name, len);\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, base);\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n-\t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) ||\n-\t\t    (file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))) {\n-\t\t\tstring_list_append_nodup(fname_kept_list, fname);\n-\t\t} else {\n+\t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n+\t\t\tstring_list_append(fname_kept_list, buf.buf);\n+\t\telse {\n \t\t\tstruct string_list_item *item;\n-\t\t\titem = string_list_append_nodup(fname_nonkept_list,\n-\t\t\t\t\t\t\tfname);\n-\t\t\tif (file_exists(mkpath(\"%s/%s.mtimes\", packdir, fname)))\n+\t\t\titem = string_list_append(fname_nonkept_list, buf.buf);\n+\t\t\tif (p->is_cruft)\n \t\t\t\titem->util = (void*)(uintptr_t)CRUFT_PACK;\n \t\t}\n \t}\n-\tclosedir(dir);\n-\tstrbuf_release(&buf);\n \n \tstring_list_sort(fname_kept_list);\n+\tstrbuf_release(&buf);\n }\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n-- \n2.41.0.320.gb3d0d9308ef\n"},{"id":"479349","messageId":"xmqqedlf4jg1.fsf@gitster.g","threadId":"59968","inReplyTo":"f14a88f1075093c870cd1d53b4e0cea10d5ab67d.1689017830.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] builtin/repack.c: only repack `.pack`s that exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-10T20:09:50Z","receivedAt":"2023-07-10T20:09:58Z","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> Add a check to see if the .pack file exists before adding it to the list\n> for repacking. This will stop a number of maintenance failures seen in\n> production but fixed by deleting the .idx files.\n\nWhen we are adding we'd eventually add both and something that lack\none of them will eventually become complete and will be part of the\nrepacking once that happens.  When we are removing (because another\nrepack has about to finish), removing either one of them will make\nthe pack ineligible, which is OK.  If somebody crashed while adding\nor removing and ended up leaving only one, not both, on the\nfilesystem, ignoring such a leftover stuff would be the least\ndisruptive for automation.  Makes sense.\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index af79266c58..284954791d 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -213,16 +213,16 @@ 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\t# avoid producing different packs due to delta/base choices\n\nOK.\n\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>  \t\tP4=$(commit_and_pack 4) &&\n> -\t\tls .git/objects/pack/*.pack >old-counts &&\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n>  \t\ttest_line_count = 4 old-counts &&\n>  \t\tgit repack -a -d --keep-pack $P1 --keep-pack $P4 &&\n> -\t\tls .git/objects/pack/*.pack >new-counts &&\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >new-counts &&\n>  \t\tgrep -q $P1 new-counts &&\n>  \t\tgrep -q $P4 new-counts &&\n>  \t\ttest_line_count = 3 new-counts &&\n\nI do not think \"sort\" in both of these added lines is doing anything\nuseful.  Does the test break without this hunk and if so how?\n\n> @@ -239,14 +239,51 @@ test_expect_success 'repack --keep-pack' '\n>  \t\t\tmv \"$from\" \"$to\" || return 1\n>  \t\tdone &&\n>  \n> +\t\t# A .idx file without a .pack should not stop us from\n> +\t\t# repacking what we can.\n> +\t\ttouch .git/objects/pack/pack-does-not-exist.idx &&\n\n\t>.git/objects/pack/pack-does-not-exist.idx &&\n\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >packs.before &&\n>  \t\tgit repack --cruft -d --keep-pack $P1 --keep-pack $P4 &&\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >packs.after &&\n>  \n> -\t\tls .git/objects/pack/*.pack >newer-counts &&\n> -\t\ttest_cmp new-counts newer-counts &&\n> +\t\ttest_cmp packs.before packs.after &&\n\n\nI'd say the changes from \"ls\" to \"find\" in the earlier hunk is\nexcusable in the name of consistency with this updated on, if the\nuse of \"sort\" matters in this hunk, but as \"ls\" gives a consistent\noutput unless you say \"ls (-U|--sort=none)\", I am not sure if we\nneed \"sort\" in this hunk, either.  So, I dunno.\n\n\"before vs after\" does look like an improvement over \"new vs newer\".\n\n> +test_expect_success 'repacking fails when missing .pack actually means missing objects' '\n> +\ttest_create_repo idx-without-pack &&\n> +\t(\n> +\t\tcd idx-without-pack &&\n> +\n> +\t\t# Avoid producing different packs due 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> +\t\tP4=$(commit_and_pack 4) &&\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n> +\t\ttest_line_count = 4 old-counts &&\n> +\n> +\t\t# Remove one .pack file\n> +\t\trm .git/objects/pack/$P2 &&\n> +\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f |\n> +\t\t\tsort >before-pack-dir &&\n> +\n> +\t\ttest_must_fail git fsck &&\n> +\t\ttest_must_fail git repack --cruft -d 2>err &&\n> +\t\tgrep \"bad object\" err &&\n> +\n> +\t\t# Before failing, the repack did not modify the\n> +\t\t# pack directory.\n> +\t\tfind .git/objects/pack -name \"*.pack\" -type f |\n> +\t\t\tsort >after-pack-dir &&\n> +\t\ttest_cmp before-pack-dir after-pack-dir\n> +\t)\n> +'\n\nDitto for the use of \"find | sort\" vs \"ls\", but otherwise it looks\nlike it is testing the right thing.\n\nThanks.\n"},{"id":"479368","messageId":"xmqqa5w34fhk.fsf@gitster.g","threadId":"59968","inReplyTo":"b3d0d9308ef1a0aab8dd5a65354044568c65388b.1689017830.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-10T21:35:19Z","receivedAt":"2023-07-10T21:35:24Z","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> However, manually enumerating a repository's packs via `readdir()` is\n> confusing and error-prone. It leads to frustrating inconsistencies\n> between which packs Git considers to be part of a repository (i.e.,\n> could be found in the list of packs from `get_all_packs()`), and which\n> packs `collect_pack_filenames()` considers to meet the same criteria.\n\nMakes sense.\n\n> One gotcha here is that we have to ignore non-local packs, since the\n> original version of `collect_pack_filenames()` only looks at the local\n> pack directory to collect existing packs.\n\nYup.\n\nDoes this \"fix\" anything, or just makes the resulting code clearer\nand protects it from future breakage?  I think it is the latter and\nnot having any test is justified.\n\nThanks.\n"},{"id":"479384","messageId":"ZK2RRdFYl6Vey7oV@nand.local","threadId":"59968","inReplyTo":"xmqqedlf4jg1.fsf@gitster.g","subject":"Re: [PATCH 1/2] builtin/repack.c: only repack `.pack`s that exist","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-11T17:28:37Z","receivedAt":"2023-07-11T17:28:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 10, 2023 at 01:09:50PM -0700, Junio C Hamano wrote:\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> >  \t\tP4=$(commit_and_pack 4) &&\n> > -\t\tls .git/objects/pack/*.pack >old-counts &&\n> > +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n> >  \t\ttest_line_count = 4 old-counts &&\n> >  \t\tgit repack -a -d --keep-pack $P1 --keep-pack $P4 &&\n> > -\t\tls .git/objects/pack/*.pack >new-counts &&\n> > +\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >new-counts &&\n> >  \t\tgrep -q $P1 new-counts &&\n> >  \t\tgrep -q $P4 new-counts &&\n> >  \t\ttest_line_count = 3 new-counts &&\n>\n> I do not think \"sort\" in both of these added lines is doing anything\n> useful.  Does the test break without this hunk and if so how?\n\nThis is an error on my part; I don't think I realized that a bare ls\ngives sorted output, so changing these to 'find ... | sort' was\nunnecessary. I'll send a version that removes that noise.\n\nThanks,\nTaylor\n"},{"id":"479385","messageId":"ZK2RcGet9dgybwwX@nand.local","threadId":"59968","inReplyTo":"xmqqa5w34fhk.fsf@gitster.g","subject":"Re: [PATCH 2/2] builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-11T17:29:20Z","receivedAt":"2023-07-11T17:29:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 10, 2023 at 02:35:19PM -0700, Junio C Hamano wrote:\n> Does this \"fix\" anything, or just makes the resulting code clearer\n> and protects it from future breakage?  I think it is the latter and\n> not having any test is justified.\n\nNope, this doesn't change any existing behavior, and there is nothing\nbroken with the original. So this is more about clarity and hardening\nthis function against future breakage.\n\nThanks,\nTaylor\n"},{"id":"479386","messageId":"cover.1689096750.git.me@ttaylorr.com","threadId":"59968","inReplyTo":"cover.1689017830.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] repack: make `collect_pack_filenames()` consistent","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-11T17:32:31Z","receivedAt":"2023-07-11T17:32:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small replacement for a series that Stolee and I worked on\ntogether to make the behavior of `collect_pack_filenames()` consistent\nwith the rest of Git's pack liveness rules.\n\nNothing much has changed since v1, with the exception of removing\nunnecessary conversions from 'ls' -> 'find ... | sort'. As always, a\nrange-diff is included below for convenience.\n\nThanks in advance for another look.\n\nDerrick Stolee (1):\n  builtin/repack.c: only repack `.pack`s that exist\n\nTaylor Blau (1):\n  builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`\n\n builtin/repack.c  | 38 +++++++++++++++-----------------------\n t/t7700-repack.sh | 36 +++++++++++++++++++++++++++++++++++-\n 2 files changed, 50 insertions(+), 24 deletions(-)\n\nRange-diff against v1:\n1:  f14a88f107 ! 1:  410d2f0165 builtin/repack.c: only repack `.pack`s that exist\n    @@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '\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    - \t\tP4=$(commit_and_pack 4) &&\n    --\t\tls .git/objects/pack/*.pack >old-counts &&\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n    - \t\ttest_line_count = 4 old-counts &&\n    - \t\tgit repack -a -d --keep-pack $P1 --keep-pack $P4 &&\n    --\t\tls .git/objects/pack/*.pack >new-counts &&\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >new-counts &&\n    - \t\tgrep -q $P1 new-counts &&\n    - \t\tgrep -q $P4 new-counts &&\n    - \t\ttest_line_count = 3 new-counts &&\n     @@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '\n      \t\t\tmv \"$from\" \"$to\" || return 1\n      \t\tdone &&\n    @@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '\n     +\t\t# repacking what we can.\n     +\t\ttouch .git/objects/pack/pack-does-not-exist.idx &&\n     +\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >packs.before &&\n      \t\tgit repack --cruft -d --keep-pack $P1 --keep-pack $P4 &&\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >packs.after &&\n      \n    --\t\tls .git/objects/pack/*.pack >newer-counts &&\n    --\t\ttest_cmp new-counts newer-counts &&\n    -+\t\ttest_cmp packs.before packs.after &&\n    - \t\tgit fsck\n    + \t\tls .git/objects/pack/*.pack >newer-counts &&\n    +@@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '\n      \t)\n      '\n      \n    @@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '\n     +\t\tP2=$(commit_and_pack 2) &&\n     +\t\tP3=$(commit_and_pack 3) &&\n     +\t\tP4=$(commit_and_pack 4) &&\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f | sort >old-counts &&\n    ++\t\tls .git/objects/pack/*.pack >old-counts &&\n     +\t\ttest_line_count = 4 old-counts &&\n     +\n     +\t\t# Remove one .pack file\n     +\t\trm .git/objects/pack/$P2 &&\n     +\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f |\n    -+\t\t\tsort >before-pack-dir &&\n    ++\t\tls .git/objects/pack/*.pack >before-pack-dir &&\n     +\n     +\t\ttest_must_fail git fsck &&\n     +\t\ttest_must_fail git repack --cruft -d 2>err &&\n    @@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '\n     +\n     +\t\t# Before failing, the repack did not modify the\n     +\t\t# pack directory.\n    -+\t\tfind .git/objects/pack -name \"*.pack\" -type f |\n    -+\t\t\tsort >after-pack-dir &&\n    ++\t\tls .git/objects/pack/*.pack >after-pack-dir &&\n     +\t\ttest_cmp before-pack-dir after-pack-dir\n     +\t)\n     +'\n2:  b3d0d9308e = 2:  ffdf85f6d3 builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`\n-- \n2.41.0.329.gffdf85f6d39\n"},{"id":"479387","messageId":"410d2f01655266419c583aba085edfb62800f174.1689096750.git.me@ttaylorr.com","threadId":"59968","inReplyTo":"cover.1689096750.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] builtin/repack.c: only repack `.pack`s that exist","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-11T17:32:34Z","receivedAt":"2023-07-11T17:32:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nIn 73320e49add (builtin/repack.c: only collect fully-formed packs,\n2023-06-07), we switched the check for which packs to collect by\nstarting at the .idx files and looking for matching .pack files. This\navoids trying to repack pack-files that have not had their pack-indexes\ninstalled yet.\n\nHowever, it does cause maintenance to halt if we find the (problematic,\nbut not insurmountable) case of a .idx file without a corresponding\n.pack file. In an environment where packfile maintenance is a critical\nfunction, such a hard stop is costly and requires human intervention to\nresolve (by deleting the .idx file).\n\nThis was not the case before. We successfully repacked through this\nscenario until the recent change to scan for .idx files.\n\nFurther, if we are actually in a case where objects are missing, we\ndetect this at a different point during the reachability walk.\n\nIn other cases, Git prepares its list of packfiles by scanning .idx\nfiles and then only adds it to the packfile list if the corresponding\n.pack file exists. It even does so without a warning! (See\nadd_packed_git() in packfile.c for details.)\n\nThis case is much less likely to occur than the failures seen before\n73320e49add. Packfiles are \"installed\" by writing the .pack file before\nthe .idx and that process can be interrupted. Packfiles _should_ be\ndeleted by deleting the .idx first, followed by the .pack file, but\nunlink_pack_path() does not do this: it deletes the .pack _first_,\nallowing a window where this process could be interrupted. We leave the\nconsideration of changing this order as a separate concern. Knowing that\nthis condition is possible from interrupted Git processes and not other\ntools lends some weight that Git should be more flexible around this\nscenario.\n\nAdd a check to see if the .pack file exists before adding it to the list\nfor repacking. This will stop a number of maintenance failures seen in\nproduction but fixed by deleting the .idx files.\n\nThis brings us closer to the case before 73320e49add in that 'git\nrepack' will not fail when there is an orphaned .idx file, at least, not\ndue to the way we scan for packfiles. In the case that the .pack file\nwas erroneously deleted without copies of its objects in other installed\npackfiles, then 'git repack' will fail due to the reachable object walk.\n\nThis does resolve the case where automated repacks will no longer be\nhalted on this case. The tests in t7700 show both these successful\nscenarios and the case of failing if the .pack was truly required.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c  |  3 +++\n t/t7700-repack.sh | 36 +++++++++++++++++++++++++++++++++++-\n 2 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 51698e3c68..724e09536e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -125,6 +125,9 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\tstrbuf_add(&buf, e->d_name, len);\n \t\tstrbuf_addstr(&buf, \".pack\");\n \n+\t\tif (!file_exists(mkpath(\"%s/%s\", packdir, buf.buf)))\n+\t\t\tcontinue;\n+\n \t\tfor (i = 0; i < extra_keep->nr; i++)\n \t\t\tif (!fspathcmp(buf.buf, extra_keep->items[i].string))\n \t\t\t\tbreak;\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex af79266c58..27b66807cd 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -213,7 +213,7 @@ 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\t# avoid producing different packs due 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@@ -239,6 +239,10 @@ test_expect_success 'repack --keep-pack' '\n \t\t\tmv \"$from\" \"$to\" || return 1\n \t\tdone &&\n \n+\t\t# A .idx file without a .pack should not stop us from\n+\t\t# repacking what we can.\n+\t\ttouch .git/objects/pack/pack-does-not-exist.idx &&\n+\n \t\tgit repack --cruft -d --keep-pack $P1 --keep-pack $P4 &&\n \n \t\tls .git/objects/pack/*.pack >newer-counts &&\n@@ -247,6 +251,36 @@ test_expect_success 'repack --keep-pack' '\n \t)\n '\n \n+test_expect_success 'repacking fails when missing .pack actually means missing objects' '\n+\ttest_create_repo idx-without-pack &&\n+\t(\n+\t\tcd idx-without-pack &&\n+\n+\t\t# Avoid producing different packs due 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+\t\tP4=$(commit_and_pack 4) &&\n+\t\tls .git/objects/pack/*.pack >old-counts &&\n+\t\ttest_line_count = 4 old-counts &&\n+\n+\t\t# Remove one .pack file\n+\t\trm .git/objects/pack/$P2 &&\n+\n+\t\tls .git/objects/pack/*.pack >before-pack-dir &&\n+\n+\t\ttest_must_fail git fsck &&\n+\t\ttest_must_fail git repack --cruft -d 2>err &&\n+\t\tgrep \"bad object\" err &&\n+\n+\t\t# Before failing, the repack did not modify the\n+\t\t# pack directory.\n+\t\tls .git/objects/pack/*.pack >after-pack-dir &&\n+\t\ttest_cmp before-pack-dir after-pack-dir\n+\t)\n+'\n+\n test_expect_success 'bitmaps are created by default in bare repos' '\n \tgit clone --bare .git bare.git &&\n \trm -f bare.git/objects/pack/*.bitmap &&\n-- \n2.41.0.329.gffdf85f6d39\n\n"},{"id":"479388","messageId":"ffdf85f6d39c3fa2e7035deeaa0837cc58237896.1689096750.git.me@ttaylorr.com","threadId":"59968","inReplyTo":"cover.1689096750.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-11T17:32:37Z","receivedAt":"2023-07-11T17:32:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When repacking, the function `collect_pack_filenames()` is responsible\nfor collecting the set of existing packs in the repository, and\npartitioning them into \"kept\" (if the pack has a \".keep\" file or was\ngiven via `--keep-pack`) and \"nonkept\" (otherwise) lists.\n\nThis function comes from the original C port of git-repack.sh from back\nin a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15),\nwhere it first appears as `get_non_kept_pack_filenames()`. At the time,\nthe implementation was a fairly direct translation from the relevant\nportion of git-repack.sh, which looped over the results of\n\n    find \"$PACKDIR\" -type f -name '*.pack'\n\neither ignoring the pack as kept, or adding it to the list of existing\npacks.\n\nSo the choice to directly translate this function in terms of\n`readdir()` in a1bbc6c0176 made sense. At the time, it was possible to\nrefine the C version in terms of packed_git structs, but was never done.\n\nHowever, manually enumerating a repository's packs via `readdir()` is\nconfusing and error-prone. It leads to frustrating inconsistencies\nbetween which packs Git considers to be part of a repository (i.e.,\ncould be found in the list of packs from `get_all_packs()`), and which\npacks `collect_pack_filenames()` considers to meet the same criteria.\n\nThis bit us in 73320e49ad (builtin/repack.c: only collect fully-formed\npacks, 2023-06-07), and again in the previous commit.\n\nPrevent these issues from biting us in the future by implementing the\n`collect_pack_filenames()` function by looping over an array of pointers\nto `packed_git` structs, ensuring that we use the same criteria to\ndetermine the set of available packs.\n\nOne gotcha here is that we have to ignore non-local packs, since the\noriginal version of `collect_pack_filenames()` only looks at the local\npack directory to collect existing packs.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 41 +++++++++++++++--------------------------\n 1 file changed, 15 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 724e09536e..ac9fc61d2e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -106,49 +106,38 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\t\t\t   struct string_list *fname_kept_list,\n \t\t\t\t   const struct string_list *extra_keep)\n {\n-\tDIR *dir;\n-\tstruct dirent *e;\n-\tchar *fname;\n+\tstruct packed_git *p;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (!(dir = opendir(packdir)))\n-\t\treturn;\n-\n-\twhile ((e = readdir(dir)) != NULL) {\n-\t\tsize_t len;\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tint i;\n+\t\tconst char *base;\n \n-\t\tif (!strip_suffix(e->d_name, \".idx\", &len))\n+\t\tif (!p->pack_local)\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\tif (!file_exists(mkpath(\"%s/%s\", packdir, buf.buf)))\n-\t\t\tcontinue;\n+\t\tbase = pack_basename(p);\n \n \t\tfor (i = 0; i < extra_keep->nr; i++)\n-\t\t\tif (!fspathcmp(buf.buf, extra_keep->items[i].string))\n+\t\t\tif (!fspathcmp(base, extra_keep->items[i].string))\n \t\t\t\tbreak;\n \n-\t\tfname = xmemdupz(e->d_name, len);\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, base);\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n-\t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) ||\n-\t\t    (file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))) {\n-\t\t\tstring_list_append_nodup(fname_kept_list, fname);\n-\t\t} else {\n+\t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n+\t\t\tstring_list_append(fname_kept_list, buf.buf);\n+\t\telse {\n \t\t\tstruct string_list_item *item;\n-\t\t\titem = string_list_append_nodup(fname_nonkept_list,\n-\t\t\t\t\t\t\tfname);\n-\t\t\tif (file_exists(mkpath(\"%s/%s.mtimes\", packdir, fname)))\n+\t\t\titem = string_list_append(fname_nonkept_list, buf.buf);\n+\t\t\tif (p->is_cruft)\n \t\t\t\titem->util = (void*)(uintptr_t)CRUFT_PACK;\n \t\t}\n \t}\n-\tclosedir(dir);\n-\tstrbuf_release(&buf);\n \n \tstring_list_sort(fname_kept_list);\n+\tstrbuf_release(&buf);\n }\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n-- \n2.41.0.329.gffdf85f6d39\n"}]}