From: Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>
Date: Sat, 11 Apr 2026 14:05:37 GMT
Subject: Re: [GSoC PATCH v4 0/5] preserve promisor files content after repack
Message-ID: <adpVMfHowCBY75I1@lorenzo-VM>
In-Reply-To: <xmqqjyuemeza.fsf@gitster.g>

On Fri, Apr 10, 2026 at 07:02:17PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > When merged to the tip of 'seen' (with a fixup to use st_mtime where
> > we need only whole second precision, to avoid using st_mtim on
> > platforms that do not have it), this seems to break linux-leaks and
> > linux-reftable-leaks CI jobs (t0410, t5616, and t5710).
> >
> > This topic standalone, without interaction with other things in
> > 'seen', breaks these three tests.
> >
> >   https://github.com/git/git/actions/runs/24267948258/job/70866907548
> >
> > This is one commit directly on top of your topic that reduces CI
> > jobs down to just two "leaks" job, and removes many test scripts
> > to leave only these three breaking ones.
> >
> >
> > I managed to also locally reproduce this failure.  Here is how
> >
> >     $ cd t && t5710-*.sh -i -v
> >
> > dies:
> >
> > Direct leak of 285 byte(s) in 1 object(s) allocated from:
> >     #0 0x55ce48ca1d4d in malloc (git+0x8cd4d) (BuildId: 1aa6efa30b2fc4772028a3dd31aba3ced49bf128)
> >     #1 0x55ce48fed3f2 in do_xmalloc wrapper.c:55:8
> >     #2 0x55ce48fed3b6 in xmalloc wrapper.c:76:9
> >     #3 0x55ce48eed314 in alloc_packed_git packfile.c:306:25
> >     #4 0x55ce48eed209 in parse_pack_index packfile.c:326:25
> >     #5 0x55ce48f65f03 in copy_promisor_content repack-promisor.c:67:14
> >     ...
> 
> 
> FWIW, the tip of 'seen' as of this writing has queued this topic
> (the latest round v5) plus the attached "SQUASH???" commit at its
> tip.
> 
> The first hunk (die when dest_pack is NULL) is absolutely positively
> a wrong thing to do.  As I said, I do not know if we want to call
> parse_pack_index() here or if we have a more appropriate helper
> function to use, but assuming that parse_pack_index() is the right
> thing to call, we should be prepared to see NULL returned.  BUG("")
> is reserved for detected programming errors, and it is absolutely a
> wrong thing to call.
>
> As the content copied by this function is supposed to be for
> debugging only, I think dying when we cannot copy is not what we
> want.  Rather, it probably makes more sense to fall back to the
> traditional behaviour (e.g., not copying and instead leaving an
> empty file, if that is what we did before this patch series).

Got it. I will add a `warning(_("..."))` if this happens, and then
immediately `return -1` to indicate that something went wrong and so the
".promisor" file will be left empty.

> The second hunk just line-wraps an overly long line.  There are
> other overly long lines in this deeply indented block (which is a
> sign that it might be worth to see if the block is better made into
> a small static helper function) that should be line-wrapped in a
> similar way, but I didn't bother.

There are only I believe 3 lines of code that exceed the soft cap of 80
characters per line. I will line-wrap these long lines.

> THe last hunk is a real bugfix for the leak (again, provided that
> parse_pack_index() is what we want to use).

Yeah, I also noticed the issue, and reported it in an email pretty much
at the same time that you sent this one.

> 
>  repack-promisor.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/repack-promisor.c b/repack-promisor.c
> index 26055212a3..c7025e97f2 100644
> --- a/repack-promisor.c
> +++ b/repack-promisor.c
> @@ -71,6 +71,8 @@ static void copy_promisor_content(struct repository *repo,
>  	dest_idx_name = mkpathdup("%s-%s.idx", packtmp, dest_hex);
>  	get_oid_hex_algop(dest_hex, &dest_oid, repo->hash_algo);
>  	dest_pack = parse_pack_index(repo, dest_oid.hash, dest_idx_name);
> +	if (!dest_pack)
> +		BUG("parse_pack_index() failed.");
>  
>  	/* Open the .promisor dest file, and fill dest_content with its content */
>  	dest_promisor_name = mkpathdup("%s-%s.promisor", packtmp, dest_hex);
> @@ -115,7 +117,8 @@ static void copy_promisor_content(struct repository *repo,
>  
>  			/* If <time> doesn't exist, retrieve it and add it to line */
>  			if (line_sections.nr < 3)
> -				strbuf_addf(&line, " %" PRItime, (timestamp_t)source_stat.st_mtime);
> +				strbuf_addf(&line, " %" PRItime,
> +					    (timestamp_t)source_stat.st_mtime);
>  
>  			/*
>  			 * Add the finalized line to dest_to_write and dest_content if it
> @@ -148,6 +151,7 @@ static void copy_promisor_content(struct repository *repo,
>  		die(_("Could not write '%s' promisor file"), dest_promisor_name);
>  
>  	close_pack_index(dest_pack);
> +	free(dest_pack);
>  	free(dest_idx_name);
>  	free(dest_promisor_name);
>  	strset_clear(&dest_content);
> -- 

Thanks Junio.

Regarding the "Should we use `parse_pack_index()` here?" question, I
have also found success using some like this:

```
	[...]
	struct packed_git *dest_pack, *p;
	struct odb_source_files *files;
	int err;

	dest_idx_name = mkpathdup("%s-%s.idx", packtmp, dest_hex);
	files = odb_source_files_downcast(repo->objects->sources);
	dest_pack = packfile_store_load_pack(files->packed, dest_idx_name, 0);
	[...]
```

This code passes all the tests without any leaks.

Now, in all honesty, I don't know the implications of using
`parse_pack_index()` compared to using `packfile_store_load_pack()` and
`odb_source_files_downcast()`. Unfortunately these functions are rarely
used in the code, so I don't have many examples to look at.

Let me know what is your opinion on this. In the meantime I will explore
this possibility further.

Thanks a lot,

Lorenzo