git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 19 of 27 in “packfile: remove `packfile_store_get_packs()`”
  1. 0/6 packfile: remove `packfile_store_get_packs()`Patrick Steinhardt, Oct 7, 2025
  2. 1/6 object-name: convert to use `packfile_store_get_all_packs()`Patrick Steinhardt, Oct 7, 2025
  3. Taylor BlauOct 8, 2025
  4. 2/6 builtin/gc: convert to use `packfile_store_get_all_packs()`Patrick Steinhardt, Oct 7, 2025
  5. Taylor BlauOct 8, 2025
  6. Patrick SteinhardtOct 9, 2025
  7. 3/6 builtin/grep: simplify how we preload packsPatrick Steinhardt, Oct 7, 2025
  8. Taylor BlauOct 8, 2025
  9. 4/6 packfile: drop `packfile_store_get_packs()`Patrick Steinhardt, Oct 7, 2025
  10. Taylor BlauOct 8, 2025
  11. 5/6 packfile: introduce macro to iterate through packsPatrick Steinhardt, Oct 7, 2025
  12. Taylor BlauOct 8, 2025
  13. Patrick SteinhardtOct 9, 2025
  14. 6/6 packfile: rename `packfile_store_get_all_packs()`Patrick Steinhardt, Oct 7, 2025
  15. Taylor BlauOct 8, 2025
  16. Patrick SteinhardtOct 9, 2025
  17. 0/6 packfile: remove `packfile_store_get_packs()`Patrick Steinhardt, Oct 9, 2025
  18. 1/6 object-name: convert to use `packfile_store_get_all_packs()`Patrick Steinhardt, Oct 9, 2025
  19. Justin ToblerOct 14, 2025
  20. 2/6 builtin/gc: convert to use `packfile_store_get_all_packs()`Patrick Steinhardt, Oct 9, 2025
  21. 3/6 builtin/grep: simplify how we preload packsPatrick Steinhardt, Oct 9, 2025
  22. 4/6 packfile: drop `packfile_store_get_packs()`Patrick Steinhardt, Oct 9, 2025
  23. Justin ToblerOct 14, 2025
  24. 5/6 packfile: introduce macro to iterate through packsPatrick Steinhardt, Oct 9, 2025
  25. Justin ToblerOct 14, 2025
  26. 6/6 packfile: rename `packfile_store_get_all_packs()`Patrick Steinhardt, Oct 9, 2025
  27. Justin ToblerOct 14, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.