Re: [GSoC PATCH v4 0/5] preserve promisor files content after repack

[Junio C Hamano <gitster@pobox.com>]

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).

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.

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


 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);
-- 
2.54.0-rc1-178-ge3ec7964f3