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

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 15, 2025, 19:57 UTC
Message-ID: <Z/66RtP7eRvxuVmD@nand.local>
In-Reply-To: <CABPp-BG-XeOCtKd-LvUNGsBGyk6rLecm=BYbUDn-6rGwi=ROzg@mail.gmail.com>

On Mon, Apr 14, 2025 at 08:10:51PM -0700, Elijah Newren wrote:

Show 15 quoted lines

> 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