Re: [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()`
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Oct 14, 2025, 19:07 UTC
- Message-ID
- <j6kcv2zs2k5346mol2ojy6xcrtsszgfrm2dlhy263xxygnqus2@jcw4ppho4cnc>
- In-Reply-To
- <20251009-pks-packfiles-convert-get-all-v2-1-0d73b87ce711@pks.im>
On 25/10/09 10:01AM, Patrick Steinhardt wrote:
Show 20 quoted lines
> When searching for abbreviated or when trying to disambiguate object IDs > we do this in two steps: > > 1. We search through the multi-pack index. > > 2. We search through all packfiles not part of any multi-pack index. > > The second step uses `packfile_store_get_packs()`, which knows to skip > loading any packfiles that are indexed by an MIDX; this is exactly what > we want. > > But that function is somewhat problematic, as its behaviour is stateful > and is influenced by `packfile_store_get_all_packs()`. This function > basically does the same as `packfile_store_get_packs()`, but in addition > it also loads all packfiles indexed by an MIDX. The problem here is that > both of these functions act on the same linked list of packfiles, and > thus depending on whether or not `get_all_packs()` was called the result > returned by `get_packs()` will be different. Consequently, all callers > of `get_packs()` need to be prepared to see MIDX'd packs even though > these should in theory be excluded.
So IIUC, calling packfile_store_get_packs() before packfile_store_get_all_packs() has been invoked results in all the packs being returned anyways. This is indeed confusing.
Show 8 quoted lines
> This interface is confusing and thus potentially dangerous, which is why > we're converting all callers of `get_packs()` to use `get_all_packs()` > instead. > > Do so for the above functions in "object-name.c". As explained, we > already know to skip any MIDX'd packs in both `find_abbrev_len_packed()` > and `find_short_packed_object()`, so it's fine to start loading MIDX'd > packfiles.
Ok, converting all the callers to `get_all_packs()` requires them to handle receiving MIDX packfiles also. It sounds like these callsites are already prepared for this reality though.
Show 26 quoted lines
> Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > object-name.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/object-name.c b/object-name.c > index f6902e140d..4e62bfa330 100644 > --- a/object-name.c > +++ b/object-name.c > @@ -213,7 +213,7 @@ static void find_short_packed_object(struct disambiguate_state *ds) > unique_in_midx(m, ds); > } > > - for (p = packfile_store_get_packs(ds->repo->objects->packfiles); p && !ds->ambiguous; > + for (p = packfile_store_get_all_packs(ds->repo->objects->packfiles); p && !ds->ambiguous; > p = p->next) > unique_in_pack(p, ds); > } > @@ -805,7 +805,7 @@ static void find_abbrev_len_packed(struct min_abbrev_data *mad) > find_abbrev_len_for_midx(m, mad); > } > > - for (p = packfile_store_get_packs(mad->repo->objects->packfiles); p; p = p->next) > + for (p = packfile_store_get_all_packs(mad->repo->objects->packfiles); p; p = p->next) > find_abbrev_len_for_pack(p, mad); > }
Looks good.
-Justin