* [PATCH 1/6] object-name: convert to use `packfile_store_get_all_packs()`
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
@ 2025-10-07 12:41 ` Patrick Steinhardt
2025-10-08 20:21 ` Taylor Blau
2025-10-07 12:41 ` [PATCH 2/6] builtin/gc: " Patrick Steinhardt
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 12:41 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
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.
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.
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);
}
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] object-name: convert to use `packfile_store_get_all_packs()`
2025-10-07 12:41 ` [PATCH 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-08 20:21 ` Taylor Blau
0 siblings, 0 replies; 27+ messages in thread
From: Taylor Blau @ 2025-10-08 20:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
On Tue, Oct 07, 2025 at 02:41:07PM +0200, Patrick Steinhardt wrote:
> 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.
>
> 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.
Well explained.
> 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);
> }
Good. As you noted, this function already handles MIDX'd packs as a
separate case, and `unique_in_pack()` discards any packs that have
their `multi_pack_index` bit set. So this caller can be converted
without issue.
> @@ -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);
> }
And the exact same is true for this case as well. Looking good.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/6] builtin/gc: convert to use `packfile_store_get_all_packs()`
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
2025-10-07 12:41 ` [PATCH 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-07 12:41 ` Patrick Steinhardt
2025-10-08 20:25 ` Taylor Blau
2025-10-07 12:41 ` [PATCH 3/6] builtin/grep: simplify how we preload packs Patrick Steinhardt
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 12:41 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
When running maintenance tasks via git-maintenance(1) we have a couple
of auto-conditions that check whether or not a specific task should be
running. One such check is for incremental repacks, which essentially
use `git multi-pack-index repack` to repack a set of smaller packfiles
into one larger packfile.
The auto-condition for this task checks how many packfiles there are
that aren't indexed by any multi-pack index. If there is a sufficient
number then we execute the above command to combine those into a single
pack and add them to the MIDX.
As we don't care about MIDX'd packs we use `packfile_store_get_packs()`,
which knows to not load any packs that are indexed by a MIDX. But as
explained in the preceding commit, we want to get rid of that function.
We already handle packfiles that have an MIDX alright by the very nature
of this function, as we explicitly count non-MIDX'd packs. As such, we
can trivially switch over to use `packfile_store_get_all_packs()`
instead.
Do so.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/gc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index e19e13d978..ab6d6d3bd1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1425,7 +1425,7 @@ static int incremental_repack_auto_condition(struct gc_config *cfg UNUSED)
if (incremental_repack_auto_limit < 0)
return 1;
- for (p = packfile_store_get_packs(the_repository->objects->packfiles);
+ for (p = packfile_store_get_all_packs(the_repository->objects->packfiles);
count < incremental_repack_auto_limit && p;
p = p->next) {
if (!p->multi_pack_index)
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] builtin/gc: convert to use `packfile_store_get_all_packs()`
2025-10-07 12:41 ` [PATCH 2/6] builtin/gc: " Patrick Steinhardt
@ 2025-10-08 20:25 ` Taylor Blau
2025-10-09 6:36 ` Patrick Steinhardt
0 siblings, 1 reply; 27+ messages in thread
From: Taylor Blau @ 2025-10-08 20:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
On Tue, Oct 07, 2025 at 02:41:08PM +0200, Patrick Steinhardt wrote:
> When running maintenance tasks via git-maintenance(1) we have a couple
> of auto-conditions that check whether or not a specific task should be
> running. One such check is for incremental repacks, which essentially
> use `git multi-pack-index repack` to repack a set of smaller packfiles
> into one larger packfile.
>
> The auto-condition for this task checks how many packfiles there are
> that aren't indexed by any multi-pack index. If there is a sufficient
> number then we execute the above command to combine those into a single
> pack and add them to the MIDX.
s/them/that pack/
> As we don't care about MIDX'd packs we use `packfile_store_get_packs()`,
> which knows to not load any packs that are indexed by a MIDX. But as
> explained in the preceding commit, we want to get rid of that function.
>
> We already handle packfiles that have an MIDX alright by the very nature
s/an/a/, s/alright//
> of this function, as we explicitly count non-MIDX'd packs. As such, we
> can trivially switch over to use `packfile_store_get_all_packs()`
> instead.
>
> Do so.
;-)
Thanks,
Taylor
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] builtin/gc: convert to use `packfile_store_get_all_packs()`
2025-10-08 20:25 ` Taylor Blau
@ 2025-10-09 6:36 ` Patrick Steinhardt
0 siblings, 0 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 6:36 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
On Wed, Oct 08, 2025 at 04:25:39PM -0400, Taylor Blau wrote:
> On Tue, Oct 07, 2025 at 02:41:08PM +0200, Patrick Steinhardt wrote:
> > When running maintenance tasks via git-maintenance(1) we have a couple
> > of auto-conditions that check whether or not a specific task should be
> > running. One such check is for incremental repacks, which essentially
> > use `git multi-pack-index repack` to repack a set of smaller packfiles
> > into one larger packfile.
> >
> > The auto-condition for this task checks how many packfiles there are
> > that aren't indexed by any multi-pack index. If there is a sufficient
> > number then we execute the above command to combine those into a single
> > pack and add them to the MIDX.
>
> s/them/that pack/
>
> > As we don't care about MIDX'd packs we use `packfile_store_get_packs()`,
> > which knows to not load any packs that are indexed by a MIDX. But as
> > explained in the preceding commit, we want to get rid of that function.
> >
> > We already handle packfiles that have an MIDX alright by the very nature
>
> s/an/a/, s/alright//
Heh, I guess the first s/an/a/ depends on how you pronounce MIDX. I
typically say "em ei di ex", and in that case it's correct to say "an".
But I think I heard you pronounce it as a single word like "midex",
where it's indeed correct to say "a".
In any case, I don't care, let's just say "a midex".
Patrick
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] builtin/grep: simplify how we preload packs
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
2025-10-07 12:41 ` [PATCH 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
2025-10-07 12:41 ` [PATCH 2/6] builtin/gc: " Patrick Steinhardt
@ 2025-10-07 12:41 ` Patrick Steinhardt
2025-10-08 20:43 ` Taylor Blau
2025-10-07 12:41 ` [PATCH 4/6] packfile: drop `packfile_store_get_packs()` Patrick Steinhardt
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 12:41 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
When using multiple threads in git-grep(1) we eagerly preload both the
gitmodules file as well as the packfiles so that the threads won't race
with one another to initialize these data structures.
For packfiles, this is done by calling `packfile_store_get_packs()`,
which first loads our packfiles and then returns a pointer to the first
such packfile. This pointer is ignored though, as all we really care
about is that `packfile_store_prepare()` was called.
Historyically, that function was file-local to "packfile.c", but that
changed with 4188332569 (packfile: move `get_multi_pack_index()` into
"midx.c", 2025-09-02). We can thus simplify the code by calling that
function directly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/grep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 13841fbf00..53cccf2d25 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1214,7 +1214,7 @@ int cmd_grep(int argc,
if (recurse_submodules)
repo_read_gitmodules(the_repository, 1);
if (startup_info->have_repository)
- (void)packfile_store_get_packs(the_repository->objects->packfiles);
+ packfile_store_prepare(the_repository->objects->packfiles);
start_threads(&opt);
} else {
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] builtin/grep: simplify how we preload packs
2025-10-07 12:41 ` [PATCH 3/6] builtin/grep: simplify how we preload packs Patrick Steinhardt
@ 2025-10-08 20:43 ` Taylor Blau
0 siblings, 0 replies; 27+ messages in thread
From: Taylor Blau @ 2025-10-08 20:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
On Tue, Oct 07, 2025 at 02:41:09PM +0200, Patrick Steinhardt wrote:
> When using multiple threads in git-grep(1) we eagerly preload both the
> gitmodules file as well as the packfiles so that the threads won't race
> with one another to initialize these data structures.
>
> For packfiles, this is done by calling `packfile_store_get_packs()`,
> which first loads our packfiles and then returns a pointer to the first
> such packfile. This pointer is ignored though, as all we really care
> about is that `packfile_store_prepare()` was called.
>
> Historyically, that function was file-local to "packfile.c", but that
s/Historyically/Historically
> changed with 4188332569 (packfile: move `get_multi_pack_index()` into
> "midx.c", 2025-09-02). We can thus simplify the code by calling that
> function directly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/grep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 13841fbf00..53cccf2d25 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1214,7 +1214,7 @@ int cmd_grep(int argc,
> if (recurse_submodules)
> repo_read_gitmodules(the_repository, 1);
> if (startup_info->have_repository)
> - (void)packfile_store_get_packs(the_repository->objects->packfiles);
> + packfile_store_prepare(the_repository->objects->packfiles);
Makes sense. That function literally calls packfile_store_prepare() on
its argument, and then returns store->packs. Since we don't care about
the result as you note, calling packfile_store_prepare() directly makes
sense.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/6] packfile: drop `packfile_store_get_packs()`
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (2 preceding siblings ...)
2025-10-07 12:41 ` [PATCH 3/6] builtin/grep: simplify how we preload packs Patrick Steinhardt
@ 2025-10-07 12:41 ` Patrick Steinhardt
2025-10-08 20:44 ` Taylor Blau
2025-10-07 12:41 ` [PATCH 5/6] packfile: introduce macro to iterate through packs Patrick Steinhardt
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 12:41 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In the preceding commits we have removed all remaining callers of
`packfile_store_get_packs()`, the function is thus unused now. Remove
it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
packfile.c | 6 ------
packfile.h | 6 ------
2 files changed, 12 deletions(-)
diff --git a/packfile.c b/packfile.c
index 5a7caec292..db748b0bd4 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1027,12 +1027,6 @@ void packfile_store_reprepare(struct packfile_store *store)
packfile_store_prepare(store);
}
-struct packed_git *packfile_store_get_packs(struct packfile_store *store)
-{
- packfile_store_prepare(store);
- return store->packs;
-}
-
struct packed_git *packfile_store_get_all_packs(struct packfile_store *store)
{
packfile_store_prepare(store);
diff --git a/packfile.h b/packfile.h
index e7a5792b6c..3f38c63476 100644
--- a/packfile.h
+++ b/packfile.h
@@ -136,12 +136,6 @@ void packfile_store_reprepare(struct packfile_store *store);
void packfile_store_add_pack(struct packfile_store *store,
struct packed_git *pack);
-/*
- * Get packs managed by the given store. Does not load the MIDX or any packs
- * referenced by it.
- */
-struct packed_git *packfile_store_get_packs(struct packfile_store *store);
-
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 5/6] packfile: introduce macro to iterate through packs
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (3 preceding siblings ...)
2025-10-07 12:41 ` [PATCH 4/6] packfile: drop `packfile_store_get_packs()` Patrick Steinhardt
@ 2025-10-07 12:41 ` Patrick Steinhardt
2025-10-08 21:15 ` Taylor Blau
2025-10-07 12:41 ` [PATCH 6/6] packfile: rename `packfile_store_get_all_packs()` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 12:41 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
We have a bunch of different sites that want to iterate through all
packs of a given `struct packfile_store`. This pattern is somewhat
verbose and repetitive, which makes it somewhat cumbersome.
Introduce a new macro `packfile_store_for_each_pack()` that removes some
of the boilerplate.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/cat-file.c | 2 +-
builtin/count-objects.c | 2 +-
builtin/fsck.c | 12 ++++++------
builtin/gc.c | 14 +++++++-------
builtin/pack-objects.c | 14 +++++++-------
builtin/pack-redundant.c | 12 ++++--------
connected.c | 2 +-
http-backend.c | 4 ++--
http.c | 2 +-
object-name.c | 8 +++++---
pack-bitmap.c | 6 +++---
pack-objects.c | 4 ++--
packfile.c | 4 ++--
packfile.h | 6 ++++++
repack-cruft.c | 2 +-
repack-geometry.c | 2 +-
repack.c | 2 +-
server-info.c | 2 +-
t/helper/test-find-pack.c | 3 ++-
t/helper/test-pack-mtimes.c | 2 +-
20 files changed, 55 insertions(+), 50 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ee6715fa52..a565c5f02b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -855,7 +855,7 @@ static void batch_each_object(struct batch_options *opt,
struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *pack;
- for (pack = packfile_store_get_all_packs(packs); pack; pack = pack->next) {
+ packfile_store_for_each_pack(packs, pack) {
if (bitmap_index_contains_pack(bitmap, pack) ||
open_pack_index(pack))
continue;
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index f2f407c2a7..09e3696823 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -130,7 +130,7 @@ int cmd_count_objects(int argc,
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (!p->pack_local)
continue;
if (open_pack_index(p))
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 8ee95e0d67..5462c442dc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -869,18 +869,19 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
{
struct packfile_store *packs = r->objects->packfiles;
struct progress *progress = NULL;
+ struct packed_git *p;
uint32_t pack_count = 0;
int res = 0;
if (show_progress) {
- for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next)
+ packfile_store_for_each_pack(packs, p)
pack_count++;
progress = start_delayed_progress(the_repository,
"Verifying reverse pack-indexes", pack_count);
pack_count = 0;
}
- for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
int load_error = load_pack_revindex_from_disk(p);
if (load_error < 0) {
@@ -1012,8 +1013,7 @@ int cmd_fsck(int argc,
struct progress *progress = NULL;
if (show_progress) {
- for (p = packfile_store_get_all_packs(packs); p;
- p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (open_pack_index(p))
continue;
total += p->num_objects;
@@ -1022,8 +1022,8 @@ int cmd_fsck(int argc,
progress = start_progress(the_repository,
_("Checking objects"), total);
}
- for (p = packfile_store_get_all_packs(packs); p;
- p = p->next) {
+
+ packfile_store_for_each_pack(packs, p) {
/* verify gives error messages itself */
if (verify_pack(the_repository,
p, fsck_obj_buffer,
diff --git a/builtin/gc.c b/builtin/gc.c
index ab6d6d3bd1..16fc56e5cc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -490,7 +490,7 @@ static struct packed_git *find_base_packs(struct string_list *packs,
struct packfile_store *packfiles = the_repository->objects->packfiles;
struct packed_git *p, *base = NULL;
- for (p = packfile_store_get_all_packs(packfiles); p; p = p->next) {
+ packfile_store_for_each_pack(packfiles, p) {
if (!p->pack_local || p->is_cruft)
continue;
if (limit) {
@@ -511,12 +511,12 @@ static int too_many_packs(struct gc_config *cfg)
{
struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- int cnt;
+ int cnt = 0;
if (cfg->gc_auto_pack_limit <= 0)
return 0;
- for (cnt = 0, p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (!p->pack_local)
continue;
if (p->pack_keep)
@@ -1425,9 +1425,9 @@ static int incremental_repack_auto_condition(struct gc_config *cfg UNUSED)
if (incremental_repack_auto_limit < 0)
return 1;
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles);
- count < incremental_repack_auto_limit && p;
- p = p->next) {
+ packfile_store_for_each_pack(the_repository->objects->packfiles, p) {
+ if (count >= incremental_repack_auto_limit)
+ break;
if (!p->multi_pack_index)
count++;
}
@@ -1494,7 +1494,7 @@ static off_t get_auto_pack_size(void)
struct repository *r = the_repository;
odb_reprepare(r->objects);
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
+ packfile_store_for_each_pack(r->objects->packfiles, p) {
if (p->pack_size > max_size) {
second_largest_size = max_size;
max_size = p->pack_size;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index fe987fbb8b..65fb70f806 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3856,7 +3856,7 @@ static void read_packs_list_from_stdin(struct rev_info *revs)
string_list_sort(&exclude_packs);
string_list_remove_duplicates(&exclude_packs, 0);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
const char *pack_name = pack_basename(p);
if ((item = string_list_lookup(&include_packs, pack_name)))
@@ -4107,7 +4107,7 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
*/
- for (p = packfile_store_get_all_packs(packs); p; p = p->next)
+ packfile_store_for_each_pack(packs, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
@@ -4145,7 +4145,7 @@ static void read_cruft_objects(void)
string_list_sort(&discard_packs);
string_list_sort(&fresh_packs);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
const char *pack_name = pack_basename(p);
struct string_list_item *item;
@@ -4446,7 +4446,7 @@ static void loosen_unused_packed_objects(void)
uint32_t loosened_objects_nr = 0;
struct object_id oid;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (!p->pack_local || p->pack_keep || p->pack_keep_in_core)
continue;
@@ -4753,7 +4753,7 @@ static void add_extra_kept_packs(const struct string_list *names)
if (!names->nr)
return;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
const char *name = basename(p->pack_name);
int i;
@@ -5194,7 +5194,7 @@ int cmd_pack_objects(int argc,
struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next)
+ packfile_store_for_each_pack(packs, p)
if (p->pack_local && p->pack_keep)
break;
if (!p) /* no keep-able packs found */
@@ -5209,7 +5209,7 @@ int cmd_pack_objects(int argc,
struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (!p->pack_local) {
have_non_local_packs = 1;
break;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index dd28171f0a..035a2c86a2 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -567,28 +567,24 @@ static struct pack_list * add_pack(struct packed_git *p)
static struct pack_list * add_pack_file(const char *filename)
{
struct packfile_store *packs = the_repository->objects->packfiles;
- struct packed_git *p = packfile_store_get_all_packs(packs);
+ struct packed_git *p;
if (strlen(filename) < 40)
die("Bad pack filename: %s", filename);
- while (p) {
+ packfile_store_for_each_pack(packs, p)
if (strstr(p->pack_name, filename))
return add_pack(p);
- p = p->next;
- }
die("Filename %s not found in packed_git", filename);
}
static void load_all(void)
{
struct packfile_store *packs = the_repository->objects->packfiles;
- struct packed_git *p = packfile_store_get_all_packs(packs);
+ struct packed_git *p;
- while (p) {
+ packfile_store_for_each_pack(packs, p)
add_pack(p);
- p = p->next;
- }
}
int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, struct repository *repo UNUSED) {
diff --git a/connected.c b/connected.c
index b288a18b17..e6b048d176 100644
--- a/connected.c
+++ b/connected.c
@@ -77,7 +77,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (!p->pack_promisor)
continue;
if (find_pack_entry_one(oid, p))
diff --git a/http-backend.c b/http-backend.c
index 9084058f1e..8ba2d37dad 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -609,13 +609,13 @@ static void get_info_packs(struct strbuf *hdr, char *arg UNUSED)
size_t cnt = 0;
select_getanyfile(hdr);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (p->pack_local)
cnt++;
}
strbuf_grow(&buf, cnt * 53 + 2);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (p->pack_local)
strbuf_addf(&buf, "P %s\n", p->pack_name + objdirlen + 6);
}
diff --git a/http.c b/http.c
index 7e3af1e72f..2730352f0e 100644
--- a/http.c
+++ b/http.c
@@ -2425,7 +2425,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
* If we already have the pack locally, no need to fetch its index or
* even add it to list; we already have all of its objects.
*/
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (hasheq(p->hash, sha1, the_repository->hash_algo))
return 0;
}
diff --git a/object-name.c b/object-name.c
index 4e62bfa330..e998dc31b4 100644
--- a/object-name.c
+++ b/object-name.c
@@ -213,9 +213,11 @@ static void find_short_packed_object(struct disambiguate_state *ds)
unique_in_midx(m, ds);
}
- for (p = packfile_store_get_all_packs(ds->repo->objects->packfiles); p && !ds->ambiguous;
- p = p->next)
+ packfile_store_for_each_pack(ds->repo->objects->packfiles, p) {
+ if (ds->ambiguous)
+ break;
unique_in_pack(p, ds);
+ }
}
static int finish_object_disambiguation(struct disambiguate_state *ds,
@@ -805,7 +807,7 @@ static void find_abbrev_len_packed(struct min_abbrev_data *mad)
find_abbrev_len_for_midx(m, mad);
}
- for (p = packfile_store_get_all_packs(mad->repo->objects->packfiles); p; p = p->next)
+ packfile_store_for_each_pack(mad->repo->objects->packfiles, p)
find_abbrev_len_for_pack(p, mad);
}
diff --git a/pack-bitmap.c b/pack-bitmap.c
index ac71035d77..2e30066b27 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -664,7 +664,7 @@ static int open_pack_bitmap(struct repository *r,
struct packed_git *p;
int ret = -1;
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
+ packfile_store_for_each_pack(r->objects->packfiles, p) {
if (open_pack_bitmap_1(bitmap_git, p) == 0) {
ret = 0;
/*
@@ -3347,6 +3347,7 @@ static int verify_bitmap_file(const struct git_hash_algo *algop,
int verify_bitmap_files(struct repository *r)
{
struct odb_source *source;
+ struct packed_git *p;
int res = 0;
odb_prepare_alternates(r->objects);
@@ -3362,8 +3363,7 @@ int verify_bitmap_files(struct repository *r)
free(midx_bitmap_name);
}
- for (struct packed_git *p = packfile_store_get_all_packs(r->objects->packfiles);
- p; p = p->next) {
+ packfile_store_for_each_pack(r->objects->packfiles, p) {
char *pack_bitmap_name = pack_bitmap_filename(p);
res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
free(pack_bitmap_name);
diff --git a/pack-objects.c b/pack-objects.c
index d8eb679735..2dfaae4886 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -96,13 +96,13 @@ static void prepare_in_pack_by_idx(struct packing_data *pdata)
* (i.e. in_pack_idx also zero) should return NULL.
*/
mapping[cnt++] = NULL;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next, cnt++) {
+ packfile_store_for_each_pack(packs, p) {
if (cnt == nr) {
free(mapping);
return;
}
p->index = cnt;
- mapping[cnt] = p;
+ mapping[cnt++] = p;
}
pdata->in_pack_by_idx = mapping;
}
diff --git a/packfile.c b/packfile.c
index db748b0bd4..00abb058db 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2099,7 +2099,7 @@ struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)
* covers, one kept and one not kept, but the midx returns only
* the non-kept version.
*/
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
+ packfile_store_for_each_pack(r->objects->packfiles, p) {
if ((p->pack_keep && (flags & ON_DISK_KEEP_PACKS)) ||
(p->pack_keep_in_core && (flags & IN_CORE_KEEP_PACKS))) {
ALLOC_GROW(packs, nr + 1, alloc);
@@ -2196,7 +2196,7 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,
int r = 0;
int pack_errors = 0;
- for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next) {
+ packfile_store_for_each_pack(repo->objects->packfiles, p) {
if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
continue;
if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&
diff --git a/packfile.h b/packfile.h
index 3f38c63476..b80f79c9aa 100644
--- a/packfile.h
+++ b/packfile.h
@@ -136,6 +136,12 @@ void packfile_store_reprepare(struct packfile_store *store);
void packfile_store_add_pack(struct packfile_store *store,
struct packed_git *pack);
+/*
+ * Load and iterate through all packs of the given packfile store.
+ */
+#define packfile_store_for_each_pack(store, p) \
+ for (p = packfile_store_get_all_packs(store); p; p = p->next)
+
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
diff --git a/repack-cruft.c b/repack-cruft.c
index accb98bcdb..34e7a640de 100644
--- a/repack-cruft.c
+++ b/repack-cruft.c
@@ -12,7 +12,7 @@ static void combine_small_cruft_packs(FILE *in, off_t combine_cruft_below_size,
struct strbuf buf = STRBUF_INIT;
size_t i;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (!(p->is_cruft && p->pack_local))
continue;
diff --git a/repack-geometry.c b/repack-geometry.c
index 56d651c9ce..7bb940ad10 100644
--- a/repack-geometry.c
+++ b/repack-geometry.c
@@ -33,7 +33,7 @@ void pack_geometry_init(struct pack_geometry *geometry,
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
if (args->local && !p->pack_local)
/*
* When asked to only repack local packfiles we skip
diff --git a/repack.c b/repack.c
index 1982a48165..f169df86ce 100644
--- a/repack.c
+++ b/repack.c
@@ -128,7 +128,7 @@ void existing_packs_collect(struct existing_packs *existing,
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
size_t i;
const char *base;
diff --git a/server-info.c b/server-info.c
index 1d33de821e..804bc3c7c4 100644
--- a/server-info.c
+++ b/server-info.c
@@ -293,7 +293,7 @@ static void init_pack_info(struct repository *r, const char *infofile, int force
int i;
size_t alloc = 0;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ packfile_store_for_each_pack(packs, p) {
/* we ignore things on alternate path since they are
* not available to the pullers in general.
*/
diff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c
index e001dc3066..fdc29a6b69 100644
--- a/t/helper/test-find-pack.c
+++ b/t/helper/test-find-pack.c
@@ -39,11 +39,12 @@ int cmd__find_pack(int argc, const char **argv)
if (repo_get_oid(the_repository, argv[0], &oid))
die("cannot parse %s as an object name", argv[0]);
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles); p; p = p->next)
+ packfile_store_for_each_pack(the_repository->objects->packfiles, p) {
if (find_pack_entry_one(&oid, p)) {
printf("%s\n", p->pack_name);
actual_count++;
}
+ }
if (count > -1 && count != actual_count)
die("bad packfile count %d instead of %d", actual_count, count);
diff --git a/t/helper/test-pack-mtimes.c b/t/helper/test-pack-mtimes.c
index 7c428c1601..6322473fbf 100644
--- a/t/helper/test-pack-mtimes.c
+++ b/t/helper/test-pack-mtimes.c
@@ -37,7 +37,7 @@ int cmd__pack_mtimes(int argc, const char **argv)
if (argc != 2)
usage(pack_mtimes_usage);
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles); p; p = p->next) {
+ packfile_store_for_each_pack(the_repository->objects->packfiles, p) {
strbuf_addstr(&buf, basename(p->pack_name));
strbuf_strip_suffix(&buf, ".pack");
strbuf_addstr(&buf, ".mtimes");
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 5/6] packfile: introduce macro to iterate through packs
2025-10-07 12:41 ` [PATCH 5/6] packfile: introduce macro to iterate through packs Patrick Steinhardt
@ 2025-10-08 21:15 ` Taylor Blau
2025-10-09 6:36 ` Patrick Steinhardt
0 siblings, 1 reply; 27+ messages in thread
From: Taylor Blau @ 2025-10-08 21:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
On Tue, Oct 07, 2025 at 02:41:11PM +0200, Patrick Steinhardt wrote:
> We have a bunch of different sites that want to iterate through all
> packs of a given `struct packfile_store`. This pattern is somewhat
> verbose and repetitive, which makes it somewhat cumbersome.
>
> Introduce a new macro `packfile_store_for_each_pack()` that removes some
> of the boilerplate.
Makes sense.
Since we're talking about adding a convenience macro, I wonder if it
might make sense to name/treat this one slightly differently. I would
imagine something like:
repo_for_each_pack(the_repository, p) { ... }
I understand that packfile_store_for_each_pack() is trying to operate on
a single packfile_source. In practice, would someone ever want to
iterate over the packfiles in some source but not another one? It's hard
to know the answer to that question right now without any other
implementations.
Perhaps in the meantime you could imagine doing something similar to
your macro, but instead of reading from the store directly, it could
read from the repository's store _and_ BUG() if there is more than one
store to choose from.
I get that that may be counterproductive to your goal here of getting
the tree prepared to handle multiple object sources. Another approach
might be to keep the proposed semantics, but call it "for_each_pack()"
instead of "packfile_store_for_each_pack()".
I know that the latter is our preferred convention, but I am not sure
that the rule makes total sense in this case. It is obvious from
"for_each_pack()" alone that we are iterating over packs, and the only
way to obtain a collection of packs is from the a packfile_store. So
here I think that the "packfile_store_" prefix is doing more harm than
good by making the code overly verbose.
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 8ee95e0d67..5462c442dc 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -869,18 +869,19 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
> {
> struct packfile_store *packs = r->objects->packfiles;
> struct progress *progress = NULL;
> + struct packed_git *p;
I wondered if this needed to be in scope for the whole function, and
looking just at the first portion of this hunk it seems like it does
not. But continuing to read further down shows other spots where it is
beneficial to have "p" defined at the scope of the whole function.
> uint32_t pack_count = 0;
> int res = 0;
>
> if (show_progress) {
> - for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next)
> + packfile_store_for_each_pack(packs, p)
> pack_count++;
This conversion looks obviously correct. Though I wonder, as an aside,
do you think it makes sense to have a packfile_store_num_packs()
function or similar that does this for us?
Thanks,
Taylor
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/6] packfile: introduce macro to iterate through packs
2025-10-08 21:15 ` Taylor Blau
@ 2025-10-09 6:36 ` Patrick Steinhardt
0 siblings, 0 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 6:36 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
On Wed, Oct 08, 2025 at 05:15:54PM -0400, Taylor Blau wrote:
> On Tue, Oct 07, 2025 at 02:41:11PM +0200, Patrick Steinhardt wrote:
> > We have a bunch of different sites that want to iterate through all
> > packs of a given `struct packfile_store`. This pattern is somewhat
> > verbose and repetitive, which makes it somewhat cumbersome.
> >
> > Introduce a new macro `packfile_store_for_each_pack()` that removes some
> > of the boilerplate.
>
> Makes sense.
>
> Since we're talking about adding a convenience macro, I wonder if it
> might make sense to name/treat this one slightly differently. I would
> imagine something like:
>
> repo_for_each_pack(the_repository, p) { ... }
>
> I understand that packfile_store_for_each_pack() is trying to operate on
> a single packfile_source. In practice, would someone ever want to
> iterate over the packfiles in some source but not another one? It's hard
> to know the answer to that question right now without any other
> implementations.
I don't think so, so I'm fine with this direction. In fact, it might
even make my life easier. See further down.
> Perhaps in the meantime you could imagine doing something similar to
> your macro, but instead of reading from the store directly, it could
> read from the repository's store _and_ BUG() if there is more than one
> store to choose from.
I don't think we should BUG() when there's multiple different packfile
stores, as it is mostly likely the intent of the caller anyway to
iterate through all of them.
But that being said, there is a different reason why we may want to
BUG() eventually: if we get pluggable ODBs, then one of the object
sources may not be a source that uses packs in the first place. And in
_that_ case we definitely should alert the developer that something is
wrong and BUG().
So I'll adapt this series to use your proposed `repo_for_each_pack()`
solution.
> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index 8ee95e0d67..5462c442dc 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -869,18 +869,19 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
> > {
> > struct packfile_store *packs = r->objects->packfiles;
> > struct progress *progress = NULL;
> > + struct packed_git *p;
>
> I wondered if this needed to be in scope for the whole function, and
> looking just at the first portion of this hunk it seems like it does
> not. But continuing to read further down shows other spots where it is
> beneficial to have "p" defined at the scope of the whole function.
>
> > uint32_t pack_count = 0;
> > int res = 0;
> >
> > if (show_progress) {
> > - for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next)
> > + packfile_store_for_each_pack(packs, p)
> > pack_count++;
>
> This conversion looks obviously correct. Though I wonder, as an aside,
> do you think it makes sense to have a packfile_store_num_packs()
> function or similar that does this for us?
We probably could, but I don't think we have enough callsites to warrant
it now.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] packfile: rename `packfile_store_get_all_packs()`
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (4 preceding siblings ...)
2025-10-07 12:41 ` [PATCH 5/6] packfile: introduce macro to iterate through packs Patrick Steinhardt
@ 2025-10-07 12:41 ` Patrick Steinhardt
2025-10-08 20:48 ` Taylor Blau
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 12:41 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In a preceding commit we have removed `packfile_store_get_packs()`. With
this function removed it's somewhat useless to still have the "all"
infix in `packfile_store_get_all_packs()`. Rename the latter to drop
that infix.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fast-import.c | 4 ++--
builtin/pack-objects.c | 4 ++--
packfile.c | 2 +-
packfile.h | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index b1d5549815..fea914cf9e 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -976,7 +976,7 @@ static int store_object(
if (e->idx.offset) {
duplicate_count_by_type[type]++;
return 1;
- } else if (find_oid_pack(&oid, packfile_store_get_all_packs(packs))) {
+ } else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {
e->type = type;
e->pack_id = MAX_PACK_ID;
e->idx.offset = 1; /* just not zero! */
@@ -1177,7 +1177,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
duplicate_count_by_type[OBJ_BLOB]++;
truncate_pack(&checkpoint);
- } else if (find_oid_pack(&oid, packfile_store_get_all_packs(packs))) {
+ } else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {
e->type = OBJ_BLOB;
e->pack_id = MAX_PACK_ID;
e->idx.offset = 1; /* just not zero! */
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 65fb70f806..821598c261 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4398,7 +4398,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
struct packed_git *p;
p = (last_found != (void *)1) ? last_found :
- packfile_store_get_all_packs(packs);
+ packfile_store_get_packs(packs);
while (p) {
if ((!p->pack_local || p->pack_keep ||
@@ -4408,7 +4408,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
return 1;
}
if (p == last_found)
- p = packfile_store_get_all_packs(packs);
+ p = packfile_store_get_packs(packs);
else
p = p->next;
if (p == last_found)
diff --git a/packfile.c b/packfile.c
index 00abb058db..45a75c0307 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1027,7 +1027,7 @@ void packfile_store_reprepare(struct packfile_store *store)
packfile_store_prepare(store);
}
-struct packed_git *packfile_store_get_all_packs(struct packfile_store *store)
+struct packed_git *packfile_store_get_packs(struct packfile_store *store)
{
packfile_store_prepare(store);
diff --git a/packfile.h b/packfile.h
index b80f79c9aa..a8f96b1459 100644
--- a/packfile.h
+++ b/packfile.h
@@ -140,13 +140,13 @@ void packfile_store_add_pack(struct packfile_store *store,
* Load and iterate through all packs of the given packfile store.
*/
#define packfile_store_for_each_pack(store, p) \
- for (p = packfile_store_get_all_packs(store); p; p = p->next)
+ for (p = packfile_store_get_packs(store); p; p = p->next)
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
*/
-struct packed_git *packfile_store_get_all_packs(struct packfile_store *store);
+struct packed_git *packfile_store_get_packs(struct packfile_store *store);
/*
* Get all packs in most-recently-used order.
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] packfile: rename `packfile_store_get_all_packs()`
2025-10-07 12:41 ` [PATCH 6/6] packfile: rename `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-08 20:48 ` Taylor Blau
2025-10-09 6:36 ` Patrick Steinhardt
0 siblings, 1 reply; 27+ messages in thread
From: Taylor Blau @ 2025-10-08 20:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
On Tue, Oct 07, 2025 at 02:41:12PM +0200, Patrick Steinhardt wrote:
> ---
> builtin/fast-import.c | 4 ++--
> builtin/pack-objects.c | 4 ++--
> packfile.c | 2 +-
> packfile.h | 4 ++--
> 4 files changed, 7 insertions(+), 7 deletions(-)
Hmm. I wonder if we should perform this step at a later date. My fear is
that another topic in fight might introduce a new use of the "get_packs"
assuming the old semantics.
Merging this topic and that hypothetical one together wouldn't produce a
textual conflict, but it could introduce bugs where the hypothetical new
code expects the old behavior.
Perhaps I'm overthinking this, but figured I'd write down the concern
nonetheless.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] packfile: rename `packfile_store_get_all_packs()`
2025-10-08 20:48 ` Taylor Blau
@ 2025-10-09 6:36 ` Patrick Steinhardt
0 siblings, 0 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 6:36 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
On Wed, Oct 08, 2025 at 04:48:21PM -0400, Taylor Blau wrote:
> On Tue, Oct 07, 2025 at 02:41:12PM +0200, Patrick Steinhardt wrote:
> > ---
> > builtin/fast-import.c | 4 ++--
> > builtin/pack-objects.c | 4 ++--
> > packfile.c | 2 +-
> > packfile.h | 4 ++--
> > 4 files changed, 7 insertions(+), 7 deletions(-)
>
> Hmm. I wonder if we should perform this step at a later date. My fear is
> that another topic in fight might introduce a new use of the "get_packs"
> assuming the old semantics.
>
> Merging this topic and that hypothetical one together wouldn't produce a
> textual conflict, but it could introduce bugs where the hypothetical new
> code expects the old behavior.
>
> Perhaps I'm overthinking this, but figured I'd write down the concern
> nonetheless.
I think that in many cases, a callsite that doesn't handle MIDX'd packs
specifically with `get_packs()` is almost guaranteed to be wrong in some
cases anyway due to `get_packs()` and `get_all_packs()` influencing each
other's results. In this series we already saw that the callsites handle
this correctly even though they use `get_packs()`, and any new callsites
would probably have to do the same.
So I don't think this is too worrisome overall.
Patrick
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()`
2025-10-07 12:41 [PATCH 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (5 preceding siblings ...)
2025-10-07 12:41 ` [PATCH 6/6] packfile: rename `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
` (6 more replies)
6 siblings, 7 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
Hi,
this patch series follows up on the discussion at [1]. Originally, these
patches were part of the series that introduced the packfile store, but
we decided to split them out into a separate series.
In any case, the goal of this series is to remove the confusing
difference between `packfiles_store_get_{,all_}packs()`. It's not really
obvious to the caller what the difference is. But even worse, the result
of these functions depends on whether or not `get_all_packs()` was ever
executed before `get_packs()`.
The series is built on top of 821f583da6 (The thirteenth batcn,
2025-09-29) with tb/incremental-midx-part-3.1 at c886af90f8 (SQUASH???
play well with other topics by preemptively including "repository.h",
2025-09-29) merged into it. This is done to fix some minor conflicts
with that patch series.
Changes in v2:
- Some commit message improvements.
- Adapt `packfile_store_for_each_pack()` to `repo_for_each_pack()`. On
the one hand this provides easier ergonomics. On the other hand this
will eventually allow us to handle object databases of a different
type more easily.
- Link to v1: https://lore.kernel.org/r/20251007-pks-packfiles-convert-get-all-v1-0-428227657a89@pks.im
Thanks!
Patrick
[1]: <aK5hpwcCgjkgQB1N@nand.local>
---
Patrick Steinhardt (6):
object-name: convert to use `packfile_store_get_all_packs()`
builtin/gc: convert to use `packfile_store_get_all_packs()`
builtin/grep: simplify how we preload packs
packfile: drop `packfile_store_get_packs()`
packfile: introduce macro to iterate through packs
packfile: rename `packfile_store_get_all_packs()`
builtin/cat-file.c | 3 +--
builtin/count-objects.c | 3 +--
builtin/fast-import.c | 4 ++--
builtin/fsck.c | 15 ++++++---------
builtin/gc.c | 16 +++++++---------
builtin/grep.c | 2 +-
builtin/pack-objects.c | 26 +++++++++-----------------
builtin/pack-redundant.c | 14 ++++----------
connected.c | 3 +--
http-backend.c | 5 ++---
http.c | 3 +--
object-name.c | 8 +++++---
pack-bitmap.c | 6 +++---
pack-objects.c | 5 ++---
packfile.c | 10 ++--------
packfile.h | 10 ++++++----
repack-cruft.c | 3 +--
repack-geometry.c | 3 +--
repack.c | 3 +--
server-info.c | 3 +--
t/helper/test-find-pack.c | 3 ++-
t/helper/test-pack-mtimes.c | 2 +-
22 files changed, 60 insertions(+), 90 deletions(-)
Range-diff versus v1:
1: c52009c363 = 1: 04550dbdcf object-name: convert to use `packfile_store_get_all_packs()`
2: 20389a8027 ! 2: 2677bc631f builtin/gc: convert to use `packfile_store_get_all_packs()`
@@ Commit message
The auto-condition for this task checks how many packfiles there are
that aren't indexed by any multi-pack index. If there is a sufficient
number then we execute the above command to combine those into a single
- pack and add them to the MIDX.
+ pack and add that pack to the MIDX.
As we don't care about MIDX'd packs we use `packfile_store_get_packs()`,
which knows to not load any packs that are indexed by a MIDX. But as
explained in the preceding commit, we want to get rid of that function.
- We already handle packfiles that have an MIDX alright by the very nature
- of this function, as we explicitly count non-MIDX'd packs. As such, we
- can trivially switch over to use `packfile_store_get_all_packs()`
- instead.
+ We already handle packfiles that have a MIDX by the very nature of this
+ function, as we explicitly count non-MIDX'd packs. As such, we can
+ trivially switch over to use `packfile_store_get_all_packs()` instead.
Do so.
3: 1357bdce48 ! 3: 7bee72fd98 builtin/grep: simplify how we preload packs
@@ Commit message
such packfile. This pointer is ignored though, as all we really care
about is that `packfile_store_prepare()` was called.
- Historyically, that function was file-local to "packfile.c", but that
+ Historically, that function was file-local to "packfile.c", but that
changed with 4188332569 (packfile: move `get_multi_pack_index()` into
"midx.c", 2025-09-02). We can thus simplify the code by calling that
function directly.
4: f2275c97ae = 4: e709bd4ac9 packfile: drop `packfile_store_get_packs()`
5: 6e57bdd081 ! 5: 2000ef6983 packfile: introduce macro to iterate through packs
@@ Commit message
packs of a given `struct packfile_store`. This pattern is somewhat
verbose and repetitive, which makes it somewhat cumbersome.
- Introduce a new macro `packfile_store_for_each_pack()` that removes some
- of the boilerplate.
+ Introduce a new macro `repo_for_each_pack()` that removes some of the
+ boilerplate.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/cat-file.c ##
@@ builtin/cat-file.c: static void batch_each_object(struct batch_options *opt,
- struct packfile_store *packs = the_repository->objects->packfiles;
+
+ if (bitmap && !for_each_bitmapped_object(bitmap, &opt->objects_filter,
+ batch_one_object_bitmapped, &payload)) {
+- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *pack;
- for (pack = packfile_store_get_all_packs(packs); pack; pack = pack->next) {
-+ packfile_store_for_each_pack(packs, pack) {
++ repo_for_each_pack(the_repository, pack) {
if (bitmap_index_contains_pack(bitmap, pack) ||
open_pack_index(pack))
continue;
## builtin/count-objects.c ##
+@@ builtin/count-objects.c: int cmd_count_objects(int argc,
+ count_loose, count_cruft, NULL, NULL);
+
+ if (verbose) {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct packed_git *p;
+ unsigned long num_pack = 0;
+ off_t size_pack = 0;
@@ builtin/count-objects.c: int cmd_count_objects(int argc,
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
if (open_pack_index(p))
## builtin/fsck.c ##
-@@ builtin/fsck.c: static int check_pack_rev_indexes(struct repository *r, int show_progress)
+@@ builtin/fsck.c: static int mark_packed_for_connectivity(const struct object_id *oid,
+
+ static int check_pack_rev_indexes(struct repository *r, int show_progress)
{
- struct packfile_store *packs = r->objects->packfiles;
+- struct packfile_store *packs = r->objects->packfiles;
struct progress *progress = NULL;
+ struct packed_git *p;
uint32_t pack_count = 0;
@@ builtin/fsck.c: static int check_pack_rev_indexes(struct repository *r, int show
if (show_progress) {
- for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next)
-+ packfile_store_for_each_pack(packs, p)
++ repo_for_each_pack(r, p)
pack_count++;
progress = start_delayed_progress(the_repository,
"Verifying reverse pack-indexes", pack_count);
@@ builtin/fsck.c: static int check_pack_rev_indexes(struct repository *r, int show
}
- for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(r, p) {
int load_error = load_pack_revindex_from_disk(p);
if (load_error < 0) {
+@@ builtin/fsck.c: int cmd_fsck(int argc,
+ for_each_packed_object(the_repository,
+ mark_packed_for_connectivity, NULL, 0);
+ } else {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+-
+ odb_prepare_alternates(the_repository->objects);
+ for (source = the_repository->objects->sources; source; source = source->next)
+ fsck_source(source);
@@ builtin/fsck.c: int cmd_fsck(int argc,
struct progress *progress = NULL;
if (show_progress) {
- for (p = packfile_store_get_all_packs(packs); p;
- p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (open_pack_index(p))
continue;
total += p->num_objects;
@@ builtin/fsck.c: int cmd_fsck(int argc,
- for (p = packfile_store_get_all_packs(packs); p;
- p = p->next) {
+
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
/* verify gives error messages itself */
if (verify_pack(the_repository,
p, fsck_obj_buffer,
## builtin/gc.c ##
-@@ builtin/gc.c: static struct packed_git *find_base_packs(struct string_list *packs,
- struct packfile_store *packfiles = the_repository->objects->packfiles;
+@@ builtin/gc.c: static int too_many_loose_objects(struct gc_config *cfg)
+ static struct packed_git *find_base_packs(struct string_list *packs,
+ unsigned long limit)
+ {
+- struct packfile_store *packfiles = the_repository->objects->packfiles;
struct packed_git *p, *base = NULL;
- for (p = packfile_store_get_all_packs(packfiles); p; p = p->next) {
-+ packfile_store_for_each_pack(packfiles, p) {
++ repo_for_each_pack(the_repository, p) {
if (!p->pack_local || p->is_cruft)
continue;
if (limit) {
-@@ builtin/gc.c: static int too_many_packs(struct gc_config *cfg)
+@@ builtin/gc.c: static struct packed_git *find_base_packs(struct string_list *packs,
+
+ static int too_many_packs(struct gc_config *cfg)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
+- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- int cnt;
+ int cnt = 0;
@@ builtin/gc.c: static int too_many_packs(struct gc_config *cfg)
return 0;
- for (cnt = 0, p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
if (p->pack_keep)
@@ builtin/gc.c: static int incremental_repack_auto_condition(struct gc_config *cfg
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles);
- count < incremental_repack_auto_limit && p;
- p = p->next) {
-+ packfile_store_for_each_pack(the_repository->objects->packfiles, p) {
++ repo_for_each_pack(the_repository, p) {
+ if (count >= incremental_repack_auto_limit)
+ break;
if (!p->multi_pack_index)
@@ builtin/gc.c: static off_t get_auto_pack_size(void)
odb_reprepare(r->objects);
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
-+ packfile_store_for_each_pack(r->objects->packfiles, p) {
++ repo_for_each_pack(r, p) {
if (p->pack_size > max_size) {
second_largest_size = max_size;
max_size = p->pack_size;
## builtin/pack-objects.c ##
+@@ builtin/pack-objects.c: static int pack_mtime_cmp(const void *_a, const void *_b)
+
+ static void read_packs_list_from_stdin(struct rev_info *revs)
+ {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct strbuf buf = STRBUF_INIT;
+ struct string_list include_packs = STRING_LIST_INIT_DUP;
+ struct string_list exclude_packs = STRING_LIST_INIT_DUP;
+ struct string_list_item *item = NULL;
+-
+ struct packed_git *p;
+
+ while (strbuf_getline(&buf, stdin) != EOF) {
@@ builtin/pack-objects.c: static void read_packs_list_from_stdin(struct rev_info *revs)
string_list_sort(&exclude_packs);
string_list_remove_duplicates(&exclude_packs, 0);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
const char *pack_name = pack_basename(p);
if ((item = string_list_lookup(&include_packs, pack_name)))
+@@ builtin/pack-objects.c: static void enumerate_cruft_objects(void)
+
+ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
+ {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct packed_git *p;
+ struct rev_info revs;
+ int ret;
@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
*/
- for (p = packfile_store_get_all_packs(packs); p; p = p->next)
-+ packfile_store_for_each_pack(packs, p)
++ repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
+@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
+
+ static void read_cruft_objects(void)
+ {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct strbuf buf = STRBUF_INIT;
+ struct string_list discard_packs = STRING_LIST_INIT_DUP;
+ struct string_list fresh_packs = STRING_LIST_INIT_DUP;
@@ builtin/pack-objects.c: static void read_cruft_objects(void)
string_list_sort(&discard_packs);
string_list_sort(&fresh_packs);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
const char *pack_name = pack_basename(p);
struct string_list_item *item;
-@@ builtin/pack-objects.c: static void loosen_unused_packed_objects(void)
+@@ builtin/pack-objects.c: static int loosened_object_can_be_discarded(const struct object_id *oid,
+
+ static void loosen_unused_packed_objects(void)
+ {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct packed_git *p;
+ uint32_t i;
uint32_t loosened_objects_nr = 0;
struct object_id oid;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (!p->pack_local || p->pack_keep || p->pack_keep_in_core)
continue;
-@@ builtin/pack-objects.c: static void add_extra_kept_packs(const struct string_list *names)
+@@ builtin/pack-objects.c: static void get_object_list(struct rev_info *revs, struct strvec *argv)
+
+ static void add_extra_kept_packs(const struct string_list *names)
+ {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct packed_git *p;
+
if (!names->nr)
return;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
const char *name = basename(p->pack_name);
int i;
@@ builtin/pack-objects.c: int cmd_pack_objects(int argc,
- struct packfile_store *packs = the_repository->objects->packfiles;
+
+ add_extra_kept_packs(&keep_pack_list);
+ if (ignore_packed_keep_on_disk) {
+- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next)
-+ packfile_store_for_each_pack(packs, p)
++ repo_for_each_pack(the_repository, p)
if (p->pack_local && p->pack_keep)
break;
if (!p) /* no keep-able packs found */
@@ builtin/pack-objects.c: int cmd_pack_objects(int argc,
- struct packfile_store *packs = the_repository->objects->packfiles;
+ * want to unset "local" based on looking at packs, as
+ * it also covers non-local objects
+ */
+- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (!p->pack_local) {
have_non_local_packs = 1;
break;
## builtin/pack-redundant.c ##
@@ builtin/pack-redundant.c: static struct pack_list * add_pack(struct packed_git *p)
+
static struct pack_list * add_pack_file(const char *filename)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
+- struct packfile_store *packs = the_repository->objects->packfiles;
- struct packed_git *p = packfile_store_get_all_packs(packs);
+ struct packed_git *p;
@@ builtin/pack-redundant.c: static struct pack_list * add_pack(struct packed_git *
die("Bad pack filename: %s", filename);
- while (p) {
-+ packfile_store_for_each_pack(packs, p)
++ repo_for_each_pack(the_repository, p)
if (strstr(p->pack_name, filename))
return add_pack(p);
- p = p->next;
@@ builtin/pack-redundant.c: static struct pack_list * add_pack(struct packed_git *
static void load_all(void)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
+- struct packfile_store *packs = the_repository->objects->packfiles;
- struct packed_git *p = packfile_store_get_all_packs(packs);
+ struct packed_git *p;
- while (p) {
-+ packfile_store_for_each_pack(packs, p)
++ repo_for_each_pack(the_repository, p)
add_pack(p);
- p = p->next;
- }
@@ builtin/pack-redundant.c: static struct pack_list * add_pack(struct packed_git *
## connected.c ##
@@ connected.c: int check_connected(oid_iterate_fn fn, void *cb_data,
- struct packfile_store *packs = the_repository->objects->packfiles;
+ */
+ odb_reprepare(the_repository->objects);
+ do {
+- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (!p->pack_promisor)
continue;
if (find_pack_entry_one(oid, p))
## http-backend.c ##
-@@ http-backend.c: static void get_info_packs(struct strbuf *hdr, char *arg UNUSED)
+@@ http-backend.c: static void get_head(struct strbuf *hdr, char *arg UNUSED)
+ static void get_info_packs(struct strbuf *hdr, char *arg UNUSED)
+ {
+ size_t objdirlen = strlen(repo_get_object_directory(the_repository));
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct strbuf buf = STRBUF_INIT;
+ struct packed_git *p;
size_t cnt = 0;
select_getanyfile(hdr);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (p->pack_local)
cnt++;
}
strbuf_grow(&buf, cnt * 53 + 2);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (p->pack_local)
strbuf_addf(&buf, "P %s\n", p->pack_name + objdirlen + 6);
}
## http.c ##
+@@ http.c: static char *fetch_pack_index(unsigned char *hash, const char *base_url)
+ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
+ unsigned char *sha1, const char *base_url)
+ {
+- struct packfile_store *packs = the_repository->objects->packfiles;
+ struct packed_git *new_pack, *p;
+ char *tmp_idx = NULL;
+ int ret;
@@ http.c: static int fetch_and_setup_pack_index(struct packed_git **packs_head,
* If we already have the pack locally, no need to fetch its index or
* even add it to list; we already have all of its objects.
*/
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(the_repository, p) {
if (hasheq(p->hash, sha1, the_repository->hash_algo))
return 0;
}
@@ object-name.c: static void find_short_packed_object(struct disambiguate_state *d
- for (p = packfile_store_get_all_packs(ds->repo->objects->packfiles); p && !ds->ambiguous;
- p = p->next)
-+ packfile_store_for_each_pack(ds->repo->objects->packfiles, p) {
++ repo_for_each_pack(ds->repo, p) {
+ if (ds->ambiguous)
+ break;
unique_in_pack(p, ds);
@@ object-name.c: static void find_abbrev_len_packed(struct min_abbrev_data *mad)
}
- for (p = packfile_store_get_all_packs(mad->repo->objects->packfiles); p; p = p->next)
-+ packfile_store_for_each_pack(mad->repo->objects->packfiles, p)
++ repo_for_each_pack(mad->repo, p)
find_abbrev_len_for_pack(p, mad);
}
@@ pack-bitmap.c: static int open_pack_bitmap(struct repository *r,
int ret = -1;
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
-+ packfile_store_for_each_pack(r->objects->packfiles, p) {
++ repo_for_each_pack(r, p) {
if (open_pack_bitmap_1(bitmap_git, p) == 0) {
ret = 0;
/*
@@ pack-bitmap.c: int verify_bitmap_files(struct repository *r)
- for (struct packed_git *p = packfile_store_get_all_packs(r->objects->packfiles);
- p; p = p->next) {
-+ packfile_store_for_each_pack(r->objects->packfiles, p) {
++ repo_for_each_pack(r, p) {
char *pack_bitmap_name = pack_bitmap_filename(p);
res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
free(pack_bitmap_name);
## pack-objects.c ##
+@@ pack-objects.c: struct object_entry *packlist_find(struct packing_data *pdata,
+
+ static void prepare_in_pack_by_idx(struct packing_data *pdata)
+ {
+- struct packfile_store *packs = pdata->repo->objects->packfiles;
+ struct packed_git **mapping, *p;
+ int cnt = 0, nr = 1U << OE_IN_PACK_BITS;
+
@@ pack-objects.c: static void prepare_in_pack_by_idx(struct packing_data *pdata)
* (i.e. in_pack_idx also zero) should return NULL.
*/
mapping[cnt++] = NULL;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next, cnt++) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(pdata->repo, p) {
if (cnt == nr) {
free(mapping);
return;
@@ packfile.c: struct packed_git **kept_pack_cache(struct repository *r, unsigned f
* the non-kept version.
*/
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
-+ packfile_store_for_each_pack(r->objects->packfiles, p) {
++ repo_for_each_pack(r, p) {
if ((p->pack_keep && (flags & ON_DISK_KEEP_PACKS)) ||
(p->pack_keep_in_core && (flags & IN_CORE_KEEP_PACKS))) {
ALLOC_GROW(packs, nr + 1, alloc);
@@ packfile.c: int for_each_packed_object(struct repository *repo, each_packed_obje
int pack_errors = 0;
- for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next) {
-+ packfile_store_for_each_pack(repo->objects->packfiles, p) {
++ repo_for_each_pack(repo, p) {
if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
continue;
if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&
@@ packfile.h: void packfile_store_reprepare(struct packfile_store *store);
struct packed_git *pack);
+/*
-+ * Load and iterate through all packs of the given packfile store.
++ * Load and iterate through all packs of the given repository. This helper
++ * function will yield packfiles from all object sources connected to the
++ * repository.
+ */
-+#define packfile_store_for_each_pack(store, p) \
-+ for (p = packfile_store_get_all_packs(store); p; p = p->next)
++#define repo_for_each_pack(repo, p) \
++ for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next)
+
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
## repack-cruft.c ##
-@@ repack-cruft.c: static void combine_small_cruft_packs(FILE *in, off_t combine_cruft_below_size,
+@@
+ static void combine_small_cruft_packs(FILE *in, off_t combine_cruft_below_size,
+ struct existing_packs *existing)
+ {
+- struct packfile_store *packs = existing->repo->objects->packfiles;
+ struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
size_t i;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(existing->repo, p) {
if (!(p->is_cruft && p->pack_local))
continue;
## repack-geometry.c ##
@@ repack-geometry.c: void pack_geometry_init(struct pack_geometry *geometry,
+ struct existing_packs *existing,
+ const struct pack_objects_args *args)
+ {
+- struct packfile_store *packs = existing->repo->objects->packfiles;
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(existing->repo, p) {
if (args->local && !p->pack_local)
/*
* When asked to only repack local packfiles we skip
## repack.c ##
-@@ repack.c: void existing_packs_collect(struct existing_packs *existing,
+@@ repack.c: int finish_pack_objects_cmd(const struct git_hash_algo *algop,
+ void existing_packs_collect(struct existing_packs *existing,
+ const struct string_list *extra_keep)
+ {
+- struct packfile_store *packs = existing->repo->objects->packfiles;
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(existing->repo, p) {
size_t i;
const char *base;
## server-info.c ##
-@@ server-info.c: static void init_pack_info(struct repository *r, const char *infofile, int force
+@@ server-info.c: static int compare_info(const void *a_, const void *b_)
+
+ static void init_pack_info(struct repository *r, const char *infofile, int force)
+ {
+- struct packfile_store *packs = r->objects->packfiles;
+ struct packed_git *p;
+ int stale;
int i;
size_t alloc = 0;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
-+ packfile_store_for_each_pack(packs, p) {
++ repo_for_each_pack(r, p) {
/* we ignore things on alternate path since they are
* not available to the pullers in general.
*/
@@ t/helper/test-find-pack.c: int cmd__find_pack(int argc, const char **argv)
die("cannot parse %s as an object name", argv[0]);
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles); p; p = p->next)
-+ packfile_store_for_each_pack(the_repository->objects->packfiles, p) {
++ repo_for_each_pack(the_repository, p) {
if (find_pack_entry_one(&oid, p)) {
printf("%s\n", p->pack_name);
actual_count++;
@@ t/helper/test-pack-mtimes.c: int cmd__pack_mtimes(int argc, const char **argv)
usage(pack_mtimes_usage);
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles); p; p = p->next) {
-+ packfile_store_for_each_pack(the_repository->objects->packfiles, p) {
++ repo_for_each_pack(the_repository, p) {
strbuf_addstr(&buf, basename(p->pack_name));
strbuf_strip_suffix(&buf, ".pack");
strbuf_addstr(&buf, ".mtimes");
6: ceceb9cc71 ! 6: a688fef987 packfile: rename `packfile_store_get_all_packs()`
@@ packfile.c: void packfile_store_reprepare(struct packfile_store *store)
## packfile.h ##
@@ packfile.h: void packfile_store_add_pack(struct packfile_store *store,
- * Load and iterate through all packs of the given packfile store.
+ * repository.
*/
- #define packfile_store_for_each_pack(store, p) \
-- for (p = packfile_store_get_all_packs(store); p; p = p->next)
-+ for (p = packfile_store_get_packs(store); p; p = p->next)
+ #define repo_for_each_pack(repo, p) \
+- for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next)
++ for (p = packfile_store_get_packs(repo->objects->packfiles); p; p = p->next)
/*
* Get all packs managed by the given store, including packfiles that are
---
base-commit: a0cb7ee3bf6c8398ab18e7b0dfabec106312b2f8
change-id: 20250926-pks-packfiles-convert-get-all-4236f6e2ed9b
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()`
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-14 19:07 ` Justin Tobler
2025-10-09 8:01 ` [PATCH v2 2/6] builtin/gc: " Patrick Steinhardt
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
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.
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.
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);
}
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()`
2025-10-09 8:01 ` [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-14 19:07 ` Justin Tobler
0 siblings, 0 replies; 27+ messages in thread
From: Justin Tobler @ 2025-10-14 19:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau
On 25/10/09 10:01AM, Patrick Steinhardt wrote:
> 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.
> 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.
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 2/6] builtin/gc: convert to use `packfile_store_get_all_packs()`
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 3/6] builtin/grep: simplify how we preload packs Patrick Steinhardt
` (4 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
When running maintenance tasks via git-maintenance(1) we have a couple
of auto-conditions that check whether or not a specific task should be
running. One such check is for incremental repacks, which essentially
use `git multi-pack-index repack` to repack a set of smaller packfiles
into one larger packfile.
The auto-condition for this task checks how many packfiles there are
that aren't indexed by any multi-pack index. If there is a sufficient
number then we execute the above command to combine those into a single
pack and add that pack to the MIDX.
As we don't care about MIDX'd packs we use `packfile_store_get_packs()`,
which knows to not load any packs that are indexed by a MIDX. But as
explained in the preceding commit, we want to get rid of that function.
We already handle packfiles that have a MIDX by the very nature of this
function, as we explicitly count non-MIDX'd packs. As such, we can
trivially switch over to use `packfile_store_get_all_packs()` instead.
Do so.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/gc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index e19e13d978..ab6d6d3bd1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1425,7 +1425,7 @@ static int incremental_repack_auto_condition(struct gc_config *cfg UNUSED)
if (incremental_repack_auto_limit < 0)
return 1;
- for (p = packfile_store_get_packs(the_repository->objects->packfiles);
+ for (p = packfile_store_get_all_packs(the_repository->objects->packfiles);
count < incremental_repack_auto_limit && p;
p = p->next) {
if (!p->multi_pack_index)
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v2 3/6] builtin/grep: simplify how we preload packs
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 1/6] object-name: convert to use `packfile_store_get_all_packs()` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 2/6] builtin/gc: " Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-09 8:01 ` [PATCH v2 4/6] packfile: drop `packfile_store_get_packs()` Patrick Steinhardt
` (3 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
When using multiple threads in git-grep(1) we eagerly preload both the
gitmodules file as well as the packfiles so that the threads won't race
with one another to initialize these data structures.
For packfiles, this is done by calling `packfile_store_get_packs()`,
which first loads our packfiles and then returns a pointer to the first
such packfile. This pointer is ignored though, as all we really care
about is that `packfile_store_prepare()` was called.
Historically, that function was file-local to "packfile.c", but that
changed with 4188332569 (packfile: move `get_multi_pack_index()` into
"midx.c", 2025-09-02). We can thus simplify the code by calling that
function directly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/grep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 13841fbf00..53cccf2d25 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1214,7 +1214,7 @@ int cmd_grep(int argc,
if (recurse_submodules)
repo_read_gitmodules(the_repository, 1);
if (startup_info->have_repository)
- (void)packfile_store_get_packs(the_repository->objects->packfiles);
+ packfile_store_prepare(the_repository->objects->packfiles);
start_threads(&opt);
} else {
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v2 4/6] packfile: drop `packfile_store_get_packs()`
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (2 preceding siblings ...)
2025-10-09 8:01 ` [PATCH v2 3/6] builtin/grep: simplify how we preload packs Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-14 19:11 ` Justin Tobler
2025-10-09 8:01 ` [PATCH v2 5/6] packfile: introduce macro to iterate through packs Patrick Steinhardt
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In the preceding commits we have removed all remaining callers of
`packfile_store_get_packs()`, the function is thus unused now. Remove
it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
packfile.c | 6 ------
packfile.h | 6 ------
2 files changed, 12 deletions(-)
diff --git a/packfile.c b/packfile.c
index 5a7caec292..db748b0bd4 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1027,12 +1027,6 @@ void packfile_store_reprepare(struct packfile_store *store)
packfile_store_prepare(store);
}
-struct packed_git *packfile_store_get_packs(struct packfile_store *store)
-{
- packfile_store_prepare(store);
- return store->packs;
-}
-
struct packed_git *packfile_store_get_all_packs(struct packfile_store *store)
{
packfile_store_prepare(store);
diff --git a/packfile.h b/packfile.h
index e7a5792b6c..3f38c63476 100644
--- a/packfile.h
+++ b/packfile.h
@@ -136,12 +136,6 @@ void packfile_store_reprepare(struct packfile_store *store);
void packfile_store_add_pack(struct packfile_store *store,
struct packed_git *pack);
-/*
- * Get packs managed by the given store. Does not load the MIDX or any packs
- * referenced by it.
- */
-struct packed_git *packfile_store_get_packs(struct packfile_store *store);
-
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/6] packfile: drop `packfile_store_get_packs()`
2025-10-09 8:01 ` [PATCH v2 4/6] packfile: drop `packfile_store_get_packs()` Patrick Steinhardt
@ 2025-10-14 19:11 ` Justin Tobler
0 siblings, 0 replies; 27+ messages in thread
From: Justin Tobler @ 2025-10-14 19:11 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau
On 25/10/09 10:01AM, Patrick Steinhardt wrote:
> In the preceding commits we have removed all remaining callers of
> `packfile_store_get_packs()`, the function is thus unused now. Remove
> it.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> -/*
> - * Get packs managed by the given store. Does not load the MIDX or any packs
> - * referenced by it.
> - */
> -struct packed_git *packfile_store_get_packs(struct packfile_store *store);
Nice cleanup. Now with `packfile_store_get_packs()` gone, the confusing
stateful behavior here is no more. :)
-Justin
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 5/6] packfile: introduce macro to iterate through packs
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (3 preceding siblings ...)
2025-10-09 8:01 ` [PATCH v2 4/6] packfile: drop `packfile_store_get_packs()` Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-14 19:17 ` Justin Tobler
2025-10-09 8:01 ` [PATCH v2 6/6] packfile: rename `packfile_store_get_all_packs()` Patrick Steinhardt
2025-10-14 19:19 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Justin Tobler
6 siblings, 1 reply; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
We have a bunch of different sites that want to iterate through all
packs of a given `struct packfile_store`. This pattern is somewhat
verbose and repetitive, which makes it somewhat cumbersome.
Introduce a new macro `repo_for_each_pack()` that removes some of the
boilerplate.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/cat-file.c | 3 +--
builtin/count-objects.c | 3 +--
builtin/fsck.c | 15 ++++++---------
builtin/gc.c | 16 +++++++---------
builtin/pack-objects.c | 22 +++++++---------------
builtin/pack-redundant.c | 14 ++++----------
connected.c | 3 +--
http-backend.c | 5 ++---
http.c | 3 +--
object-name.c | 8 +++++---
pack-bitmap.c | 6 +++---
pack-objects.c | 5 ++---
packfile.c | 4 ++--
packfile.h | 8 ++++++++
repack-cruft.c | 3 +--
repack-geometry.c | 3 +--
repack.c | 3 +--
server-info.c | 3 +--
t/helper/test-find-pack.c | 3 ++-
t/helper/test-pack-mtimes.c | 2 +-
20 files changed, 57 insertions(+), 75 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ee6715fa52..0ab076aeb3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -852,10 +852,9 @@ static void batch_each_object(struct batch_options *opt,
if (bitmap && !for_each_bitmapped_object(bitmap, &opt->objects_filter,
batch_one_object_bitmapped, &payload)) {
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *pack;
- for (pack = packfile_store_get_all_packs(packs); pack; pack = pack->next) {
+ repo_for_each_pack(the_repository, pack) {
if (bitmap_index_contains_pack(bitmap, pack) ||
open_pack_index(pack))
continue;
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index f2f407c2a7..18f6e33b6f 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -122,7 +122,6 @@ int cmd_count_objects(int argc,
count_loose, count_cruft, NULL, NULL);
if (verbose) {
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
unsigned long num_pack = 0;
off_t size_pack = 0;
@@ -130,7 +129,7 @@ int cmd_count_objects(int argc,
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
if (open_pack_index(p))
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 8ee95e0d67..b1a650c673 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -867,20 +867,20 @@ static int mark_packed_for_connectivity(const struct object_id *oid,
static int check_pack_rev_indexes(struct repository *r, int show_progress)
{
- struct packfile_store *packs = r->objects->packfiles;
struct progress *progress = NULL;
+ struct packed_git *p;
uint32_t pack_count = 0;
int res = 0;
if (show_progress) {
- for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next)
+ repo_for_each_pack(r, p)
pack_count++;
progress = start_delayed_progress(the_repository,
"Verifying reverse pack-indexes", pack_count);
pack_count = 0;
}
- for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(r, p) {
int load_error = load_pack_revindex_from_disk(p);
if (load_error < 0) {
@@ -1000,8 +1000,6 @@ int cmd_fsck(int argc,
for_each_packed_object(the_repository,
mark_packed_for_connectivity, NULL, 0);
} else {
- struct packfile_store *packs = the_repository->objects->packfiles;
-
odb_prepare_alternates(the_repository->objects);
for (source = the_repository->objects->sources; source; source = source->next)
fsck_source(source);
@@ -1012,8 +1010,7 @@ int cmd_fsck(int argc,
struct progress *progress = NULL;
if (show_progress) {
- for (p = packfile_store_get_all_packs(packs); p;
- p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (open_pack_index(p))
continue;
total += p->num_objects;
@@ -1022,8 +1019,8 @@ int cmd_fsck(int argc,
progress = start_progress(the_repository,
_("Checking objects"), total);
}
- for (p = packfile_store_get_all_packs(packs); p;
- p = p->next) {
+
+ repo_for_each_pack(the_repository, p) {
/* verify gives error messages itself */
if (verify_pack(the_repository,
p, fsck_obj_buffer,
diff --git a/builtin/gc.c b/builtin/gc.c
index ab6d6d3bd1..541d7471f1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -487,10 +487,9 @@ static int too_many_loose_objects(struct gc_config *cfg)
static struct packed_git *find_base_packs(struct string_list *packs,
unsigned long limit)
{
- struct packfile_store *packfiles = the_repository->objects->packfiles;
struct packed_git *p, *base = NULL;
- for (p = packfile_store_get_all_packs(packfiles); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local || p->is_cruft)
continue;
if (limit) {
@@ -509,14 +508,13 @@ static struct packed_git *find_base_packs(struct string_list *packs,
static int too_many_packs(struct gc_config *cfg)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- int cnt;
+ int cnt = 0;
if (cfg->gc_auto_pack_limit <= 0)
return 0;
- for (cnt = 0, p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
if (p->pack_keep)
@@ -1425,9 +1423,9 @@ static int incremental_repack_auto_condition(struct gc_config *cfg UNUSED)
if (incremental_repack_auto_limit < 0)
return 1;
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles);
- count < incremental_repack_auto_limit && p;
- p = p->next) {
+ repo_for_each_pack(the_repository, p) {
+ if (count >= incremental_repack_auto_limit)
+ break;
if (!p->multi_pack_index)
count++;
}
@@ -1494,7 +1492,7 @@ static off_t get_auto_pack_size(void)
struct repository *r = the_repository;
odb_reprepare(r->objects);
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
+ repo_for_each_pack(r, p) {
if (p->pack_size > max_size) {
second_largest_size = max_size;
max_size = p->pack_size;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index fe987fbb8b..50618e1073 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3831,12 +3831,10 @@ static int pack_mtime_cmp(const void *_a, const void *_b)
static void read_packs_list_from_stdin(struct rev_info *revs)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct strbuf buf = STRBUF_INIT;
struct string_list include_packs = STRING_LIST_INIT_DUP;
struct string_list exclude_packs = STRING_LIST_INIT_DUP;
struct string_list_item *item = NULL;
-
struct packed_git *p;
while (strbuf_getline(&buf, stdin) != EOF) {
@@ -3856,7 +3854,7 @@ static void read_packs_list_from_stdin(struct rev_info *revs)
string_list_sort(&exclude_packs);
string_list_remove_duplicates(&exclude_packs, 0);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
const char *pack_name = pack_basename(p);
if ((item = string_list_lookup(&include_packs, pack_name)))
@@ -4077,7 +4075,6 @@ static void enumerate_cruft_objects(void)
static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
struct rev_info revs;
int ret;
@@ -4107,7 +4104,7 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
*/
- for (p = packfile_store_get_all_packs(packs); p; p = p->next)
+ repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
@@ -4124,7 +4121,6 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
static void read_cruft_objects(void)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct strbuf buf = STRBUF_INIT;
struct string_list discard_packs = STRING_LIST_INIT_DUP;
struct string_list fresh_packs = STRING_LIST_INIT_DUP;
@@ -4145,7 +4141,7 @@ static void read_cruft_objects(void)
string_list_sort(&discard_packs);
string_list_sort(&fresh_packs);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
const char *pack_name = pack_basename(p);
struct string_list_item *item;
@@ -4440,13 +4436,12 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,
static void loosen_unused_packed_objects(void)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
uint32_t i;
uint32_t loosened_objects_nr = 0;
struct object_id oid;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local || p->pack_keep || p->pack_keep_in_core)
continue;
@@ -4747,13 +4742,12 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
static void add_extra_kept_packs(const struct string_list *names)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
if (!names->nr)
return;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
const char *name = basename(p->pack_name);
int i;
@@ -5191,10 +5185,9 @@ int cmd_pack_objects(int argc,
add_extra_kept_packs(&keep_pack_list);
if (ignore_packed_keep_on_disk) {
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next)
+ repo_for_each_pack(the_repository, p)
if (p->pack_local && p->pack_keep)
break;
if (!p) /* no keep-able packs found */
@@ -5206,10 +5199,9 @@ int cmd_pack_objects(int argc,
* want to unset "local" based on looking at packs, as
* it also covers non-local objects
*/
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local) {
have_non_local_packs = 1;
break;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index dd28171f0a..fca7f195d6 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -566,29 +566,23 @@ static struct pack_list * add_pack(struct packed_git *p)
static struct pack_list * add_pack_file(const char *filename)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
- struct packed_git *p = packfile_store_get_all_packs(packs);
+ struct packed_git *p;
if (strlen(filename) < 40)
die("Bad pack filename: %s", filename);
- while (p) {
+ repo_for_each_pack(the_repository, p)
if (strstr(p->pack_name, filename))
return add_pack(p);
- p = p->next;
- }
die("Filename %s not found in packed_git", filename);
}
static void load_all(void)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
- struct packed_git *p = packfile_store_get_all_packs(packs);
+ struct packed_git *p;
- while (p) {
+ repo_for_each_pack(the_repository, p)
add_pack(p);
- p = p->next;
- }
}
int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, struct repository *repo UNUSED) {
diff --git a/connected.c b/connected.c
index b288a18b17..79403108dd 100644
--- a/connected.c
+++ b/connected.c
@@ -74,10 +74,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
*/
odb_reprepare(the_repository->objects);
do {
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *p;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_promisor)
continue;
if (find_pack_entry_one(oid, p))
diff --git a/http-backend.c b/http-backend.c
index 9084058f1e..52f0483dd3 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -603,19 +603,18 @@ static void get_head(struct strbuf *hdr, char *arg UNUSED)
static void get_info_packs(struct strbuf *hdr, char *arg UNUSED)
{
size_t objdirlen = strlen(repo_get_object_directory(the_repository));
- struct packfile_store *packs = the_repository->objects->packfiles;
struct strbuf buf = STRBUF_INIT;
struct packed_git *p;
size_t cnt = 0;
select_getanyfile(hdr);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (p->pack_local)
cnt++;
}
strbuf_grow(&buf, cnt * 53 + 2);
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (p->pack_local)
strbuf_addf(&buf, "P %s\n", p->pack_name + objdirlen + 6);
}
diff --git a/http.c b/http.c
index 7e3af1e72f..17130823f0 100644
--- a/http.c
+++ b/http.c
@@ -2416,7 +2416,6 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)
static int fetch_and_setup_pack_index(struct packed_git **packs_head,
unsigned char *sha1, const char *base_url)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
struct packed_git *new_pack, *p;
char *tmp_idx = NULL;
int ret;
@@ -2425,7 +2424,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
* If we already have the pack locally, no need to fetch its index or
* even add it to list; we already have all of its objects.
*/
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
if (hasheq(p->hash, sha1, the_repository->hash_algo))
return 0;
}
diff --git a/object-name.c b/object-name.c
index 4e62bfa330..766c757042 100644
--- a/object-name.c
+++ b/object-name.c
@@ -213,9 +213,11 @@ static void find_short_packed_object(struct disambiguate_state *ds)
unique_in_midx(m, ds);
}
- for (p = packfile_store_get_all_packs(ds->repo->objects->packfiles); p && !ds->ambiguous;
- p = p->next)
+ repo_for_each_pack(ds->repo, p) {
+ if (ds->ambiguous)
+ break;
unique_in_pack(p, ds);
+ }
}
static int finish_object_disambiguation(struct disambiguate_state *ds,
@@ -805,7 +807,7 @@ static void find_abbrev_len_packed(struct min_abbrev_data *mad)
find_abbrev_len_for_midx(m, mad);
}
- for (p = packfile_store_get_all_packs(mad->repo->objects->packfiles); p; p = p->next)
+ repo_for_each_pack(mad->repo, p)
find_abbrev_len_for_pack(p, mad);
}
diff --git a/pack-bitmap.c b/pack-bitmap.c
index ac71035d77..291e1a9cf4 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -664,7 +664,7 @@ static int open_pack_bitmap(struct repository *r,
struct packed_git *p;
int ret = -1;
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
+ repo_for_each_pack(r, p) {
if (open_pack_bitmap_1(bitmap_git, p) == 0) {
ret = 0;
/*
@@ -3347,6 +3347,7 @@ static int verify_bitmap_file(const struct git_hash_algo *algop,
int verify_bitmap_files(struct repository *r)
{
struct odb_source *source;
+ struct packed_git *p;
int res = 0;
odb_prepare_alternates(r->objects);
@@ -3362,8 +3363,7 @@ int verify_bitmap_files(struct repository *r)
free(midx_bitmap_name);
}
- for (struct packed_git *p = packfile_store_get_all_packs(r->objects->packfiles);
- p; p = p->next) {
+ repo_for_each_pack(r, p) {
char *pack_bitmap_name = pack_bitmap_filename(p);
res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
free(pack_bitmap_name);
diff --git a/pack-objects.c b/pack-objects.c
index d8eb679735..d6adf0759c 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -86,7 +86,6 @@ struct object_entry *packlist_find(struct packing_data *pdata,
static void prepare_in_pack_by_idx(struct packing_data *pdata)
{
- struct packfile_store *packs = pdata->repo->objects->packfiles;
struct packed_git **mapping, *p;
int cnt = 0, nr = 1U << OE_IN_PACK_BITS;
@@ -96,13 +95,13 @@ static void prepare_in_pack_by_idx(struct packing_data *pdata)
* (i.e. in_pack_idx also zero) should return NULL.
*/
mapping[cnt++] = NULL;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next, cnt++) {
+ repo_for_each_pack(pdata->repo, p) {
if (cnt == nr) {
free(mapping);
return;
}
p->index = cnt;
- mapping[cnt] = p;
+ mapping[cnt++] = p;
}
pdata->in_pack_by_idx = mapping;
}
diff --git a/packfile.c b/packfile.c
index db748b0bd4..ab5859518d 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2099,7 +2099,7 @@ struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)
* covers, one kept and one not kept, but the midx returns only
* the non-kept version.
*/
- for (p = packfile_store_get_all_packs(r->objects->packfiles); p; p = p->next) {
+ repo_for_each_pack(r, p) {
if ((p->pack_keep && (flags & ON_DISK_KEEP_PACKS)) ||
(p->pack_keep_in_core && (flags & IN_CORE_KEEP_PACKS))) {
ALLOC_GROW(packs, nr + 1, alloc);
@@ -2196,7 +2196,7 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,
int r = 0;
int pack_errors = 0;
- for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next) {
+ repo_for_each_pack(repo, p) {
if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
continue;
if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&
diff --git a/packfile.h b/packfile.h
index 3f38c63476..49484a9b09 100644
--- a/packfile.h
+++ b/packfile.h
@@ -136,6 +136,14 @@ void packfile_store_reprepare(struct packfile_store *store);
void packfile_store_add_pack(struct packfile_store *store,
struct packed_git *pack);
+/*
+ * Load and iterate through all packs of the given repository. This helper
+ * function will yield packfiles from all object sources connected to the
+ * repository.
+ */
+#define repo_for_each_pack(repo, p) \
+ for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next)
+
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
diff --git a/repack-cruft.c b/repack-cruft.c
index accb98bcdb..544a6f2df3 100644
--- a/repack-cruft.c
+++ b/repack-cruft.c
@@ -7,12 +7,11 @@
static void combine_small_cruft_packs(FILE *in, off_t combine_cruft_below_size,
struct existing_packs *existing)
{
- struct packfile_store *packs = existing->repo->objects->packfiles;
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
size_t i;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(existing->repo, p) {
if (!(p->is_cruft && p->pack_local))
continue;
diff --git a/repack-geometry.c b/repack-geometry.c
index 56d651c9ce..4e51003ecc 100644
--- a/repack-geometry.c
+++ b/repack-geometry.c
@@ -29,11 +29,10 @@ void pack_geometry_init(struct pack_geometry *geometry,
struct existing_packs *existing,
const struct pack_objects_args *args)
{
- struct packfile_store *packs = existing->repo->objects->packfiles;
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(existing->repo, p) {
if (args->local && !p->pack_local)
/*
* When asked to only repack local packfiles we skip
diff --git a/repack.c b/repack.c
index 1982a48165..d690e68718 100644
--- a/repack.c
+++ b/repack.c
@@ -124,11 +124,10 @@ int finish_pack_objects_cmd(const struct git_hash_algo *algop,
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep)
{
- struct packfile_store *packs = existing->repo->objects->packfiles;
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(existing->repo, p) {
size_t i;
const char *base;
diff --git a/server-info.c b/server-info.c
index 1d33de821e..b9a710544a 100644
--- a/server-info.c
+++ b/server-info.c
@@ -287,13 +287,12 @@ static int compare_info(const void *a_, const void *b_)
static void init_pack_info(struct repository *r, const char *infofile, int force)
{
- struct packfile_store *packs = r->objects->packfiles;
struct packed_git *p;
int stale;
int i;
size_t alloc = 0;
- for (p = packfile_store_get_all_packs(packs); p; p = p->next) {
+ repo_for_each_pack(r, p) {
/* we ignore things on alternate path since they are
* not available to the pullers in general.
*/
diff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c
index e001dc3066..fc4b8a77b3 100644
--- a/t/helper/test-find-pack.c
+++ b/t/helper/test-find-pack.c
@@ -39,11 +39,12 @@ int cmd__find_pack(int argc, const char **argv)
if (repo_get_oid(the_repository, argv[0], &oid))
die("cannot parse %s as an object name", argv[0]);
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles); p; p = p->next)
+ repo_for_each_pack(the_repository, p) {
if (find_pack_entry_one(&oid, p)) {
printf("%s\n", p->pack_name);
actual_count++;
}
+ }
if (count > -1 && count != actual_count)
die("bad packfile count %d instead of %d", actual_count, count);
diff --git a/t/helper/test-pack-mtimes.c b/t/helper/test-pack-mtimes.c
index 7c428c1601..7a8ee1de24 100644
--- a/t/helper/test-pack-mtimes.c
+++ b/t/helper/test-pack-mtimes.c
@@ -37,7 +37,7 @@ int cmd__pack_mtimes(int argc, const char **argv)
if (argc != 2)
usage(pack_mtimes_usage);
- for (p = packfile_store_get_all_packs(the_repository->objects->packfiles); p; p = p->next) {
+ repo_for_each_pack(the_repository, p) {
strbuf_addstr(&buf, basename(p->pack_name));
strbuf_strip_suffix(&buf, ".pack");
strbuf_addstr(&buf, ".mtimes");
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 5/6] packfile: introduce macro to iterate through packs
2025-10-09 8:01 ` [PATCH v2 5/6] packfile: introduce macro to iterate through packs Patrick Steinhardt
@ 2025-10-14 19:17 ` Justin Tobler
0 siblings, 0 replies; 27+ messages in thread
From: Justin Tobler @ 2025-10-14 19:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau
On 25/10/09 10:01AM, Patrick Steinhardt wrote:
> We have a bunch of different sites that want to iterate through all
> packs of a given `struct packfile_store`. This pattern is somewhat
> verbose and repetitive, which makes it somewhat cumbersome.
>
> Introduce a new macro `repo_for_each_pack()` that removes some of the
> boilerplate.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> diff --git a/packfile.h b/packfile.h
> index 3f38c63476..49484a9b09 100644
> --- a/packfile.h
> +++ b/packfile.h
> @@ -136,6 +136,14 @@ void packfile_store_reprepare(struct packfile_store *store);
> void packfile_store_add_pack(struct packfile_store *store,
> struct packed_git *pack);
>
> +/*
> + * Load and iterate through all packs of the given repository. This helper
> + * function will yield packfiles from all object sources connected to the
> + * repository.
> + */
> +#define repo_for_each_pack(repo, p) \
> + for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next)
The added macro here is a nice quality of life improvement and the
updated call sites all look good.
-Justin
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 6/6] packfile: rename `packfile_store_get_all_packs()`
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (4 preceding siblings ...)
2025-10-09 8:01 ` [PATCH v2 5/6] packfile: introduce macro to iterate through packs Patrick Steinhardt
@ 2025-10-09 8:01 ` Patrick Steinhardt
2025-10-14 19:19 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Justin Tobler
6 siblings, 0 replies; 27+ messages in thread
From: Patrick Steinhardt @ 2025-10-09 8:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In a preceding commit we have removed `packfile_store_get_packs()`. With
this function removed it's somewhat useless to still have the "all"
infix in `packfile_store_get_all_packs()`. Rename the latter to drop
that infix.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fast-import.c | 4 ++--
builtin/pack-objects.c | 4 ++--
packfile.c | 2 +-
packfile.h | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index b1d5549815..fea914cf9e 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -976,7 +976,7 @@ static int store_object(
if (e->idx.offset) {
duplicate_count_by_type[type]++;
return 1;
- } else if (find_oid_pack(&oid, packfile_store_get_all_packs(packs))) {
+ } else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {
e->type = type;
e->pack_id = MAX_PACK_ID;
e->idx.offset = 1; /* just not zero! */
@@ -1177,7 +1177,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
duplicate_count_by_type[OBJ_BLOB]++;
truncate_pack(&checkpoint);
- } else if (find_oid_pack(&oid, packfile_store_get_all_packs(packs))) {
+ } else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {
e->type = OBJ_BLOB;
e->pack_id = MAX_PACK_ID;
e->idx.offset = 1; /* just not zero! */
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 50618e1073..3a19bddd57 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4394,7 +4394,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
struct packed_git *p;
p = (last_found != (void *)1) ? last_found :
- packfile_store_get_all_packs(packs);
+ packfile_store_get_packs(packs);
while (p) {
if ((!p->pack_local || p->pack_keep ||
@@ -4404,7 +4404,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
return 1;
}
if (p == last_found)
- p = packfile_store_get_all_packs(packs);
+ p = packfile_store_get_packs(packs);
else
p = p->next;
if (p == last_found)
diff --git a/packfile.c b/packfile.c
index ab5859518d..1ae2b2fe1e 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1027,7 +1027,7 @@ void packfile_store_reprepare(struct packfile_store *store)
packfile_store_prepare(store);
}
-struct packed_git *packfile_store_get_all_packs(struct packfile_store *store)
+struct packed_git *packfile_store_get_packs(struct packfile_store *store)
{
packfile_store_prepare(store);
diff --git a/packfile.h b/packfile.h
index 49484a9b09..c9d0b93446 100644
--- a/packfile.h
+++ b/packfile.h
@@ -142,13 +142,13 @@ void packfile_store_add_pack(struct packfile_store *store,
* repository.
*/
#define repo_for_each_pack(repo, p) \
- for (p = packfile_store_get_all_packs(repo->objects->packfiles); p; p = p->next)
+ for (p = packfile_store_get_packs(repo->objects->packfiles); p; p = p->next)
/*
* Get all packs managed by the given store, including packfiles that are
* referenced by multi-pack indices.
*/
-struct packed_git *packfile_store_get_all_packs(struct packfile_store *store);
+struct packed_git *packfile_store_get_packs(struct packfile_store *store);
/*
* Get all packs in most-recently-used order.
--
2.51.0.764.g787ff6f08a.dirty
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()`
2025-10-09 8:01 ` [PATCH v2 0/6] packfile: remove `packfile_store_get_packs()` Patrick Steinhardt
` (5 preceding siblings ...)
2025-10-09 8:01 ` [PATCH v2 6/6] packfile: rename `packfile_store_get_all_packs()` Patrick Steinhardt
@ 2025-10-14 19:19 ` Justin Tobler
6 siblings, 0 replies; 27+ messages in thread
From: Justin Tobler @ 2025-10-14 19:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau
On 25/10/09 10:01AM, Patrick Steinhardt wrote:
> Changes in v2:
> - Some commit message improvements.
> - Adapt `packfile_store_for_each_pack()` to `repo_for_each_pack()`. On
> the one hand this provides easier ergonomics. On the other hand this
> will eventually allow us to handle object databases of a different
> type more easily.
> - Link to v1: https://lore.kernel.org/r/20251007-pks-packfiles-convert-get-all-v1-0-428227657a89@pks.im
Thanks Patrick for the pleasant read. I've reviewed this version at it
looks good to me. :)
-Justin
^ permalink raw reply [flat|nested] 27+ messages in thread