Re: [PATCH v4 0/9] repack: avoid MIDX'ing cruft pack(s) where possible

On Wed, May 28, 2025 at 07:20:07PM -0400, Taylor Blau wrote:

Hmm. My tool for submitting patches botched the range-diff here. The correct range-diff is:

1:  986bef29b5 ! 1:  2753e29648 pack-objects: limit scope in 'add_object_entry_from_pack()'
    @@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct objec
      	if (p) {
     -		struct rev_info *revs = _data;
      		struct object_info oi = OBJECT_INFO_INIT;
    --
    +
      		oi.typep = &type;
    -+
    - 		if (packed_object_info(the_repository, p, ofs, &oi) < 0) {
    +@@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct object_id *oid,
      			die(_("could not get type of object %s in pack %s"),
      			    oid_to_hex(oid), p->pack_name);
      		} else if (type == OBJ_COMMIT) {
2:  6f8fe8a4e1 = 2:  32b49d9073 pack-objects: factor out handling '--stdin-packs'
3:  2a235461a6 = 3:  a797ff3a83 pack-objects: declare 'rev_info' for '--stdin-packs' earlier
4:  240e90b68d = 4:  29bf05633a pack-objects: perform name-hash traversal for unpacked objects
5:  9a18fa2e52 = 5:  0696fa1736 pack-objects: fix typo in 'show_object_pack_hint()'
6:  6c997853f1 = 6:  1cc45b4472 pack-objects: swap 'show_{object,commit}_pack_hint'
7:  0ff699f056 = 7:  3e3d929bd0 pack-objects: introduce '--stdin-packs=follow'
8:  58891101f3 ! 8:  52a069ef48 repack: exclude cruft pack(s) from the MIDX where possible
    @@ Commit message
         MIDX with '--write-midx' to ensure that the resulting MIDX was always
         closed under reachability in order to generate reachability bitmaps.

    -    Suppose (prior to this patch) you have a once-unreachable object packed
    -    in a cruft pack, which later on becomes reachable from one or more
    -    objects in a geometrically repacked pack. That once-unreachable object
    -    *won't* appear in the new pack, since the cruft pack was specified as
    -    neither included nor excluded to 'pack-objects --stdin-packs'. If the
    +    While the previous patch added the '--stdin-packs=follow' option to
    +    pack-objects, it is not yet on by default. Given that, suppose you have
    +    a once-unreachable object packed in a cruft pack, which later becomes
    +    reachable from one or more objects in a geometrically repacked pack.
    +    That once-unreachable object *won't* appear in the new pack, since the
    +    cruft pack was not specified as included or excluded when the
    +    geometrically repacked pack was created with 'pack-objects
    +    --stdin-packs' (*not* '--stdin-packs=follow', which is not on). If that
         new pack is included in a MIDX without the cruft pack, then trying to
         generate bitmaps for that MIDX may fail. This happens when the bitmap
         selection process picks one or more commits which reach the
    -    once-unreachable objects, commit ddee3703b3 ensures that the MIDX will
    -    be closed under reachability. Without it, we would fail to generate a
    -    MIDX bitmap.
    +    once-unreachable objects.

    +    To mitigate this failure mode, commit ddee3703b3 ensures that the MIDX
    +    will be closed under reachability by including cruft pack(s). If cruft
    +    pack(s) were not included, we would fail to generate a MIDX bitmap. But
         ddee3703b3 alludes to the fact that this is sub-optimal by saying

             [...] it's desirable to avoid including cruft packs in the MIDX

Thanks, Taylor