On Wed, May 28, 2025 at 5:07 PM Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> On Wed, May 28, 2025 at 07:20:07PM -0400, Taylor Blau wrote:
> > Range-diff against v3:
>
> 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
Thanks, this version addresses all the issues I brought up; this round
looks good to me.
