{"thread":{"id":"59886","subject":"[PATCH] repack: only repack .packs that exist","startedAt":"2023-06-20T19:03:09Z","lastAt":"2023-07-10T19:40:03Z","messageCount":6,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478575","messageId":"pull.1546.git.1687287782439.gitgitgadget@gmail.com","threadId":"59886","inReplyTo":null,"subject":"[PATCH] repack: only repack .packs that exist","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-20T19:03:02Z","receivedAt":"2023-06-20T19:03:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?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>\n---\n    [RFC] repack: only repack .packs that exist\n    \n    This is based on tb/collect-pack-filenames-fix.\n    \n    This is an RFC since there is some internal disagreement as to whether\n    this is a safe scenario to \"ignore\" during a repack.\n    \n    I'm of the opinion that blocking on the case where there are no missing\n    objects is unnecessary noise, and matches our behavior before the change\n    to scan for '.idx' files.\n    \n    I'm submitting this to the list to gather more opinions about the safety\n    and/or necessity of this change before moving forward with a full\n    review.\n    \n    This is related to the deletion of .packs before .idx files that I\n    submitted as [1]\n    \n    [1]\n    https://lore.kernel.org/git/pull.1547.git.1687287675248.gitgitgadget@gmail.com\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1546%2Fderrickstolee%2Frepack-ignore-idx-no-pack-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1546/derrickstolee/repack-ignore-idx-no-pack-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1546\n\n builtin/repack.c  |  3 +++\n t/t7700-repack.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 1e21a21ea82..b1695c9b2eb 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -123,6 +123,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 08b5ba0c150..e780efe5e0c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\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 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+\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 >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 >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\nbase-commit: 73320e49add4b56aba9bf5236a216498fa8ccc22\n-- \ngitgitgadget\n"},{"id":"478617","messageId":"ZJK8VsH27a4bPYgh@nand.local","threadId":"59886","inReplyTo":"pull.1546.git.1687287782439.gitgitgadget@gmail.com","subject":"Re: [PATCH] repack: only repack .packs that exist","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-21T09:01:10Z","receivedAt":"2023-06-21T09:02:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 20, 2023 at 07:03:02PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> In 73320e49add (builtin/repack.c: only collect fully-formed packs,\n> 2023-06-07), we switched the check for which packs to collect by\n> starting at the .idx files and looking for matching .pack files. This\n> avoids trying to repack pack-files that have not had their pack-indexes\n> installed yet.\n\nRight; the motivation behind this change is based on the principle that\nwe consider a packfile (and any corresponding metadata it might have,\nlike .keep, .bitmap, .rev, etc.) to be accessible exactly when there is\nan .idx file in place.\n\nSo if pack-P.idx exists, we expect to be able to read the contents of\npack-P.pack (and any other pack-P.* files that happen to be laying\naround). The idea with 73320e49add is to make `collect_pack_filenames()`\nconsistent with the same pack existence check that we use everywhere\nelse.\n\n> However, it does cause maintenance to halt if we find the (problematic,\n> but not insurmountable) case of a .idx file without a corresponding\n> .pack file. In an environment where packfile maintenance is a critical\n> function, such a hard stop is costly and requires human intervention to\n> resolve (by deleting the .idx file).\n\nDo all cases fall into the \"problematic, but not insurmountable\"\ncategory? I'm not entirely sure. One instance that is definitely OK is\nhaving a partially unstaged pack whose objects were repacked elsewhere,\nbut the 'repack' process doing so died between unlink()-ing the \".pack\"\nfile and the \".idx\". This matches the patch that you and I discussed\nyesterday to switch the order and restore git-repack.sh's original\nbehavior, which is correct.\n\nBut what about someone accidentally deleting a \".pack\" file and leaving\naround its index? Or copying files from one repository to another and\nonly partially copying the contents of $GIT_DIR/objects/pack?\n\nWhat makes me leery about this kind of change is that there are\npotentially many more cases like the above. And even if the interesting\ncases are somehow limited to the above, continuing on in the face of a\nmalformed pack group doesn't seem like the right thing for repack to be\ndoing.\n\n> This was not the case before. We successfully repacked through this\n> scenario until the recent change to scan for .idx files.\n\nRight, my feeling is that the new behavior post-73320e49add is arguably\nmore correct.\n\n> In other cases, Git prepares its list of packfiles by scanning .idx\n> files and then only adds it to the packfile list if the corresponding\n> .pack file exists. It even does so without a warning! (See\n> add_packed_git() in packfile.c for details.)\n\nThis is by design: we only try to open the pack itself when we call\n`open_packed_git()`. All packs start out initialized with just their\n\".idx\" as you note.\n\nThis feels similar to what you are trying to do here, but I do think\nthat it's different. While we're performing object reads, there may be\nconcurrent writers adding (e.g. from pushes) or removing packs (e.g.\nfrom repacking) in the repository. So there we have to be resilient\nto thinking that we might have a copy of some object in a given pack,\nbut still being able to handle the case where that pack disappears and\nthe object is actually found in some other pack.\n\nRepacking feels different to me. There we are modifying the packs\nthemselves, so having a incorrectly staged pack group feels like a\ncondition that we should stop on, because we can no longer completely\ntrust the data that we are operating on.\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 08b5ba0c150..e780efe5e0c 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\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\nIf we do end up pursuing this patch, it may be worthwhile to ensure that\nthe dangling .idx file remains in place after the repack is complete.\nYou test for this implicitly below, but doing so here explicitly feels\nworthwhile to me.\n\nThanks,\nTaylor\n"},{"id":"478869","messageId":"20230627075427.GE1226768@coredump.intra.peff.net","threadId":"59886","inReplyTo":"pull.1546.git.1687287782439.gitgitgadget@gmail.com","subject":"Re: [PATCH] repack: only repack .packs that exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T07:54:27Z","receivedAt":"2023-06-27T07:55:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 20, 2023 at 07:03:02PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n> \n> In 73320e49add (builtin/repack.c: only collect fully-formed packs,\n> 2023-06-07), we switched the check for which packs to collect by\n> starting at the .idx files and looking for matching .pack files. This\n> avoids trying to repack pack-files that have not had their pack-indexes\n> installed yet.\n> \n> However, it does cause maintenance to halt if we find the (problematic,\n> but not insurmountable) case of a .idx file without a corresponding\n> .pack file. In an environment where packfile maintenance is a critical\n> function, such a hard stop is costly and requires human intervention to\n> resolve (by deleting the .idx file).\n> \n> This was not the case before. We successfully repacked through this\n> scenario until the recent change to scan for .idx files.\n> \n> Further, if we are actually in a case where objects are missing, we\n> detect this at a different point during the reachability walk.\n\nI'd worry less about missing objects here, and more that we could be in\na race in which git-repack sees some partial state and decides to take\nsome action that causes harm. For example, in a \"git repack -ad\", could\nwe later mistakenly delete a .idx file because we thought it had no\nmatching pack? The repack itself would be happy, but it might be causing\nheadaches for later processes.\n\nI _think_ the answer is \"no\", because any file we skip via\ncollect_pack_filenames() won't make it into the list of either the\nexisting nonkept or kept packs, and we use \"nonkept\" list to do the\ndeletion.\n\nSo it seems like missing a pack here (whether racily or not) could at\nmost result in cruft being left in place, or a bad --geometric decision\nbeing made, etc. Which makes the stakes reasonably low, I think.\n\n> In other cases, Git prepares its list of packfiles by scanning .idx\n> files and then only adds it to the packfile list if the corresponding\n> .pack file exists. It even does so without a warning! (See\n> add_packed_git() in packfile.c for details.)\n\nInteresting. I'd have expected a warning. ;)\n\nI also kind of wonder if this repack code should simply be loading and\niterating the packed_git list, but that is a much bigger change.\n\n> This case is much less likely to occur than the failures seen before\n> 73320e49add. Packfiles are \"installed\" by writing the .pack file before\n> the .idx and that process can be interrupted. Packfiles _should_ be\n> deleted by deleting the .idx first, followed by the .pack file, but\n> unlink_pack_path() does not do this: it deletes the .pack _first_,\n> allowing a window where this process could be interrupted. We leave the\n> consideration of changing this order as a separate concern. Knowing that\n> this condition is possible from interrupted Git processes and not other\n> tools lends some weight that Git should be more flexible around this\n> scenario.\n\nHmm. So it really sounds to me like unlink_pack_path() is doing the\nwrong thing. And once fixed, we would (assuming no other similar spots)\ncease to see any races with this code. At which point would we still\nwant the looseness provided by this patch?\n\nI.e., is the concern concurrent races, or is it leftover cruft (possibly\nfrom a killed process, crash, etc)?\n\nIf the latter, then we might still want to quietly skip them. But it\nmakes me wonder how such .idx files should get cleaned up in the longer\nterm.\n\nI'm kind of guessing that what you've seen is actually leftover cruft,\nif only because I wouldn't expect concurrent unlinking to be very common\n(it implies two repacks running at the same time, which is generally a\nbad idea). Whereas new packs being written would be common (via\nindex-pack, etc from pushes).\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\nOK. The fact that this brings things in line with add_packed_git() is\ncompelling to me, and I think the stakes are pretty low for this causing\nus to do something confusing.\n\nIt does make me feel like we should be doing something similar to\ngit-prune's prune_cruft(): any non-.pack file in objects/pack with an\nold mtime and no matching .pack file should be a candidate for deletion.\nI don't _think_ we have anything like that now, but it would clean these\nin the longer term. I'm OK with that being a separate topic, though.\n\n-Peff\n"},{"id":"479007","messageId":"ZJ1N2I6sDfxhrJo8@nand.local","threadId":"59886","inReplyTo":"20230627075427.GE1226768@coredump.intra.peff.net","subject":"Re: [PATCH] repack: only repack .packs that exist","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-29T09:24:40Z","receivedAt":"2023-06-29T09:25:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 27, 2023 at 03:54:27AM -0400, Jeff King wrote:\n> > In other cases, Git prepares its list of packfiles by scanning .idx\n> > files and then only adds it to the packfile list if the corresponding\n> > .pack file exists. It even does so without a warning! (See\n> > add_packed_git() in packfile.c for details.)\n>\n> Interesting. I'd have expected a warning. ;)\n\nReading this all over, I was incorrect in my original suggestion that\nnot checking for the existence of the matching \".pack\" file was the\nexpected thing to do.\n\nWe don't try to open the \".pack\" file itself in add_packed_git(), that\nhappens later in open_packed_git() if we end up actually needing to read\nobjects from the pack, or want to retain a handle on it for later if\nwe're worried that the \".pack\" file itself might get deleted.\n\nBut that add_packed_git() silently ignores a pack which has its \".idx\"\nfile and not its \".pack\" file is behavior that I wasn't aware of, and\nhad somehow missed when I reread the function last week.\n\nStolee: I apologize for having missed this important detail. I think in\nthat sense matching the behavior of add_packed_git() here is the right\nthing to do, and that this patch makes sense to me.\n\n> I also kind of wonder if this repack code should simply be loading and\n> iterating the packed_git list, but that is a much bigger change.\n\nI have wanted to do this for a while ;-). The minimal patch is less\ninvasive than I had thought:\n\n--- 8< ---\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 1e21a21ea8..e854c832fd 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -104,46 +104,33 @@ 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-\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+\t\tconst char *base = basename(p->pack_name);\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--- >8 ---\n\nI think you could probably go further than this, since having to store\nthe suffix-less pack names in the fname_kept and fname_nonkept lists is\nkind of weird.\n\nIt would be nice if we could store pointers to the actual packed_git\nstructs themselves in place of those lists instead, but I'm not\nimmediately sure how feasible it would be to do since we re-prepare the\nobject store between enumerating and then removing these packs.\n\nThanks,\nTaylor\n"},{"id":"479092","messageId":"20230702131117.GB1036686@coredump.intra.peff.net","threadId":"59886","inReplyTo":"ZJ1N2I6sDfxhrJo8@nand.local","subject":"Re: [PATCH] repack: only repack .packs that exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T13:11:17Z","receivedAt":"2023-07-02T13:13:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 29, 2023 at 05:24:40AM -0400, Taylor Blau wrote:\n\n> > I also kind of wonder if this repack code should simply be loading and\n> > iterating the packed_git list, but that is a much bigger change.\n> \n> I have wanted to do this for a while ;-). The minimal patch is less\n> invasive than I had thought:\n\nYeah, I agree it's not too bad. If we want to go that route, though, I\nthink we should do it on top of Stolee's patch, though (which makes it a\npure cleanup once the behaviors are aligned).\n\nI'm also not sure if you'd need to do anything tricky with alternate\nobject dirs (it looks like the existing code ignores them entirely, so I\nguess we'd want to skip entries without pack_local set).\n\n> [...]\n> I think you could probably go further than this, since having to store\n> the suffix-less pack names in the fname_kept and fname_nonkept lists is\n> kind of weird.\n> \n> It would be nice if we could store pointers to the actual packed_git\n> structs themselves in place of those lists instead, but I'm not\n> immediately sure how feasible it would be to do since we re-prepare the\n> object store between enumerating and then removing these packs.\n\nI think that would work, because we do not ever drop packed_git entries\nfrom the list (even if the files were deleted between prepare/reprepare).\nBut it also creates a subtle memory ownership dependency/assumption\nbetween the two bits of code. It seems clearer to me to just copy the\nnames out to our own lists here (i.e., the patch you showed).\n\n-Peff\n"},{"id":"479348","messageId":"ZKxejN7JQDWpNTAz@nand.local","threadId":"59886","inReplyTo":"20230702131117.GB1036686@coredump.intra.peff.net","subject":"Re: [PATCH] repack: only repack .packs that exist","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-10T19:39:56Z","receivedAt":"2023-07-10T19:40:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jul 02, 2023 at 09:11:17AM -0400, Jeff King wrote:\n> On Thu, Jun 29, 2023 at 05:24:40AM -0400, Taylor Blau wrote:\n>\n> > > I also kind of wonder if this repack code should simply be loading and\n> > > iterating the packed_git list, but that is a much bigger change.\n> >\n> > I have wanted to do this for a while ;-). The minimal patch is less\n> > invasive than I had thought:\n>\n> Yeah, I agree it's not too bad. If we want to go that route, though, I\n> think we should do it on top of Stolee's patch, though (which makes it a\n> pure cleanup once the behaviors are aligned).\n>\n> I'm also not sure if you'd need to do anything tricky with alternate\n> object dirs (it looks like the existing code ignores them entirely, so I\n> guess we'd want to skip entries without pack_local set).\n\nYep, we definitely want a `if (!p->is_local) continue` in there to be\nconsistent with the existing behavior.\n\n> > [...]\n> > I think you could probably go further than this, since having to store\n> > the suffix-less pack names in the fname_kept and fname_nonkept lists is\n> > kind of weird.\n> >\n> > It would be nice if we could store pointers to the actual packed_git\n> > structs themselves in place of those lists instead, but I'm not\n> > immediately sure how feasible it would be to do since we re-prepare the\n> > object store between enumerating and then removing these packs.\n>\n> I think that would work, because we do not ever drop packed_git entries\n> from the list (even if the files were deleted between prepare/reprepare).\n> But it also creates a subtle memory ownership dependency/assumption\n> between the two bits of code. It seems clearer to me to just copy the\n> names out to our own lists here (i.e., the patch you showed).\n\nYeah, I agree. I thought that it might clean things up further, and to\nan extent it does:\n\n    https://github.com/ttaylorr/git/compare/tb/collect-packs-readdir\n\n, but I think that the memory ownership issue is sufficiently subtle\nthat the clean-up isn't really worth it.\n\nI put the above patch together with Stolee's, and sent the result back\nto the list here:\n\n    https://lore.kernel.org/git/cover.1689017830.git.me@ttaylorr.com/T/#t\n\nThanks,\nTaylor\n"}]}