On Tue, Aug 08, 2023 at 03:59:43PM -0400, Taylor Blau wrote:
> > 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
