From: Taylor Blau <me@ttaylorr•com>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org, Karthik Nayak <karthik.188@gmail•com>,
Jeff King <peff@peff•net>
Subject: Re: [PATCH v2 14/16] packfile: remove `get_packed_git()`
Date: Tue, 26 Aug 2025 21:38:47 -0400 [thread overview]
Message-ID: <aK5hpwcCgjkgQB1N@nand.local> (raw)
In-Reply-To: <20250821-b4-pks-packfiles-store-v2-14-d10623355e9f@pks.im>
On Thu, Aug 21, 2025 at 09:39:12AM +0200, Patrick Steinhardt wrote:
> We have two different functions to retrieve packfiles for a packfile
> store:
>
> - `get_packed_git()` returns the list of packfiles after having called
> `prepare_packed_git()`.
>
> - `get_all_packs()` calls `prepare_packed_git()`, as well, but also
> calls `prepare_midx_pack()` for each pack.
Yeah, having two of these functions that are named so similarly as to
suggest they do the same thing (even though they don't) is unfortunate,
and I am glad that we are looking at it here.
> This means that the latter function also properly loads the info of
> whether or not a packfile is part of a multi-pack index. Preparing this
> extra information also shouldn't be significantly more expensive:
Right; get_packed_git() only loads the non-MIDX'd packs, and
get_all_packs() loads everything (regardless whether or not a pack is
part of the MIDX or not).
Are all of the get_packed_git() callers prepared to handle packs that
are in the MIDX? Looking through them:
- builtin/gc.c::incremental_repack_auto_condition() skips over
'p->multi_pack_index', so this one is fine to convert.
- builtin/grep.c::cmd_grep() calls get_packed_git() but doesn't
actually use the result, so this should be fine to convert, though I
think there is some subtlty here.
- builtin/pack-objects.c::want_object_in_pack_mtime() takes a separate
pass over the MIDX'd packs before calling get_packed_git_mru() (which
itself calls prepare_packed_git()). I think in practice this is OK,
since we will have already handled the MIDX'd packs, but this
function is now iterating over packs in the MIDX twice, so it may be
worth adding a "if (p->multi_pack_index) continue;" in there.
- object-name.c::find_short_packed_object() handles MIDX'd packs
separately, and unique_in_pack() is a noop for MIDX'd packs, so this
one is fine.
- object-name.c::find_abbrev_len_packed() is OK for the same reasons.
So I think that want_object_in_pack_mtime() may need a small tweak, and
I am not 100% certain that cmd_grep() is OK to convert.
> - We have already loaded all packfiles via `prepare_packed_git_one()`.
> So given that multi-pack indices may only refer to packfiles in the
> same object directory we know that we already loaded each packfile.
>
> - The multi-pack index was prepared via `packfile_store_prepare()`
> already, which calls `prepare_multi_pack_index_one()`.
>
> - So all that remains to be done is to look up the index of the pack
> in its multi-pack index so that we can store that info in both the
> pack itself and the MIDX.
This clearly shows that get_all_packs() is not meaningfully more
expensive than get_packed_git(), but I think that may be obscuring some
of the details above on why it is OK (or not) to transition these calls
over.
> So it is somewhat confusing to readers that one of these two functions
> claims to load "all" packfiles while the other one doesn't, even though
> the ultimate difference is way more nuanced.
>
> Convert all of these sites to use `get_all_packs()` instead and remove
> `get_packed_git()`. There doesn't seem to be a good reason to discern
> these two functions.
The last sentence here threw me off a bit. I think the patch message
would benefit from some of the reasoning above about why it is OK to
transition callers over from one function to the other.
As an aside, I am not convinced that having one caller (in pack-objects)
that cares about whether or not it sees a MIDX'd pack is that great of a
reason to have separate functions. But I am also not convinced that it
isn't either. If we kept both, I think they would benefit from having
more distinct names, like get_all_packs() and get_non_midx_packs() or
something.
Thanks,
Taylor
next prev parent reply other threads:[~2025-08-27 1:38 UTC|newest]
Thread overview: 181+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-19 8:19 [PATCH 00/16] packfile: carve out a new packfile store Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 01/16] packfile: introduce a new `struct packfile_store` Patrick Steinhardt
2025-08-19 9:47 ` Karthik Nayak
2025-08-20 4:58 ` Patrick Steinhardt
2025-08-19 17:32 ` Junio C Hamano
2025-08-20 4:58 ` Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 02/16] odb: move list of packfiles into " Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 03/16] odb: move initialization bit " Patrick Steinhardt
2025-08-19 9:57 ` Karthik Nayak
2025-08-19 16:24 ` Junio C Hamano
2025-08-20 8:04 ` Karthik Nayak
2025-08-22 23:50 ` Junio C Hamano
2025-08-26 12:19 ` [PATCH] Documentation: note styling for bit fields Karthik Nayak
2025-08-20 4:58 ` [PATCH 03/16] odb: move initialization bit into `struct packfile_store` Patrick Steinhardt
2025-08-20 6:24 ` Junio C Hamano
2025-08-19 8:19 ` [PATCH 04/16] odb: move packfile map " Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 05/16] odb: move MRU list of packfiles " Patrick Steinhardt
2025-08-20 12:44 ` Karthik Nayak
2025-08-20 19:20 ` Jeff King
2025-08-21 6:40 ` Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 06/16] odb: move kept cache " Patrick Steinhardt
2025-08-19 18:56 ` Junio C Hamano
2025-08-20 4:58 ` Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 07/16] packfile: reorder functions to avoid function declaration Patrick Steinhardt
2025-08-19 19:18 ` Junio C Hamano
2025-08-19 8:19 ` [PATCH 08/16] packfile: refactor `prepare_packed_git()` to work on packfile store Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 09/16] packfile: split up responsibilities of `reprepare_packed_git()` Patrick Steinhardt
2025-08-20 13:17 ` Karthik Nayak
2025-08-19 8:19 ` [PATCH 10/16] packfile: refactor `install_packed_git()` to work on packfile store Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 11/16] packfile: always add packfiles to MRU when adding a pack Patrick Steinhardt
2025-08-20 13:35 ` Karthik Nayak
2025-08-19 8:19 ` [PATCH 12/16] packfile: introduce function to load and add packfiles Patrick Steinhardt
2025-08-20 13:41 ` Karthik Nayak
2025-08-21 6:40 ` Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 13/16] packfile: move `get_multi_pack_index()` into "midx.c" Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 14/16] packfile: remove `get_packed_git()` Patrick Steinhardt
2025-08-20 13:50 ` Karthik Nayak
2025-08-21 6:40 ` Patrick Steinhardt
2025-08-20 13:51 ` Karthik Nayak
2025-08-19 8:19 ` [PATCH 15/16] packfile: refactor `get_all_packs()` to work on packfile store Patrick Steinhardt
2025-08-20 13:53 ` Karthik Nayak
2025-08-21 6:40 ` Patrick Steinhardt
2025-08-19 8:19 ` [PATCH 16/16] packfile: refactor `get_packed_git_mru()` " Patrick Steinhardt
2025-08-19 17:13 ` [PATCH 00/16] packfile: carve out a new " Junio C Hamano
2025-08-20 13:55 ` Karthik Nayak
2025-08-21 7:38 ` [PATCH v2 " Patrick Steinhardt
2025-08-21 7:38 ` [PATCH v2 01/16] packfile: introduce a new `struct packfile_store` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 02/16] odb: move list of packfiles into " Patrick Steinhardt
2025-08-25 23:42 ` Taylor Blau
2025-09-02 8:50 ` Patrick Steinhardt
2025-09-02 17:21 ` Taylor Blau
2025-09-02 17:42 ` Junio C Hamano
2025-09-03 5:58 ` Patrick Steinhardt
2025-09-11 23:16 ` Taylor Blau
2025-09-15 7:44 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 03/16] odb: move initialization bit " Patrick Steinhardt
2025-08-26 1:40 ` Taylor Blau
2025-08-21 7:39 ` [PATCH v2 04/16] odb: move packfile map " Patrick Steinhardt
2025-08-26 1:41 ` Taylor Blau
2025-08-21 7:39 ` [PATCH v2 05/16] odb: move MRU list of packfiles " Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 06/16] odb: move kept cache " Patrick Steinhardt
2025-08-26 1:46 ` Taylor Blau
2025-09-02 8:50 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 07/16] packfile: reorder functions to avoid function declaration Patrick Steinhardt
2025-08-26 1:47 ` Taylor Blau
2025-08-21 7:39 ` [PATCH v2 08/16] packfile: refactor `prepare_packed_git()` to work on packfile store Patrick Steinhardt
2025-08-26 1:58 ` Taylor Blau
2025-08-21 7:39 ` [PATCH v2 09/16] packfile: split up responsibilities of `reprepare_packed_git()` Patrick Steinhardt
2025-08-26 2:10 ` Taylor Blau
2025-09-02 8:50 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 10/16] packfile: refactor `install_packed_git()` to work on packfile store Patrick Steinhardt
2025-08-26 2:11 ` Taylor Blau
2025-09-02 8:50 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 11/16] packfile: always add packfiles to MRU when adding a pack Patrick Steinhardt
2025-08-27 1:04 ` Taylor Blau
2025-09-02 8:50 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 12/16] packfile: introduce function to load and add packfiles Patrick Steinhardt
2025-08-27 1:12 ` Taylor Blau
2025-08-21 7:39 ` [PATCH v2 13/16] packfile: move `get_multi_pack_index()` into "midx.c" Patrick Steinhardt
2025-08-27 1:20 ` Taylor Blau
2025-08-21 7:39 ` [PATCH v2 14/16] packfile: remove `get_packed_git()` Patrick Steinhardt
2025-08-27 1:38 ` Taylor Blau [this message]
2025-09-02 8:50 ` Patrick Steinhardt
2025-09-11 23:25 ` Taylor Blau
2025-09-15 7:30 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 15/16] packfile: refactor `get_all_packs()` to work on packfile store Patrick Steinhardt
2025-08-27 1:45 ` Taylor Blau
2025-09-02 8:51 ` Patrick Steinhardt
2025-09-11 23:33 ` Taylor Blau
2025-09-15 7:44 ` Patrick Steinhardt
2025-08-21 7:39 ` [PATCH v2 16/16] packfile: refactor `get_packed_git_mru()` " Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 00/15] packfile: carve out a new " Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 01/15] packfile: introduce a new `struct packfile_store` Patrick Steinhardt
2025-09-09 7:49 ` Karthik Nayak
2025-09-02 10:48 ` [PATCH v3 02/15] odb: move list of packfiles into " Patrick Steinhardt
2025-09-09 8:00 ` Karthik Nayak
2025-09-09 11:09 ` Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 03/15] odb: move initialization bit " Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 04/15] odb: move packfile map " Patrick Steinhardt
2025-09-09 8:22 ` Karthik Nayak
2025-09-09 11:01 ` Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 05/15] odb: move MRU list of packfiles " Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 06/15] odb: move kept cache " Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 07/15] packfile: reorder functions to avoid function declaration Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 08/15] packfile: refactor `prepare_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 09/15] packfile: split up responsibilities of `reprepare_packed_git()` Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 10/15] packfile: refactor `install_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 11/15] packfile: introduce function to load and add packfiles Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 12/15] packfile: move `get_multi_pack_index()` into "midx.c" Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 13/15] packfile: remove `get_packed_git()` Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 14/15] packfile: refactor `get_all_packs()` to work on packfile store Patrick Steinhardt
2025-09-02 10:48 ` [PATCH v3 15/15] packfile: refactor `get_packed_git_mru()` " Patrick Steinhardt
2025-09-02 16:40 ` [PATCH v3 00/15] packfile: carve out a new " Junio C Hamano
2025-09-11 23:34 ` Taylor Blau
2025-09-09 9:33 ` Karthik Nayak
2025-09-09 11:02 ` [PATCH v4 " Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 01/15] packfile: introduce a new `struct packfile_store` Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 02/15] odb: move list of packfiles into " Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 03/15] odb: move initialization bit " Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 04/15] odb: move packfile map " Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 05/15] odb: move MRU list of packfiles " Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 06/15] odb: move kept cache " Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 07/15] packfile: reorder functions to avoid function declaration Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 08/15] packfile: refactor `prepare_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 09/15] packfile: split up responsibilities of `reprepare_packed_git()` Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 10/15] packfile: refactor `install_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 11/15] packfile: introduce function to load and add packfiles Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 12/15] packfile: move `get_multi_pack_index()` into "midx.c" Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 13/15] packfile: remove `get_packed_git()` Patrick Steinhardt
2025-09-11 23:37 ` Taylor Blau
2025-09-09 11:03 ` [PATCH v4 14/15] packfile: refactor `get_all_packs()` to work on packfile store Patrick Steinhardt
2025-09-09 11:03 ` [PATCH v4 15/15] packfile: refactor `get_packed_git_mru()` " Patrick Steinhardt
2025-09-10 7:35 ` [PATCH v4 00/15] packfile: carve out a new " Karthik Nayak
2025-09-11 23:40 ` Taylor Blau
2025-09-11 23:42 ` Taylor Blau
2025-09-15 7:25 ` Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 " Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 01/15] packfile: introduce a new `struct packfile_store` Patrick Steinhardt
2025-09-17 21:26 ` Justin Tobler
2025-09-23 9:34 ` Patrick Steinhardt
2025-09-24 21:56 ` Justin Tobler
2025-09-15 8:54 ` [PATCH v5 02/15] odb: move list of packfiles into " Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 03/15] odb: move initialization bit " Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 04/15] odb: move packfile map " Patrick Steinhardt
2025-09-17 22:15 ` Justin Tobler
2025-09-23 9:35 ` Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 05/15] odb: move MRU list of packfiles " Patrick Steinhardt
2025-09-17 21:59 ` Justin Tobler
2025-09-15 8:54 ` [PATCH v5 06/15] odb: move kept cache " Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 07/15] packfile: reorder functions to avoid function declaration Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 08/15] packfile: refactor `prepare_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 09/15] packfile: split up responsibilities of `reprepare_packed_git()` Patrick Steinhardt
2025-09-17 22:32 ` Justin Tobler
2025-09-23 9:34 ` Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 10/15] packfile: refactor `install_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 11/15] packfile: introduce function to load and add packfiles Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 12/15] packfile: move `get_multi_pack_index()` into "midx.c" Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 13/15] packfile: refactor `get_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 14/15] packfile: refactor `get_all_packs()` " Patrick Steinhardt
2025-09-15 8:54 ` [PATCH v5 15/15] packfile: refactor `get_packed_git_mru()` " Patrick Steinhardt
2025-09-23 10:16 ` [PATCH v6 00/15] packfile: carve out a new " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 01/15] packfile: introduce a new `struct packfile_store` Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 02/15] odb: move list of packfiles into " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 03/15] odb: move initialization bit " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 04/15] odb: move packfile map " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 05/15] odb: move MRU list of packfiles " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 06/15] odb: move kept cache " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 07/15] packfile: reorder functions to avoid function declaration Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 08/15] packfile: refactor `prepare_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 09/15] packfile: split up responsibilities of `reprepare_packed_git()` Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 10/15] packfile: refactor `install_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 11/15] packfile: introduce function to load and add packfiles Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 12/15] packfile: move `get_multi_pack_index()` into "midx.c" Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 13/15] packfile: refactor `get_packed_git()` to work on packfile store Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 14/15] packfile: refactor `get_all_packs()` " Patrick Steinhardt
2025-09-23 10:17 ` [PATCH v6 15/15] packfile: refactor `get_packed_git_mru()` " Patrick Steinhardt
2025-09-24 21:58 ` [PATCH v6 00/15] packfile: carve out a new " Justin Tobler
2025-09-25 16:08 ` Junio C Hamano
2025-09-26 5:26 ` Patrick Steinhardt
2025-09-28 22:05 ` Taylor Blau
2025-09-29 21:39 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aK5hpwcCgjkgQB1N@nand.local \
--to=me@ttaylorr$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=karthik.188@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox