Re: [PATCH v2 6/8] pack-objects: perform name-hash traversal for unpacked objects

[Taylor Blau <me@ttaylorr.com>]

On Mon, Apr 14, 2025 at 08:10:51PM -0700, Elijah Newren wrote:
> On Mon, Apr 14, 2025 at 1:06 PM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > With '--unpacked', pack-objects adds loose objects (which don't appear
> > in any of the excluded packs from '--stdin-packs') to the output pack
> > without considering them as reachability tips for the name-hash
> > traversal.
> >
> > This was an oversight in the original implementation of '--stdin-packs',
> > since the code which enumerates and adds loose objects to the output
> > pack (`add_unreachable_loose_objects()`) did not have access to the
> > 'rev_info' struct found in `read_packs_list_from_stdin()`.
> >
> > Excluding unpacked objects from that traversal doesn't effect the
>
> s/effect/affect/ ?

Oops, yes.

> Should this patch have some tests demonstrating the difference in
> which objects are included?

No; this patch doesn't actually change the set of objects we include
with '--stdin-packs' in conjunction with '--unpacked', it just alters
their name-hash values in an attempt to produce better deltas.

I don't think we have any tests that check this traversal in the packed
or unpacked case, though we could probably add some. It's not obvious
how we'd test that the traversal actually produced better/different
deltas, but we could at least check that it happened with the trace2
identifier "pack-objects/stdin_packs_hints".

I think it's probably worth doing at some point, though I don't think I
see it as especially urgent, unless you feel strongly otherwise.

Thanks,
Taylor