From: Toon Claes <toon@iotcl•com>
To: Patrick Steinhardt <ps@pks•im>, git@vger•kernel.org
Cc: Justin Tobler <jltobler@gmail•com>
Subject: Re: [PATCH v2 04/10] packfile: refactor misleading code when unusing pack windows
Date: Wed, 07 Jan 2026 11:13:11 +0100 [thread overview]
Message-ID: <87fr8hpw7c.fsf@iotcl.com> (raw)
In-Reply-To: <20251218-b4-pks-pack-store-via-source-v2-4-62849007ce21@pks.im>
Patrick Steinhardt <ps@pks•im> writes:
> The function `unuse_one_window()` is responsible for unmapping one of
> the packfile windows, which is done when we have exceeded the allowed
> number of window.
>
> The function receives a `struct packed_git` as input, which serves as an
> additional packfile that should be considered to be closed. If not
> given, we seemingly skip that and instead go through all of the
> repository's packfiles. The conditional that checks whether we have a
> packfile though does not make much sense anymore, as we dereference the
> packfile regardless of whether or not it is a `NULL` pointer to derive
> the repository's packfile store.
>
> The function was originally introduced via f0e17e86e1 (pack: move
> release_pack_memory(), 2017-08-18), and here we indeed had a caller that
> passed a `NULL` pointer. That caller was later removed via 9827d4c185
> (packfile: drop release_pack_memory(), 2019-08-12), so starting with
> that commit we always pass a `struct packed_git`. In 9c5ce06d74
> (packfile: use `repository` from `packed_git` directly, 2024-12-03) we
> then inadvertently started to rely on the fact that the pointer is never
> `NULL` because we use it now to identify the repository.
>
> Arguably, it didn't really make sense in the first place that the caller
> provides a packfile, as the selected window would have been overridden
> anyway by the subsequent loop over all packfiles if there was an older
> window. So the overall logic is quite misleading overall. The only case
> where it _could_ make a difference is when there were two packfiles with
> the same `last_used` value, but that case doesn't ever happen because
> the `pack_used_ctr` is strictly increasing.
I didn't even think about this edge case, so thanks for clarifying.
> Refactor the code so that we instead pass in the object database to
> help make the code less misleading.
Nice improvement.
--
Cheers,
Toon
next prev parent reply other threads:[~2026-01-07 10:13 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-15 7:36 [PATCH 00/10] Start tracking packfiles per object database source Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 01/10] packfile: create store via its owning source Patrick Steinhardt
2025-12-15 21:30 ` Justin Tobler
2025-12-16 8:36 ` Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 02/10] packfile: pass source to `prepare_pack()` Patrick Steinhardt
2025-12-15 21:38 ` Justin Tobler
2025-12-15 7:36 ` [PATCH 03/10] packfile: refactor kept-pack cache to work with packfile stores Patrick Steinhardt
2025-12-15 21:56 ` Justin Tobler
2025-12-16 9:09 ` Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 04/10] packfile: refactor misleading code when unusing pack windows Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 05/10] packfile: move packfile store into object source Patrick Steinhardt
2025-12-18 0:52 ` Justin Tobler
2025-12-18 6:50 ` Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 06/10] packfile: only prepare owning store in `packfile_store_get_packs()` Patrick Steinhardt
2025-12-18 0:58 ` Justin Tobler
2025-12-15 7:36 ` [PATCH 07/10] packfile: only prepare owning store in `packfile_store_prepare()` Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 08/10] packfile: inline `find_kept_pack_entry()` Patrick Steinhardt
2025-12-18 1:06 ` Justin Tobler
2025-12-18 6:48 ` Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 09/10] packfile: refactor `find_pack_entry()` to work on the packfile store Patrick Steinhardt
2025-12-15 7:36 ` [PATCH 10/10] packfile: move MIDX into " Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 00/10] Start tracking packfiles per object database source Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 01/10] packfile: create store via its owning source Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 02/10] packfile: pass source to `prepare_pack()` Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 03/10] packfile: refactor kept-pack cache to work with packfile stores Patrick Steinhardt
2026-01-06 20:42 ` Toon Claes
2025-12-18 6:55 ` [PATCH v2 04/10] packfile: refactor misleading code when unusing pack windows Patrick Steinhardt
2026-01-07 10:13 ` Toon Claes [this message]
2025-12-18 6:55 ` [PATCH v2 05/10] packfile: move packfile store into object source Patrick Steinhardt
2026-01-07 13:11 ` Toon Claes
2025-12-18 6:55 ` [PATCH v2 06/10] packfile: only prepare owning store in `packfile_store_get_packs()` Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 07/10] packfile: only prepare owning store in `packfile_store_prepare()` Patrick Steinhardt
2026-01-07 13:15 ` Toon Claes
2025-12-18 6:55 ` [PATCH v2 08/10] packfile: inline `find_kept_pack_entry()` Patrick Steinhardt
2026-01-08 7:16 ` Kristoffer Haugsbakk
2026-01-09 6:17 ` Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 09/10] packfile: refactor `find_pack_entry()` to work on the packfile store Patrick Steinhardt
2025-12-18 6:55 ` [PATCH v2 10/10] packfile: move MIDX into " Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 00/10] Start tracking packfiles per object database source Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 01/10] packfile: create store via its owning source Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 02/10] packfile: pass source to `prepare_pack()` Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 03/10] packfile: refactor kept-pack cache to work with packfile stores Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 04/10] packfile: refactor misleading code when unusing pack windows Patrick Steinhardt
2026-01-12 14:52 ` Karthik Nayak
2026-01-09 8:33 ` [PATCH v3 05/10] packfile: move packfile store into object source Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 06/10] packfile: only prepare owning store in `packfile_store_get_packs()` Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 07/10] packfile: only prepare owning store in `packfile_store_prepare()` Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 08/10] packfile: inline `find_kept_pack_entry()` Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 09/10] packfile: refactor `find_pack_entry()` to work on the packfile store Patrick Steinhardt
2026-01-09 8:33 ` [PATCH v3 10/10] packfile: move MIDX into " Patrick Steinhardt
2026-01-11 5:46 ` [PATCH v3 00/10] Start tracking packfiles per object database source Junio C Hamano
2026-01-12 15:27 ` Justin Tobler
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=87fr8hpw7c.fsf@iotcl.com \
--to=toon@iotcl$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jltobler@gmail$(echo .)com \
--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