{"thread":{"id":"60300","subject":"[PATCH] builtin/repack.c: avoid making cruft packs preferred","startedAt":"2023-10-03T16:27:57Z","lastAt":"2023-10-05T20:28:03Z","messageCount":10,"participants":["Taylor Blau","Jeff King","Junio C Hamano","Patrick Steinhardt","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482570","messageId":"19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com","threadId":"60300","inReplyTo":null,"subject":"[PATCH] builtin/repack.c: avoid making cruft packs preferred","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T16:27:51Z","receivedAt":"2023-10-03T16:27:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When doing a `--geometric` repack, we make sure that the preferred pack\n(if writing a MIDX) is the largest pack that we *didn't* repack. That\nhas the effect of keeping the preferred pack in sync with the pack\ncontaining a majority of the repository's reachable objects.\n\nBut if the repository happens to double in size, we'll repack\neverything. Here we don't specify any `--preferred-pack`, and instead\nlet the MIDX code choose.\n\nIn the past, that worked fine, since there would only be one pack to\nchoose from: the one we just wrote. But it's no longer necessarily the\ncase that there is one pack to choose from. It's possible that the\nrepository also has a cruft pack, too.\n\nIf the cruft pack happens to come earlier in lexical order (and has an\nearlier mtime than any non-cruft pack), we'll pick that pack as\npreferred. This makes it impossible to reuse chunks of the reachable\npack verbatim from pack-objects, so is sub-optimal.\n\nLuckily, this is a somewhat rare circumstance to be in, since we would\nhave to repack the entire repository during a `--geometric` repack, and\nthe cruft pack would have to sort ahead of the pack we just created.\n\nNote that this behavior is usually just a performance regression. But\nit's possible it could be a correctness issue.\n\nSuppose an object was duplicated among the cruft and non-cruft pack. The\nMIDX will pick the one from the pack with the lowest mtime, which will\nalways be the cruft one. But if the non-cruft pack happens to sort\nearlier in lexical order, we'll treat that one as preferred, but not all\nduplicates will be resolved in favor of that pack.\n\nSo if we happened to have an object which appears in both packs\n(e.g., due to a cruft object being freshened, causing it to appear\nloose, and then repacking it via the `--geometric` repack) it's possible\nthe duplicate would be picked from the non-preferred pack.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nI've had this sitting in my patch queue for a while now. It's a\nnon-critical performance fix that avoids the repack/MIDX machinery from\never choosing a cruft pack as preferred when writing a MIDX bitmap\nwithout a given --preferred-pack.\n\nThere is no correctness issue here, but choosing a pack with few/no\nreachable objects means that our pack reuse mechanism will rarely kick\nin, resulting in performance degradation.\n\n builtin/repack.c        | 47 ++++++++++++++++++++++++++++++++++++++++-\n t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++\n 2 files changed, 85 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 04770b15fe..a1a893d952 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -355,6 +355,18 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n \treturn data;\n }\n\n+static int has_pack_ext(const struct generated_pack_data *data,\n+\t\t\tconst char *ext)\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n+\t\tif (strcmp(exts[i].name, ext))\n+\t\t\tcontinue;\n+\t\treturn !!data->tempfiles[i];\n+\t}\n+\tBUG(\"unknown pack extension: '%s'\", ext);\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@@ -772,6 +784,7 @@ static void midx_included_packs(struct string_list *include,\n\n static int write_midx_included_packs(struct string_list *include,\n \t\t\t\t     struct pack_geometry *geometry,\n+\t\t\t\t     struct string_list *names,\n \t\t\t\t     const char *refs_snapshot,\n \t\t\t\t     int show_progress, int write_bitmaps)\n {\n@@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,\n \tif (preferred)\n \t\tstrvec_pushf(&cmd.args, \"--preferred-pack=%s\",\n \t\t\t     pack_basename(preferred));\n+\telse if (names->nr) {\n+\t\t/* The largest pack was repacked, meaning that either\n+\t\t * one or two packs exist depending on whether the\n+\t\t * repository has a cruft pack or not.\n+\t\t *\n+\t\t * Select the non-cruft one as preferred to encourage\n+\t\t * pack-reuse among packs containing reachable objects\n+\t\t * over unreachable ones.\n+\t\t *\n+\t\t * (Note we could write multiple packs here if\n+\t\t * `--max-pack-size` was given, but any one of them\n+\t\t * will suffice, so pick the first one.)\n+\t\t */\n+\t\tfor_each_string_list_item(item, names) {\n+\t\t\tstruct generated_pack_data *data = item->util;\n+\t\t\tif (has_pack_ext(data, \".mtimes\"))\n+\t\t\t\tcontinue;\n+\n+\t\t\tstrvec_pushf(&cmd.args, \"--preferred-pack=pack-%s.pack\",\n+\t\t\t\t     item->string);\n+\t\t\tbreak;\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * No packs were kept, and no packs were written. The\n+\t\t * only thing remaining are .keep packs (unless\n+\t\t * --pack-kept-objects was given).\n+\t\t *\n+\t\t * Set the `--preferred-pack` arbitrarily here.\n+\t\t */\n+\t\t;\n+\t}\n\n \tif (refs_snapshot)\n \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\n@@ -1360,7 +1405,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n \t\tmidx_included_packs(&include, &existing, &names, &geometry);\n\n-\t\tret = write_midx_included_packs(&include, &geometry,\n+\t\tret = write_midx_included_packs(&include, &geometry, &names,\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex dc86ca8269..be3735dff0 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -372,4 +372,43 @@ test_expect_success '--max-cruft-size ignores non-local packs' '\n \t)\n '\n\n+test_expect_success 'reachable packs are preferred over cruft ones' '\n+\trepo=\"cruft-preferred-packs\" &&\n+\tgit init \"$repo\" &&\n+\t(\n+\t\tcd \"$repo\" &&\n+\n+\t\t# This test needs to exercise careful control over when a MIDX\n+\t\t# is and is not written. Unset the corresponding TEST variable\n+\t\t# accordingly.\n+\t\tsane_unset GIT_TEST_MULTI_PACK_INDEX &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit --no-tag cruft &&\n+\n+\t\tnon_cruft=\"$(echo base | git pack-objects --revs $packdir/pack)\" &&\n+\t\t# Write a cruft pack which both (a) sorts ahead of the non-cruft\n+\t\t# pack in lexical order, and (b) has an older mtime to appease\n+\t\t# the MIDX preferred pack selection routine.\n+\t\tcruft=\"$(echo pack-$non_cruft.pack | git pack-objects --cruft $packdir/pack-A)\" &&\n+\t\ttest-tool chmtime -1000 $packdir/pack-A-$cruft.pack &&\n+\n+\t\ttest_commit other &&\n+\t\tgit repack -d &&\n+\n+\t\tgit repack --geometric 2 -d --write-midx --write-bitmap-index &&\n+\n+\t\t# After repacking, there are two packs left: one reachable one\n+\t\t# (which is the result of combining both of the existing two\n+\t\t# non-cruft packs), and one cruft pack.\n+\t\tfind .git/objects/pack -type f -name \"*.pack\" >packs &&\n+\t\ttest_line_count = 2 packs &&\n+\n+\t\t# Make sure that the pack we just wrote is marked as preferred,\n+\t\t# not the cruft one.\n+\t\tpack=\"$(test-tool read-midx --preferred-pack $objdir)\" &&\n+\t\ttest_path_is_missing \"$packdir/$(basename \"$pack\" \".idx\").mtimes\"\n+\t)\n+'\n+\n test_done\n--\n2.42.0.311.ga7f7a0e966.dirty\n"},{"id":"482571","messageId":"ZRxBlrjyuBmJnx3p@nand.local","threadId":"60300","inReplyTo":"19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T16:30:14Z","receivedAt":"2023-10-03T16:30:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:\n> I've had this sitting in my patch queue for a while now. It's a\n> non-critical performance fix that avoids the repack/MIDX machinery from\n> ever choosing a cruft pack as preferred when writing a MIDX bitmap\n> without a given --preferred-pack.\n>\n> There is no correctness issue here, but choosing a pack with few/no\n> reachable objects means that our pack reuse mechanism will rarely kick\n> in, resulting in performance degradation.\n>\n>  builtin/repack.c        | 47 ++++++++++++++++++++++++++++++++++++++++-\n>  t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++\n>  2 files changed, 85 insertions(+), 1 deletion(-)\n\nOops, I should have mentioned that this is meant to be applied on top of\n'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry\nabout that.\n\nThanks,\nTaylor\n"},{"id":"482600","messageId":"20231003201617.GE1562@coredump.intra.peff.net","threadId":"60300","inReplyTo":"19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:16:17Z","receivedAt":"2023-10-03T20:16:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:\n\n> Note that this behavior is usually just a performance regression. But\n> it's possible it could be a correctness issue.\n> \n> Suppose an object was duplicated among the cruft and non-cruft pack. The\n> MIDX will pick the one from the pack with the lowest mtime, which will\n> always be the cruft one. But if the non-cruft pack happens to sort\n> earlier in lexical order, we'll treat that one as preferred, but not all\n> duplicates will be resolved in favor of that pack.\n> \n> So if we happened to have an object which appears in both packs\n> (e.g., due to a cruft object being freshened, causing it to appear\n> loose, and then repacking it via the `--geometric` repack) it's possible\n> the duplicate would be picked from the non-preferred pack.\n\nI'm not sure I understand how that is a correctness issue. The contents\nof the object are the same in either pack. Or do you mean that the\npack-reuse code in pack-objects.c may get confused and try to use the\nwrong pack/offset when sending the object out? I would think it would\nalways be coming from the preferred pack for that code (so the outcome\nis just that we fail to do the pack-reuse optimization very well, but we\ndon't generate a wrong answer).\n\nOther than that, the explanation and patch make perfect sense to me, and\nthe patch looks good.\n\n-Peff\n"},{"id":"482601","messageId":"xmqqa5szzcdz.fsf@gitster.g","threadId":"60300","inReplyTo":"ZRxBlrjyuBmJnx3p@nand.local","subject":"Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-03T20:20:24Z","receivedAt":"2023-10-03T20:20: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> On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:\n>> I've had this sitting in my patch queue for a while now. It's a\n>> non-critical performance fix that avoids the repack/MIDX machinery from\n>> ever choosing a cruft pack as preferred when writing a MIDX bitmap\n>> without a given --preferred-pack.\n>>\n>> There is no correctness issue here, but choosing a pack with few/no\n>> reachable objects means that our pack reuse mechanism will rarely kick\n>> in, resulting in performance degradation.\n>>\n>>  builtin/repack.c        | 47 ++++++++++++++++++++++++++++++++++++++++-\n>>  t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++\n>>  2 files changed, 85 insertions(+), 1 deletion(-)\n>\n> Oops, I should have mentioned that this is meant to be applied on top of\n> 'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry\n> about that.\n\nSorry, but I do not follow.  tb/multi-cruft-pack was merged to\n'master' at c0b5d46d (Documentation/gitformat-pack.txt: drop mixed\nversion section, 2023-08-28) but back then t7704 did not exist.  Do\nyou mean the other topic in-flight from you about max-cruft-size?\n\n"},{"id":"482623","messageId":"ZRyKLqpDs8K8lr4Q@nand.local","threadId":"60300","inReplyTo":"xmqqa5szzcdz.fsf@gitster.g","subject":"Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T21:39:58Z","receivedAt":"2023-10-03T21:40:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 01:20:24PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:\n> >> I've had this sitting in my patch queue for a while now. It's a\n> >> non-critical performance fix that avoids the repack/MIDX machinery from\n> >> ever choosing a cruft pack as preferred when writing a MIDX bitmap\n> >> without a given --preferred-pack.\n> >>\n> >> There is no correctness issue here, but choosing a pack with few/no\n> >> reachable objects means that our pack reuse mechanism will rarely kick\n> >> in, resulting in performance degradation.\n> >>\n> >>  builtin/repack.c        | 47 ++++++++++++++++++++++++++++++++++++++++-\n> >>  t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++\n> >>  2 files changed, 85 insertions(+), 1 deletion(-)\n> >\n> > Oops, I should have mentioned that this is meant to be applied on top of\n> > 'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry\n> > about that.\n>\n> Sorry, but I do not follow.  tb/multi-cruft-pack was merged to\n> 'master' at c0b5d46d (Documentation/gitformat-pack.txt: drop mixed\n> version section, 2023-08-28) but back then t7704 did not exist.  Do\n> you mean the other topic in-flight from you about max-cruft-size?\n\nMy mistake -- this should be applied on top of the patches at this\nseries:\n\n    https://lore.kernel.org/git/cover.1696293862.git.me@ttaylorr.com/\n\nwhich is the other topic that you're referring to. I ran \"git\nfor-each-ref 'refs/remotes/origin/tb/*' | grep cruft\" and mistakenly\ngrabbed 'tb/multi-cruft-pack' thinking that that was that series and not\nthe one already merged to 'master'.\n\nBut it looks like the series linked above hasn't yet grown a topic\nbranch in gitster/git yet, hence my mistake. Sorry about that!\n\nThanks,\nTaylor\n"},{"id":"482624","messageId":"ZRyNHRf+tQwV+T6P@nand.local","threadId":"60300","inReplyTo":"20231003201617.GE1562@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T21:52:29Z","receivedAt":"2023-10-03T21:52:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:16:17PM -0400, Jeff King wrote:\n> On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:\n>\n> > Note that this behavior is usually just a performance regression. But\n> > it's possible it could be a correctness issue.\n> >\n> > Suppose an object was duplicated among the cruft and non-cruft pack. The\n> > MIDX will pick the one from the pack with the lowest mtime, which will\n> > always be the cruft one. But if the non-cruft pack happens to sort\n> > earlier in lexical order, we'll treat that one as preferred, but not all\n> > duplicates will be resolved in favor of that pack.\n> >\n> > So if we happened to have an object which appears in both packs\n> > (e.g., due to a cruft object being freshened, causing it to appear\n> > loose, and then repacking it via the `--geometric` repack) it's possible\n> > the duplicate would be picked from the non-preferred pack.\n>\n> I'm not sure I understand how that is a correctness issue. The contents\n> of the object are the same in either pack. Or do you mean that the\n> pack-reuse code in pack-objects.c may get confused and try to use the\n> wrong pack/offset when sending the object out? I would think it would\n> always be coming from the preferred pack for that code (so the outcome\n> is just that we fail to do the pack-reuse optimization very well, but we\n> don't generate a wrong answer).\n\nAdmittedly I'm not 100% sure of my interpretation here either, since I\nwrote most of this patch's log message nearly two years ago ;-).\n\nI think the issue was as follows:\n\n  - you have an object duplicated among some cruft and non-cruft pack\n  - the cruft pack happens to have an older mtime than the non-cruft one\n  - the repack reused no non-cruft packs, which (before this patch)\n    would let the MIDX avoid picking a preferred pack\n  - if the non-cruft pack happens to sort ahead of the cruft one in\n    lexical order, it'll appear in the first few bits of the bitmap's\n    ordering, causing us to treat it as if it were the preferred pack\n  - ...but the MIDX will resolve duplicate objects in favor of the\n    oldest pack when neither are preferred, causing us to pick a\n    duplicate from the non-\"preferred\" pack.\n\n(The last point is what causes the pack-bitmap reading code to get\nconfused, since it expects and makes assumption based on resolving ties\nin favor of the preferred pack.)\n\nThat feels right to me, but admittedly my memory is pretty hazy on those\nbitmap bugs. However, I don't think that this is an issue anymore, since\nthis patch was written before we had 177c0d6e63 (midx: infer preferred\npack when not given one, 2021-08-31) in GitHub's private fork, and the\npatch message here incorrectly refers to statements about GitHub's fork,\nnot git.git.\n\nBut since we have 177c0d6e63 in git.git, this paragraph can and should\nbe dropped as finding ourselves in that situation is impossible, since\nwe would infer a preferred pack when not given one, and resolve\nduplicates in favor of it.\n\nThat makes this purely a performance issue.\n\n> Other than that, the explanation and patch make perfect sense to me, and\n> the patch looks good.\n\nThanks!\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"482625","messageId":"035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com","threadId":"60300","inReplyTo":"19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/repack.c: avoid making cruft packs preferred","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T21:54:19Z","receivedAt":"2023-10-03T21:54:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When doing a `--geometric` repack, we make sure that the preferred pack\n(if writing a MIDX) is the largest pack that we *didn't* repack. That\nhas the effect of keeping the preferred pack in sync with the pack\ncontaining a majority of the repository's reachable objects.\n\nBut if the repository happens to double in size, we'll repack\neverything. Here we don't specify any `--preferred-pack`, and instead\nlet the MIDX code choose.\n\nIn the past, that worked fine, since there would only be one pack to\nchoose from: the one we just wrote. But it's no longer necessarily the\ncase that there is one pack to choose from. It's possible that the\nrepository also has a cruft pack, too.\n\nIf the cruft pack happens to come earlier in lexical order (and has an\nearlier mtime than any non-cruft pack), we'll pick that pack as\npreferred. This makes it impossible to reuse chunks of the reachable\npack verbatim from pack-objects, so is sub-optimal.\n\nLuckily, this is a somewhat rare circumstance to be in, since we would\nhave to repack the entire repository during a `--geometric` repack, and\nthe cruft pack would have to sort ahead of the pack we just created.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nA small re-roll of the original patch, removing a few paragraphs from\nthe proposed log message that are irrelevant and impossible to produce\nin today's git.git.\n\nFor more details, see <ZRyNHRf+tQwV+T6P@nand.local>.\n\nRange-diff against v1:\n1:  19d9aae08e ! 1:  0353939351 builtin/repack.c: avoid making cruft packs preferred\n    @@ Commit message\n         have to repack the entire repository during a `--geometric` repack, and\n         the cruft pack would have to sort ahead of the pack we just created.\n\n    -    Note that this behavior is usually just a performance regression. But\n    -    it's possible it could be a correctness issue.\n    -\n    -    Suppose an object was duplicated among the cruft and non-cruft pack. The\n    -    MIDX will pick the one from the pack with the lowest mtime, which will\n    -    always be the cruft one. But if the non-cruft pack happens to sort\n    -    earlier in lexical order, we'll treat that one as preferred, but not all\n    -    duplicates will be resolved in favor of that pack.\n    -\n    -    So if we happened to have an object which appears in both packs\n    -    (e.g., due to a cruft object being freshened, causing it to appear\n    -    loose, and then repacking it via the `--geometric` repack) it's possible\n    -    the duplicate would be picked from the non-preferred pack.\n    -\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n\n      ## builtin/repack.c ##\n\n builtin/repack.c        | 47 ++++++++++++++++++++++++++++++++++++++++-\n t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++\n 2 files changed, 85 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 04770b15fe..a1a893d952 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -355,6 +355,18 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n \treturn data;\n }\n\n+static int has_pack_ext(const struct generated_pack_data *data,\n+\t\t\tconst char *ext)\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n+\t\tif (strcmp(exts[i].name, ext))\n+\t\t\tcontinue;\n+\t\treturn !!data->tempfiles[i];\n+\t}\n+\tBUG(\"unknown pack extension: '%s'\", ext);\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@@ -772,6 +784,7 @@ static void midx_included_packs(struct string_list *include,\n\n static int write_midx_included_packs(struct string_list *include,\n \t\t\t\t     struct pack_geometry *geometry,\n+\t\t\t\t     struct string_list *names,\n \t\t\t\t     const char *refs_snapshot,\n \t\t\t\t     int show_progress, int write_bitmaps)\n {\n@@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,\n \tif (preferred)\n \t\tstrvec_pushf(&cmd.args, \"--preferred-pack=%s\",\n \t\t\t     pack_basename(preferred));\n+\telse if (names->nr) {\n+\t\t/* The largest pack was repacked, meaning that either\n+\t\t * one or two packs exist depending on whether the\n+\t\t * repository has a cruft pack or not.\n+\t\t *\n+\t\t * Select the non-cruft one as preferred to encourage\n+\t\t * pack-reuse among packs containing reachable objects\n+\t\t * over unreachable ones.\n+\t\t *\n+\t\t * (Note we could write multiple packs here if\n+\t\t * `--max-pack-size` was given, but any one of them\n+\t\t * will suffice, so pick the first one.)\n+\t\t */\n+\t\tfor_each_string_list_item(item, names) {\n+\t\t\tstruct generated_pack_data *data = item->util;\n+\t\t\tif (has_pack_ext(data, \".mtimes\"))\n+\t\t\t\tcontinue;\n+\n+\t\t\tstrvec_pushf(&cmd.args, \"--preferred-pack=pack-%s.pack\",\n+\t\t\t\t     item->string);\n+\t\t\tbreak;\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * No packs were kept, and no packs were written. The\n+\t\t * only thing remaining are .keep packs (unless\n+\t\t * --pack-kept-objects was given).\n+\t\t *\n+\t\t * Set the `--preferred-pack` arbitrarily here.\n+\t\t */\n+\t\t;\n+\t}\n\n \tif (refs_snapshot)\n \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\n@@ -1360,7 +1405,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n \t\tmidx_included_packs(&include, &existing, &names, &geometry);\n\n-\t\tret = write_midx_included_packs(&include, &geometry,\n+\t\tret = write_midx_included_packs(&include, &geometry, &names,\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex dc86ca8269..be3735dff0 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -372,4 +372,43 @@ test_expect_success '--max-cruft-size ignores non-local packs' '\n \t)\n '\n\n+test_expect_success 'reachable packs are preferred over cruft ones' '\n+\trepo=\"cruft-preferred-packs\" &&\n+\tgit init \"$repo\" &&\n+\t(\n+\t\tcd \"$repo\" &&\n+\n+\t\t# This test needs to exercise careful control over when a MIDX\n+\t\t# is and is not written. Unset the corresponding TEST variable\n+\t\t# accordingly.\n+\t\tsane_unset GIT_TEST_MULTI_PACK_INDEX &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit --no-tag cruft &&\n+\n+\t\tnon_cruft=\"$(echo base | git pack-objects --revs $packdir/pack)\" &&\n+\t\t# Write a cruft pack which both (a) sorts ahead of the non-cruft\n+\t\t# pack in lexical order, and (b) has an older mtime to appease\n+\t\t# the MIDX preferred pack selection routine.\n+\t\tcruft=\"$(echo pack-$non_cruft.pack | git pack-objects --cruft $packdir/pack-A)\" &&\n+\t\ttest-tool chmtime -1000 $packdir/pack-A-$cruft.pack &&\n+\n+\t\ttest_commit other &&\n+\t\tgit repack -d &&\n+\n+\t\tgit repack --geometric 2 -d --write-midx --write-bitmap-index &&\n+\n+\t\t# After repacking, there are two packs left: one reachable one\n+\t\t# (which is the result of combining both of the existing two\n+\t\t# non-cruft packs), and one cruft pack.\n+\t\tfind .git/objects/pack -type f -name \"*.pack\" >packs &&\n+\t\ttest_line_count = 2 packs &&\n+\n+\t\t# Make sure that the pack we just wrote is marked as preferred,\n+\t\t# not the cruft one.\n+\t\tpack=\"$(test-tool read-midx --preferred-pack $objdir)\" &&\n+\t\ttest_path_is_missing \"$packdir/$(basename \"$pack\" \".idx\").mtimes\"\n+\t)\n+'\n+\n test_done\n--\n2.42.0.341.g9d86323b73.dirty\n"},{"id":"482642","messageId":"20231004131314.GB607079@coredump.intra.peff.net","threadId":"60300","inReplyTo":"035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-04T13:13:14Z","receivedAt":"2023-10-04T13:13:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote:\n\n> A small re-roll of the original patch, removing a few paragraphs from\n> the proposed log message that are irrelevant and impossible to produce\n> in today's git.git.\n> \n> For more details, see <ZRyNHRf+tQwV+T6P@nand.local>.\n\nThanks, this looks good to me, and the explanation in the linked message\nmakes sense.\n\n-Peff\n"},{"id":"482685","messageId":"ZR6ajQa8bh5HKsnr@tanuki","threadId":"60300","inReplyTo":"035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-05T11:14:21Z","receivedAt":"2023-10-05T14:43:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote:\n[snip]\n> @@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,\n>  \tif (preferred)\n>  \t\tstrvec_pushf(&cmd.args, \"--preferred-pack=%s\",\n>  \t\t\t     pack_basename(preferred));\n> +\telse if (names->nr) {\n> +\t\t/* The largest pack was repacked, meaning that either\n> +\t\t * one or two packs exist depending on whether the\n> +\t\t * repository has a cruft pack or not.\n\nNit: this comment will grow stale soonish once your patch series lands\nthat introduces a maximum packfile size for cruft packs as there can be\narbitrarily many cruft packs from thereon.\n\n> +\t\t * Select the non-cruft one as preferred to encourage\n> +\t\t * pack-reuse among packs containing reachable objects\n> +\t\t * over unreachable ones.\n> +\t\t *\n> +\t\t * (Note we could write multiple packs here if\n> +\t\t * `--max-pack-size` was given, but any one of them\n> +\t\t * will suffice, so pick the first one.)\n> +\t\t */\n\nWell, okay, you kind of acknowledge this here.\n\nThe rest of this patch series looks good to me and makes sense. I don't\nreally think that this comment here is worth a reroll.\n\nPatrick\n"},{"id":"482711","messageId":"CAPig+cSp1MVVikAxtuq8XQPeoEtsaXETOywMTqM_CBDguASMUA@mail.gmail.com","threadId":"60300","inReplyTo":"ZR6ajQa8bh5HKsnr@tanuki","subject":"Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-05T20:27:46Z","receivedAt":"2023-10-05T20:28:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 5, 2023 at 10:46 AM Patrick Steinhardt <ps@pks.im> wrote:\n> On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote:\n> > +             /* The largest pack was repacked, meaning that either\n> > +              * one or two packs exist depending on whether the\n> > +              * repository has a cruft pack or not.\n>\n> Nit: this comment will grow stale soonish once your patch series lands\n> that introduces a maximum packfile size for cruft packs as there can be\n> arbitrarily many cruft packs from thereon.\n>\n> The rest of this patch series looks good to me and makes sense. I don't\n> really think that this comment here is worth a reroll.\n\nIf you do happen to reroll for some reason, though, perhaps take care\nof the minor style violation, as well. Use:\n\n    /*\n     * Multi-line comment\n     * style.\n     */\n\nrather than:\n\n    /* Multi-line comment\n     * style.\n     */\n"}]}