From: Taylor Blau <me@ttaylorr•com>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 3/8] midx: stop using linked list when closing MIDX
Date: Thu, 10 Jul 2025 19:22:58 -0400 [thread overview]
Message-ID: <aHBLUg7Y1cgf8i7k@nand.local> (raw)
In-Reply-To: <20250709-b4-pks-midx-via-odb-alternate-v1-3-f31150d21331@pks.im>
On Wed, Jul 09, 2025 at 09:54:51AM +0200, Patrick Steinhardt wrote:
> When calling `close_midx()` we not only close the multi-pack index for
> one object source, but instead we iterate through the whole linked list
> of MIDXs to close all of them. This linked list is about to go away in
> favor of using the new per-source pointer to its respective MIDX.
>
> Refactor the function to iterate through sources instead.
>
> Signed-off-by: Patrick Steinhardt <ps@pks•im>
> ---
> midx.c | 11 ++++++-----
> packfile.c | 10 +++++-----
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index a91231bfcdf..416b3e8b54f 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -401,7 +401,6 @@ void close_midx(struct multi_pack_index *m)
> if (!m)
> return;
>
> - close_midx(m->next);
OK, so previously this recursive call to "close_midx()" would have freed
up the `m->next` MIDX...
> close_midx(m->base_midx);
>
> munmap((unsigned char *)m->data, m->data_len);
> @@ -835,11 +834,13 @@ void clear_midx_file(struct repository *r)
>
> get_midx_filename(r->hash_algo, &midx, r->objects->sources->path);
>
> - if (r->objects && r->objects->multi_pack_index) {
> - close_midx(r->objects->multi_pack_index);
> - r->objects->multi_pack_index = NULL;
> - for (struct odb_source *source = r->objects->sources; source; source = source->next)
> + if (r->objects) {
> + for (struct odb_source *source = r->objects->sources; source; source = source->next) {
> + if (source->multi_pack_index)
> + close_midx(source->multi_pack_index);
> source->multi_pack_index = NULL;
...and then this line would NULL the now-free()'d memory out.
But instead we are directly iterating through the sources and both
closing and NULL-ing out their respective MIDXs (if any).
As an aside: I know we do the C99-style for loop with declarations in
many places, but in this instance it seems to have produced an awfully
long line. I wonder if in this instance it would be better to write:
struct odb_source *source;
for (source = r->objects->sources; source; source = source->next) {
/* ... */
}
That's still a little lengthy, but it's fewer than 80 characters ;-).
> + }
> + r->objects->multi_pack_index = NULL;
Presumably this pointer will go away at some point in the future as
well?
> }
>
> if (remove_path(midx.buf))
> diff --git a/packfile.c b/packfile.c
> index b43dd2fe6cb..546c161d0c1 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -369,12 +369,12 @@ void close_object_store(struct object_database *o)
> else
> close_pack(p);
>
> - if (o->multi_pack_index) {
> - close_midx(o->multi_pack_index);
> - o->multi_pack_index = NULL;
> - for (struct odb_source *source = o->sources; source; source = source->next)
> - source->multi_pack_index = NULL;
> + for (struct odb_source *source = o->sources; source; source = source->next) {
Same comment here as well.
Thanks,
Taylor
next prev parent reply other threads:[~2025-07-10 23:23 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-09 7:54 [PATCH 0/8] odb: track multi-pack-indices via their object sources Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 1/8] midx: start tracking per object database source Patrick Steinhardt
2025-07-10 20:56 ` Justin Tobler
2025-07-10 23:10 ` Taylor Blau
2025-07-10 23:19 ` Taylor Blau
2025-07-15 8:26 ` Patrick Steinhardt
2025-07-15 8:26 ` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 2/8] packfile: refactor `prepare_packed_git_one()` to work on sources Patrick Steinhardt
2025-07-10 21:07 ` Justin Tobler
2025-07-10 23:14 ` Taylor Blau
2025-07-15 8:26 ` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 3/8] midx: stop using linked list when closing MIDX Patrick Steinhardt
2025-07-10 21:31 ` Justin Tobler
2025-07-15 8:26 ` Patrick Steinhardt
2025-07-10 23:22 ` Taylor Blau [this message]
2025-07-15 8:26 ` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 4/8] midx: track whether we have loaded the MIDX Patrick Steinhardt
2025-07-10 22:16 ` Justin Tobler
2025-07-10 23:26 ` Taylor Blau
2025-07-15 8:27 ` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 5/8] packfile: refactor `get_multi_pack_index()` to work on sources Patrick Steinhardt
2025-07-10 22:35 ` Justin Tobler
2025-07-10 23:56 ` Taylor Blau
2025-07-15 8:27 ` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 6/8] packfile: stop using linked MIDX list in `find_pack_entry()` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 7/8] packfile: stop using linked MIDX list in `get_all_packs()` Patrick Steinhardt
2025-07-09 7:54 ` [PATCH 8/8] midx: remove now-unused linked list of multi-pack indices Patrick Steinhardt
2025-07-10 22:48 ` Justin Tobler
2025-07-09 22:04 ` [PATCH 0/8] odb: track multi-pack-indices via their object sources Junio C Hamano
2025-07-10 23:58 ` Taylor Blau
2025-07-15 8:27 ` Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 0/7] " Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 1/7] midx: start tracking per object database source Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 2/7] packfile: refactor `prepare_packed_git_one()` to work on sources Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 3/7] midx: stop using linked list when closing MIDX Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 4/7] packfile: refactor `get_multi_pack_index()` to work on sources Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 5/7] packfile: stop using linked MIDX list in `find_pack_entry()` Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 6/7] packfile: stop using linked MIDX list in `get_all_packs()` Patrick Steinhardt
2025-07-15 11:29 ` [PATCH v2 7/7] midx: remove now-unused linked list of multi-pack indices Patrick Steinhardt
2025-07-15 21:59 ` [PATCH v2 0/7] odb: track multi-pack-indices via their object sources Justin Tobler
2025-07-23 21:22 ` Junio C Hamano
2025-07-24 8:00 ` Patrick Steinhardt
2025-08-12 21:58 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aHBLUg7Y1cgf8i7k@nand.local \
--to=me@ttaylorr$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox