{"thread":{"id":"65309","subject":"[PATCH 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","startedAt":"2026-03-19T22:24:15Z","lastAt":"2026-03-27T20:43:06Z","messageCount":41,"participants":["Taylor Blau","Jeff King","Patrick Steinhardt","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"539427","messageId":"cover.1773959041.git.me@ttaylorr.com","threadId":"65309","inReplyTo":null,"subject":"[PATCH 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-19T22:24:12Z","receivedAt":"2026-03-19T22:24:15Z","isPatch":true,"body":"This series came from an issue I saw in GitHub's infrastructure where a\nparticular repository was failing to repack with the following in its\nlog output:\n\n    warning: Failed to write bitmap index. Packfile doesn't have full closure (object XYZ is missing)\n\nThis was following a geometric repack that generated a MIDX and\nattempted to generate its corresponding MIDX bitmap. Ordinarily, we\nshould never expect the above when generating MIDX bitmaps, unless:\n\n - Object XYZ is indeed missing from the repository (though we should\n   have seen an earlier failure during the MIDX write itself), or\n\n - Object XYZ is somehow not included in the MIDX, but is reachable from\n   another object which is.\n\nAfter validating that object XYZ was indeed present at the time the\nrepack failed, I tried to figure out why we would ever generate a MIDX\nwhose set of objects was not closed under reachability.\n\nThe precise details are laid out in the fourth patch, but the gist is\nthat:\n\n 1. A pack whose objects are *not* closed under reachability was deemed\n    large enough by the geometric repack machinery so as to not need a\n    repack.\n\n 2. When generating the new pack with the non-closed pack was marked as\n    excluded, some object in an included pack reached an object (XYZ) in\n    an unknown pack whose only reachability path involved walking\n    through the parents of an object in another excluded pack. That\n\n 3. We wrote a MIDX containing the new pack, along with all of the\n    large-enough packs from the previous step\n\n 4. Because XYZ was in an unknown (likely cruft) pack, the resulting\n    MIDX does not contain a copy of object XYZ, but does contain a copy\n    of an object which reaches it, making it impossible to write\n    bitmaps.\n\nThis series introduces a special denotation for packs which are excluded\nbut not guaranteed to be closed under reachability. By marking a pack as\nexcluded via '!' (as opposed to the traditional '^'), we will now pick\nup copies of objects reachable from objects in those pack(s), but\nexclude objects which appear in excluded packs (open or closed).\n\nIn practice this means that the resulting pack contains:\n\n 1. All objects in the set difference between the included packs vs. the\n    excluded ones (open or closed).\n\n 2. All objects reachable from at least one object in either an included\n    pack or an excluded-open pack which (a) do not appear in an excluded\n    pack (of either kind), and (b) has a reachability path that does not\n    involve objects in the excluded-closed packs.\n\nThe series contains a couple of minor fixups and some refactoring that I\ndid along the way to make the substantive changes easier to read. The\nfirst commit is a cleanup, the second is a refactoring, and the\nremaining patches demonstrate, explain, and fix the bug.\n\n(As a general side-note, I am somewhat unhappy with the growing number\nof ways to mark a pack as \"kept\", and tried to untangle this by letting\npacks hold an arbitrary bitset of flags that is opaque to the object\ntraversal machinery. This ended up being doable but rather complicated,\nso I ended up punting on it for now.)\n\nThanks in advance for your review!\n\nTaylor Blau (5):\n  pack-objects: plug leak in `read_stdin_packs()`\n  pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`\n  t7704: demonstrate failure with once-cruft objects above the geometric\n    split\n  pack-objects: support excluded-open packs with --stdin-packs\n  repack: mark non-MIDX packs above the split as excluded-open\n\n Documentation/git-pack-objects.adoc |  25 ++-\n builtin/pack-objects.c              | 234 ++++++++++++++++++++--------\n builtin/repack.c                    |  19 ++-\n packfile.c                          |   3 +-\n packfile.h                          |   2 +\n t/t5331-pack-objects-stdin.sh       | 105 +++++++++++++\n t/t7704-repack-cruft.sh             |  22 +++\n 7 files changed, 332 insertions(+), 78 deletions(-)\n\n\nbase-commit: 7ff1e8dc1e1680510c96e69965b3fa81372c5037\n-- \n2.53.0.614.gc4fd52e751a\n"},{"id":"539428","messageId":"1dac74f1e4a370097117754a6b1fbb6fa2b382a6.1773959041.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH 1/5] pack-objects: plug leak in `read_stdin_packs()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-19T22:24:15Z","receivedAt":"2026-03-19T22:24:18Z","isPatch":true,"body":"The `read_stdin_packs()` function added originally via 339bce27f4f\n(builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)\ndeclares a `rev_info` struct but neglects to call `release_revisions()`\non it before returning, creating a leak.\n\nThe related change in 97ec43247c0 (pack-objects: declare 'rev_info' for\n'--stdin-packs' earlier, 2025-06-23) carried forward this oversight and\ndid not address it.\n\nEnsure that we call `release_revisions()` appropriately to prevent a\nleak from this function.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cd013c0b68a..9a89bc5c4c9 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3968,6 +3968,8 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \t\t\t     show_object_pack_hint,\n \t\t\t     &mode);\n \n+\trelease_revisions(&revs);\n+\n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_found\",\n \t\t\t   stdin_packs_found_nr);\n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_hints\",\n-- \n2.53.0.614.gc4fd52e751a\n\n"},{"id":"539429","messageId":"ea6fdbcc46f608c3fbe65298e9ca91faf43a1b16.1773959041.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-19T22:24:20Z","receivedAt":"2026-03-19T22:24:22Z","isPatch":true,"body":"The '--stdin-packs' mode of pack-objects maintains two separate\nstring_lists: one for included packs, and one for excluded packs. Each\nlist stores the pack basename as a string and the corresponding\n`packed_git` pointer in its `->util` field.\n\nThis works, but makes it awkward to extend the set of pack \"kinds\" that\npack-objects can accept via stdin, since each new kind would need its\nown string_list and duplicated handling. A future commit will want to do\njust this, so prepare for that change by handling the various \"kinds\" of\npacks specified over stdin in a more generic fashion.\n\nNamely, replace the two `string_list`s with a single `strmap` keyed on\nthe pack basename, with values pointing to a new `struct\nstdin_pack_info`. This struct tracks both the `packed_git` pointer and a\n`kind` bitfield indicating whether the pack was specified as included or\nexcluded.\n\nExtract the logic for sorting packs by mtime and adding their objects\ninto a separate `stdin_packs_add_entries()` helper.\n\nWhile we could have used a `string_list`, we must handle the case where\nthe same pack is specified more than once. With a `string_list` only, we\nwould have to pay a quadratic cost to either (a) insert elements into\ntheir sorted positions, or (b) a repeated linear search, which is\naccidentally quadratic. For that reason, use a strmap instead.\n\nThis patch does not include any functional changes.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 153 +++++++++++++++++++++++++----------------\n 1 file changed, 92 insertions(+), 61 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 9a89bc5c4c9..72c9ddbed6b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -28,6 +28,7 @@\n #include \"reachable.h\"\n #include \"oid-array.h\"\n #include \"strvec.h\"\n+#include \"strmap.h\"\n #include \"list.h\"\n #include \"packfile.h\"\n #include \"object-file.h\"\n@@ -3837,90 +3838,120 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n \t\treturn 0;\n }\n \n-static void read_packs_list_from_stdin(struct rev_info *revs)\n+struct stdin_pack_info {\n+\tstruct packed_git *p;\n+\tenum {\n+\t\tSTDIN_PACK_INCLUDE = (1<<0),\n+\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+\t} kind;\n+};\n+\n+static void stdin_packs_add_pack_entries(struct strmap *packs,\n+\t\t\t\t\t struct rev_info *revs)\n+{\n+\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *entry;\n+\n+\tstrmap_for_each_entry(packs, &iter, entry) {\n+\t\tstruct stdin_pack_info *info = entry->value;\n+\t\tif (!info->p)\n+\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n+\n+\t\tstring_list_append(&keys, entry->key)->util = info->p;\n+\t}\n+\n+\t/*\n+\t * Order packs by ascending mtime; use QSORT directly to access the\n+\t * string_list_item's ->util pointer, which string_list_sort() does not\n+\t * provide.\n+\t */\n+\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n+\n+\tfor_each_string_list_item(item, &keys) {\n+\t\tstruct stdin_pack_info *info = strmap_get(packs, item->string);\n+\t\tif (!info->p)\n+\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\n+\t\tif (info->kind & STDIN_PACK_INCLUDE)\n+\t\t\tfor_each_object_in_pack(info->p,\n+\t\t\t\t\t\tadd_object_entry_from_pack,\n+\t\t\t\t\t\trevs,\n+\t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n+\t}\n+\n+\tstring_list_clear(&keys, 0);\n+}\n+\n+static void stdin_packs_read_input(struct rev_info *revs)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *item = NULL;\n+\tstruct strmap packs = STRMAP_INIT;\n \tstruct packed_git *p;\n \n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\tif (!buf.len)\n+\t\tstruct stdin_pack_info *info;\n+\t\tconst char *key = buf.buf;\n+\n+\t\tif (!key || !*key)\n \t\t\tcontinue;\n \n+\t\tif (*key == '^')\n+\t\t\tkey++;\n+\n+\t\tinfo = strmap_get(&packs, key);\n+\t\tif (!info) {\n+\t\t\tCALLOC_ARRAY(info, 1);\n+\t\t\tstrmap_put(&packs, key, info);\n+\t\t}\n+\n \t\tif (*buf.buf == '^')\n-\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n+\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n \t\telse\n-\t\t\tstring_list_append(&include_packs, buf.buf);\n+\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n \n \t\tstrbuf_reset(&buf);\n \t}\n \n-\tstring_list_sort_u(&include_packs, 0);\n-\tstring_list_sort_u(&exclude_packs, 0);\n-\n \trepo_for_each_pack(the_repository, p) {\n-\t\tconst char *pack_name = pack_basename(p);\n+\t\tstruct stdin_pack_info *info;\n \n-\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n+\t\tinfo = strmap_get(&packs, pack_basename(p));\n+\t\tif (!info)\n+\t\t\tcontinue;\n+\n+\t\tif (info->kind & STDIN_PACK_INCLUDE) {\n \t\t\tif (exclude_promisor_objects && p->pack_promisor)\n \t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n-\t\t\titem->util = p;\n+\n+\t\t\t/*\n+\t\t\t * Arguments we got on stdin may not even be\n+\t\t\t * packs. First check that to avoid segfaulting\n+\t\t\t * later on in e.g.  pack_mtime_cmp(), excluded\n+\t\t\t * packs are handled below.\n+\t\t\t */\n+\t\t\tif (!is_pack_valid(p))\n+\t\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n \t\t}\n-\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n-\t\t\titem->util = p;\n-\t}\n \n-\t/*\n-\t * Arguments we got on stdin may not even be packs. First\n-\t * check that to avoid segfaulting later on in\n-\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n-\t *\n-\t * Since we first parsed our STDIN and then sorted the input\n-\t * lines the pack we error on will be whatever line happens to\n-\t * sort first. This is lazy, it's enough that we report one\n-\t * bad case here, we don't need to report the first/last one,\n-\t * or all of them.\n-\t */\n-\tfor_each_string_list_item(item, &include_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tif (!p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n-\t\tif (!is_pack_valid(p))\n-\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n-\t}\n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_CLOSED) {\n+\t\t\t/*\n+\t\t\t * Marking excluded packs as kept in-core so\n+\t\t\t * that later calls to add_object_entry()\n+\t\t\t * discards any objects that are also found in\n+\t\t\t * excluded packs.\n+\t\t\t */\n+\t\t\tp->pack_keep_in_core = 1;\n+\t\t}\n \n-\t/*\n-\t * Then, handle all of the excluded packs, marking them as\n-\t * kept in-core so that later calls to add_object_entry()\n-\t * discards any objects that are also found in excluded packs.\n-\t */\n-\tfor_each_string_list_item(item, &exclude_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tif (!p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n-\t\tp->pack_keep_in_core = 1;\n+\t\tinfo->p = p;\n \t}\n \n-\t/*\n-\t * Order packs by ascending mtime; use QSORT directly to access the\n-\t * string_list_item's ->util pointer, which string_list_sort() does not\n-\t * provide.\n-\t */\n-\tQSORT(include_packs.items, include_packs.nr, pack_mtime_cmp);\n-\n-\tfor_each_string_list_item(item, &include_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tfor_each_object_in_pack(p,\n-\t\t\t\t\tadd_object_entry_from_pack,\n-\t\t\t\t\trevs,\n-\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n-\t}\n+\tstdin_packs_add_pack_entries(&packs, revs);\n \n \tstrbuf_release(&buf);\n-\tstring_list_clear(&include_packs, 0);\n-\tstring_list_clear(&exclude_packs, 0);\n+\tstrmap_clear(&packs, 1);\n }\n \n static void add_unreachable_loose_objects(struct rev_info *revs);\n@@ -3957,7 +3988,7 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n-\tread_packs_list_from_stdin(&revs);\n+\tstdin_packs_read_input(&revs);\n \tif (rev_list_unpacked)\n \t\tadd_unreachable_loose_objects(&revs);\n \n-- \n2.53.0.614.gc4fd52e751a\n\n"},{"id":"539430","messageId":"0ec9bba92ad4ca0bac1f063ad05294f2df12323d.1773959041.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH 3/5] t7704: demonstrate failure with once-cruft objects above the geometric split","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-19T22:24:22Z","receivedAt":"2026-03-19T22:24:24Z","isPatch":true,"body":"Add a test demonstrating a case where geometric repacking fails to\nproduce a pack with full object closure, thus making it impossible to\nwrite a reachability bitmap.\n\nMark the test with 'test_expect_failure' for now. The subsequent commit\nwill explain the precise failure mode, and implement a fix.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t7704-repack-cruft.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex aa2e2e6ad88..77133395b5d 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -869,4 +869,26 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'\n \t)\n '\n \n+test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n+\tgit config repack.midxMustContainCruft false &&\n+\n+\ttest_commit reachable &&\n+\ttest_commit unreachable &&\n+\n+\tunreachable=\"$(git rev-parse HEAD)\" &&\n+\n+\tgit reset --hard HEAD^ &&\n+\tgit tag -d unreachable &&\n+\tgit reflog expire --all --expire=all &&\n+\n+\tgit repack --cruft -d &&\n+\n+\techo $unreachable | git pack-objects .git/objects/pack/pack &&\n+\n+\ttest_commit new &&\n+\n+\tgit update-ref refs/heads/other $unreachable &&\n+\tgit repack --geometric=2 -d --write-midx --write-bitmap-index\n+'\n+\n test_done\n-- \n2.53.0.614.gc4fd52e751a\n\n"},{"id":"539431","messageId":"bd78919e19cfa968556ad4241391120ed56e9dce.1773959041.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-19T22:24:25Z","receivedAt":"2026-03-19T22:24:27Z","isPatch":true,"body":"In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n2025-06-23), pack-objects learned to traverse through commits in\nincluded packs when using '--stdin-packs=follow', rescuing reachable\nobjects from unlisted packs into the output.\n\nWhen we encounter a commit in an excluded pack during this rescuing\nphase we will traverse through its parents. But because we set\n`revs.no_kept_objects = 1`, commit simplification will prevent us from\nshowing it via `get_revision()`. (In practice, `--stdin-packs=follow`\nwalks commits down to the roots, but only opens up trees for ones that\ndo not appear in an excluded pack.)\n\nBut there are certain cases where we *do* need to see the parents of an\nobject in an excluded pack. Namely, if an object is rescue-able, but\nonly reachable from object(s) which appear in excluded packs, then\ncommit simplification will exclude those commits from the object\ntraversal, and we will never see a copy of that object, and thus not\nrescue it.\n\nThis is what causes the failure in the previous commit during repacking.\nWhen performing a geometric repack, packs above the geometric split that\nweren't part of the previous MIDX (e.g., packs pushed directly into\n`$GIT_DIR/objects/pack`) may not have full object closure.  When those\npacks are listed as excluded via the '^' marker, the reachability\ntraversal encounters the sequence described above, and may miss objects\nwhich we expect to rescue with `--stdin-packs=follow`.\n\nIntroduce a new \"excluded-open\" pack prefix, '!'. Like '^'-prefixed\npacks, objects from '!'-prefixed packs are excluded from the resulting\npack. But unlike '^', commits in '!'-prefixed packs *are* used as\nstarting points for the follow traversal, and the traversal does not\ntreat them as a closure boundary.\n\nIn order to distinguish excluded-closed from excluded-open packs during\nthe traversal, introduce a new `pack_keep_in_core_open` bit on\n`struct packed_git`, along with a corresponding `KEPT_PACK_IN_CORE_OPEN`\nflag for the kept-pack cache.\n\nIn `add_object_entry_from_pack()`, move the `want_object_in_pack()`\ncheck to *after* `add_pending_oid()`. This is necessary so that commits\nfrom excluded-open packs are added as traversal tips even though their\nobjects won't appear in the output. The `include_check` and\n`include_check_obj` callbacks on `rev_info` are used to halt the walk at\nclosed-excluded packs, since objects behind a '^' boundary are\nguaranteed to have closure and need not be rescued.\n\nThe following commit will make use of this new functionality within the\nrepack layer to resolve the test failure demonstrated in the previous\ncommit.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-pack-objects.adoc |  25 +++++--\n builtin/pack-objects.c              |  87 ++++++++++++++++++++---\n packfile.c                          |   3 +-\n packfile.h                          |   2 +\n t/t5331-pack-objects-stdin.sh       | 105 ++++++++++++++++++++++++++++\n 5 files changed, 203 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 71b9682485c..b78175fbe1b 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -94,13 +94,24 @@ base-name::\n \tincluded packs (those not beginning with `^`), excluding any\n \tobjects listed in the excluded packs (beginning with `^`).\n +\n-When `mode` is \"follow\", objects from packs not listed on stdin receive\n-special treatment. Objects within unlisted packs will be included if\n-those objects are (1) reachable from the included packs, and (2) not\n-found in any excluded packs. This mode is useful, for example, to\n-resurrect once-unreachable objects found in cruft packs to generate\n-packs which are closed under reachability up to the boundary set by the\n-excluded packs.\n+When `mode` is \"follow\" packs may additionally be prefixed with `!`,\n+indicating that they are excluded but not necessarily closed under\n+reachability.  In addition to objects in included packs, the resulting\n+pack may include additional objects based on the following:\n++\n+--\n+* If any packs are marked with `!`, then objects reachable from such\n+  packs or included ones via objects outside of excluded-closed packs\n+  will be included. In this case, all `^` packs are treated as closed\n+  under reachability.\n+* Otherwise (if there are no `!` packs), objects within unlisted packs\n+  will be included if those objects are (1) reachable from the\n+  included packs, and (2) not found in any excluded packs.\n+--\n++\n+This mode is useful, for example, to resurrect once-unreachable\n+objects found in cruft packs to generate packs which are closed under\n+reachability up to the boundary set by the excluded packs.\n +\n Incompatible with `--revs`, or options that imply `--revs` (such as\n `--all`), with the exception of `--unpacked`, which is compatible.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 72c9ddbed6b..0d25a856717 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -217,6 +217,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_open;\n static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n@@ -1618,7 +1619,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t/*\n \t * Then handle .keep first, as we have a fast(er) path there.\n \t */\n-\tif (ignore_packed_keep_on_disk || ignore_packed_keep_in_core) {\n+\tif (ignore_packed_keep_on_disk || ignore_packed_keep_in_core ||\n+\t    ignore_packed_keep_in_core_open) {\n \t\t/*\n \t\t * Set the flags for the kept-pack cache to be the ones we want\n \t\t * to ignore.\n@@ -1632,6 +1634,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\t\tflags |= KEPT_PACK_ON_DISK;\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= KEPT_PACK_IN_CORE;\n+\t\tif (ignore_packed_keep_in_core_open)\n+\t\t\tflags |= KEPT_PACK_IN_CORE_OPEN;\n \n \t\t/*\n \t\t * If the object is in a pack that we want to ignore, *and* we\n@@ -1643,6 +1647,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\t\t\treturn 0;\n \t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n \t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core_open && p->pack_keep_in_core_open)\n+\t\t\t\treturn 0;\n \t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n \t\t\t\treturn 0;\n \t\t} else {\n@@ -3750,8 +3756,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\treturn 0;\n \n \tofs = nth_packed_object_offset(p, pos);\n-\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n-\t\treturn 0;\n \n \tif (p) {\n \t\tstruct object_info oi = OBJECT_INFO_INIT;\n@@ -3763,15 +3767,26 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t} else if (type == OBJ_COMMIT) {\n \t\t\tstruct rev_info *revs = _data;\n \t\t\t/*\n-\t\t\t * commits in included packs are used as starting points for the\n-\t\t\t * subsequent revision walk\n+\t\t\t * commits in included packs are used as starting points\n+\t\t\t * for the subsequent revision walk\n+\t\t\t *\n+\t\t\t * Note that we do want to walk through commits that are\n+\t\t\t * present in excluded-open ('!') packs to pick up any\n+\t\t\t * objects reachable from them not present in the\n+\t\t\t * excluded-closed ('^') packs.\n+\t\t\t *\n+\t\t\t * However, we'll only add those objects to the packing\n+\t\t\t * list after checking `want_object_in_pack()` below.\n \t\t\t */\n \t\t\tadd_pending_oid(revs, NULL, oid, 0);\n \t\t}\n-\n-\t\tstdin_packs_found_nr++;\n \t}\n \n+\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n+\t\treturn 0;\n+\n+\tstdin_packs_found_nr++;\n+\n \tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n \n \treturn 0;\n@@ -3838,11 +3853,23 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n \t\treturn 0;\n }\n \n+static int stdin_packs_include_check_obj(struct object *obj, void *data UNUSED)\n+{\n+\treturn !has_object_kept_pack(to_pack.repo, &obj->oid,\n+\t\t\t\t     KEPT_PACK_IN_CORE);\n+}\n+\n+static int stdin_packs_include_check(struct commit *commit, void *data)\n+{\n+\treturn stdin_packs_include_check_obj((struct object *)commit, data);\n+}\n+\n struct stdin_pack_info {\n \tstruct packed_git *p;\n \tenum {\n \t\tSTDIN_PACK_INCLUDE = (1<<0),\n \t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+\t\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n \t} kind;\n };\n \n@@ -3874,7 +3901,19 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \t\tif (!info->p)\n \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n \n-\t\tif (info->kind & STDIN_PACK_INCLUDE)\n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n+\t\t\t/*\n+\t\t\t * When open-excluded packs (\"!\") are present, stop\n+\t\t\t * the parent walk at closed-excluded (\"^\") packs.\n+\t\t\t * Objects behind a \"^\" boundary are guaranteed to\n+\t\t\t * have closure and should not be rescued.\n+\t\t\t */\n+\t\t\trevs->include_check = stdin_packs_include_check;\n+\t\t\trevs->include_check_obj = stdin_packs_include_check_obj;\n+\t\t}\n+\n+\t\tif ((info->kind & STDIN_PACK_INCLUDE) ||\n+\t\t    (info->kind & STDIN_PACK_EXCLUDE_OPEN))\n \t\t\tfor_each_object_in_pack(info->p,\n \t\t\t\t\t\tadd_object_entry_from_pack,\n \t\t\t\t\t\trevs,\n@@ -3884,7 +3923,8 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \tstring_list_clear(&keys, 0);\n }\n \n-static void stdin_packs_read_input(struct rev_info *revs)\n+static void stdin_packs_read_input(struct rev_info *revs,\n+\t\t\t\t   enum stdin_packs_mode mode)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strmap packs = STRMAP_INIT;\n@@ -3897,7 +3937,8 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \t\tif (!key || !*key)\n \t\t\tcontinue;\n \n-\t\tif (*key == '^')\n+\t\tif (*key == '^' ||\n+\t\t    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))\n \t\t\tkey++;\n \n \t\tinfo = strmap_get(&packs, key);\n@@ -3908,6 +3949,8 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \n \t\tif (*buf.buf == '^')\n \t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n+\t\telse if (*buf.buf == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n+\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_OPEN;\n \t\telse\n \t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n \n@@ -3945,6 +3988,20 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \t\t\tp->pack_keep_in_core = 1;\n \t\t}\n \n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n+\t\t\t/*\n+\t\t\t * Marking excluded open packs as kept in-core\n+\t\t\t * (open) for the same reason as we marked\n+\t\t\t * exclude closed packs as kept in-core.\n+\t\t\t *\n+\t\t\t * Use a separate flag here to ensure we don't\n+\t\t\t * halt our traversal at these packs, since they\n+\t\t\t * are not guaranteed to have closure.\n+\t\t\t *\n+\t\t\t */\n+\t\t\tp->pack_keep_in_core_open = 1;\n+\t\t}\n+\n \t\tinfo->p = p;\n \t}\n \n@@ -3988,7 +4045,15 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n-\tstdin_packs_read_input(&revs);\n+\tif (mode == STDIN_PACKS_MODE_FOLLOW) {\n+\t\t/*\n+\t\t * In '--stdin-packs=follow' mode, additionally ignore\n+\t\t * objects in excluded-open packs to prevent them from\n+\t\t * appearing in the resulting pack.\n+\t\t */\n+\t\tignore_packed_keep_in_core_open = 1;\n+\t}\n+\tstdin_packs_read_input(&revs, mode);\n \tif (rev_list_unpacked)\n \t\tadd_unreachable_loose_objects(&revs);\n \ndiff --git a/packfile.c b/packfile.c\nindex 215a23e42be..076e444e32a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2246,7 +2246,8 @@ struct packed_git **packfile_store_get_kept_pack_cache(struct packfile_store *st\n \t\t\tstruct packed_git *p = e->pack;\n \n \t\t\tif ((p->pack_keep && (flags & KEPT_PACK_ON_DISK)) ||\n-\t\t\t    (p->pack_keep_in_core && (flags & KEPT_PACK_IN_CORE))) {\n+\t\t\t    (p->pack_keep_in_core && (flags & KEPT_PACK_IN_CORE)) ||\n+\t\t\t    (p->pack_keep_in_core_open && (flags & KEPT_PACK_IN_CORE_OPEN))) {\n \t\t\t\tALLOC_GROW(packs, nr + 1, alloc);\n \t\t\t\tpacks[nr++] = p;\n \t\t\t}\ndiff --git a/packfile.h b/packfile.h\nindex 8b04a258a7b..b7735c1977d 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -28,6 +28,7 @@ struct packed_git {\n \tunsigned pack_local:1,\n \t\t pack_keep:1,\n \t\t pack_keep_in_core:1,\n+\t\t pack_keep_in_core_open:1,\n \t\t freshened:1,\n \t\t do_not_close:1,\n \t\t pack_promisor:1,\n@@ -266,6 +267,7 @@ int packfile_store_freshen_object(struct packfile_store *store,\n enum kept_pack_type {\n \tKEPT_PACK_ON_DISK = (1 << 0),\n \tKEPT_PACK_IN_CORE = (1 << 1),\n+\tKEPT_PACK_IN_CORE_OPEN = (1 << 2),\n };\n \n /*\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 7eb79bc2cdb..c74b5861af3 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -415,4 +415,109 @@ test_expect_success '--stdin-packs=follow tolerates missing commits' '\n \tstdin_packs__follow_with_only HEAD HEAD^{tree}\n '\n \n+test_expect_success '--stdin-packs=follow with open-excluded packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\t# Create the following commit structure:\n+\t\t#\n+\t\t#   A <-- B <-- D     (main)\n+\t\t#         ^\n+\t\t#          \\\n+\t\t#           C        (other)\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\tgit checkout -B other &&\n+\t\ttest_commit C &&\n+\t\tgit checkout main &&\n+\t\ttest_commit D &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tD=\"$(echo B..D | git pack-objects --revs $packdir/pack)\" &&\n+\n+\t\tC_ONLY=\"$(git rev-parse other | git pack-objects $packdir/pack)\" &&\n+\n+\t\tgit prune-packed &&\n+\n+\t\t# Create a pack using --stdin-packs=follow where:\n+\t\t#\n+\t\t#  - pack D is included,\n+\t\t#  - pack C_ONLY is excluded, but open,\n+\t\t#  - pack B is excluded, but closed, and\n+\t\t#  - packs A and C are unknown\n+\t\t#\n+\t\t# The resulting pack should therefore contain:\n+\t\t#\n+\t\t#  - objects from the included pack D,\n+\t\t#  - A.t (rescued via D^{tree}), and\n+\t\t#  - C^{tree} and C.t (rescued via pack C_ONLY)\n+\t\t#\n+\t\t# , but should omit:\n+\t\t#\n+\t\t#  - C (excluded via C_ONLY),\n+\t\t#  - objects from pack B (trivially excluded-closed)\n+\t\t#  - A and A^{tree} (ancestors of B)\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack <<-EOF\n+\t\tpack-$D.pack\n+\t\t!pack-$C_ONLY.pack\n+\t\t^pack-$B.pack\n+\t\tEOF\n+\t\t) &&\n+\n+\t\t{\n+\t\t\tobjects_in_packs $D &&\n+\t\t\tgit rev-parse A:A.t \"C^{tree}\" C:C.t\n+\t\t} >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with !-delimited pack without follow' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\n+\t\tcat >in <<-EOF &&\n+\t\t!pack-$A.pack\n+\t\tpack-$B.pack\n+\t\tpack-$C.pack\n+\t\tEOF\n+\n+\t\t# Without --stdin-packs=follow, we treat the first\n+\t\t# line of input as a literal packfile name, and thus\n+\t\t# expect pack-objects to complain of a missing pack\n+\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n+\t\t\t>/dev/null <in 2>err &&\n+\t\ttest_grep \"could not find pack .!pack-$A.pack.\" err &&\n+\n+\t\t# With --stdin-packs=follow, we treat the second line\n+\t\t# of input as indicating pack-$A.pack is an excluded\n+\t\t# open pack, and thus expect pack-objects to succeed\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&\n+\n+\t\tobjects_in_packs $B $C >expect &&\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.53.0.614.gc4fd52e751a\n\n"},{"id":"539432","messageId":"c4fd52e751a2eb9f9283f6fe1f360cf1f0793942.1773959041.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH 5/5] repack: mark non-MIDX packs above the split as excluded-open","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-19T22:24:28Z","receivedAt":"2026-03-19T22:24:30Z","isPatch":true,"body":"In 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where\npossible, 2025-06-23), geometric repacking learned to exclude cruft\npacks from the MIDX when 'repack.midxMustContainCruft' is set to\n'false'.\n\nThis works because packs generated with '--stdin-packs=follow' rescue\nany once-unreachable objects that later become reachable, making the\nresulting packs closed under reachability without needing the cruft pack\nin the MIDX.\n\nHowever, packs above the geometric split that were not part of the\nprevious MIDX may not have full object closure.  When such packs are\nmarked as excluded-closed ('^'), pack-objects treats them as a\nreachability boundary and does not traverse through them during the\nfollow pass, potentially leaving the resulting pack without full\nclosure.\n\nFix this by marking packs above the geometric split that were not in the\nprevious MIDX as excluded-open ('!') instead of excluded-closed ('^').\nThis causes pack-objects to walk through their commits during the follow\npass, rescuing any reachable objects not present in the closed-excluded\npacks.\n\nNote that MIDXs which were generated prior to this change and are\nunlucky enough to not be closed under reachability may still exhibit\nthis bug, as we treat all MIDX'd packs as closed. That is true in an\noverwhelming number of cases, since in order to have a non-closed MIDX\nyou would have to:\n\n - Generate a pack via an earlier geometric repack that is not closed\n   under reachability.\n\n - Store that pack in the MIDX.\n\n - Avoid picking any commits to receive reachability bitmaps which\n   happen to reach objects from which the missing objects are reachable.\n\nIn the extremely rare chance that all of the above should happen, an\nall-into-one repack will resolve the issue.\n\nUnfortunately, there is no perfect way to determine whether a MIDX'd\npack is closed outside of ensuring that there is a '1' bit in at least\none bitmap for every bit position corresponding to objects in that pack.\nWhile this is possible to do, this approach would treat MIDX'd packs as\nopen in cases where there is at least one object that is not reachable\nfrom the subset of commits selected for bitmapping.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c        | 19 +++++++++++++++++--\n t/t7704-repack-cruft.sh |  2 +-\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f6bb04bef72..4c5a82c2c8d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -369,8 +369,23 @@ int cmd_repack(int argc,\n \t\t */\n \t\tfor (i = 0; i < geometry.split; i++)\n \t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n+\t\tfor (i = geometry.split; i < geometry.pack_nr; i++) {\n+\t\t\tconst char *basename = pack_basename(geometry.pack[i]);\n+\t\t\tchar marker = '^';\n+\n+\t\t\tif (!midx_must_contain_cruft &&\n+\t\t\t    !string_list_has_string(&existing.midx_packs,\n+\t\t\t\t\t\t    basename)) {\n+\t\t\t\t/*\n+\t\t\t\t * Assume non-MIDX'd packs are not\n+\t\t\t\t * necessarily closed under\n+\t\t\t\t * reachability.\n+\t\t\t\t */\n+\t\t\t\tmarker = '!';\n+\t\t\t}\n+\n+\t\t\tfprintf(in, \"%c%s\\n\", marker, basename);\n+\t\t}\n \t\tfclose(in);\n \t}\n \ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 77133395b5d..9e03b04315d 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -869,7 +869,7 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'\n \t)\n '\n \n-test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n+test_expect_success 'repack rescues once-cruft objects above geometric split' '\n \tgit config repack.midxMustContainCruft false &&\n \n \ttest_commit reachable &&\n-- \n2.53.0.614.gc4fd52e751a\n"},{"id":"539607","messageId":"20260321165711.GA718452@coredump.intra.peff.net","threadId":"65309","inReplyTo":"bd78919e19cfa968556ad4241391120ed56e9dce.1773959041.git.me@ttaylorr.com","subject":"Re: [PATCH 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-21T16:57:11Z","receivedAt":"2026-03-21T16:57:13Z","isPatch":true,"body":"On Thu, Mar 19, 2026 at 06:24:25PM -0400, Taylor Blau wrote:\n\n> @@ -3750,8 +3756,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n>  \t\treturn 0;\n>  \n>  \tofs = nth_packed_object_offset(p, pos);\n> -\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n> -\t\treturn 0;\n>  \n>  \tif (p) {\n>  \t\tstruct object_info oi = OBJECT_INFO_INIT;\n\nI haven't read all of the patches carefully yet, but Coverity observed\nthat without this call to want_object_in_pack(), we are left with \"p\"\nthat came from the caller. If that \"p\" is ever NULL, then the\nnth_packed_object_offset() call above will segfault. But if it is not,\nthen the \"if (p)\" below is pointless.\n\nAFAICT it will never be NULL, and the conditional is now pointless. But\nit wasn't before your patch, since want_object_in_pack() may set the\nfound_pack to NULL if the object is not wanted.\n\n-Peff\n"},{"id":"539661","messageId":"acAwZ1ARhvsTSpO5@nand.local","threadId":"65309","inReplyTo":"20260321165711.GA718452@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-22T18:09:43Z","receivedAt":"2026-03-22T18:09:45Z","isPatch":true,"body":"On Sat, Mar 21, 2026 at 12:57:11PM -0400, Jeff King wrote:\n> On Thu, Mar 19, 2026 at 06:24:25PM -0400, Taylor Blau wrote:\n>\n> > @@ -3750,8 +3756,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n> >  \t\treturn 0;\n> >\n> >  \tofs = nth_packed_object_offset(p, pos);\n> > -\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n> > -\t\treturn 0;\n> >\n> >  \tif (p) {\n> >  \t\tstruct object_info oi = OBJECT_INFO_INIT;\n>\n> I haven't read all of the patches carefully yet, but Coverity observed\n> that without this call to want_object_in_pack(), we are left with \"p\"\n> that came from the caller. If that \"p\" is ever NULL, then the\n> nth_packed_object_offset() call above will segfault. But if it is not,\n> then the \"if (p)\" below is pointless.\n>\n> AFAICT it will never be NULL, and the conditional is now pointless. But\n> it wasn't before your patch, since want_object_in_pack() may set the\n> found_pack to NULL if the object is not wanted.\n\nI think that's right. Looking through the code again, the \"if (p)\"\nconditional is indeed no longer useful, since the sole caller of this\nfunction (the callback to `for_each_object_in_pack()`) will always pass\na non-NULL `p` pointer.\n\nIt's tempting to add something like:\n\n    if (!p)\n        BUG(\"add_object_entry_from_pack: expected non-NULL pack\");\n\nBut I wonder if we should instead store the result of calling\n`want_object_in_pack()` into a separate variable, only creating an\nobject entry if \"want == 1\". That would have the effect of *not* marking\nobjects as traversal tips if want_object_in_pack() makes `p` NULL.\n\nI think that's OK, since what we really care about is having objects in\nboth included and excluded-open packs being processed as traversal tips.\nBecause we're enumerating only objects that are in included and\nexcluded-open packs:\n\n * Objects in excluded-open packs will cause `want_found_object()` to\n   return 0, propagating that through `want_object_in_pack()` without\n   setting \"p\" to NULL. We'll process these objects as traversal tips,\n   but not create an object entry for them, which is what we want.\n\n * Objects in included packs will return -1 from `want_found_object()`,\n   causing us to NULL out \"p\" and search through other packs. If we find\n   another pack containing that object, then we'll set \"p\" to that pack\n   and return 1, otherwise we'll return 0 if no such pack is found.\n\nI think that's the right behavior, so doing something like this instead:\n\n--- 8< ---\n@@ -3742,6 +3749,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n {\n \toff_t ofs;\n \tenum object_type type = OBJ_NONE;\n+\tint want;\n\n \tdisplay_progress(progress_state, ++nr_seen);\n\n@@ -3749,8 +3757,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\treturn 0;\n\n \tofs = nth_packed_object_offset(p, pos);\n-\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n-\t\treturn 0;\n+\twant = want_object_in_pack(oid, 0, &p, &ofs);\n\n \tif (p) {\n \t\tstruct object_info oi = OBJECT_INFO_INIT;\n@@ -3762,8 +3769,16 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t} else if (type == OBJ_COMMIT) {\n \t\t\tstruct rev_info *revs = _data;\n \t\t\t/*\n-\t\t\t * commits in included packs are used as starting points for the\n-\t\t\t * subsequent revision walk\n+\t\t\t * commits in included packs are used as starting points\n+\t\t\t * for the subsequent revision walk\n+\t\t\t *\n+\t\t\t * Note that we do want to walk through commits that are\n+\t\t\t * present in excluded-open ('!') packs to pick up any\n+\t\t\t * objects reachable from them not present in the\n+\t\t\t * excluded-closed ('^') packs.\n+\t\t\t *\n+\t\t\t * However, we'll only add those objects to the packing\n+\t\t\t * list after checking `want_object_in_pack()` below.\n \t\t\t */\n \t\t\tadd_pending_oid(revs, NULL, oid, 0);\n \t\t}\n@@ -3771,7 +3786,8 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\tstdin_packs_found_nr++;\n \t}\n\n-\tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n+\tif (want)\n+\t\tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n\n \treturn 0;\n }\n--- >8 ---\n\nmay be more readable. I can't shake the feeling that there is some\nimportant case that this is missing, though, what do you think?\n\nThanks,\nTaylor\n"},{"id":"539810","messageId":"acI_pTWTcJN6QaK1@pks.im","threadId":"65309","inReplyTo":"1dac74f1e4a370097117754a6b1fbb6fa2b382a6.1773959041.git.me@ttaylorr.com","subject":"Re: [PATCH 1/5] pack-objects: plug leak in `read_stdin_packs()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-24T07:39:17Z","receivedAt":"2026-03-24T07:39:30Z","isPatch":true,"body":"On Thu, Mar 19, 2026 at 06:24:15PM -0400, Taylor Blau wrote:\n> The `read_stdin_packs()` function added originally via 339bce27f4f\n> (builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)\n> declares a `rev_info` struct but neglects to call `release_revisions()`\n> on it before returning, creating a leak.\n> \n> The related change in 97ec43247c0 (pack-objects: declare 'rev_info' for\n> '--stdin-packs' earlier, 2025-06-23) carried forward this oversight and\n> did not address it.\n> \n> Ensure that we call `release_revisions()` appropriately to prevent a\n> leak from this function.\n\nWould be curious to learn why none of our tests fail with this. The fix\nlooks obviously correct though, and there are no other early exits that\nmight need fixing here.\n\nPatrick\n"},{"id":"539811","messageId":"acI_sP6ZEdw-xGpR@pks.im","threadId":"65309","inReplyTo":"ea6fdbcc46f608c3fbe65298e9ca91faf43a1b16.1773959041.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-24T07:39:28Z","receivedAt":"2026-03-24T07:39:34Z","isPatch":true,"body":"On Thu, Mar 19, 2026 at 06:24:20PM -0400, Taylor Blau wrote:\n> The '--stdin-packs' mode of pack-objects maintains two separate\n> string_lists: one for included packs, and one for excluded packs. Each\n> list stores the pack basename as a string and the corresponding\n> `packed_git` pointer in its `->util` field.\n> \n> This works, but makes it awkward to extend the set of pack \"kinds\" that\n> pack-objects can accept via stdin, since each new kind would need its\n> own string_list and duplicated handling. A future commit will want to do\n> just this, so prepare for that change by handling the various \"kinds\" of\n> packs specified over stdin in a more generic fashion.\n> \n> Namely, replace the two `string_list`s with a single `strmap` keyed on\n> the pack basename, with values pointing to a new `struct\n> stdin_pack_info`. This struct tracks both the `packed_git` pointer and a\n> `kind` bitfield indicating whether the pack was specified as included or\n> excluded.\n\nOkay.\n\n> Extract the logic for sorting packs by mtime and adding their objects\n> into a separate `stdin_packs_add_entries()` helper.\n\nRight, the ordering was my first question. Interestingly though, that\nfunction doesn't seem to be added in this commit... ah, it's called\n`stdin_packs_add_pack_entries()`.\n\n> While we could have used a `string_list`, we must handle the case where\n> the same pack is specified more than once. With a `string_list` only, we\n> would have to pay a quadratic cost to either (a) insert elements into\n> their sorted positions, or (b) a repeated linear search, which is\n> accidentally quadratic. For that reason, use a strmap instead.\n\nWe could of course just add them and deduplicate in a later step via\n`string_list_sort_u()`. But I assume that you want to handle the case\nwhere we have duplicates specially, either by merging the duplicate into\nthe existing pack info or by aborting.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 9a89bc5c4c9..72c9ddbed6b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3837,90 +3838,120 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n>  \t\treturn 0;\n>  }\n>  \n> -static void read_packs_list_from_stdin(struct rev_info *revs)\n> +struct stdin_pack_info {\n> +\tstruct packed_git *p;\n> +\tenum {\n> +\t\tSTDIN_PACK_INCLUDE = (1<<0),\n> +\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n\nIt might make sense to provide a sentence for each of the enums to\nexplain what they do.\n\n> +\t} kind;\n> +};\n\nOkay, this is the new structure that allows us to track the packfile\nkind in a way that is more extensible. Makes sense.\n\n> +static void stdin_packs_add_pack_entries(struct strmap *packs,\n> +\t\t\t\t\t struct rev_info *revs)\n> +{\n> +\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list_item *item;\n> +\tstruct hashmap_iter iter;\n> +\tstruct strmap_entry *entry;\n> +\n> +\tstrmap_for_each_entry(packs, &iter, entry) {\n> +\t\tstruct stdin_pack_info *info = entry->value;\n> +\t\tif (!info->p)\n> +\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n> +\n> +\t\tstring_list_append(&keys, entry->key)->util = info->p;\n> +\t}\n> +\n> +\t/*\n> +\t * Order packs by ascending mtime; use QSORT directly to access the\n> +\t * string_list_item's ->util pointer, which string_list_sort() does not\n> +\t * provide.\n> +\t */\n> +\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n\nOkay. I was briefly wondering whether it would make more sense to use\n`string_list_sort()`, but I guess it doesn't buy us much.\n\n> +\tfor_each_string_list_item(item, &keys) {\n> +\t\tstruct stdin_pack_info *info = strmap_get(packs, item->string);\n\nWe could avoid this extra lookup if you instead were to store the pack\ninfo in the `item->util` field.\n\n> +\t\tif (!info->p)\n> +\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n\nThis case basically cannot happen as we already `die()` further up,\nright? Should we rather `BUG()` or drop the check completely?\n\n> +\t\tif (info->kind & STDIN_PACK_INCLUDE)\n> +\t\t\tfor_each_object_in_pack(info->p,\n> +\t\t\t\t\t\tadd_object_entry_from_pack,\n> +\t\t\t\t\t\trevs,\n> +\t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n> +\t}\n> +\n> +\tstring_list_clear(&keys, 0);\n> +}\n> +\n> +static void stdin_packs_read_input(struct rev_info *revs)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n> -\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n> -\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n> -\tstruct string_list_item *item = NULL;\n> +\tstruct strmap packs = STRMAP_INIT;\n>  \tstruct packed_git *p;\n>  \n>  \twhile (strbuf_getline(&buf, stdin) != EOF) {\n> -\t\tif (!buf.len)\n> +\t\tstruct stdin_pack_info *info;\n> +\t\tconst char *key = buf.buf;\n> +\n> +\t\tif (!key || !*key)\n\nThe first case of `!key` cannot ever happen as strbufs always have `buf`\nset.\n\n>  \t\t\tcontinue;\n>  \n> +\t\tif (*key == '^')\n> +\t\t\tkey++;\n> +\n> +\t\tinfo = strmap_get(&packs, key);\n> +\t\tif (!info) {\n> +\t\t\tCALLOC_ARRAY(info, 1);\n> +\t\t\tstrmap_put(&packs, key, info);\n> +\t\t}\n> +\n>  \t\tif (*buf.buf == '^')\n> -\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n> +\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n>  \t\telse\n> -\t\t\tstring_list_append(&include_packs, buf.buf);\n> +\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n\nI was briefly wondering whether we need error handling for the case\nwhere a pack is marked both as excluded and included. But we didn't have\nit beforehand, either.\n\n>  \t\tstrbuf_reset(&buf);\n>  \t}\n>  \n> -\tstring_list_sort_u(&include_packs, 0);\n> -\tstring_list_sort_u(&exclude_packs, 0);\n> -\n>  \trepo_for_each_pack(the_repository, p) {\n> -\t\tconst char *pack_name = pack_basename(p);\n> +\t\tstruct stdin_pack_info *info;\n>  \n> -\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n> +\t\tinfo = strmap_get(&packs, pack_basename(p));\n> +\t\tif (!info)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (info->kind & STDIN_PACK_INCLUDE) {\n>  \t\t\tif (exclude_promisor_objects && p->pack_promisor)\n>  \t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n> -\t\t\titem->util = p;\n> +\n> +\t\t\t/*\n> +\t\t\t * Arguments we got on stdin may not even be\n> +\t\t\t * packs. First check that to avoid segfaulting\n> +\t\t\t * later on in e.g.  pack_mtime_cmp(), excluded\n> +\t\t\t * packs are handled below.\n> +\t\t\t */\n> +\t\t\tif (!is_pack_valid(p))\n> +\t\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n\nHm. Doesn't this change behaviour though? Beforehand, we would have\nchecked the packfile for every included pack. Now we only check the\npackfile for every included pack that was yielded by\n`repo_for_each_pack()`. So if an included pack wasn't yielded at all we\nwouldn't notice that it doesn't exist?\n\nI guess an easy fix would be to mark every pack that we have processed\nas seen in the pack info, and then loop over all pack infos a second\ntime to verify that we've seen all that we expected to see.\n\nWhich you in fact already do :) That post-processing happens in\n`stdin_packs_add_pack_entries()`, where you verify that the `p` pointer\nis set as expected. And if it's not we die with a message that the pack\nwasn't found. Good.\n\nThis was a bit more demanding to review, but I very much like the\noutcome of this.\n\nPatrick\n"},{"id":"540021","messageId":"acRpruzeYfivaCs7@nand.local","threadId":"65309","inReplyTo":"acI_pTWTcJN6QaK1@pks.im","subject":"Re: [PATCH 1/5] pack-objects: plug leak in `read_stdin_packs()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:03:10Z","receivedAt":"2026-03-25T23:03:12Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 08:39:17AM +0100, Patrick Steinhardt wrote:\n> On Thu, Mar 19, 2026 at 06:24:15PM -0400, Taylor Blau wrote:\n> > The `read_stdin_packs()` function added originally via 339bce27f4f\n> > (builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)\n> > declares a `rev_info` struct but neglects to call `release_revisions()`\n> > on it before returning, creating a leak.\n> >\n> > The related change in 97ec43247c0 (pack-objects: declare 'rev_info' for\n> > '--stdin-packs' earlier, 2025-06-23) carried forward this oversight and\n> > did not address it.\n> >\n> > Ensure that we call `release_revisions()` appropriately to prevent a\n> > leak from this function.\n>\n> Would be curious to learn why none of our tests fail with this. The fix\n> looks obviously correct though, and there are no other early exits that\n> might need fixing here.\n\nI believe it's because the only memory we allocate here is in\nrevs->pending, but we copy it to old_pending in prepare_revision_walk()\nand all object_array_clear() on it.\n\nSince our traversal doesn't use any other fields of rev_info that would\ncause it to allocate memory, we don't see any leaks in our tests.\n\nI'll rewords the commit message to clarify that this is preventing the\n*potential* of a leak, not an actual leak.\n\nThanks,\nTaylor\n"},{"id":"540022","messageId":"acRsNHna6IJHQNZq@nand.local","threadId":"65309","inReplyTo":"acI_sP6ZEdw-xGpR@pks.im","subject":"Re: [PATCH 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:13:56Z","receivedAt":"2026-03-25T23:13:58Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 08:39:28AM +0100, Patrick Steinhardt wrote:\n> > Extract the logic for sorting packs by mtime and adding their objects\n> > into a separate `stdin_packs_add_entries()` helper.\n>\n> Right, the ordering was my first question. Interestingly though, that\n> function doesn't seem to be added in this commit... ah, it's called\n> `stdin_packs_add_pack_entries()`.\n\nAh, good catch. I had originally called it `stdin_packs_add_entries()`\nbut renamed it before sending, apparently without adjusting the commit\nmessage appropriately.\n\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 9a89bc5c4c9..72c9ddbed6b 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -3837,90 +3838,120 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n> >  \t\treturn 0;\n> >  }\n> >\n> > -static void read_packs_list_from_stdin(struct rev_info *revs)\n> > +struct stdin_pack_info {\n> > +\tstruct packed_git *p;\n> > +\tenum {\n> > +\t\tSTDIN_PACK_INCLUDE = (1<<0),\n> > +\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n>\n> It might make sense to provide a sentence for each of the enums to\n> explain what they do.\n\nI'm not opposed, but I am not sure what information would be helpful to\nadd here, since these correspond one-to-one with the three possible\nprefixes for packfile names we receive with --stdin-packs.\n\n> > +static void stdin_packs_add_pack_entries(struct strmap *packs,\n> > +\t\t\t\t\t struct rev_info *revs)\n> > +{\n> > +\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n> > +\tstruct string_list_item *item;\n> > +\tstruct hashmap_iter iter;\n> > +\tstruct strmap_entry *entry;\n> > +\n> > +\tstrmap_for_each_entry(packs, &iter, entry) {\n> > +\t\tstruct stdin_pack_info *info = entry->value;\n> > +\t\tif (!info->p)\n> > +\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n> > +\n> > +\t\tstring_list_append(&keys, entry->key)->util = info->p;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Order packs by ascending mtime; use QSORT directly to access the\n> > +\t * string_list_item's ->util pointer, which string_list_sort() does not\n> > +\t * provide.\n> > +\t */\n> > +\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n>\n> Okay. I was briefly wondering whether it would make more sense to use\n> `string_list_sort()`, but I guess it doesn't buy us much.\n\nYeah. This is actually carried forward from the existing implementation,\nand uses the separate QSORT() because `string_list_sort()` doesn't\nprovide access to the `util` field of the items, which we need to sort\nby mtime.\n\n> > +\tfor_each_string_list_item(item, &keys) {\n> > +\t\tstruct stdin_pack_info *info = strmap_get(packs, item->string);\n>\n> We could avoid this extra lookup if you instead were to store the pack\n> info in the `item->util` field.\n\nGood idea. Funnily enough, we already assign ->util = info->p in the\nloop above, but never use it. Something like this on top should clean\nthings up nicely:\n\n--- 8< ---\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 72c9ddbed6b..c9b33d1673d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3859,7 +3859,7 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \t\tif (!info->p)\n \t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n\n-\t\tstring_list_append(&keys, entry->key)->util = info->p;\n+\t\tstring_list_append(&keys, entry->key)->util = info;\n \t}\n\n \t/*\n@@ -3870,9 +3870,7 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n\n \tfor_each_string_list_item(item, &keys) {\n-\t\tstruct stdin_pack_info *info = strmap_get(packs, item->string);\n-\t\tif (!info->p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t\tstruct stdin_pack_info *info = item->util;\n\n \t\tif (info->kind & STDIN_PACK_INCLUDE)\n \t\t\tfor_each_object_in_pack(info->p,\n--- >8 ---\n\n> > +\t\tif (!info->p)\n> > +\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n>\n> This case basically cannot happen as we already `die()` further up,\n> right? Should we rather `BUG()` or drop the check completely?\n\nI think we should drop the check completely here, there's no way that we\nwould have a NULL 'info->p' by this point with the check that exists a\nfew lines up.\n\n> > +\t\tif (info->kind & STDIN_PACK_INCLUDE)\n> > +\t\t\tfor_each_object_in_pack(info->p,\n> > +\t\t\t\t\t\tadd_object_entry_from_pack,\n> > +\t\t\t\t\t\trevs,\n> > +\t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n> > +\t}\n> > +\n> > +\tstring_list_clear(&keys, 0);\n> > +}\n> > +\n> > +static void stdin_packs_read_input(struct rev_info *revs)\n> >  {\n> >  \tstruct strbuf buf = STRBUF_INIT;\n> > -\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n> > -\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n> > -\tstruct string_list_item *item = NULL;\n> > +\tstruct strmap packs = STRMAP_INIT;\n> >  \tstruct packed_git *p;\n> >\n> >  \twhile (strbuf_getline(&buf, stdin) != EOF) {\n> > -\t\tif (!buf.len)\n> > +\t\tstruct stdin_pack_info *info;\n> > +\t\tconst char *key = buf.buf;\n> > +\n> > +\t\tif (!key || !*key)\n>\n> The first case of `!key` cannot ever happen as strbufs always have `buf`\n> set.\n\nYou're right, this is just muscle memory, but the left-hand side of the\ncondition is unnecessary. I'll remove it.\n\n> >  \t\t\tcontinue;\n> >\n> > +\t\tif (*key == '^')\n> > +\t\t\tkey++;\n> > +\n> > +\t\tinfo = strmap_get(&packs, key);\n> > +\t\tif (!info) {\n> > +\t\t\tCALLOC_ARRAY(info, 1);\n> > +\t\t\tstrmap_put(&packs, key, info);\n> > +\t\t}\n> > +\n> >  \t\tif (*buf.buf == '^')\n> > -\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n> > +\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n> >  \t\telse\n> > -\t\t\tstring_list_append(&include_packs, buf.buf);\n> > +\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n>\n> I was briefly wondering whether we need error handling for the case\n> where a pack is marked both as excluded and included. But we didn't have\n> it beforehand, either.\n\nYeah, I think this is a consequence of 752b465c3c0 (pack-objects: fix\nerror when same packfile is included and excluded, 2023-04-14).\n\n> > [snip]\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Arguments we got on stdin may not even be\n> > +\t\t\t * packs. First check that to avoid segfaulting\n> > +\t\t\t * later on in e.g.  pack_mtime_cmp(), excluded\n> > +\t\t\t * packs are handled below.\n> > +\t\t\t */\n> > +\t\t\tif (!is_pack_valid(p))\n> > +\t\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n>\n> Hm. Doesn't this change behaviour though? Beforehand, we would have\n> checked the packfile for every included pack. Now we only check the\n> packfile for every included pack that was yielded by\n> `repo_for_each_pack()`. So if an included pack wasn't yielded at all we\n> wouldn't notice that it doesn't exist?\n>\n> I guess an easy fix would be to mark every pack that we have processed\n> as seen in the pack info, and then loop over all pack infos a second\n> time to verify that we've seen all that we expected to see.\n>\n> Which you in fact already do :) That post-processing happens in\n> `stdin_packs_add_pack_entries()`, where you verify that the `p` pointer\n> is set as expected. And if it's not we die with a message that the pack\n> wasn't found. Good.\n\nThanks for double checking.\n\n> This was a bit more demanding to review, but I very much like the\n> outcome of this.\n\nYeah, I really struggled to try and find a productive way to break this\nup into smaller changes. But in the end I couldn't find any good splits\nthat I liked, hence the larger-than-usual patch.\n\nThanks for reviewing it, I think that it makes the rest of the series a\nlittle more palatable, and the resulting code is easier to reason about\nIMHO.\n\nThanks,\nTaylor\n"},{"id":"540023","messageId":"acRtj1ZualwdKwjz@nand.local","threadId":"65309","inReplyTo":"acAwZ1ARhvsTSpO5@nand.local","subject":"Re: [PATCH 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:19:43Z","receivedAt":"2026-03-25T23:19:45Z","isPatch":true,"body":"On Sun, Mar 22, 2026 at 02:09:43PM -0400, Taylor Blau wrote:\n> It's tempting to add something like:\n>\n>     if (!p)\n>         BUG(\"add_object_entry_from_pack: expected non-NULL pack\");\n>\n> But I wonder if we should instead store the result of calling\n> `want_object_in_pack()` into a separate variable, only creating an\n> object entry if \"want == 1\". That would have the effect of *not* marking\n> objects as traversal tips if want_object_in_pack() makes `p` NULL.\n\nI ended up talking myself out of this.\n\nThere's no reason to call want_object_in_pack() early, as it may change\nthe very pack pointer we wish to use to determine the object type of the\ngiven object.\n\nOnce we have determined the object type, then we are free to call\nwant_object_in_pack() to determine if we want to add the object to the\nresulting pack.\n\nThanks,\nTaylor\n"},{"id":"540024","messageId":"cover.1774482700.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH v2 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:51:42Z","receivedAt":"2026-03-25T23:51:45Z","isPatch":true,"body":"This is a small reroll of my series to fix an issue where MIDX bitmaps\nfail to generate after a geometric repack in certain scenarios where the\nset of MIDX'd objects is not closed under reachability.\n\nThe main changes since last time are:\n\n * Clarification in the first patch that the added `release_revisions()`\n   call prevents a *potential* leak, not an actual one.\n\n * Cleanup in the second patch (where we convert the --stdin-packs\n   handling to use a strmap) based on Patrick's review.\n\n * Dropped an unnecessary \"if (p)\" conditional in the fourth patch's\n   `add_object_entry_from_pack()` callback that is unnecessary.\n\nOtherwise, the series is unchanged from the original round. As usual, a\nrange-diff is included below for convenience.\n\nThanks again for your review!\n\nTaylor Blau (5):\n  pack-objects: plug leak in `read_stdin_packs()`\n  pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`\n  t7704: demonstrate failure with once-cruft objects above the geometric\n    split\n  pack-objects: support excluded-open packs with --stdin-packs\n  repack: mark non-MIDX packs above the split as excluded-open\n\n Documentation/git-pack-objects.adoc |  25 ++-\n builtin/pack-objects.c              | 254 +++++++++++++++++++---------\n builtin/repack.c                    |  19 ++-\n packfile.c                          |   3 +-\n packfile.h                          |   2 +\n t/t5331-pack-objects-stdin.sh       | 105 ++++++++++++\n t/t7704-repack-cruft.sh             |  22 +++\n 7 files changed, 339 insertions(+), 91 deletions(-)\n\nRange-diff against v1:\n1:  1dac74f1e4a ! 1:  1fabd88f5e3 pack-objects: plug leak in `read_stdin_packs()`\n    @@ Commit message\n         The `read_stdin_packs()` function added originally via 339bce27f4f\n         (builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)\n         declares a `rev_info` struct but neglects to call `release_revisions()`\n    -    on it before returning, creating a leak.\n    +    on it before returning, creating the potential for a leak.\n     \n         The related change in 97ec43247c0 (pack-objects: declare 'rev_info' for\n         '--stdin-packs' earlier, 2025-06-23) carried forward this oversight and\n         did not address it.\n     \n         Ensure that we call `release_revisions()` appropriately to prevent a\n    -    leak from this function.\n    +    potential leak from this function. Note that in practice our `rev_info`\n    +    here does not have a present leak, hence t5331 passes cleanly before\n    +    this commit, even when built with SANITIZE=leak.\n     \n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n2:  ea6fdbcc46f ! 2:  d5cb793f0eb pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`\n    @@ Commit message\n         excluded.\n     \n         Extract the logic for sorting packs by mtime and adding their objects\n    -    into a separate `stdin_packs_add_entries()` helper.\n    +    into a separate `stdin_packs_add_pack_entries()` helper.\n     \n         While we could have used a `string_list`, we must handle the case where\n         the same pack is specified more than once. With a `string_list` only, we\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\tif (!info->p)\n     +\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n     +\n    -+\t\tstring_list_append(&keys, entry->key)->util = info->p;\n    ++\t\tstring_list_append(&keys, entry->key)->util = info;\n     +\t}\n     +\n     +\t/*\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n     +\n     +\tfor_each_string_list_item(item, &keys) {\n    -+\t\tstruct stdin_pack_info *info = strmap_get(packs, item->string);\n    -+\t\tif (!info->p)\n    -+\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n    ++\t\tstruct stdin_pack_info *info = item->util;\n     +\n     +\t\tif (info->kind & STDIN_PACK_INCLUDE)\n     +\t\t\tfor_each_object_in_pack(info->p,\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\tstruct stdin_pack_info *info;\n     +\t\tconst char *key = buf.buf;\n     +\n    -+\t\tif (!key || !*key)\n    ++\t\tif (!*key)\n      \t\t\tcontinue;\n    - \n     +\t\tif (*key == '^')\n     +\t\t\tkey++;\n     +\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\t\tCALLOC_ARRAY(info, 1);\n     +\t\t\tstrmap_put(&packs, key, info);\n     +\t\t}\n    -+\n    + \n      \t\tif (*buf.buf == '^')\n     -\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n     +\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n3:  0ec9bba92ad = 3:  d8f0577077c t7704: demonstrate failure with once-cruft objects above the geometric split\n4:  bd78919e19c ! 4:  e028dfbc9fb pack-objects: support excluded-open packs with --stdin-packs\n    @@ Commit message\n         In `add_object_entry_from_pack()`, move the `want_object_in_pack()`\n         check to *after* `add_pending_oid()`. This is necessary so that commits\n         from excluded-open packs are added as traversal tips even though their\n    -    objects won't appear in the output. The `include_check` and\n    -    `include_check_obj` callbacks on `rev_info` are used to halt the walk at\n    -    closed-excluded packs, since objects behind a '^' boundary are\n    -    guaranteed to have closure and need not be rescued.\n    +    objects won't appear in the output. As a consequence, the caller\n    +    `for_each_object_in_pack()` will always provide a non-NULL 'p', hence we\n    +    are able to drop the \"if (p)\" conditional.\n    +\n    +    The `include_check` and `include_check_obj` callbacks on `rev_info` are\n    +    used to halt the walk at closed-excluded packs, since objects behind a\n    +    '^' boundary are guaranteed to have closure and need not be rescued.\n     \n         The following commit will make use of this new functionality within the\n         repack layer to resolve the test failure demonstrated in the previous\n    @@ builtin/pack-objects.c: static int want_found_object(const struct object_id *oid\n      \t\t\t\treturn 0;\n      \t\t} else {\n     @@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct object_id *oid,\n    + \t\t\t\t      void *_data)\n    + {\n    + \toff_t ofs;\n    ++\tstruct object_info oi = OBJECT_INFO_INIT;\n    + \tenum object_type type = OBJ_NONE;\n    + \n    + \tdisplay_progress(progress_state, ++nr_seen);\n    +@@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct object_id *oid,\n    + \tif (have_duplicate_entry(oid, 0))\n      \t\treturn 0;\n      \n    ++\tstdin_packs_found_nr++;\n    ++\n      \tofs = nth_packed_object_offset(p, pos);\n    --\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n    --\t\treturn 0;\n    ++\n    ++\toi.typep = &type;\n    ++\tif (packed_object_info(p, ofs, &oi) < 0) {\n    ++\t\tdie(_(\"could not get type of object %s in pack %s\"),\n    ++\t\t    oid_to_hex(oid), p->pack_name);\n    ++\t} else if (type == OBJ_COMMIT) {\n    ++\t\tstruct rev_info *revs = _data;\n    ++\t\t/*\n    ++\t\t * commits in included packs are used as starting points\n    ++\t\t * for the subsequent revision walk\n    ++\t\t *\n    ++\t\t * Note that we do want to walk through commits that are\n    ++\t\t * present in excluded-open ('!') packs to pick up any\n    ++\t\t * objects reachable from them not present in the\n    ++\t\t * excluded-closed ('^') packs.\n    ++\t\t *\n    ++\t\t * However, we'll only add those objects to the packing\n    ++\t\t * list after checking `want_object_in_pack()` below.\n    ++\t\t */\n    ++\t\tadd_pending_oid(revs, NULL, oid, 0);\n    ++\t}\n    ++\n    + \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n    + \t\treturn 0;\n      \n    - \tif (p) {\n    - \t\tstruct object_info oi = OBJECT_INFO_INIT;\n    -@@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct object_id *oid,\n    - \t\t} else if (type == OBJ_COMMIT) {\n    - \t\t\tstruct rev_info *revs = _data;\n    - \t\t\t/*\n    +-\tif (p) {\n    +-\t\tstruct object_info oi = OBJECT_INFO_INIT;\n    +-\n    +-\t\toi.typep = &type;\n    +-\t\tif (packed_object_info(p, ofs, &oi) < 0) {\n    +-\t\t\tdie(_(\"could not get type of object %s in pack %s\"),\n    +-\t\t\t    oid_to_hex(oid), p->pack_name);\n    +-\t\t} else if (type == OBJ_COMMIT) {\n    +-\t\t\tstruct rev_info *revs = _data;\n    +-\t\t\t/*\n     -\t\t\t * commits in included packs are used as starting points for the\n     -\t\t\t * subsequent revision walk\n    -+\t\t\t * commits in included packs are used as starting points\n    -+\t\t\t * for the subsequent revision walk\n    -+\t\t\t *\n    -+\t\t\t * Note that we do want to walk through commits that are\n    -+\t\t\t * present in excluded-open ('!') packs to pick up any\n    -+\t\t\t * objects reachable from them not present in the\n    -+\t\t\t * excluded-closed ('^') packs.\n    -+\t\t\t *\n    -+\t\t\t * However, we'll only add those objects to the packing\n    -+\t\t\t * list after checking `want_object_in_pack()` below.\n    - \t\t\t */\n    - \t\t\tadd_pending_oid(revs, NULL, oid, 0);\n    - \t\t}\n    +-\t\t\t */\n    +-\t\t\tadd_pending_oid(revs, NULL, oid, 0);\n    +-\t\t}\n     -\n     -\t\tstdin_packs_found_nr++;\n    - \t}\n    - \n    -+\tif (!want_object_in_pack(oid, 0, &p, &ofs))\n    -+\t\treturn 0;\n    -+\n    -+\tstdin_packs_found_nr++;\n    -+\n    +-\t}\n    +-\n      \tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n      \n      \treturn 0;\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n      };\n      \n     @@ builtin/pack-objects.c: static void stdin_packs_add_pack_entries(struct strmap *packs,\n    - \t\tif (!info->p)\n    - \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n    + \tfor_each_string_list_item(item, &keys) {\n    + \t\tstruct stdin_pack_info *info = item->util;\n      \n     -\t\tif (info->kind & STDIN_PACK_INCLUDE)\n     +\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n    @@ builtin/pack-objects.c: static void stdin_packs_add_pack_entries(struct strmap *\n      \tstruct strbuf buf = STRBUF_INIT;\n      \tstruct strmap packs = STRMAP_INIT;\n     @@ builtin/pack-objects.c: static void stdin_packs_read_input(struct rev_info *revs)\n    - \t\tif (!key || !*key)\n    + \n    + \t\tif (!*key)\n      \t\t\tcontinue;\n    - \n     -\t\tif (*key == '^')\n     +\t\tif (*key == '^' ||\n     +\t\t    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))\n5:  c4fd52e751a = 5:  23cb9f33dba repack: mark non-MIDX packs above the split as excluded-open\n\nbase-commit: 7ff1e8dc1e1680510c96e69965b3fa81372c5037\n-- \n2.53.0.614.g164f3b634ec\n"},{"id":"540025","messageId":"1fabd88f5e3950e505bf24735bf3eae2437db7b6.1774482701.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774482700.git.me@ttaylorr.com","subject":"[PATCH v2 1/5] pack-objects: plug leak in `read_stdin_packs()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:51:48Z","receivedAt":"2026-03-25T23:51:50Z","isPatch":true,"body":"The `read_stdin_packs()` function added originally via 339bce27f4f\n(builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)\ndeclares a `rev_info` struct but neglects to call `release_revisions()`\non it before returning, creating the potential for a leak.\n\nThe related change in 97ec43247c0 (pack-objects: declare 'rev_info' for\n'--stdin-packs' earlier, 2025-06-23) carried forward this oversight and\ndid not address it.\n\nEnsure that we call `release_revisions()` appropriately to prevent a\npotential leak from this function. Note that in practice our `rev_info`\nhere does not have a present leak, hence t5331 passes cleanly before\nthis commit, even when built with SANITIZE=leak.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cd013c0b68a..9a89bc5c4c9 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3968,6 +3968,8 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \t\t\t     show_object_pack_hint,\n \t\t\t     &mode);\n \n+\trelease_revisions(&revs);\n+\n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_found\",\n \t\t\t   stdin_packs_found_nr);\n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_hints\",\n-- \n2.53.0.614.g164f3b634ec\n\n"},{"id":"540026","messageId":"d5cb793f0eb0028f1f521fec4723ad2b00592638.1774482701.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774482700.git.me@ttaylorr.com","subject":"[PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:51:50Z","receivedAt":"2026-03-25T23:51:52Z","isPatch":true,"body":"The '--stdin-packs' mode of pack-objects maintains two separate\nstring_lists: one for included packs, and one for excluded packs. Each\nlist stores the pack basename as a string and the corresponding\n`packed_git` pointer in its `->util` field.\n\nThis works, but makes it awkward to extend the set of pack \"kinds\" that\npack-objects can accept via stdin, since each new kind would need its\nown string_list and duplicated handling. A future commit will want to do\njust this, so prepare for that change by handling the various \"kinds\" of\npacks specified over stdin in a more generic fashion.\n\nNamely, replace the two `string_list`s with a single `strmap` keyed on\nthe pack basename, with values pointing to a new `struct\nstdin_pack_info`. This struct tracks both the `packed_git` pointer and a\n`kind` bitfield indicating whether the pack was specified as included or\nexcluded.\n\nExtract the logic for sorting packs by mtime and adding their objects\ninto a separate `stdin_packs_add_pack_entries()` helper.\n\nWhile we could have used a `string_list`, we must handle the case where\nthe same pack is specified more than once. With a `string_list` only, we\nwould have to pay a quadratic cost to either (a) insert elements into\ntheir sorted positions, or (b) a repeated linear search, which is\naccidentally quadratic. For that reason, use a strmap instead.\n\nThis patch does not include any functional changes.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 150 ++++++++++++++++++++++++-----------------\n 1 file changed, 89 insertions(+), 61 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 9a89bc5c4c9..068b87d2af4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -28,6 +28,7 @@\n #include \"reachable.h\"\n #include \"oid-array.h\"\n #include \"strvec.h\"\n+#include \"strmap.h\"\n #include \"list.h\"\n #include \"packfile.h\"\n #include \"object-file.h\"\n@@ -3837,90 +3838,117 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n \t\treturn 0;\n }\n \n-static void read_packs_list_from_stdin(struct rev_info *revs)\n+struct stdin_pack_info {\n+\tstruct packed_git *p;\n+\tenum {\n+\t\tSTDIN_PACK_INCLUDE = (1<<0),\n+\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+\t} kind;\n+};\n+\n+static void stdin_packs_add_pack_entries(struct strmap *packs,\n+\t\t\t\t\t struct rev_info *revs)\n+{\n+\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *entry;\n+\n+\tstrmap_for_each_entry(packs, &iter, entry) {\n+\t\tstruct stdin_pack_info *info = entry->value;\n+\t\tif (!info->p)\n+\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n+\n+\t\tstring_list_append(&keys, entry->key)->util = info;\n+\t}\n+\n+\t/*\n+\t * Order packs by ascending mtime; use QSORT directly to access the\n+\t * string_list_item's ->util pointer, which string_list_sort() does not\n+\t * provide.\n+\t */\n+\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n+\n+\tfor_each_string_list_item(item, &keys) {\n+\t\tstruct stdin_pack_info *info = item->util;\n+\n+\t\tif (info->kind & STDIN_PACK_INCLUDE)\n+\t\t\tfor_each_object_in_pack(info->p,\n+\t\t\t\t\t\tadd_object_entry_from_pack,\n+\t\t\t\t\t\trevs,\n+\t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n+\t}\n+\n+\tstring_list_clear(&keys, 0);\n+}\n+\n+static void stdin_packs_read_input(struct rev_info *revs)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *item = NULL;\n+\tstruct strmap packs = STRMAP_INIT;\n \tstruct packed_git *p;\n \n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\tif (!buf.len)\n+\t\tstruct stdin_pack_info *info;\n+\t\tconst char *key = buf.buf;\n+\n+\t\tif (!*key)\n \t\t\tcontinue;\n+\t\tif (*key == '^')\n+\t\t\tkey++;\n+\n+\t\tinfo = strmap_get(&packs, key);\n+\t\tif (!info) {\n+\t\t\tCALLOC_ARRAY(info, 1);\n+\t\t\tstrmap_put(&packs, key, info);\n+\t\t}\n \n \t\tif (*buf.buf == '^')\n-\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n+\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n \t\telse\n-\t\t\tstring_list_append(&include_packs, buf.buf);\n+\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n \n \t\tstrbuf_reset(&buf);\n \t}\n \n-\tstring_list_sort_u(&include_packs, 0);\n-\tstring_list_sort_u(&exclude_packs, 0);\n-\n \trepo_for_each_pack(the_repository, p) {\n-\t\tconst char *pack_name = pack_basename(p);\n+\t\tstruct stdin_pack_info *info;\n \n-\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n+\t\tinfo = strmap_get(&packs, pack_basename(p));\n+\t\tif (!info)\n+\t\t\tcontinue;\n+\n+\t\tif (info->kind & STDIN_PACK_INCLUDE) {\n \t\t\tif (exclude_promisor_objects && p->pack_promisor)\n \t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n-\t\t\titem->util = p;\n+\n+\t\t\t/*\n+\t\t\t * Arguments we got on stdin may not even be\n+\t\t\t * packs. First check that to avoid segfaulting\n+\t\t\t * later on in e.g.  pack_mtime_cmp(), excluded\n+\t\t\t * packs are handled below.\n+\t\t\t */\n+\t\t\tif (!is_pack_valid(p))\n+\t\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n \t\t}\n-\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n-\t\t\titem->util = p;\n-\t}\n \n-\t/*\n-\t * Arguments we got on stdin may not even be packs. First\n-\t * check that to avoid segfaulting later on in\n-\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n-\t *\n-\t * Since we first parsed our STDIN and then sorted the input\n-\t * lines the pack we error on will be whatever line happens to\n-\t * sort first. This is lazy, it's enough that we report one\n-\t * bad case here, we don't need to report the first/last one,\n-\t * or all of them.\n-\t */\n-\tfor_each_string_list_item(item, &include_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tif (!p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n-\t\tif (!is_pack_valid(p))\n-\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n-\t}\n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_CLOSED) {\n+\t\t\t/*\n+\t\t\t * Marking excluded packs as kept in-core so\n+\t\t\t * that later calls to add_object_entry()\n+\t\t\t * discards any objects that are also found in\n+\t\t\t * excluded packs.\n+\t\t\t */\n+\t\t\tp->pack_keep_in_core = 1;\n+\t\t}\n \n-\t/*\n-\t * Then, handle all of the excluded packs, marking them as\n-\t * kept in-core so that later calls to add_object_entry()\n-\t * discards any objects that are also found in excluded packs.\n-\t */\n-\tfor_each_string_list_item(item, &exclude_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tif (!p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n-\t\tp->pack_keep_in_core = 1;\n+\t\tinfo->p = p;\n \t}\n \n-\t/*\n-\t * Order packs by ascending mtime; use QSORT directly to access the\n-\t * string_list_item's ->util pointer, which string_list_sort() does not\n-\t * provide.\n-\t */\n-\tQSORT(include_packs.items, include_packs.nr, pack_mtime_cmp);\n-\n-\tfor_each_string_list_item(item, &include_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tfor_each_object_in_pack(p,\n-\t\t\t\t\tadd_object_entry_from_pack,\n-\t\t\t\t\trevs,\n-\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n-\t}\n+\tstdin_packs_add_pack_entries(&packs, revs);\n \n \tstrbuf_release(&buf);\n-\tstring_list_clear(&include_packs, 0);\n-\tstring_list_clear(&exclude_packs, 0);\n+\tstrmap_clear(&packs, 1);\n }\n \n static void add_unreachable_loose_objects(struct rev_info *revs);\n@@ -3957,7 +3985,7 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n-\tread_packs_list_from_stdin(&revs);\n+\tstdin_packs_read_input(&revs);\n \tif (rev_list_unpacked)\n \t\tadd_unreachable_loose_objects(&revs);\n \n-- \n2.53.0.614.g164f3b634ec\n\n"},{"id":"540027","messageId":"d8f0577077cf699032292b7f49c7636c43ca3af1.1774482701.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774482700.git.me@ttaylorr.com","subject":"[PATCH v2 3/5] t7704: demonstrate failure with once-cruft objects above the geometric split","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:51:53Z","receivedAt":"2026-03-25T23:51:55Z","isPatch":true,"body":"Add a test demonstrating a case where geometric repacking fails to\nproduce a pack with full object closure, thus making it impossible to\nwrite a reachability bitmap.\n\nMark the test with 'test_expect_failure' for now. The subsequent commit\nwill explain the precise failure mode, and implement a fix.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t7704-repack-cruft.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex aa2e2e6ad88..77133395b5d 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -869,4 +869,26 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'\n \t)\n '\n \n+test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n+\tgit config repack.midxMustContainCruft false &&\n+\n+\ttest_commit reachable &&\n+\ttest_commit unreachable &&\n+\n+\tunreachable=\"$(git rev-parse HEAD)\" &&\n+\n+\tgit reset --hard HEAD^ &&\n+\tgit tag -d unreachable &&\n+\tgit reflog expire --all --expire=all &&\n+\n+\tgit repack --cruft -d &&\n+\n+\techo $unreachable | git pack-objects .git/objects/pack/pack &&\n+\n+\ttest_commit new &&\n+\n+\tgit update-ref refs/heads/other $unreachable &&\n+\tgit repack --geometric=2 -d --write-midx --write-bitmap-index\n+'\n+\n test_done\n-- \n2.53.0.614.g164f3b634ec\n\n"},{"id":"540028","messageId":"e028dfbc9fb5f53d706b1cfb8ee0759b6f1c4575.1774482701.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774482700.git.me@ttaylorr.com","subject":"[PATCH v2 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:51:56Z","receivedAt":"2026-03-25T23:51:58Z","isPatch":true,"body":"In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n2025-06-23), pack-objects learned to traverse through commits in\nincluded packs when using '--stdin-packs=follow', rescuing reachable\nobjects from unlisted packs into the output.\n\nWhen we encounter a commit in an excluded pack during this rescuing\nphase we will traverse through its parents. But because we set\n`revs.no_kept_objects = 1`, commit simplification will prevent us from\nshowing it via `get_revision()`. (In practice, `--stdin-packs=follow`\nwalks commits down to the roots, but only opens up trees for ones that\ndo not appear in an excluded pack.)\n\nBut there are certain cases where we *do* need to see the parents of an\nobject in an excluded pack. Namely, if an object is rescue-able, but\nonly reachable from object(s) which appear in excluded packs, then\ncommit simplification will exclude those commits from the object\ntraversal, and we will never see a copy of that object, and thus not\nrescue it.\n\nThis is what causes the failure in the previous commit during repacking.\nWhen performing a geometric repack, packs above the geometric split that\nweren't part of the previous MIDX (e.g., packs pushed directly into\n`$GIT_DIR/objects/pack`) may not have full object closure.  When those\npacks are listed as excluded via the '^' marker, the reachability\ntraversal encounters the sequence described above, and may miss objects\nwhich we expect to rescue with `--stdin-packs=follow`.\n\nIntroduce a new \"excluded-open\" pack prefix, '!'. Like '^'-prefixed\npacks, objects from '!'-prefixed packs are excluded from the resulting\npack. But unlike '^', commits in '!'-prefixed packs *are* used as\nstarting points for the follow traversal, and the traversal does not\ntreat them as a closure boundary.\n\nIn order to distinguish excluded-closed from excluded-open packs during\nthe traversal, introduce a new `pack_keep_in_core_open` bit on\n`struct packed_git`, along with a corresponding `KEPT_PACK_IN_CORE_OPEN`\nflag for the kept-pack cache.\n\nIn `add_object_entry_from_pack()`, move the `want_object_in_pack()`\ncheck to *after* `add_pending_oid()`. This is necessary so that commits\nfrom excluded-open packs are added as traversal tips even though their\nobjects won't appear in the output. As a consequence, the caller\n`for_each_object_in_pack()` will always provide a non-NULL 'p', hence we\nare able to drop the \"if (p)\" conditional.\n\nThe `include_check` and `include_check_obj` callbacks on `rev_info` are\nused to halt the walk at closed-excluded packs, since objects behind a\n'^' boundary are guaranteed to have closure and need not be rescued.\n\nThe following commit will make use of this new functionality within the\nrepack layer to resolve the test failure demonstrated in the previous\ncommit.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-pack-objects.adoc |  25 +++++--\n builtin/pack-objects.c              | 110 ++++++++++++++++++++++------\n packfile.c                          |   3 +-\n packfile.h                          |   2 +\n t/t5331-pack-objects-stdin.sh       | 105 ++++++++++++++++++++++++++\n 5 files changed, 213 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 71b9682485c..b78175fbe1b 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -94,13 +94,24 @@ base-name::\n \tincluded packs (those not beginning with `^`), excluding any\n \tobjects listed in the excluded packs (beginning with `^`).\n +\n-When `mode` is \"follow\", objects from packs not listed on stdin receive\n-special treatment. Objects within unlisted packs will be included if\n-those objects are (1) reachable from the included packs, and (2) not\n-found in any excluded packs. This mode is useful, for example, to\n-resurrect once-unreachable objects found in cruft packs to generate\n-packs which are closed under reachability up to the boundary set by the\n-excluded packs.\n+When `mode` is \"follow\" packs may additionally be prefixed with `!`,\n+indicating that they are excluded but not necessarily closed under\n+reachability.  In addition to objects in included packs, the resulting\n+pack may include additional objects based on the following:\n++\n+--\n+* If any packs are marked with `!`, then objects reachable from such\n+  packs or included ones via objects outside of excluded-closed packs\n+  will be included. In this case, all `^` packs are treated as closed\n+  under reachability.\n+* Otherwise (if there are no `!` packs), objects within unlisted packs\n+  will be included if those objects are (1) reachable from the\n+  included packs, and (2) not found in any excluded packs.\n+--\n++\n+This mode is useful, for example, to resurrect once-unreachable\n+objects found in cruft packs to generate packs which are closed under\n+reachability up to the boundary set by the excluded packs.\n +\n Incompatible with `--revs`, or options that imply `--revs` (such as\n `--all`), with the exception of `--unpacked`, which is compatible.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 068b87d2af4..b8f5b9bf718 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -217,6 +217,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_open;\n static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n@@ -1618,7 +1619,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t/*\n \t * Then handle .keep first, as we have a fast(er) path there.\n \t */\n-\tif (ignore_packed_keep_on_disk || ignore_packed_keep_in_core) {\n+\tif (ignore_packed_keep_on_disk || ignore_packed_keep_in_core ||\n+\t    ignore_packed_keep_in_core_open) {\n \t\t/*\n \t\t * Set the flags for the kept-pack cache to be the ones we want\n \t\t * to ignore.\n@@ -1632,6 +1634,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\t\tflags |= KEPT_PACK_ON_DISK;\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= KEPT_PACK_IN_CORE;\n+\t\tif (ignore_packed_keep_in_core_open)\n+\t\t\tflags |= KEPT_PACK_IN_CORE_OPEN;\n \n \t\t/*\n \t\t * If the object is in a pack that we want to ignore, *and* we\n@@ -1643,6 +1647,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\t\t\treturn 0;\n \t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n \t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core_open && p->pack_keep_in_core_open)\n+\t\t\t\treturn 0;\n \t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n \t\t\t\treturn 0;\n \t\t} else {\n@@ -3742,6 +3748,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t\t\t      void *_data)\n {\n \toff_t ofs;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type type = OBJ_NONE;\n \n \tdisplay_progress(progress_state, ++nr_seen);\n@@ -3749,29 +3756,34 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \tif (have_duplicate_entry(oid, 0))\n \t\treturn 0;\n \n+\tstdin_packs_found_nr++;\n+\n \tofs = nth_packed_object_offset(p, pos);\n+\n+\toi.typep = &type;\n+\tif (packed_object_info(p, ofs, &oi) < 0) {\n+\t\tdie(_(\"could not get type of object %s in pack %s\"),\n+\t\t    oid_to_hex(oid), p->pack_name);\n+\t} else if (type == OBJ_COMMIT) {\n+\t\tstruct rev_info *revs = _data;\n+\t\t/*\n+\t\t * commits in included packs are used as starting points\n+\t\t * for the subsequent revision walk\n+\t\t *\n+\t\t * Note that we do want to walk through commits that are\n+\t\t * present in excluded-open ('!') packs to pick up any\n+\t\t * objects reachable from them not present in the\n+\t\t * excluded-closed ('^') packs.\n+\t\t *\n+\t\t * However, we'll only add those objects to the packing\n+\t\t * list after checking `want_object_in_pack()` below.\n+\t\t */\n+\t\tadd_pending_oid(revs, NULL, oid, 0);\n+\t}\n+\n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n \t\treturn 0;\n \n-\tif (p) {\n-\t\tstruct object_info oi = OBJECT_INFO_INIT;\n-\n-\t\toi.typep = &type;\n-\t\tif (packed_object_info(p, ofs, &oi) < 0) {\n-\t\t\tdie(_(\"could not get type of object %s in pack %s\"),\n-\t\t\t    oid_to_hex(oid), p->pack_name);\n-\t\t} else if (type == OBJ_COMMIT) {\n-\t\t\tstruct rev_info *revs = _data;\n-\t\t\t/*\n-\t\t\t * commits in included packs are used as starting points for the\n-\t\t\t * subsequent revision walk\n-\t\t\t */\n-\t\t\tadd_pending_oid(revs, NULL, oid, 0);\n-\t\t}\n-\n-\t\tstdin_packs_found_nr++;\n-\t}\n-\n \tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n \n \treturn 0;\n@@ -3838,11 +3850,23 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n \t\treturn 0;\n }\n \n+static int stdin_packs_include_check_obj(struct object *obj, void *data UNUSED)\n+{\n+\treturn !has_object_kept_pack(to_pack.repo, &obj->oid,\n+\t\t\t\t     KEPT_PACK_IN_CORE);\n+}\n+\n+static int stdin_packs_include_check(struct commit *commit, void *data)\n+{\n+\treturn stdin_packs_include_check_obj((struct object *)commit, data);\n+}\n+\n struct stdin_pack_info {\n \tstruct packed_git *p;\n \tenum {\n \t\tSTDIN_PACK_INCLUDE = (1<<0),\n \t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+\t\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n \t} kind;\n };\n \n@@ -3872,7 +3896,19 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \tfor_each_string_list_item(item, &keys) {\n \t\tstruct stdin_pack_info *info = item->util;\n \n-\t\tif (info->kind & STDIN_PACK_INCLUDE)\n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n+\t\t\t/*\n+\t\t\t * When open-excluded packs (\"!\") are present, stop\n+\t\t\t * the parent walk at closed-excluded (\"^\") packs.\n+\t\t\t * Objects behind a \"^\" boundary are guaranteed to\n+\t\t\t * have closure and should not be rescued.\n+\t\t\t */\n+\t\t\trevs->include_check = stdin_packs_include_check;\n+\t\t\trevs->include_check_obj = stdin_packs_include_check_obj;\n+\t\t}\n+\n+\t\tif ((info->kind & STDIN_PACK_INCLUDE) ||\n+\t\t    (info->kind & STDIN_PACK_EXCLUDE_OPEN))\n \t\t\tfor_each_object_in_pack(info->p,\n \t\t\t\t\t\tadd_object_entry_from_pack,\n \t\t\t\t\t\trevs,\n@@ -3882,7 +3918,8 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \tstring_list_clear(&keys, 0);\n }\n \n-static void stdin_packs_read_input(struct rev_info *revs)\n+static void stdin_packs_read_input(struct rev_info *revs,\n+\t\t\t\t   enum stdin_packs_mode mode)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strmap packs = STRMAP_INIT;\n@@ -3894,7 +3931,8 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \n \t\tif (!*key)\n \t\t\tcontinue;\n-\t\tif (*key == '^')\n+\t\tif (*key == '^' ||\n+\t\t    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))\n \t\t\tkey++;\n \n \t\tinfo = strmap_get(&packs, key);\n@@ -3905,6 +3943,8 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \n \t\tif (*buf.buf == '^')\n \t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n+\t\telse if (*buf.buf == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n+\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_OPEN;\n \t\telse\n \t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n \n@@ -3942,6 +3982,20 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \t\t\tp->pack_keep_in_core = 1;\n \t\t}\n \n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n+\t\t\t/*\n+\t\t\t * Marking excluded open packs as kept in-core\n+\t\t\t * (open) for the same reason as we marked\n+\t\t\t * exclude closed packs as kept in-core.\n+\t\t\t *\n+\t\t\t * Use a separate flag here to ensure we don't\n+\t\t\t * halt our traversal at these packs, since they\n+\t\t\t * are not guaranteed to have closure.\n+\t\t\t *\n+\t\t\t */\n+\t\t\tp->pack_keep_in_core_open = 1;\n+\t\t}\n+\n \t\tinfo->p = p;\n \t}\n \n@@ -3985,7 +4039,15 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n-\tstdin_packs_read_input(&revs);\n+\tif (mode == STDIN_PACKS_MODE_FOLLOW) {\n+\t\t/*\n+\t\t * In '--stdin-packs=follow' mode, additionally ignore\n+\t\t * objects in excluded-open packs to prevent them from\n+\t\t * appearing in the resulting pack.\n+\t\t */\n+\t\tignore_packed_keep_in_core_open = 1;\n+\t}\n+\tstdin_packs_read_input(&revs, mode);\n \tif (rev_list_unpacked)\n \t\tadd_unreachable_loose_objects(&revs);\n \ndiff --git a/packfile.c b/packfile.c\nindex 215a23e42be..076e444e32a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2246,7 +2246,8 @@ struct packed_git **packfile_store_get_kept_pack_cache(struct packfile_store *st\n \t\t\tstruct packed_git *p = e->pack;\n \n \t\t\tif ((p->pack_keep && (flags & KEPT_PACK_ON_DISK)) ||\n-\t\t\t    (p->pack_keep_in_core && (flags & KEPT_PACK_IN_CORE))) {\n+\t\t\t    (p->pack_keep_in_core && (flags & KEPT_PACK_IN_CORE)) ||\n+\t\t\t    (p->pack_keep_in_core_open && (flags & KEPT_PACK_IN_CORE_OPEN))) {\n \t\t\t\tALLOC_GROW(packs, nr + 1, alloc);\n \t\t\t\tpacks[nr++] = p;\n \t\t\t}\ndiff --git a/packfile.h b/packfile.h\nindex 8b04a258a7b..b7735c1977d 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -28,6 +28,7 @@ struct packed_git {\n \tunsigned pack_local:1,\n \t\t pack_keep:1,\n \t\t pack_keep_in_core:1,\n+\t\t pack_keep_in_core_open:1,\n \t\t freshened:1,\n \t\t do_not_close:1,\n \t\t pack_promisor:1,\n@@ -266,6 +267,7 @@ int packfile_store_freshen_object(struct packfile_store *store,\n enum kept_pack_type {\n \tKEPT_PACK_ON_DISK = (1 << 0),\n \tKEPT_PACK_IN_CORE = (1 << 1),\n+\tKEPT_PACK_IN_CORE_OPEN = (1 << 2),\n };\n \n /*\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 7eb79bc2cdb..c74b5861af3 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -415,4 +415,109 @@ test_expect_success '--stdin-packs=follow tolerates missing commits' '\n \tstdin_packs__follow_with_only HEAD HEAD^{tree}\n '\n \n+test_expect_success '--stdin-packs=follow with open-excluded packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\t# Create the following commit structure:\n+\t\t#\n+\t\t#   A <-- B <-- D     (main)\n+\t\t#         ^\n+\t\t#          \\\n+\t\t#           C        (other)\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\tgit checkout -B other &&\n+\t\ttest_commit C &&\n+\t\tgit checkout main &&\n+\t\ttest_commit D &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tD=\"$(echo B..D | git pack-objects --revs $packdir/pack)\" &&\n+\n+\t\tC_ONLY=\"$(git rev-parse other | git pack-objects $packdir/pack)\" &&\n+\n+\t\tgit prune-packed &&\n+\n+\t\t# Create a pack using --stdin-packs=follow where:\n+\t\t#\n+\t\t#  - pack D is included,\n+\t\t#  - pack C_ONLY is excluded, but open,\n+\t\t#  - pack B is excluded, but closed, and\n+\t\t#  - packs A and C are unknown\n+\t\t#\n+\t\t# The resulting pack should therefore contain:\n+\t\t#\n+\t\t#  - objects from the included pack D,\n+\t\t#  - A.t (rescued via D^{tree}), and\n+\t\t#  - C^{tree} and C.t (rescued via pack C_ONLY)\n+\t\t#\n+\t\t# , but should omit:\n+\t\t#\n+\t\t#  - C (excluded via C_ONLY),\n+\t\t#  - objects from pack B (trivially excluded-closed)\n+\t\t#  - A and A^{tree} (ancestors of B)\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack <<-EOF\n+\t\tpack-$D.pack\n+\t\t!pack-$C_ONLY.pack\n+\t\t^pack-$B.pack\n+\t\tEOF\n+\t\t) &&\n+\n+\t\t{\n+\t\t\tobjects_in_packs $D &&\n+\t\t\tgit rev-parse A:A.t \"C^{tree}\" C:C.t\n+\t\t} >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with !-delimited pack without follow' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\n+\t\tcat >in <<-EOF &&\n+\t\t!pack-$A.pack\n+\t\tpack-$B.pack\n+\t\tpack-$C.pack\n+\t\tEOF\n+\n+\t\t# Without --stdin-packs=follow, we treat the first\n+\t\t# line of input as a literal packfile name, and thus\n+\t\t# expect pack-objects to complain of a missing pack\n+\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n+\t\t\t>/dev/null <in 2>err &&\n+\t\ttest_grep \"could not find pack .!pack-$A.pack.\" err &&\n+\n+\t\t# With --stdin-packs=follow, we treat the second line\n+\t\t# of input as indicating pack-$A.pack is an excluded\n+\t\t# open pack, and thus expect pack-objects to succeed\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&\n+\n+\t\tobjects_in_packs $B $C >expect &&\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.53.0.614.g164f3b634ec\n\n"},{"id":"540029","messageId":"23cb9f33dbac735feeb4fa9b5e7676ab871e2c94.1774482701.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774482700.git.me@ttaylorr.com","subject":"[PATCH v2 5/5] repack: mark non-MIDX packs above the split as excluded-open","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-25T23:51:58Z","receivedAt":"2026-03-25T23:52:00Z","isPatch":true,"body":"In 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where\npossible, 2025-06-23), geometric repacking learned to exclude cruft\npacks from the MIDX when 'repack.midxMustContainCruft' is set to\n'false'.\n\nThis works because packs generated with '--stdin-packs=follow' rescue\nany once-unreachable objects that later become reachable, making the\nresulting packs closed under reachability without needing the cruft pack\nin the MIDX.\n\nHowever, packs above the geometric split that were not part of the\nprevious MIDX may not have full object closure.  When such packs are\nmarked as excluded-closed ('^'), pack-objects treats them as a\nreachability boundary and does not traverse through them during the\nfollow pass, potentially leaving the resulting pack without full\nclosure.\n\nFix this by marking packs above the geometric split that were not in the\nprevious MIDX as excluded-open ('!') instead of excluded-closed ('^').\nThis causes pack-objects to walk through their commits during the follow\npass, rescuing any reachable objects not present in the closed-excluded\npacks.\n\nNote that MIDXs which were generated prior to this change and are\nunlucky enough to not be closed under reachability may still exhibit\nthis bug, as we treat all MIDX'd packs as closed. That is true in an\noverwhelming number of cases, since in order to have a non-closed MIDX\nyou would have to:\n\n - Generate a pack via an earlier geometric repack that is not closed\n   under reachability.\n\n - Store that pack in the MIDX.\n\n - Avoid picking any commits to receive reachability bitmaps which\n   happen to reach objects from which the missing objects are reachable.\n\nIn the extremely rare chance that all of the above should happen, an\nall-into-one repack will resolve the issue.\n\nUnfortunately, there is no perfect way to determine whether a MIDX'd\npack is closed outside of ensuring that there is a '1' bit in at least\none bitmap for every bit position corresponding to objects in that pack.\nWhile this is possible to do, this approach would treat MIDX'd packs as\nopen in cases where there is at least one object that is not reachable\nfrom the subset of commits selected for bitmapping.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c        | 19 +++++++++++++++++--\n t/t7704-repack-cruft.sh |  2 +-\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f6bb04bef72..4c5a82c2c8d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -369,8 +369,23 @@ int cmd_repack(int argc,\n \t\t */\n \t\tfor (i = 0; i < geometry.split; i++)\n \t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n+\t\tfor (i = geometry.split; i < geometry.pack_nr; i++) {\n+\t\t\tconst char *basename = pack_basename(geometry.pack[i]);\n+\t\t\tchar marker = '^';\n+\n+\t\t\tif (!midx_must_contain_cruft &&\n+\t\t\t    !string_list_has_string(&existing.midx_packs,\n+\t\t\t\t\t\t    basename)) {\n+\t\t\t\t/*\n+\t\t\t\t * Assume non-MIDX'd packs are not\n+\t\t\t\t * necessarily closed under\n+\t\t\t\t * reachability.\n+\t\t\t\t */\n+\t\t\t\tmarker = '!';\n+\t\t\t}\n+\n+\t\t\tfprintf(in, \"%c%s\\n\", marker, basename);\n+\t\t}\n \t\tfclose(in);\n \t}\n \ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 77133395b5d..9e03b04315d 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -869,7 +869,7 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'\n \t)\n '\n \n-test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n+test_expect_success 'repack rescues once-cruft objects above geometric split' '\n \tgit config repack.midxMustContainCruft false &&\n \n \ttest_commit reachable &&\n-- \n2.53.0.614.g164f3b634ec\n"},{"id":"540136","messageId":"9e320604-7367-4f48-a943-f7d22feb2672@gmail.com","threadId":"65309","inReplyTo":"d5cb793f0eb0028f1f521fec4723ad2b00592638.1774482701.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-26T20:40:00Z","receivedAt":"2026-03-26T20:40:02Z","isPatch":true,"body":"On 3/25/2026 7:51 PM, Taylor Blau wrote:\n\n> -static void read_packs_list_from_stdin(struct rev_info *revs)\n> +struct stdin_pack_info {\n> +\tstruct packed_git *p;\n> +\tenum {\n> +\t\tSTDIN_PACK_INCLUDE = (1<<0),\n> +\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n> +\t} kind;\n> +};\n\nI kind of wish this enum wasn't anonymous. And it matters later.\nLet's call this 'enum pack_input_kind' for now.\n\n> +static void stdin_packs_read_input(struct rev_info *revs)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n> -\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n> -\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n> -\tstruct string_list_item *item = NULL;\n> +\tstruct strmap packs = STRMAP_INIT;\n>  \tstruct packed_git *p;\n>  \n>  \twhile (strbuf_getline(&buf, stdin) != EOF) {\n> -\t\tif (!buf.len)\n> +\t\tstruct stdin_pack_info *info;\n> +\t\tconst char *key = buf.buf;\n> +\n> +\t\tif (!*key)\n>  \t\t\tcontinue;\n> +\t\tif (*key == '^')\n> +\t\t\tkey++;...\n>  \t\tif (*buf.buf == '^')\n> -\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n> +\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n>  \t\telse\n> -\t\t\tstring_list_append(&include_packs, buf.buf);\n> +\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n>  \n\nIt took me a while to figure out what was going on with checking\n*key == '^' and later checking *buf.buf == '^'. We should probably\ncombine them to the same condition:\n\n\tconst char *key = buf.buf;\n\tenum pack_input_kind kind = STDIN_PACK_INCLUDE;\n\n\tif (*key == '^') {\n\t\tkey++;\n\t\tkind |= STDIN_PACK_EXCLUDE_CLOSED;\n\t}\n\n\tinfo = strmap_get(&packs, key);\n\tif (!info) {\n\t\tCALLOC_ARRAY(info, 1);\n\t\tstrmap_put(&packs, key, info);\n\t\tinfo->kind = kind;\n\t}\n\n\tstrbuf_reset(&buf);\n\nThis feels easier to read, for me.\n\n>  \t\tstrbuf_reset(&buf);\n>  \t}\n>  \n> -\tstring_list_sort_u(&include_packs, 0);\n> -\tstring_list_sort_u(&exclude_packs, 0);\n> -\n>  \trepo_for_each_pack(the_repository, p) {\n> -\t\tconst char *pack_name = pack_basename(p);\n> +\t\tstruct stdin_pack_info *info;\n>  \n> -\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n> +\t\tinfo = strmap_get(&packs, pack_basename(p));\n> +\t\tif (!info)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (info->kind & STDIN_PACK_INCLUDE) {\n...\n>  \t\t}\n\n> +\t\tif (info->kind & STDIN_PACK_EXCLUDE_CLOSED) {\n\nThis does help confirm that a pack could be in both categories, so\nusing flag bits helps.\n\nThanks,\n-Stolee\n"},{"id":"540138","messageId":"124c202b-9641-4445-bb38-9cd70c836844@gmail.com","threadId":"65309","inReplyTo":"e028dfbc9fb5f53d706b1cfb8ee0759b6f1c4575.1774482701.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-26T20:48:07Z","receivedAt":"2026-03-26T20:48:10Z","isPatch":true,"body":"On 3/25/2026 7:51 PM, Taylor Blau wrote:\n\n>  struct stdin_pack_info {\n>  \tstruct packed_git *p;\n>  \tenum {\n>  \t\tSTDIN_PACK_INCLUDE = (1<<0),\n>  \t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n> +\t\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n>  \t} kind;\n\nEspecially because we're extending this, I feel more confident\nthat this would improve by being a named enum. Perhaps these\nmodes could get comments explaining their differences and how\nthey might operate in combination.\n\n>  \t\tif (!*key)\n>  \t\t\tcontinue;\n> -\t\tif (*key == '^')\n> +\t\tif (*key == '^' ||\n> +\t\t    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))\n>  \t\t\tkey++;\n\nThis part is getting more complicated, giving potentially more\nreason to start with a single branching point in patch 2.\n\n> diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\n> index 7eb79bc2cdb..c74b5861af3 100755\n> --- a/t/t5331-pack-objects-stdin.sh\n> +++ b/t/t5331-pack-objects-stdin.sh\n> @@ -415,4 +415,109 @@ test_expect_success '--stdin-packs=follow tolerates missing commits' '\n>  \tstdin_packs__follow_with_only HEAD HEAD^{tree}\n>  '\n\nI see that this isn't yet the fix that turns failure into success.\nI look forward to seeing that changeover.\n\nThanks,\n-Stolee\n\n"},{"id":"540139","messageId":"2b1a7624-d9cc-48b1-a224-646cafabb359@gmail.com","threadId":"65309","inReplyTo":"23cb9f33dbac735feeb4fa9b5e7676ab871e2c94.1774482701.git.me@ttaylorr.com","subject":"Re: [PATCH v2 5/5] repack: mark non-MIDX packs above the split as excluded-open","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-26T20:49:40Z","receivedAt":"2026-03-26T20:49:42Z","isPatch":true,"body":"On 3/25/2026 7:51 PM, Taylor Blau wrote:\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index f6bb04bef72..4c5a82c2c8d 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -369,8 +369,23 @@ int cmd_repack(int argc,\n>  \t\t */\n>  \t\tfor (i = 0; i < geometry.split; i++)\n>  \t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n> -\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n> -\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n> +\t\tfor (i = geometry.split; i < geometry.pack_nr; i++) {\n> +\t\t\tconst char *basename = pack_basename(geometry.pack[i]);\n> +\t\t\tchar marker = '^';\n> +\n> +\t\t\tif (!midx_must_contain_cruft &&\n> +\t\t\t    !string_list_has_string(&existing.midx_packs,\n> +\t\t\t\t\t\t    basename)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Assume non-MIDX'd packs are not\n> +\t\t\t\t * necessarily closed under\n> +\t\t\t\t * reachability.\n> +\t\t\t\t */\n> +\t\t\t\tmarker = '!';\n> +\t\t\t}\n> +\n> +\t\t\tfprintf(in, \"%c%s\\n\", marker, basename);\n> +\t\t}\n>  \t\tfclose(in);\n\n> -test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n> +test_expect_success 'repack rescues once-cruft objects above geometric split' '\n\nI appreciate the brevity of this behavior change after you\nestablished the new building blocks that make such a\nconcise change possible.\n\nThanks,\n-Stolee\n\n"},{"id":"540140","messageId":"4511ea3d-35b0-4a62-8dac-250a86c0e0f4@gmail.com","threadId":"65309","inReplyTo":"cover.1774482700.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-26T20:51:09Z","receivedAt":"2026-03-26T20:51:12Z","isPatch":true,"body":"On 3/25/2026 7:51 PM, Taylor Blau wrote:\n> This is a small reroll of my series to fix an issue where MIDX bitmaps\n> fail to generate after a geometric repack in certain scenarios where the\n> set of MIDX'd objects is not closed under reachability.\n> \n> The main changes since last time are:\n> \n>  * Clarification in the first patch that the added `release_revisions()`\n>    call prevents a *potential* leak, not an actual one.\n> \n>  * Cleanup in the second patch (where we convert the --stdin-packs\n>    handling to use a strmap) based on Patrick's review.\n> \n>  * Dropped an unnecessary \"if (p)\" conditional in the fourth patch's\n>    `add_object_entry_from_pack()` callback that is unnecessary.\n> \n> Otherwise, the series is unchanged from the original round. As usual, a\n> range-diff is included below for convenience.\n> \n\nSorry I didn't get to v1 in time. This was an interesting series\nand fixes a bug well. My only quibbles are about some minor code\nstyle things, but I do hope you'll consider them. I struggled to\nread a few things and the changes I recommend seemed to make the\nlogic more obvious.\n\nThanks,\n-Stolee\n\n\n"},{"id":"540145","messageId":"acWoqXUwVUB2/65T@nand.local","threadId":"65309","inReplyTo":"9e320604-7367-4f48-a943-f7d22feb2672@gmail.com","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-26T21:44:09Z","receivedAt":"2026-03-26T21:44:11Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 04:40:00PM -0400, Derrick Stolee wrote:\n> On 3/25/2026 7:51 PM, Taylor Blau wrote:\n>\n> > -static void read_packs_list_from_stdin(struct rev_info *revs)\n> > +struct stdin_pack_info {\n> > +\tstruct packed_git *p;\n> > +\tenum {\n> > +\t\tSTDIN_PACK_INCLUDE = (1<<0),\n> > +\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n> > +\t} kind;\n> > +};\n>\n> I kind of wish this enum wasn't anonymous. And it matters later.\n> Let's call this 'enum pack_input_kind' for now.\n\nHmm. I don't feel strongly about this, but I'm not sure I follow the\nreasoning here. The enum is truly only meant to be used within the\ncontext of a stdin_pack_info struct, so it felt natural to keep it\nanonymous above.\n\nI'm happy to change this if you feel strongly about it, but TBH I am not\nsure I see the benefit of doing so.\n\n> It took me a while to figure out what was going on with checking\n> *key == '^' and later checking *buf.buf == '^'. We should probably\n> combine them to the same condition:\n>\n> \tconst char *key = buf.buf;\n> \tenum pack_input_kind kind = STDIN_PACK_INCLUDE;\n>\n> \tif (*key == '^') {\n> \t\tkey++;\n> \t\tkind |= STDIN_PACK_EXCLUDE_CLOSED;\n> \t}\n>\n> \tinfo = strmap_get(&packs, key);\n> \tif (!info) {\n> \t\tCALLOC_ARRAY(info, 1);\n> \t\tstrmap_put(&packs, key, info);\n> \t\tinfo->kind = kind;\n> \t}\n>\n> \tstrbuf_reset(&buf);\n>\n> This feels easier to read, for me.\n\nI agree that the above is a little easier to read, but I'm not sure it\nhandles the case of specifying the same pack multiple times. I had\noriginally written it in a similar way as what you suggested above, but\nit breaks if I write something like:\n\n    cat <<EOF | git pack-objects --stdin\n    pack-XYZ.pack\n    ^pack-XYZ.pack\n    EOF\n\nIt should produce a pack with no objects, but I think the code above\nwould effectively ignore the second line because we already have a\nstrmap entry for pack-XYZ.pack so we never set the additional flag bits.\n\nThanks,\nTaylor\n"},{"id":"540146","messageId":"acWoxy1NoDjI+J4Z@nand.local","threadId":"65309","inReplyTo":"2b1a7624-d9cc-48b1-a224-646cafabb359@gmail.com","subject":"Re: [PATCH v2 5/5] repack: mark non-MIDX packs above the split as excluded-open","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-26T21:44:39Z","receivedAt":"2026-03-26T21:44:41Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 04:49:40PM -0400, Derrick Stolee wrote:\n> I appreciate the brevity of this behavior change after you\n> established the new building blocks that make such a\n> concise change possible.\n\n;-).\n\nThanks,\nTaylor\n"},{"id":"540147","messageId":"acWpT2POwnfI2Yzn@nand.local","threadId":"65309","inReplyTo":"4511ea3d-35b0-4a62-8dac-250a86c0e0f4@gmail.com","subject":"Re: [PATCH v2 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-26T21:46:55Z","receivedAt":"2026-03-26T21:46:57Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 04:51:09PM -0400, Derrick Stolee wrote:\n> Sorry I didn't get to v1 in time. This was an interesting series\n> and fixes a bug well. My only quibbles are about some minor code\n> style things, but I do hope you'll consider them. I struggled to\n> read a few things and the changes I recommend seemed to make the\n> logic more obvious.\n\nThanks for reviewing! The two main comments I picked up from you were:\n\n * whether or not the new enum should be non-anonymous\n * changing how we lookup entries in the strmap when processing input\n\nOn the latter of those two, I think that the suggestion you made would\nbreak one of the test cases around handling the same pack being\nspecified multiple times with different flags[^1], so I opted to keep\nthe current logic there.\n\nOn the former, I don't feel strongly either way. If you do, I'm happy to\nmake that change, but if not I think the series should be good to go\nas-is.\n\nThanks,\nTaylor\n\n[^1]: I wish this weren't the case, but this is my fault for not\n  forbidding it when I initially wrote this feature.\n"},{"id":"540148","messageId":"xmqq8qbensw5.fsf@gitster.g","threadId":"65309","inReplyTo":"acWoqXUwVUB2/65T@nand.local","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-26T22:11:06Z","receivedAt":"2026-03-26T22:11:09Z","isPatch":true,"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Thu, Mar 26, 2026 at 04:40:00PM -0400, Derrick Stolee wrote:\n>> On 3/25/2026 7:51 PM, Taylor Blau wrote:\n>>\n>> > -static void read_packs_list_from_stdin(struct rev_info *revs)\n>> > +struct stdin_pack_info {\n>> > +\tstruct packed_git *p;\n>> > +\tenum {\n>> > +\t\tSTDIN_PACK_INCLUDE = (1<<0),\n>> > +\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n>> > +\t} kind;\n>> > +};\n>>\n>> I kind of wish this enum wasn't anonymous. And it matters later.\n>> Let's call this 'enum pack_input_kind' for now.\n>\n> Hmm. I don't feel strongly about this, but I'm not sure I follow the\n> reasoning here. The enum is truly only meant to be used within the\n> context of a stdin_pack_info struct, so it felt natural to keep it\n> anonymous above.\n\nThe only thing that makes it beneficial to have a name is if a\ncode like this one ...\n\n>> \tconst char *key = buf.buf;\n>> \tenum pack_input_kind kind = STDIN_PACK_INCLUDE;\n>>\n>> \tif (*key == '^') {\n>> \t\tkey++;\n>> \t\tkind |= STDIN_PACK_EXCLUDE_CLOSED;\n>> \t}\n\n... that uses the type to define its own variable outside the\ncontext of the struct needs to be written, right?\n\nIf these STDIN_PACK_* constants would ever appear _only_ within the\ncontext of talking about the .kind member of the stdin_pack_info\nstruct and cannot possibly appear anywhere else, then there is no\npoint naming the enum.\n\nA free-standing variable could use \"int\" or \"unsigned\" as the base\nlanguage C does not differentiate different enums as separate types,\nbut let's not go there, as -Wenum-compare and other warnings do give\nus opportunity to do better than that.\n\n\n"},{"id":"540149","messageId":"acWz48NfB+dlbHAz@nand.local","threadId":"65309","inReplyTo":"xmqq8qbensw5.fsf@gitster.g","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-26T22:32:03Z","receivedAt":"2026-03-26T22:32:05Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 03:11:06PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > On Thu, Mar 26, 2026 at 04:40:00PM -0400, Derrick Stolee wrote:\n> >> On 3/25/2026 7:51 PM, Taylor Blau wrote:\n> >>\n> >> > -static void read_packs_list_from_stdin(struct rev_info *revs)\n> >> > +struct stdin_pack_info {\n> >> > +\tstruct packed_git *p;\n> >> > +\tenum {\n> >> > +\t\tSTDIN_PACK_INCLUDE = (1<<0),\n> >> > +\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n> >> > +\t} kind;\n> >> > +};\n> >>\n> >> I kind of wish this enum wasn't anonymous. And it matters later.\n> >> Let's call this 'enum pack_input_kind' for now.\n> >\n> > Hmm. I don't feel strongly about this, but I'm not sure I follow the\n> > reasoning here. The enum is truly only meant to be used within the\n> > context of a stdin_pack_info struct, so it felt natural to keep it\n> > anonymous above.\n>\n> The only thing that makes it beneficial to have a name is if a\n> code like this one ...\n>\n> >> \tconst char *key = buf.buf;\n> >> \tenum pack_input_kind kind = STDIN_PACK_INCLUDE;\n> >>\n> >> \tif (*key == '^') {\n> >> \t\tkey++;\n> >> \t\tkind |= STDIN_PACK_EXCLUDE_CLOSED;\n> >> \t}\n>\n> ... that uses the type to define its own variable outside the\n> context of the struct needs to be written, right?\n>\n> If these STDIN_PACK_* constants would ever appear _only_ within the\n> context of talking about the .kind member of the stdin_pack_info\n> struct and cannot possibly appear anywhere else, then there is no\n> point naming the enum.\n\nYup, I agree. I'm inclined to leave the enum anonymous for now, since\nthe only place we would need a name for it is the suggestion Stolee made\nabove, which I think does not correctly handle an edge case where packs\nare specified multiple times.\n\nAs it is currently, the enum is only used in the context of the .kind\nmember of the stdin_pack_info struct, and we don't ever declare a\nint/unsigned variable to hold the kind outside of that context (which\nwould be gross ;-)).\n\nThanks,\nTaylor\n"},{"id":"540150","messageId":"acW1MDhumRUvk21U@nand.local","threadId":"65309","inReplyTo":"d5cb793f0eb0028f1f521fec4723ad2b00592638.1774482701.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-26T22:37:36Z","receivedAt":"2026-03-26T22:37:38Z","isPatch":true,"body":"On Wed, Mar 25, 2026 at 07:51:50PM -0400, Taylor Blau wrote:\n> +static void stdin_packs_add_pack_entries(struct strmap *packs,\n> +\t\t\t\t\t struct rev_info *revs)\n> +{\n> +\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list_item *item;\n> +\tstruct hashmap_iter iter;\n> +\tstruct strmap_entry *entry;\n> +\n> +\tstrmap_for_each_entry(packs, &iter, entry) {\n> +\t\tstruct stdin_pack_info *info = entry->value;\n> +\t\tif (!info->p)\n> +\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n> +\n> +\t\tstring_list_append(&keys, entry->key)->util = info;\n> +\t}\n> +\n> +\t/*\n> +\t * Order packs by ascending mtime; use QSORT directly to access the\n> +\t * string_list_item's ->util pointer, which string_list_sort() does not\n> +\t * provide.\n> +\t */\n> +\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n\nYikes, this is definitely not right. pack_mtime_cmp expects the ->util\nfield to be a pointer to a packed_git structure, not a stdin_pack_info\none.\n\nIndeed, this fails the ASan CI builds, which I didn't notice as I sent\nthis series off towards the very end of my workday yesterday.\n\nI'll need to resubmit this series to fix this, but I'll hold off on\ndoing so until the discussion in response to Stolee's review of this\nround settles first.\n\nThanks,\nTaylor\n"},{"id":"540160","messageId":"b6e6ea33-76f0-42f8-9546-2e900f239530@gmail.com","threadId":"65309","inReplyTo":"acWz48NfB+dlbHAz@nand.local","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-27T00:29:57Z","receivedAt":"2026-03-27T00:29:59Z","isPatch":true,"body":"On 3/26/26 6:32 PM, Taylor Blau wrote:\n> On Thu, Mar 26, 2026 at 03:11:06PM -0700, Junio C Hamano wrote:\n>> Taylor Blau <me@ttaylorr.com> writes:\n\n>> If these STDIN_PACK_* constants would ever appear _only_ within the\n>> context of talking about the .kind member of the stdin_pack_info\n>> struct and cannot possibly appear anywhere else, then there is no\n>> point naming the enum.\n> \n> Yup, I agree. I'm inclined to leave the enum anonymous for now, since\n> the only place we would need a name for it is the suggestion Stolee made\n> above, which I think does not correctly handle an edge case where packs\n> are specified multiple times.\n\nI see that I messed up where a '|=' should be and where a '=' should be.\n\n\tconst char *key = buf.buf;\n\tenum pack_input_kind kind = STDIN_PACK_INCLUDE;\n\n\tif (*key == '^') {\n\t\tkey++;\n\n\t\t/* THIS ONE SHOULD BE EQUAL */\n\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n\t}\n\n\tinfo = strmap_get(&packs, key);\n\tif (!info) {\n\t\tCALLOC_ARRAY(info, 1);\n\t\tstrmap_put(&packs, key, info);\n\n\t\t/* THIS ONE SHOULD BE ADDING THE FLAG */\n\t\tinfo->kind |= kind;\n\t}\n\n\tstrbuf_reset(&buf);\n\nSorry that I was less careful with the code and hadn't tested it\nlocally. (I still haven't, but I still think it's worth a little\nmore attempt to benefit from this structure, especially with your\naddition of handling '!' later.\n\n> As it is currently, the enum is only used in the context of the .kind\n> member of the stdin_pack_info struct, and we don't ever declare a\n> int/unsigned variable to hold the kind outside of that context (which\n> would be gross ;-)).\n\nOnce you start creating a type, that tends to create the desire to use\nthat type in a new way. My _preference_ is to name the type because it\nunlocks new ways of working with the data without needing to rewrite\nthe definition.\n\nIf the small tweak to my version works, I do think that the readability\nof the new organization would be worth it.\n\nThanks,\n-Stolee\n\n"},{"id":"540183","messageId":"xmqqzf3tmfqf.fsf@gitster.g","threadId":"65309","inReplyTo":"acWz48NfB+dlbHAz@nand.local","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-27T15:52:56Z","receivedAt":"2026-03-27T15:53:00Z","isPatch":true,"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> If these STDIN_PACK_* constants would ever appear _only_ within the\n>> context of talking about the .kind member of the stdin_pack_info\n>> struct and cannot possibly appear anywhere else, then there is no\n>> point naming the enum.\n>\n> Yup, I agree. I'm inclined to leave the enum anonymous for now, since\n> the only place we would need a name for it is the suggestion Stolee made\n> above, which I think does not correctly handle an edge case where packs\n> are specified multiple times.\n>\n> As it is currently, the enum is only used in the context of the .kind\n> member of the stdin_pack_info struct, and we don't ever declare a\n> int/unsigned variable to hold the kind outside of that context (which\n> would be gross ;-)).\n\n\"As it is currently\" is vastly different from \"cannot possibly\nappear\", though ;-)\n"},{"id":"540210","messageId":"acbDkI2vDXYu3mvL@nand.local","threadId":"65309","inReplyTo":"b6e6ea33-76f0-42f8-9546-2e900f239530@gmail.com","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T17:51:12Z","receivedAt":"2026-03-27T17:51:17Z","isPatch":true,"body":"On Thu, Mar 26, 2026 at 08:29:57PM -0400, Derrick Stolee wrote:\n> On 3/26/26 6:32 PM, Taylor Blau wrote:\n> > On Thu, Mar 26, 2026 at 03:11:06PM -0700, Junio C Hamano wrote:\n> > > Taylor Blau <me@ttaylorr.com> writes:\n>\n> > > If these STDIN_PACK_* constants would ever appear _only_ within the\n> > > context of talking about the .kind member of the stdin_pack_info\n> > > struct and cannot possibly appear anywhere else, then there is no\n> > > point naming the enum.\n> >\n> > Yup, I agree. I'm inclined to leave the enum anonymous for now, since\n> > the only place we would need a name for it is the suggestion Stolee made\n> > above, which I think does not correctly handle an edge case where packs\n> > are specified multiple times.\n>\n> I see that I messed up where a '|=' should be and where a '=' should be.\n>\n> \tconst char *key = buf.buf;\n> \tenum pack_input_kind kind = STDIN_PACK_INCLUDE;\n>\n> \tif (*key == '^') {\n> \t\tkey++;\n>\n> \t\t/* THIS ONE SHOULD BE EQUAL */\n> \t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n> \t}\n>\n> \tinfo = strmap_get(&packs, key);\n> \tif (!info) {\n> \t\tCALLOC_ARRAY(info, 1);\n> \t\tstrmap_put(&packs, key, info);\n>\n> \t\t/* THIS ONE SHOULD BE ADDING THE FLAG */\n> \t\tinfo->kind |= kind;\n\nRight, though the problem is not that we're setting the wrong flag bits\n(though I agree in the previous version of this suggestion that we\nshould have been OR-ing them in), but that we're not setting any flag\nbits if the same pack is specified multiple times.\n\nApplying the following:\n\n--- 8< ---\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 52bad8cea90..37c69f307d2 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3833,13 +3833,15 @@ static void show_commit_pack_hint(struct commit *commit, void *data)\n\n }\n\n+enum stdin_pack_info_kind {\n+\tSTDIN_PACK_INCLUDE = (1<<0),\n+\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n+};\n+\n struct stdin_pack_info {\n \tstruct packed_git *p;\n-\tenum {\n-\t\tSTDIN_PACK_INCLUDE = (1<<0),\n-\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n-\t\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n-\t} kind;\n+\tenum stdin_pack_info_kind kind;\n };\n\n static int pack_mtime_cmp(const void *_a, const void *_b)\n@@ -3927,26 +3929,26 @@ static void stdin_packs_read_input(struct rev_info *revs,\n\n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\tstruct stdin_pack_info *info;\n+\t\tenum stdin_pack_info_kind kind = STDIN_PACK_INCLUDE;\n \t\tconst char *key = buf.buf;\n\n \t\tif (!*key)\n \t\t\tcontinue;\n-\t\tif (*key == '^' ||\n-\t\t    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))\n+\t\telse if (*key == '^')\n+\t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n+\t\telse if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n+\t\t\tkind = STDIN_PACK_EXCLUDE_OPEN;\n+\n+\t\tif (kind != STDIN_PACK_INCLUDE)\n \t\t\tkey++;\n\n \t\tinfo = strmap_get(&packs, key);\n \t\tif (!info) {\n \t\t\tCALLOC_ARRAY(info, 1);\n \t\t\tstrmap_put(&packs, key, info);\n-\t\t}\n\n-\t\tif (*buf.buf == '^')\n-\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n-\t\telse if (*buf.buf == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n-\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_OPEN;\n-\t\telse\n-\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n+\t\t\tinfo->kind |= kind;\n+\t\t}\n\n \t\tstrbuf_reset(&buf);\n \t}\n--- >8 ---\n\nfails t5331.8, which verifies that pack-objects correctly handles the\nsame pack being specified as both included and excluded.\n\nBut if you do the following on top of the above:\n\n--- 8< ---\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 37c69f307d2..b6e4f950a67 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3946,10 +3946,10 @@ static void stdin_packs_read_input(struct rev_info *revs,\n \t\tif (!info) {\n \t\t\tCALLOC_ARRAY(info, 1);\n \t\t\tstrmap_put(&packs, key, info);\n-\n-\t\t\tinfo->kind |= kind;\n \t\t}\n\n+\t\tinfo->kind |= kind;\n+\n \t\tstrbuf_reset(&buf);\n \t}\n--- >8 ---\n\nThen that works as expected. I agree that the end-result is a little\neasier to read, so I'll squash this into the subsequent round.\n\n> If the small tweak to my version works, I do think that the readability\n> of the new organization would be worth it.\n\nI agree! Thanks again for the suggestion.\n\nThanks,\nTaylor\n"},{"id":"540215","messageId":"21ab7d29-5855-4830-a22d-cadfeb756cb0@gmail.com","threadId":"65309","inReplyTo":"acbDkI2vDXYu3mvL@nand.local","subject":"Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-27T18:34:01Z","receivedAt":"2026-03-27T18:34:03Z","isPatch":true,"body":"On 3/27/2026 1:51 PM, Taylor Blau wrote:\n> On Thu, Mar 26, 2026 at 08:29:57PM -0400, Derrick Stolee wrote:\n>> On 3/26/26 6:32 PM, Taylor Blau wrote:\n\n> \n> fails t5331.8, which verifies that pack-objects correctly handles the\n> same pack being specified as both included and excluded.\n> \n> But if you do the following on top of the above:\n> \n> --- 8< ---\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 37c69f307d2..b6e4f950a67 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3946,10 +3946,10 @@ static void stdin_packs_read_input(struct rev_info *revs,\n>  \t\tif (!info) {\n>  \t\t\tCALLOC_ARRAY(info, 1);\n>  \t\t\tstrmap_put(&packs, key, info);\n> -\n> -\t\t\tinfo->kind |= kind;\n>  \t\t}\n> \n> +\t\tinfo->kind |= kind;\n> +\n>  \t\tstrbuf_reset(&buf);\n>  \t}\n> --- >8 ---\n> \n> Then that works as expected. I agree that the end-result is a little\n> easier to read, so I'll squash this into the subsequent round.\n\nAh, yes. We should augment the flags even when finding a duplicate.\nThat's the fatal flaw. Thanks for working through it and fixing it.\n\n-Stolee\n\n"},{"id":"540227","messageId":"cover.1774641999.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1773959041.git.me@ttaylorr.com","subject":"[PATCH v3 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T20:06:40Z","receivedAt":"2026-03-27T20:06:43Z","isPatch":true,"body":"This is another small reroll of my series to fix an issue where MIDX\nbitmaps fail to generate after a geometric repack in certain scenarios\nwhere the set of MIDX'd objects is not closed under reachability.\n\nThe main changes since last time are:\n\n * Named enum stdin_pack_info_kind.\n\n * Refactored how we handle reading incoming packs via stdin.\n\n * Fixed a nasty case where sorting the packs in order of mtime happened\n   to work on some systems, but ASan detected a very legitimate bug.\n\nAs usual, a range-diff is included below for convenience.\n\nThanks again for your review!\n\nTaylor Blau (5):\n  pack-objects: plug leak in `read_stdin_packs()`\n  pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`\n  t7704: demonstrate failure with once-cruft objects above the geometric\n    split\n  pack-objects: support excluded-open packs with --stdin-packs\n  repack: mark non-MIDX packs above the split as excluded-open\n\n Documentation/git-pack-objects.adoc |  25 ++-\n builtin/pack-objects.c              | 301 +++++++++++++++++++---------\n builtin/repack.c                    |  19 +-\n packfile.c                          |   3 +-\n packfile.h                          |   2 +\n t/t5331-pack-objects-stdin.sh       | 105 ++++++++++\n t/t7704-repack-cruft.sh             |  22 ++\n 7 files changed, 373 insertions(+), 104 deletions(-)\n\nRange-diff against v2:\n1:  1fabd88f5e3 = 1:  d6ff4e801ab pack-objects: plug leak in `read_stdin_packs()`\n2:  d5cb793f0eb ! 2:  dd9ff1ede4a pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`\n    @@ builtin/pack-objects.c\n      #include \"list.h\"\n      #include \"packfile.h\"\n      #include \"object-file.h\"\n    -@@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b)\n    +@@ builtin/pack-objects.c: static void show_commit_pack_hint(struct commit *commit, void *data)\n    + \n    + }\n    + \n    ++/*\n    ++ * stdin_pack_info_kind specifies how a pack specified over stdin\n    ++ * should be treated when pack-objects is invoked with --stdin-packs.\n    ++ *\n    ++ *  - STDIN_PACK_INCLUDE: objects in any packs with this flag bit set\n    ++ *    should be included in the output pack, unless they appear in an\n    ++ *    excluded pack.\n    ++ *\n    ++ *  - STDIN_PACK_EXCLUDE_CLOSED: objects in any packs with this flag\n    ++ *    bit set should be excluded from the output pack.\n    ++ *\n    ++ * Objects in packs whose 'kind' bits include STDIN_PACK_INCLUDE are\n    ++ * used as traversal tips when invoked with --stdin-packs=follow.\n    ++ */\n    ++enum stdin_pack_info_kind {\n    ++\tSTDIN_PACK_INCLUDE = (1<<0),\n    ++\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n    ++};\n    ++\n    ++struct stdin_pack_info {\n    ++\tstruct packed_git *p;\n    ++\tenum stdin_pack_info_kind kind;\n    ++};\n    ++\n    + static int pack_mtime_cmp(const void *_a, const void *_b)\n    + {\n    +-\tstruct packed_git *a = ((const struct string_list_item*)_a)->util;\n    +-\tstruct packed_git *b = ((const struct string_list_item*)_b)->util;\n    ++\tstruct stdin_pack_info *a = ((const struct string_list_item*)_a)->util;\n    ++\tstruct stdin_pack_info *b = ((const struct string_list_item*)_b)->util;\n    + \n    + \t/*\n    + \t * order packs by descending mtime so that objects are laid out\n    + \t * roughly as newest-to-oldest\n    + \t */\n    +-\tif (a->mtime < b->mtime)\n    ++\tif (a->p->mtime < b->p->mtime)\n    + \t\treturn 1;\n    +-\telse if (b->mtime < a->mtime)\n    ++\telse if (b->p->mtime < a->p->mtime)\n    + \t\treturn -1;\n    + \telse\n      \t\treturn 0;\n      }\n      \n     -static void read_packs_list_from_stdin(struct rev_info *revs)\n    -+struct stdin_pack_info {\n    -+\tstruct packed_git *p;\n    -+\tenum {\n    -+\t\tSTDIN_PACK_INCLUDE = (1<<0),\n    -+\t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n    -+\t} kind;\n    -+};\n    -+\n     +static void stdin_packs_add_pack_entries(struct strmap *packs,\n     +\t\t\t\t\t struct rev_info *revs)\n    -+{\n    + {\n    +-\tstruct strbuf buf = STRBUF_INIT;\n    +-\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n    +-\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n    +-\tstruct string_list_item *item = NULL;\n    +-\tstruct packed_git *p;\n     +\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n     +\tstruct string_list_item *item;\n     +\tstruct hashmap_iter iter;\n     +\tstruct strmap_entry *entry;\n    -+\n    + \n    +-\twhile (strbuf_getline(&buf, stdin) != EOF) {\n    +-\t\tif (!buf.len)\n    +-\t\t\tcontinue;\n     +\tstrmap_for_each_entry(packs, &iter, entry) {\n     +\t\tstruct stdin_pack_info *info = entry->value;\n     +\t\tif (!info->p)\n     +\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n    -+\n    + \n    +-\t\tif (*buf.buf == '^')\n    +-\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n    +-\t\telse\n    +-\t\t\tstring_list_append(&include_packs, buf.buf);\n    +-\n    +-\t\tstrbuf_reset(&buf);\n    +-\t}\n    +-\n    +-\tstring_list_sort_u(&include_packs, 0);\n    +-\tstring_list_sort_u(&exclude_packs, 0);\n    +-\n    +-\trepo_for_each_pack(the_repository, p) {\n    +-\t\tconst char *pack_name = pack_basename(p);\n    +-\n    +-\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n    +-\t\t\tif (exclude_promisor_objects && p->pack_promisor)\n    +-\t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n    +-\t\t\titem->util = p;\n    +-\t\t}\n    +-\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n    +-\t\t\titem->util = p;\n    +-\t}\n    +-\n    +-\t/*\n    +-\t * Arguments we got on stdin may not even be packs. First\n    +-\t * check that to avoid segfaulting later on in\n    +-\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n    +-\t *\n    +-\t * Since we first parsed our STDIN and then sorted the input\n    +-\t * lines the pack we error on will be whatever line happens to\n    +-\t * sort first. This is lazy, it's enough that we report one\n    +-\t * bad case here, we don't need to report the first/last one,\n    +-\t * or all of them.\n    +-\t */\n    +-\tfor_each_string_list_item(item, &include_packs) {\n    +-\t\tstruct packed_git *p = item->util;\n    +-\t\tif (!p)\n    +-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n    +-\t\tif (!is_pack_valid(p))\n    +-\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n    +-\t}\n    +-\n    +-\t/*\n    +-\t * Then, handle all of the excluded packs, marking them as\n    +-\t * kept in-core so that later calls to add_object_entry()\n    +-\t * discards any objects that are also found in excluded packs.\n    +-\t */\n    +-\tfor_each_string_list_item(item, &exclude_packs) {\n    +-\t\tstruct packed_git *p = item->util;\n    +-\t\tif (!p)\n    +-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n    +-\t\tp->pack_keep_in_core = 1;\n     +\t\tstring_list_append(&keys, entry->key)->util = info;\n    -+\t}\n    -+\n    -+\t/*\n    -+\t * Order packs by ascending mtime; use QSORT directly to access the\n    -+\t * string_list_item's ->util pointer, which string_list_sort() does not\n    -+\t * provide.\n    -+\t */\n    + \t}\n    + \n    + \t/*\n    +@@ builtin/pack-objects.c: static void read_packs_list_from_stdin(struct rev_info *revs)\n    + \t * string_list_item's ->util pointer, which string_list_sort() does not\n    + \t * provide.\n    + \t */\n    +-\tQSORT(include_packs.items, include_packs.nr, pack_mtime_cmp);\n    +-\n    +-\tfor_each_string_list_item(item, &include_packs) {\n    +-\t\tstruct packed_git *p = item->util;\n    +-\t\tfor_each_object_in_pack(p,\n    +-\t\t\t\t\tadd_object_entry_from_pack,\n    +-\t\t\t\t\trevs,\n    +-\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n     +\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n     +\n     +\tfor_each_string_list_item(item, &keys) {\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\t\t\t\t\tadd_object_entry_from_pack,\n     +\t\t\t\t\t\trevs,\n     +\t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n    -+\t}\n    -+\n    + \t}\n    + \n     +\tstring_list_clear(&keys, 0);\n     +}\n     +\n     +static void stdin_packs_read_input(struct rev_info *revs)\n    - {\n    - \tstruct strbuf buf = STRBUF_INIT;\n    --\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n    --\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n    --\tstruct string_list_item *item = NULL;\n    ++{\n    ++\tstruct strbuf buf = STRBUF_INIT;\n     +\tstruct strmap packs = STRMAP_INIT;\n    - \tstruct packed_git *p;\n    - \n    - \twhile (strbuf_getline(&buf, stdin) != EOF) {\n    --\t\tif (!buf.len)\n    ++\tstruct packed_git *p;\n    ++\n    ++\twhile (strbuf_getline(&buf, stdin) != EOF) {\n     +\t\tstruct stdin_pack_info *info;\n    ++\t\tenum stdin_pack_info_kind kind = STDIN_PACK_INCLUDE;\n     +\t\tconst char *key = buf.buf;\n     +\n     +\t\tif (!*key)\n    - \t\t\tcontinue;\n    -+\t\tif (*key == '^')\n    ++\t\t\tcontinue;\n    ++\t\telse if (*key == '^')\n    ++\t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n    ++\n    ++\t\tif (kind != STDIN_PACK_INCLUDE)\n     +\t\t\tkey++;\n     +\n     +\t\tinfo = strmap_get(&packs, key);\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\t\tCALLOC_ARRAY(info, 1);\n     +\t\t\tstrmap_put(&packs, key, info);\n     +\t\t}\n    - \n    - \t\tif (*buf.buf == '^')\n    --\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n    -+\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n    - \t\telse\n    --\t\t\tstring_list_append(&include_packs, buf.buf);\n    -+\t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n    - \n    - \t\tstrbuf_reset(&buf);\n    - \t}\n    - \n    --\tstring_list_sort_u(&include_packs, 0);\n    --\tstring_list_sort_u(&exclude_packs, 0);\n    --\n    - \trepo_for_each_pack(the_repository, p) {\n    --\t\tconst char *pack_name = pack_basename(p);\n    ++\n    ++\t\tinfo->kind |= kind;\n    ++\n    ++\t\tstrbuf_reset(&buf);\n    ++\t}\n    ++\n    ++\trepo_for_each_pack(the_repository, p) {\n     +\t\tstruct stdin_pack_info *info;\n    - \n    --\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n    ++\n     +\t\tinfo = strmap_get(&packs, pack_basename(p));\n     +\t\tif (!info)\n     +\t\t\tcontinue;\n     +\n     +\t\tif (info->kind & STDIN_PACK_INCLUDE) {\n    - \t\t\tif (exclude_promisor_objects && p->pack_promisor)\n    - \t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n    --\t\t\titem->util = p;\n    ++\t\t\tif (exclude_promisor_objects && p->pack_promisor)\n    ++\t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n     +\n     +\t\t\t/*\n     +\t\t\t * Arguments we got on stdin may not even be\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\t\t */\n     +\t\t\tif (!is_pack_valid(p))\n     +\t\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n    - \t\t}\n    --\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n    --\t\t\titem->util = p;\n    --\t}\n    - \n    --\t/*\n    --\t * Arguments we got on stdin may not even be packs. First\n    --\t * check that to avoid segfaulting later on in\n    --\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n    --\t *\n    --\t * Since we first parsed our STDIN and then sorted the input\n    --\t * lines the pack we error on will be whatever line happens to\n    --\t * sort first. This is lazy, it's enough that we report one\n    --\t * bad case here, we don't need to report the first/last one,\n    --\t * or all of them.\n    --\t */\n    --\tfor_each_string_list_item(item, &include_packs) {\n    --\t\tstruct packed_git *p = item->util;\n    --\t\tif (!p)\n    --\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n    --\t\tif (!is_pack_valid(p))\n    --\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n    --\t}\n    ++\t\t}\n    ++\n     +\t\tif (info->kind & STDIN_PACK_EXCLUDE_CLOSED) {\n     +\t\t\t/*\n     +\t\t\t * Marking excluded packs as kept in-core so\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\t\t\t */\n     +\t\t\tp->pack_keep_in_core = 1;\n     +\t\t}\n    - \n    --\t/*\n    --\t * Then, handle all of the excluded packs, marking them as\n    --\t * kept in-core so that later calls to add_object_entry()\n    --\t * discards any objects that are also found in excluded packs.\n    --\t */\n    --\tfor_each_string_list_item(item, &exclude_packs) {\n    --\t\tstruct packed_git *p = item->util;\n    --\t\tif (!p)\n    --\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n    --\t\tp->pack_keep_in_core = 1;\n    ++\n     +\t\tinfo->p = p;\n    - \t}\n    - \n    --\t/*\n    --\t * Order packs by ascending mtime; use QSORT directly to access the\n    --\t * string_list_item's ->util pointer, which string_list_sort() does not\n    --\t * provide.\n    --\t */\n    --\tQSORT(include_packs.items, include_packs.nr, pack_mtime_cmp);\n    --\n    --\tfor_each_string_list_item(item, &include_packs) {\n    --\t\tstruct packed_git *p = item->util;\n    --\t\tfor_each_object_in_pack(p,\n    --\t\t\t\t\tadd_object_entry_from_pack,\n    --\t\t\t\t\trevs,\n    --\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n    --\t}\n    ++\t}\n    ++\n     +\tstdin_packs_add_pack_entries(&packs, revs);\n    - \n    ++\n      \tstrbuf_release(&buf);\n     -\tstring_list_clear(&include_packs, 0);\n     -\tstring_list_clear(&exclude_packs, 0);\n3:  d8f0577077c = 3:  5a5090f8da2 t7704: demonstrate failure with once-cruft objects above the geometric split\n4:  e028dfbc9fb ! 4:  efba1ab93d8 pack-objects: support excluded-open packs with --stdin-packs\n    @@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct objec\n      \tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n      \n      \treturn 0;\n    +@@ builtin/pack-objects.c: static void show_commit_pack_hint(struct commit *commit, void *data)\n    +  *  - STDIN_PACK_EXCLUDE_CLOSED: objects in any packs with this flag\n    +  *    bit set should be excluded from the output pack.\n    +  *\n    +- * Objects in packs whose 'kind' bits include STDIN_PACK_INCLUDE are\n    +- * used as traversal tips when invoked with --stdin-packs=follow.\n    ++ *  - STDIN_PACK_EXCLUDE_OPEN: objects in any packs with this flag\n    ++ *    bit set should be excluded from the output pack, but are not\n    ++ *    guaranteed to be closed under reachability.\n    ++ *\n    ++ * Objects in packs whose 'kind' bits include STDIN_PACK_INCLUDE or\n    ++ * STDIN_PACK_EXCLUDE_OPEN are used as traversal tips when invoked\n    ++ * with --stdin-packs=follow.\n    +  */\n    + enum stdin_pack_info_kind {\n    + \tSTDIN_PACK_INCLUDE = (1<<0),\n    + \tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n    ++\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n    + };\n    + \n    + struct stdin_pack_info {\n     @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b)\n      \t\treturn 0;\n      }\n    @@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b\n     +\treturn stdin_packs_include_check_obj((struct object *)commit, data);\n     +}\n     +\n    - struct stdin_pack_info {\n    - \tstruct packed_git *p;\n    - \tenum {\n    - \t\tSTDIN_PACK_INCLUDE = (1<<0),\n    - \t\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n    -+\t\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n    - \t} kind;\n    - };\n    - \n    + static void stdin_packs_add_pack_entries(struct strmap *packs,\n    + \t\t\t\t\t struct rev_info *revs)\n    + {\n     @@ builtin/pack-objects.c: static void stdin_packs_add_pack_entries(struct strmap *packs,\n      \tfor_each_string_list_item(item, &keys) {\n      \t\tstruct stdin_pack_info *info = item->util;\n    @@ builtin/pack-objects.c: static void stdin_packs_add_pack_entries(struct strmap *\n      \tstruct strbuf buf = STRBUF_INIT;\n      \tstruct strmap packs = STRMAP_INIT;\n     @@ builtin/pack-objects.c: static void stdin_packs_read_input(struct rev_info *revs)\n    - \n    - \t\tif (!*key)\n      \t\t\tcontinue;\n    --\t\tif (*key == '^')\n    -+\t\tif (*key == '^' ||\n    -+\t\t    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))\n    + \t\telse if (*key == '^')\n    + \t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n    ++\t\telse if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n    ++\t\t\tkind = STDIN_PACK_EXCLUDE_OPEN;\n    + \n    + \t\tif (kind != STDIN_PACK_INCLUDE)\n      \t\t\tkey++;\n    - \n    - \t\tinfo = strmap_get(&packs, key);\n    -@@ builtin/pack-objects.c: static void stdin_packs_read_input(struct rev_info *revs)\n    - \n    - \t\tif (*buf.buf == '^')\n    - \t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_CLOSED;\n    -+\t\telse if (*buf.buf == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n    -+\t\t\tinfo->kind |= STDIN_PACK_EXCLUDE_OPEN;\n    - \t\telse\n    - \t\t\tinfo->kind |= STDIN_PACK_INCLUDE;\n    - \n     @@ builtin/pack-objects.c: static void stdin_packs_read_input(struct rev_info *revs)\n      \t\t\tp->pack_keep_in_core = 1;\n      \t\t}\n5:  23cb9f33dba = 5:  c9ad9a0c4ae repack: mark non-MIDX packs above the split as excluded-open\n\nbase-commit: 41688c1a2312f62f44435e1a6d03b4b904b5b0ec\n-- \n2.53.0.724.gb20b077944a\n"},{"id":"540228","messageId":"d6ff4e801ab71732e45a645436ccd2cae2fa4d2d.1774641999.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774641999.git.me@ttaylorr.com","subject":"[PATCH v3 1/5] pack-objects: plug leak in `read_stdin_packs()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T20:06:43Z","receivedAt":"2026-03-27T20:06:45Z","isPatch":true,"body":"The `read_stdin_packs()` function added originally via 339bce27f4f\n(builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)\ndeclares a `rev_info` struct but neglects to call `release_revisions()`\non it before returning, creating the potential for a leak.\n\nThe related change in 97ec43247c0 (pack-objects: declare 'rev_info' for\n'--stdin-packs' earlier, 2025-06-23) carried forward this oversight and\ndid not address it.\n\nEnsure that we call `release_revisions()` appropriately to prevent a\npotential leak from this function. Note that in practice our `rev_info`\nhere does not have a present leak, hence t5331 passes cleanly before\nthis commit, even when built with SANITIZE=leak.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex da1087930cb..f640e556823 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3983,6 +3983,8 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \t\t\t     show_object_pack_hint,\n \t\t\t     &mode);\n \n+\trelease_revisions(&revs);\n+\n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_found\",\n \t\t\t   stdin_packs_found_nr);\n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_hints\",\n-- \n2.53.0.724.gb20b077944a\n\n"},{"id":"540229","messageId":"dd9ff1ede4ac0b3b18284d2363ea4aa4c1d97f8c.1774641999.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774641999.git.me@ttaylorr.com","subject":"[PATCH v3 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T20:06:46Z","receivedAt":"2026-03-27T20:06:48Z","isPatch":true,"body":"The '--stdin-packs' mode of pack-objects maintains two separate\nstring_lists: one for included packs, and one for excluded packs. Each\nlist stores the pack basename as a string and the corresponding\n`packed_git` pointer in its `->util` field.\n\nThis works, but makes it awkward to extend the set of pack \"kinds\" that\npack-objects can accept via stdin, since each new kind would need its\nown string_list and duplicated handling. A future commit will want to do\njust this, so prepare for that change by handling the various \"kinds\" of\npacks specified over stdin in a more generic fashion.\n\nNamely, replace the two `string_list`s with a single `strmap` keyed on\nthe pack basename, with values pointing to a new `struct\nstdin_pack_info`. This struct tracks both the `packed_git` pointer and a\n`kind` bitfield indicating whether the pack was specified as included or\nexcluded.\n\nExtract the logic for sorting packs by mtime and adding their objects\ninto a separate `stdin_packs_add_pack_entries()` helper.\n\nWhile we could have used a `string_list`, we must handle the case where\nthe same pack is specified more than once. With a `string_list` only, we\nwould have to pay a quadratic cost to either (a) insert elements into\ntheir sorted positions, or (b) a repeated linear search, which is\naccidentally quadratic. For that reason, use a strmap instead.\n\nThis patch does not include any functional changes.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 197 +++++++++++++++++++++++++----------------\n 1 file changed, 121 insertions(+), 76 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f640e556823..8ab7ca98a55 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -28,6 +28,7 @@\n #include \"reachable.h\"\n #include \"oid-array.h\"\n #include \"strvec.h\"\n+#include \"strmap.h\"\n #include \"list.h\"\n #include \"packfile.h\"\n #include \"object-file.h\"\n@@ -3835,87 +3836,61 @@ static void show_commit_pack_hint(struct commit *commit, void *data)\n \n }\n \n+/*\n+ * stdin_pack_info_kind specifies how a pack specified over stdin\n+ * should be treated when pack-objects is invoked with --stdin-packs.\n+ *\n+ *  - STDIN_PACK_INCLUDE: objects in any packs with this flag bit set\n+ *    should be included in the output pack, unless they appear in an\n+ *    excluded pack.\n+ *\n+ *  - STDIN_PACK_EXCLUDE_CLOSED: objects in any packs with this flag\n+ *    bit set should be excluded from the output pack.\n+ *\n+ * Objects in packs whose 'kind' bits include STDIN_PACK_INCLUDE are\n+ * used as traversal tips when invoked with --stdin-packs=follow.\n+ */\n+enum stdin_pack_info_kind {\n+\tSTDIN_PACK_INCLUDE = (1<<0),\n+\tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+};\n+\n+struct stdin_pack_info {\n+\tstruct packed_git *p;\n+\tenum stdin_pack_info_kind kind;\n+};\n+\n static int pack_mtime_cmp(const void *_a, const void *_b)\n {\n-\tstruct packed_git *a = ((const struct string_list_item*)_a)->util;\n-\tstruct packed_git *b = ((const struct string_list_item*)_b)->util;\n+\tstruct stdin_pack_info *a = ((const struct string_list_item*)_a)->util;\n+\tstruct stdin_pack_info *b = ((const struct string_list_item*)_b)->util;\n \n \t/*\n \t * order packs by descending mtime so that objects are laid out\n \t * roughly as newest-to-oldest\n \t */\n-\tif (a->mtime < b->mtime)\n+\tif (a->p->mtime < b->p->mtime)\n \t\treturn 1;\n-\telse if (b->mtime < a->mtime)\n+\telse if (b->p->mtime < a->p->mtime)\n \t\treturn -1;\n \telse\n \t\treturn 0;\n }\n \n-static void read_packs_list_from_stdin(struct rev_info *revs)\n+static void stdin_packs_add_pack_entries(struct strmap *packs,\n+\t\t\t\t\t struct rev_info *revs)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct string_list include_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list exclude_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *item = NULL;\n-\tstruct packed_git *p;\n+\tstruct string_list keys = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *entry;\n \n-\twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\tif (!buf.len)\n-\t\t\tcontinue;\n+\tstrmap_for_each_entry(packs, &iter, entry) {\n+\t\tstruct stdin_pack_info *info = entry->value;\n+\t\tif (!info->p)\n+\t\t\tdie(_(\"could not find pack '%s'\"), entry->key);\n \n-\t\tif (*buf.buf == '^')\n-\t\t\tstring_list_append(&exclude_packs, buf.buf + 1);\n-\t\telse\n-\t\t\tstring_list_append(&include_packs, buf.buf);\n-\n-\t\tstrbuf_reset(&buf);\n-\t}\n-\n-\tstring_list_sort_u(&include_packs, 0);\n-\tstring_list_sort_u(&exclude_packs, 0);\n-\n-\trepo_for_each_pack(the_repository, p) {\n-\t\tconst char *pack_name = pack_basename(p);\n-\n-\t\tif ((item = string_list_lookup(&include_packs, pack_name))) {\n-\t\t\tif (exclude_promisor_objects && p->pack_promisor)\n-\t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n-\t\t\titem->util = p;\n-\t\t}\n-\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n-\t\t\titem->util = p;\n-\t}\n-\n-\t/*\n-\t * Arguments we got on stdin may not even be packs. First\n-\t * check that to avoid segfaulting later on in\n-\t * e.g. pack_mtime_cmp(), excluded packs are handled below.\n-\t *\n-\t * Since we first parsed our STDIN and then sorted the input\n-\t * lines the pack we error on will be whatever line happens to\n-\t * sort first. This is lazy, it's enough that we report one\n-\t * bad case here, we don't need to report the first/last one,\n-\t * or all of them.\n-\t */\n-\tfor_each_string_list_item(item, &include_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tif (!p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n-\t\tif (!is_pack_valid(p))\n-\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n-\t}\n-\n-\t/*\n-\t * Then, handle all of the excluded packs, marking them as\n-\t * kept in-core so that later calls to add_object_entry()\n-\t * discards any objects that are also found in excluded packs.\n-\t */\n-\tfor_each_string_list_item(item, &exclude_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tif (!p)\n-\t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n-\t\tp->pack_keep_in_core = 1;\n+\t\tstring_list_append(&keys, entry->key)->util = info;\n \t}\n \n \t/*\n@@ -3923,19 +3898,89 @@ static void read_packs_list_from_stdin(struct rev_info *revs)\n \t * string_list_item's ->util pointer, which string_list_sort() does not\n \t * provide.\n \t */\n-\tQSORT(include_packs.items, include_packs.nr, pack_mtime_cmp);\n-\n-\tfor_each_string_list_item(item, &include_packs) {\n-\t\tstruct packed_git *p = item->util;\n-\t\tfor_each_object_in_pack(p,\n-\t\t\t\t\tadd_object_entry_from_pack,\n-\t\t\t\t\trevs,\n-\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n+\tQSORT(keys.items, keys.nr, pack_mtime_cmp);\n+\n+\tfor_each_string_list_item(item, &keys) {\n+\t\tstruct stdin_pack_info *info = item->util;\n+\n+\t\tif (info->kind & STDIN_PACK_INCLUDE)\n+\t\t\tfor_each_object_in_pack(info->p,\n+\t\t\t\t\t\tadd_object_entry_from_pack,\n+\t\t\t\t\t\trevs,\n+\t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n \t}\n \n+\tstring_list_clear(&keys, 0);\n+}\n+\n+static void stdin_packs_read_input(struct rev_info *revs)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strmap packs = STRMAP_INIT;\n+\tstruct packed_git *p;\n+\n+\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\tstruct stdin_pack_info *info;\n+\t\tenum stdin_pack_info_kind kind = STDIN_PACK_INCLUDE;\n+\t\tconst char *key = buf.buf;\n+\n+\t\tif (!*key)\n+\t\t\tcontinue;\n+\t\telse if (*key == '^')\n+\t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n+\n+\t\tif (kind != STDIN_PACK_INCLUDE)\n+\t\t\tkey++;\n+\n+\t\tinfo = strmap_get(&packs, key);\n+\t\tif (!info) {\n+\t\t\tCALLOC_ARRAY(info, 1);\n+\t\t\tstrmap_put(&packs, key, info);\n+\t\t}\n+\n+\t\tinfo->kind |= kind;\n+\n+\t\tstrbuf_reset(&buf);\n+\t}\n+\n+\trepo_for_each_pack(the_repository, p) {\n+\t\tstruct stdin_pack_info *info;\n+\n+\t\tinfo = strmap_get(&packs, pack_basename(p));\n+\t\tif (!info)\n+\t\t\tcontinue;\n+\n+\t\tif (info->kind & STDIN_PACK_INCLUDE) {\n+\t\t\tif (exclude_promisor_objects && p->pack_promisor)\n+\t\t\t\tdie(_(\"packfile %s is a promisor but --exclude-promisor-objects was given\"), p->pack_name);\n+\n+\t\t\t/*\n+\t\t\t * Arguments we got on stdin may not even be\n+\t\t\t * packs. First check that to avoid segfaulting\n+\t\t\t * later on in e.g.  pack_mtime_cmp(), excluded\n+\t\t\t * packs are handled below.\n+\t\t\t */\n+\t\t\tif (!is_pack_valid(p))\n+\t\t\t\tdie(_(\"packfile %s cannot be accessed\"), p->pack_name);\n+\t\t}\n+\n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_CLOSED) {\n+\t\t\t/*\n+\t\t\t * Marking excluded packs as kept in-core so\n+\t\t\t * that later calls to add_object_entry()\n+\t\t\t * discards any objects that are also found in\n+\t\t\t * excluded packs.\n+\t\t\t */\n+\t\t\tp->pack_keep_in_core = 1;\n+\t\t}\n+\n+\t\tinfo->p = p;\n+\t}\n+\n+\tstdin_packs_add_pack_entries(&packs, revs);\n+\n \tstrbuf_release(&buf);\n-\tstring_list_clear(&include_packs, 0);\n-\tstring_list_clear(&exclude_packs, 0);\n+\tstrmap_clear(&packs, 1);\n }\n \n static void add_unreachable_loose_objects(struct rev_info *revs);\n@@ -3972,7 +4017,7 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n-\tread_packs_list_from_stdin(&revs);\n+\tstdin_packs_read_input(&revs);\n \tif (rev_list_unpacked)\n \t\tadd_unreachable_loose_objects(&revs);\n \n-- \n2.53.0.724.gb20b077944a\n\n"},{"id":"540230","messageId":"5a5090f8da2d3df3b64bd8a79649102b5a80f7b3.1774641999.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774641999.git.me@ttaylorr.com","subject":"[PATCH v3 3/5] t7704: demonstrate failure with once-cruft objects above the geometric split","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T20:06:49Z","receivedAt":"2026-03-27T20:06:50Z","isPatch":true,"body":"Add a test demonstrating a case where geometric repacking fails to\nproduce a pack with full object closure, thus making it impossible to\nwrite a reachability bitmap.\n\nMark the test with 'test_expect_failure' for now. The subsequent commit\nwill explain the precise failure mode, and implement a fix.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t7704-repack-cruft.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex aa2e2e6ad88..77133395b5d 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -869,4 +869,26 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'\n \t)\n '\n \n+test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n+\tgit config repack.midxMustContainCruft false &&\n+\n+\ttest_commit reachable &&\n+\ttest_commit unreachable &&\n+\n+\tunreachable=\"$(git rev-parse HEAD)\" &&\n+\n+\tgit reset --hard HEAD^ &&\n+\tgit tag -d unreachable &&\n+\tgit reflog expire --all --expire=all &&\n+\n+\tgit repack --cruft -d &&\n+\n+\techo $unreachable | git pack-objects .git/objects/pack/pack &&\n+\n+\ttest_commit new &&\n+\n+\tgit update-ref refs/heads/other $unreachable &&\n+\tgit repack --geometric=2 -d --write-midx --write-bitmap-index\n+'\n+\n test_done\n-- \n2.53.0.724.gb20b077944a\n\n"},{"id":"540231","messageId":"efba1ab93d8a15036661b112ded34e229101f6d6.1774641999.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774641999.git.me@ttaylorr.com","subject":"[PATCH v3 4/5] pack-objects: support excluded-open packs with --stdin-packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T20:06:51Z","receivedAt":"2026-03-27T20:06:54Z","isPatch":true,"body":"In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n2025-06-23), pack-objects learned to traverse through commits in\nincluded packs when using '--stdin-packs=follow', rescuing reachable\nobjects from unlisted packs into the output.\n\nWhen we encounter a commit in an excluded pack during this rescuing\nphase we will traverse through its parents. But because we set\n`revs.no_kept_objects = 1`, commit simplification will prevent us from\nshowing it via `get_revision()`. (In practice, `--stdin-packs=follow`\nwalks commits down to the roots, but only opens up trees for ones that\ndo not appear in an excluded pack.)\n\nBut there are certain cases where we *do* need to see the parents of an\nobject in an excluded pack. Namely, if an object is rescue-able, but\nonly reachable from object(s) which appear in excluded packs, then\ncommit simplification will exclude those commits from the object\ntraversal, and we will never see a copy of that object, and thus not\nrescue it.\n\nThis is what causes the failure in the previous commit during repacking.\nWhen performing a geometric repack, packs above the geometric split that\nweren't part of the previous MIDX (e.g., packs pushed directly into\n`$GIT_DIR/objects/pack`) may not have full object closure.  When those\npacks are listed as excluded via the '^' marker, the reachability\ntraversal encounters the sequence described above, and may miss objects\nwhich we expect to rescue with `--stdin-packs=follow`.\n\nIntroduce a new \"excluded-open\" pack prefix, '!'. Like '^'-prefixed\npacks, objects from '!'-prefixed packs are excluded from the resulting\npack. But unlike '^', commits in '!'-prefixed packs *are* used as\nstarting points for the follow traversal, and the traversal does not\ntreat them as a closure boundary.\n\nIn order to distinguish excluded-closed from excluded-open packs during\nthe traversal, introduce a new `pack_keep_in_core_open` bit on\n`struct packed_git`, along with a corresponding `KEPT_PACK_IN_CORE_OPEN`\nflag for the kept-pack cache.\n\nIn `add_object_entry_from_pack()`, move the `want_object_in_pack()`\ncheck to *after* `add_pending_oid()`. This is necessary so that commits\nfrom excluded-open packs are added as traversal tips even though their\nobjects won't appear in the output. As a consequence, the caller\n`for_each_object_in_pack()` will always provide a non-NULL 'p', hence we\nare able to drop the \"if (p)\" conditional.\n\nThe `include_check` and `include_check_obj` callbacks on `rev_info` are\nused to halt the walk at closed-excluded packs, since objects behind a\n'^' boundary are guaranteed to have closure and need not be rescued.\n\nThe following commit will make use of this new functionality within the\nrepack layer to resolve the test failure demonstrated in the previous\ncommit.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-pack-objects.adoc |  25 ++++--\n builtin/pack-objects.c              | 116 ++++++++++++++++++++++------\n packfile.c                          |   3 +-\n packfile.h                          |   2 +\n t/t5331-pack-objects-stdin.sh       | 105 +++++++++++++++++++++++++\n 5 files changed, 218 insertions(+), 33 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 71b9682485c..b78175fbe1b 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -94,13 +94,24 @@ base-name::\n \tincluded packs (those not beginning with `^`), excluding any\n \tobjects listed in the excluded packs (beginning with `^`).\n +\n-When `mode` is \"follow\", objects from packs not listed on stdin receive\n-special treatment. Objects within unlisted packs will be included if\n-those objects are (1) reachable from the included packs, and (2) not\n-found in any excluded packs. This mode is useful, for example, to\n-resurrect once-unreachable objects found in cruft packs to generate\n-packs which are closed under reachability up to the boundary set by the\n-excluded packs.\n+When `mode` is \"follow\" packs may additionally be prefixed with `!`,\n+indicating that they are excluded but not necessarily closed under\n+reachability.  In addition to objects in included packs, the resulting\n+pack may include additional objects based on the following:\n++\n+--\n+* If any packs are marked with `!`, then objects reachable from such\n+  packs or included ones via objects outside of excluded-closed packs\n+  will be included. In this case, all `^` packs are treated as closed\n+  under reachability.\n+* Otherwise (if there are no `!` packs), objects within unlisted packs\n+  will be included if those objects are (1) reachable from the\n+  included packs, and (2) not found in any excluded packs.\n+--\n++\n+This mode is useful, for example, to resurrect once-unreachable\n+objects found in cruft packs to generate packs which are closed under\n+reachability up to the boundary set by the excluded packs.\n +\n Incompatible with `--revs`, or options that imply `--revs` (such as\n `--all`), with the exception of `--unpacked`, which is compatible.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8ab7ca98a55..d0203e72d68 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -218,6 +218,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_open;\n static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n@@ -1633,7 +1634,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t/*\n \t * Then handle .keep first, as we have a fast(er) path there.\n \t */\n-\tif (ignore_packed_keep_on_disk || ignore_packed_keep_in_core) {\n+\tif (ignore_packed_keep_on_disk || ignore_packed_keep_in_core ||\n+\t    ignore_packed_keep_in_core_open) {\n \t\t/*\n \t\t * Set the flags for the kept-pack cache to be the ones we want\n \t\t * to ignore.\n@@ -1647,6 +1649,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\t\tflags |= KEPT_PACK_ON_DISK;\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= KEPT_PACK_IN_CORE;\n+\t\tif (ignore_packed_keep_in_core_open)\n+\t\t\tflags |= KEPT_PACK_IN_CORE_OPEN;\n \n \t\t/*\n \t\t * If the object is in a pack that we want to ignore, *and* we\n@@ -1658,6 +1662,8 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\t\t\treturn 0;\n \t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n \t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core_open && p->pack_keep_in_core_open)\n+\t\t\t\treturn 0;\n \t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n \t\t\t\treturn 0;\n \t\t} else {\n@@ -3757,6 +3763,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t\t\t      void *_data)\n {\n \toff_t ofs;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type type = OBJ_NONE;\n \n \tdisplay_progress(progress_state, ++nr_seen);\n@@ -3764,29 +3771,34 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \tif (have_duplicate_entry(oid, 0))\n \t\treturn 0;\n \n+\tstdin_packs_found_nr++;\n+\n \tofs = nth_packed_object_offset(p, pos);\n+\n+\toi.typep = &type;\n+\tif (packed_object_info(p, ofs, &oi) < 0) {\n+\t\tdie(_(\"could not get type of object %s in pack %s\"),\n+\t\t    oid_to_hex(oid), p->pack_name);\n+\t} else if (type == OBJ_COMMIT) {\n+\t\tstruct rev_info *revs = _data;\n+\t\t/*\n+\t\t * commits in included packs are used as starting points\n+\t\t * for the subsequent revision walk\n+\t\t *\n+\t\t * Note that we do want to walk through commits that are\n+\t\t * present in excluded-open ('!') packs to pick up any\n+\t\t * objects reachable from them not present in the\n+\t\t * excluded-closed ('^') packs.\n+\t\t *\n+\t\t * However, we'll only add those objects to the packing\n+\t\t * list after checking `want_object_in_pack()` below.\n+\t\t */\n+\t\tadd_pending_oid(revs, NULL, oid, 0);\n+\t}\n+\n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n \t\treturn 0;\n \n-\tif (p) {\n-\t\tstruct object_info oi = OBJECT_INFO_INIT;\n-\n-\t\toi.typep = &type;\n-\t\tif (packed_object_info(p, ofs, &oi) < 0) {\n-\t\t\tdie(_(\"could not get type of object %s in pack %s\"),\n-\t\t\t    oid_to_hex(oid), p->pack_name);\n-\t\t} else if (type == OBJ_COMMIT) {\n-\t\t\tstruct rev_info *revs = _data;\n-\t\t\t/*\n-\t\t\t * commits in included packs are used as starting points for the\n-\t\t\t * subsequent revision walk\n-\t\t\t */\n-\t\t\tadd_pending_oid(revs, NULL, oid, 0);\n-\t\t}\n-\n-\t\tstdin_packs_found_nr++;\n-\t}\n-\n \tcreate_object_entry(oid, type, 0, 0, 0, p, ofs);\n \n \treturn 0;\n@@ -3847,12 +3859,18 @@ static void show_commit_pack_hint(struct commit *commit, void *data)\n  *  - STDIN_PACK_EXCLUDE_CLOSED: objects in any packs with this flag\n  *    bit set should be excluded from the output pack.\n  *\n- * Objects in packs whose 'kind' bits include STDIN_PACK_INCLUDE are\n- * used as traversal tips when invoked with --stdin-packs=follow.\n+ *  - STDIN_PACK_EXCLUDE_OPEN: objects in any packs with this flag\n+ *    bit set should be excluded from the output pack, but are not\n+ *    guaranteed to be closed under reachability.\n+ *\n+ * Objects in packs whose 'kind' bits include STDIN_PACK_INCLUDE or\n+ * STDIN_PACK_EXCLUDE_OPEN are used as traversal tips when invoked\n+ * with --stdin-packs=follow.\n  */\n enum stdin_pack_info_kind {\n \tSTDIN_PACK_INCLUDE = (1<<0),\n \tSTDIN_PACK_EXCLUDE_CLOSED = (1<<1),\n+\tSTDIN_PACK_EXCLUDE_OPEN = (1<<2),\n };\n \n struct stdin_pack_info {\n@@ -3877,6 +3895,17 @@ static int pack_mtime_cmp(const void *_a, const void *_b)\n \t\treturn 0;\n }\n \n+static int stdin_packs_include_check_obj(struct object *obj, void *data UNUSED)\n+{\n+\treturn !has_object_kept_pack(to_pack.repo, &obj->oid,\n+\t\t\t\t     KEPT_PACK_IN_CORE);\n+}\n+\n+static int stdin_packs_include_check(struct commit *commit, void *data)\n+{\n+\treturn stdin_packs_include_check_obj((struct object *)commit, data);\n+}\n+\n static void stdin_packs_add_pack_entries(struct strmap *packs,\n \t\t\t\t\t struct rev_info *revs)\n {\n@@ -3903,7 +3932,19 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \tfor_each_string_list_item(item, &keys) {\n \t\tstruct stdin_pack_info *info = item->util;\n \n-\t\tif (info->kind & STDIN_PACK_INCLUDE)\n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n+\t\t\t/*\n+\t\t\t * When open-excluded packs (\"!\") are present, stop\n+\t\t\t * the parent walk at closed-excluded (\"^\") packs.\n+\t\t\t * Objects behind a \"^\" boundary are guaranteed to\n+\t\t\t * have closure and should not be rescued.\n+\t\t\t */\n+\t\t\trevs->include_check = stdin_packs_include_check;\n+\t\t\trevs->include_check_obj = stdin_packs_include_check_obj;\n+\t\t}\n+\n+\t\tif ((info->kind & STDIN_PACK_INCLUDE) ||\n+\t\t    (info->kind & STDIN_PACK_EXCLUDE_OPEN))\n \t\t\tfor_each_object_in_pack(info->p,\n \t\t\t\t\t\tadd_object_entry_from_pack,\n \t\t\t\t\t\trevs,\n@@ -3913,7 +3954,8 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \tstring_list_clear(&keys, 0);\n }\n \n-static void stdin_packs_read_input(struct rev_info *revs)\n+static void stdin_packs_read_input(struct rev_info *revs,\n+\t\t\t\t   enum stdin_packs_mode mode)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strmap packs = STRMAP_INIT;\n@@ -3928,6 +3970,8 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \t\t\tcontinue;\n \t\telse if (*key == '^')\n \t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n+\t\telse if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n+\t\t\tkind = STDIN_PACK_EXCLUDE_OPEN;\n \n \t\tif (kind != STDIN_PACK_INCLUDE)\n \t\t\tkey++;\n@@ -3974,6 +4018,20 @@ static void stdin_packs_read_input(struct rev_info *revs)\n \t\t\tp->pack_keep_in_core = 1;\n \t\t}\n \n+\t\tif (info->kind & STDIN_PACK_EXCLUDE_OPEN) {\n+\t\t\t/*\n+\t\t\t * Marking excluded open packs as kept in-core\n+\t\t\t * (open) for the same reason as we marked\n+\t\t\t * exclude closed packs as kept in-core.\n+\t\t\t *\n+\t\t\t * Use a separate flag here to ensure we don't\n+\t\t\t * halt our traversal at these packs, since they\n+\t\t\t * are not guaranteed to have closure.\n+\t\t\t *\n+\t\t\t */\n+\t\t\tp->pack_keep_in_core_open = 1;\n+\t\t}\n+\n \t\tinfo->p = p;\n \t}\n \n@@ -4017,7 +4075,15 @@ static void read_stdin_packs(enum stdin_packs_mode mode, int rev_list_unpacked)\n \n \t/* avoids adding objects in excluded packs */\n \tignore_packed_keep_in_core = 1;\n-\tstdin_packs_read_input(&revs);\n+\tif (mode == STDIN_PACKS_MODE_FOLLOW) {\n+\t\t/*\n+\t\t * In '--stdin-packs=follow' mode, additionally ignore\n+\t\t * objects in excluded-open packs to prevent them from\n+\t\t * appearing in the resulting pack.\n+\t\t */\n+\t\tignore_packed_keep_in_core_open = 1;\n+\t}\n+\tstdin_packs_read_input(&revs, mode);\n \tif (rev_list_unpacked)\n \t\tadd_unreachable_loose_objects(&revs);\n \ndiff --git a/packfile.c b/packfile.c\nindex d4de9f3ffe8..269e420c3b9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2244,7 +2244,8 @@ struct packed_git **packfile_store_get_kept_pack_cache(struct packfile_store *st\n \t\t\tstruct packed_git *p = e->pack;\n \n \t\t\tif ((p->pack_keep && (flags & KEPT_PACK_ON_DISK)) ||\n-\t\t\t    (p->pack_keep_in_core && (flags & KEPT_PACK_IN_CORE))) {\n+\t\t\t    (p->pack_keep_in_core && (flags & KEPT_PACK_IN_CORE)) ||\n+\t\t\t    (p->pack_keep_in_core_open && (flags & KEPT_PACK_IN_CORE_OPEN))) {\n \t\t\t\tALLOC_GROW(packs, nr + 1, alloc);\n \t\t\t\tpacks[nr++] = p;\n \t\t\t}\ndiff --git a/packfile.h b/packfile.h\nindex a16ec3950d2..dd37fc1512e 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -28,6 +28,7 @@ struct packed_git {\n \tunsigned pack_local:1,\n \t\t pack_keep:1,\n \t\t pack_keep_in_core:1,\n+\t\t pack_keep_in_core_open:1,\n \t\t freshened:1,\n \t\t do_not_close:1,\n \t\t pack_promisor:1,\n@@ -266,6 +267,7 @@ int packfile_store_freshen_object(struct packfile_store *store,\n enum kept_pack_type {\n \tKEPT_PACK_ON_DISK = (1 << 0),\n \tKEPT_PACK_IN_CORE = (1 << 1),\n+\tKEPT_PACK_IN_CORE_OPEN = (1 << 2),\n };\n \n /*\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 7eb79bc2cdb..c74b5861af3 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -415,4 +415,109 @@ test_expect_success '--stdin-packs=follow tolerates missing commits' '\n \tstdin_packs__follow_with_only HEAD HEAD^{tree}\n '\n \n+test_expect_success '--stdin-packs=follow with open-excluded packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\t# Create the following commit structure:\n+\t\t#\n+\t\t#   A <-- B <-- D     (main)\n+\t\t#         ^\n+\t\t#          \\\n+\t\t#           C        (other)\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\tgit checkout -B other &&\n+\t\ttest_commit C &&\n+\t\tgit checkout main &&\n+\t\ttest_commit D &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tD=\"$(echo B..D | git pack-objects --revs $packdir/pack)\" &&\n+\n+\t\tC_ONLY=\"$(git rev-parse other | git pack-objects $packdir/pack)\" &&\n+\n+\t\tgit prune-packed &&\n+\n+\t\t# Create a pack using --stdin-packs=follow where:\n+\t\t#\n+\t\t#  - pack D is included,\n+\t\t#  - pack C_ONLY is excluded, but open,\n+\t\t#  - pack B is excluded, but closed, and\n+\t\t#  - packs A and C are unknown\n+\t\t#\n+\t\t# The resulting pack should therefore contain:\n+\t\t#\n+\t\t#  - objects from the included pack D,\n+\t\t#  - A.t (rescued via D^{tree}), and\n+\t\t#  - C^{tree} and C.t (rescued via pack C_ONLY)\n+\t\t#\n+\t\t# , but should omit:\n+\t\t#\n+\t\t#  - C (excluded via C_ONLY),\n+\t\t#  - objects from pack B (trivially excluded-closed)\n+\t\t#  - A and A^{tree} (ancestors of B)\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack <<-EOF\n+\t\tpack-$D.pack\n+\t\t!pack-$C_ONLY.pack\n+\t\t^pack-$B.pack\n+\t\tEOF\n+\t\t) &&\n+\n+\t\t{\n+\t\t\tobjects_in_packs $D &&\n+\t\t\tgit rev-parse A:A.t \"C^{tree}\" C:C.t\n+\t\t} >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with !-delimited pack without follow' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\n+\t\tcat >in <<-EOF &&\n+\t\t!pack-$A.pack\n+\t\tpack-$B.pack\n+\t\tpack-$C.pack\n+\t\tEOF\n+\n+\t\t# Without --stdin-packs=follow, we treat the first\n+\t\t# line of input as a literal packfile name, and thus\n+\t\t# expect pack-objects to complain of a missing pack\n+\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n+\t\t\t>/dev/null <in 2>err &&\n+\t\ttest_grep \"could not find pack .!pack-$A.pack.\" err &&\n+\n+\t\t# With --stdin-packs=follow, we treat the second line\n+\t\t# of input as indicating pack-$A.pack is an excluded\n+\t\t# open pack, and thus expect pack-objects to succeed\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack <in) &&\n+\n+\t\tobjects_in_packs $B $C >expect &&\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.53.0.724.gb20b077944a\n\n"},{"id":"540232","messageId":"c9ad9a0c4ae00e00c0e9aa5cf66157fd93691c6c.1774641999.git.me@ttaylorr.com","threadId":"65309","inReplyTo":"cover.1774641999.git.me@ttaylorr.com","subject":"[PATCH v3 5/5] repack: mark non-MIDX packs above the split as excluded-open","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-03-27T20:06:54Z","receivedAt":"2026-03-27T20:06:56Z","isPatch":true,"body":"In 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where\npossible, 2025-06-23), geometric repacking learned to exclude cruft\npacks from the MIDX when 'repack.midxMustContainCruft' is set to\n'false'.\n\nThis works because packs generated with '--stdin-packs=follow' rescue\nany once-unreachable objects that later become reachable, making the\nresulting packs closed under reachability without needing the cruft pack\nin the MIDX.\n\nHowever, packs above the geometric split that were not part of the\nprevious MIDX may not have full object closure.  When such packs are\nmarked as excluded-closed ('^'), pack-objects treats them as a\nreachability boundary and does not traverse through them during the\nfollow pass, potentially leaving the resulting pack without full\nclosure.\n\nFix this by marking packs above the geometric split that were not in the\nprevious MIDX as excluded-open ('!') instead of excluded-closed ('^').\nThis causes pack-objects to walk through their commits during the follow\npass, rescuing any reachable objects not present in the closed-excluded\npacks.\n\nNote that MIDXs which were generated prior to this change and are\nunlucky enough to not be closed under reachability may still exhibit\nthis bug, as we treat all MIDX'd packs as closed. That is true in an\noverwhelming number of cases, since in order to have a non-closed MIDX\nyou would have to:\n\n - Generate a pack via an earlier geometric repack that is not closed\n   under reachability.\n\n - Store that pack in the MIDX.\n\n - Avoid picking any commits to receive reachability bitmaps which\n   happen to reach objects from which the missing objects are reachable.\n\nIn the extremely rare chance that all of the above should happen, an\nall-into-one repack will resolve the issue.\n\nUnfortunately, there is no perfect way to determine whether a MIDX'd\npack is closed outside of ensuring that there is a '1' bit in at least\none bitmap for every bit position corresponding to objects in that pack.\nWhile this is possible to do, this approach would treat MIDX'd packs as\nopen in cases where there is at least one object that is not reachable\nfrom the subset of commits selected for bitmapping.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c        | 19 +++++++++++++++++--\n t/t7704-repack-cruft.sh |  2 +-\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f6bb04bef72..4c5a82c2c8d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -369,8 +369,23 @@ int cmd_repack(int argc,\n \t\t */\n \t\tfor (i = 0; i < geometry.split; i++)\n \t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n+\t\tfor (i = geometry.split; i < geometry.pack_nr; i++) {\n+\t\t\tconst char *basename = pack_basename(geometry.pack[i]);\n+\t\t\tchar marker = '^';\n+\n+\t\t\tif (!midx_must_contain_cruft &&\n+\t\t\t    !string_list_has_string(&existing.midx_packs,\n+\t\t\t\t\t\t    basename)) {\n+\t\t\t\t/*\n+\t\t\t\t * Assume non-MIDX'd packs are not\n+\t\t\t\t * necessarily closed under\n+\t\t\t\t * reachability.\n+\t\t\t\t */\n+\t\t\t\tmarker = '!';\n+\t\t\t}\n+\n+\t\t\tfprintf(in, \"%c%s\\n\", marker, basename);\n+\t\t}\n \t\tfclose(in);\n \t}\n \ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 77133395b5d..9e03b04315d 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -869,7 +869,7 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'\n \t)\n '\n \n-test_expect_failure 'repack rescues once-cruft objects above geometric split' '\n+test_expect_success 'repack rescues once-cruft objects above geometric split' '\n \tgit config repack.midxMustContainCruft false &&\n \n \ttest_commit reachable &&\n-- \n2.53.0.724.gb20b077944a\n"},{"id":"540233","messageId":"af5babcd-34ad-4933-a4dc-8c9a9fd59bd2@gmail.com","threadId":"65309","inReplyTo":"cover.1774641999.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-27T20:16:41Z","receivedAt":"2026-03-27T20:16:43Z","isPatch":true,"body":"On 3/27/2026 4:06 PM, Taylor Blau wrote:\n> This is another small reroll of my series to fix an issue where MIDX\n> bitmaps fail to generate after a geometric repack in certain scenarios\n> where the set of MIDX'd objects is not closed under reachability.\n> \n> The main changes since last time are:\n> \n>  * Named enum stdin_pack_info_kind.\n> \n>  * Refactored how we handle reading incoming packs via stdin.\n> \n>  * Fixed a nasty case where sorting the packs in order of mtime happened\n>    to work on some systems, but ASan detected a very legitimate bug.\n> \n> As usual, a range-diff is included below for convenience.\n\nThanks. This version LGTM.\n\n-Stolee\n"},{"id":"540235","messageId":"xmqq341lj960.fsf@gitster.g","threadId":"65309","inReplyTo":"af5babcd-34ad-4933-a4dc-8c9a9fd59bd2@gmail.com","subject":"Re: [PATCH v3 0/5] pack-objects: handle excluded-but-open packs via `--stdin-packs=follow`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-27T20:43:03Z","receivedAt":"2026-03-27T20:43:06Z","isPatch":true,"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 3/27/2026 4:06 PM, Taylor Blau wrote:\n>> This is another small reroll of my series to fix an issue where MIDX\n>> bitmaps fail to generate after a geometric repack in certain scenarios\n>> where the set of MIDX'd objects is not closed under reachability.\n>> \n>> The main changes since last time are:\n>> \n>>  * Named enum stdin_pack_info_kind.\n>> \n>>  * Refactored how we handle reading incoming packs via stdin.\n>> \n>>  * Fixed a nasty case where sorting the packs in order of mtime happened\n>>    to work on some systems, but ASan detected a very legitimate bug.\n>> \n>> As usual, a range-diff is included below for convenience.\n>\n> Thanks. This version LGTM.\n>\n> -Stolee\n\nYeah, these look good to me too.  Thanks, both.\n"}]}