From: Toon Claes <toon@iotcl.com>
Date: Wed, 07 Jan 2026 10:13:11 GMT
Subject: Re: [PATCH v2 04/10] packfile: refactor misleading code when unusing pack windows
Message-ID: <87fr8hpw7c.fsf@iotcl.com>
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