Re: [PATCH] repack: free geometry struct

On Tue, Aug 08, 2023 at 03:59:43PM -0400, Taylor Blau wrote:

quoted

I did actually put together the "put it on the stack" patch, which swaps
out:

  if (geometry)

for:

  if (geometric_factor)

to get around the NULL checks. But besides being noisy for little gain,
it ends up with some subtle gotchas, because we pass "&geometry" into
some functions which don't have access to "geometric_factor" (so now
extra call-sites have to keep track of "is this struct valid enough to
pass").

...I think that storing the geometric_factor value on the pack_geometry
struct itself gets around most of these issues.

This version is still somewhat noisy, but it's not too bad IMHO. I'm
fine with either your approach or mine :-).

Yeah, that is much better than what I had put together, as it means that
pack_geometry is always in a consistent state, even if we didn't call
init_pack_geometry(). So it is safe to clear() it, for example, without
checking geometric_factor again. And any sub-functions can likewise
check whether it contains any useful data. So you'd need this fixup on
top of your patch:

diff --git a/builtin/repack.c b/builtin/repack.c
index 13e4f0094e..80bffc36d4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -579,7 +579,7 @@ static void midx_included_packs(struct string_list *include,
 		string_list_insert(include, xstrfmt("%s.idx", item->string));
 	for_each_string_list_item(item, names)
 		string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
-	if (geometry) {
+	if (geometry->split_factor) {
 		struct strbuf buf = STRBUF_INIT;
 		uint32_t i;
 		for (i = geometry->split; i < geometry->pack_nr; i++) {

or else t5326 (and I think a few others) will fail.

I'd be happy to proceed that way if you want to clean it up with a
commit message, but I think we should do it on top of the leak-fix (I
wanted to say that the heap-to-stack conversion was low-risk, but both
of us introduced bugs while trying it ;) ).

-Peff