From: Karthik Nayak Date: Mon, 12 Jan 2026 14:52:56 GMT Subject: Re: [PATCH v3 04/10] packfile: refactor misleading code when unusing pack windows Message-ID: In-Reply-To: <20260109-b4-pks-pack-store-via-source-v3-4-877fd7b7bf81@pks.im> Patrick Steinhardt 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. > For my understanding, when dealing with packfiles, we mmap windows of the packfile onto memory. If this is above a threshold, we need to evict one of these mappings, this is done on LRU basis via the `unuse_one_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. Yup reading the function for the first time, that struck me immediately too. > 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. > Nice investigation, that explains. > 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. > > Refactor the code so that we instead pass in the object database to > help make the code less misleading. > > Signed-off-by: Patrick Steinhardt > --- > packfile.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/packfile.c b/packfile.c > index 191344eb1c..3700612465 100644 > --- a/packfile.c > +++ b/packfile.c > @@ -355,16 +355,15 @@ static void scan_windows(struct packed_git *p, > } > } > > -static int unuse_one_window(struct packed_git *current) > +static int unuse_one_window(struct object_database *odb) > { > struct packfile_list_entry *e; > struct packed_git *lru_p = NULL; > struct pack_window *lru_w = NULL, *lru_l = NULL; > > - if (current) > - scan_windows(current, &lru_p, &lru_w, &lru_l); > - for (e = current->repo->objects->packfiles->packs.head; e; e = e->next) > + for (e = odb->packfiles->packs.head; e; e = e->next) > scan_windows(e->pack, &lru_p, &lru_w, &lru_l); > + > This is much nicer indeed. > if (lru_p) { > munmap(lru_w->base, lru_w->len); > pack_mapped -= lru_w->len; > @@ -740,8 +739,8 @@ unsigned char *use_pack(struct packed_git *p, > win->len = (size_t)len; > pack_mapped += win->len; > > - while (settings->packed_git_limit < pack_mapped > - && unuse_one_window(p)) > + while (settings->packed_git_limit < pack_mapped && > + unuse_one_window(p->repo->objects)) > ; /* nothing */ > win->base = xmmap_gently(NULL, win->len, > PROT_READ, MAP_PRIVATE, > > -- > 2.52.0.542.g9473a8513b.dirty