From: Lorenzo Pegorari Date: Sat, 11 Apr 2026 01:59:09 GMT Subject: Re: [GSoC PATCH v5 2/6] repack-promisor add helper to fill promisor file after repack Message-ID: In-Reply-To: On Fri, Apr 10, 2026 at 04:30:40PM -0700, Junio C Hamano wrote: > LorenzoPegorari writes: > > > + dest_pack = parse_pack_index(repo, dest_oid.hash, dest_idx_name); > > parse_pack_index() has this comment: > > /* > * Parse the pack idx file found at idx_path and create a packed_git struct > * which can be used with find_pack_entry_one(). > * > * You probably don't want to use this function! It skips most of the normal > * sanity checks (including whether we even have the matching .pack file), > * and does not add the resulting packed_git struct to the internal list of > * packs. You probably want add_packed_git() instead. > */ > struct packed_git *parse_pack_index(struct repository *r, unsigned char *sha1, > const char *idx_path); > > The function can return NULL, but this caller does not seem to be > prepared for it to return NULL (i.e., the loop introduced by the > repo_for_each_pack() macro we see below, nobody assumes dest_pack > could be NULL). That's true. The function is created assuming that the promisor files are correctly formed, but this is unrelated to the promisor files content, so it should be checked to avoid any issue. I will add a `die()` if `parse_pack_index()` returns `NULL`, since this means that something bad happened considering that we literally just created the pack that we want to retrieve. > But what pack index file are we parsing here? `parse_pack_index()` is used to create a temporary `packed_git` for the just created pack, so that we can then check if (for each line of each ".promisor" file) appears in it (and then append that line to the ".promisor" file of the just created pack). > Isn't it already part > of the running system that we should be able to find on the list of > packfiles in the packfile store? No, the new pack's `packed_git` doesn't appear in the `repo_for_each_pack` macro loop, if that is what you are asking. > Is this because we lack "find a > packfile on this packfile store by name" API, because what we want > to find if each of s we have appear in the particular packfile > or not, and packfile_list_find_oid() is not sufficiently precise (i.e. > "the object appears in one of the packfile on the list" is not what > we want to know, "the object appears in this particular packfile" is)? Yes, it's pretty much this. > Patrick CC'ed primarily because this part of the API and the > data structures have been reshuffled to add quite a lot of > abstraction since I last looked at the area. > > As close_pack_index(dest_pack) does not release resources held by > dest_pack itself (even though the region of mmaped memory that is > pointed at by its index_data member is unmapped), I think that is > where the memory leak is breaking the CI jobs (see my other message). > > But I am not sure if the use of parse_pack_index() - close_pack_index() > API is the right thing to use here. I struggled a lot to understand how to deal with a pack opened with `parse_pack_index()`, considering that this function is only used once in the codebase. In the end I decided to simply use `close_pack_index()` as it is done in this other instance where `parse_pack_index()` is used. Having done more research now, I realized that the issue is that the code is missing a `free(dest_pack)`. With this line, no leak happens, and all GitHub Actions-based CI tests are green. Also, I think that `close_pack_index()` is indeed the correct way to close a `packed_git` opened with `parse_pack_index()` (instead of using the generic `close_pack()`, or not closing it at all). Finally, having looked more deeply at that other instance where `parse_pack_index()` is used, I believe there actually is a possible memory leak that might rarely ever happen. Isn't there a `free()` missing, like this: --- http.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/http.c b/http.c index 8ea1b9d1f6..e765852071 100644 --- a/http.c +++ b/http.c @@ -2446,8 +2446,10 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs, if (!ret) close_pack_index(new_pack); free(tmp_idx); - if (ret) + if (ret) { + free(new_pack); return -1; + } packfile_list_prepend(packs, new_pack); return 0; -- > > + /* Open the .promisor dest file, and fill dest_content with its content */ > > + dest_promisor_name = mkpathdup("%s-%s.promisor", packtmp, dest_hex); > > + dest = xfopen(dest_promisor_name, "r+"); > > + while (strbuf_getline(&line, dest) != EOF) > > + strset_add(&dest_content, line.buf); > > + > > + repo_for_each_pack(repo, p) { > > + FILE *source; > > + struct stat source_stat; > > + > > + if (!p->pack_promisor) > > + continue; > > + > > + if (not_repacked_basenames && > > + strset_contains(not_repacked_basenames, pack_basename(p))) > > + continue; > > + Thanks Junio, Lorenzo