Re: [PATCH v2 3/3] Revert "pack-objects: lazily set up "struct rev_info", don't leak"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 29, 2022, 10:25 UTC
Message-ID: <221129.86bkoqjb8v.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y4VfrsYwWkestQ05@coredump.intra.peff.net>

On Mon, Nov 28 2022, Jeff King wrote:

Show 37 quoted lines

> On Mon, Nov 28, 2022 at 10:57:02PM +0100, René Scharfe wrote:
>
>> >> diff --git a/revision.c b/revision.c
>> >> index 439e34a7c5..6a51ef9418 100644
>> >> --- a/revision.c
>> >> +++ b/revision.c
>> >> @@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)
>> >>  	release_revisions_mailmap(revs->mailmap);
>> >>  	free_grep_patterns(&revs->grep_filter);
>> >>  	/* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */
>> >> +	FREE_AND_NULL(revs->diffopt.parseopts);
>> >>  	diff_free(&revs->pruning);
>> >>  	reflog_walk_info_release(revs->reflog_info);
>> >>  	release_revisions_topo_walk_info(revs->topo_walk_info);
>> >
>> > At this point I'm unclear on what & why this is needed? I.e. once we
>> > narrowly fix the >1 "--filter" options what still fails?
>> 
>> As I wrote: A call to an initialization function followed by a call to a
>> cleanup function and nothing else shouldn't leak.  There are examples of
>> repo_init_revisions()+release_revisions() without setup_revisions() or
>> diff_setup_done() beyond pack-objects.  I mentioned prune, but there are
>> more, e.g. in sequencer.c.
>
> I tend to agree with you; an init+release combo should release all
> memory. We _can_ work around it in the caller here, but I think we are
> better to correct the root cause.
>
> I do think what Ævar is saying is: once we have fixed the bug, why are
> more changes needed? I.e., why would we get rid of the lazy-init. IMHO
> the answer is that the lazy-init is a more complex pattern, and requires
> more boilerplate code (which can lead to more bugs, as we saw). So once
> the bug is fixed, this is purely about cleanup/simplification (if one
> ignores the C-standard issues), but I tend to think it is one worth
> doing.
>
> (Apologies to Ævar if I'm mis-stating your position).

My position isn't that it isn't worth doing, but that we can clearly proceed in smaller steps here, and changing one thing at a time is better.

So far in this thread we've had, in relation to 3/3 (and I may have lost track of some points made):

1. It's fixing the bug where you provide --filter N times
2. It's refactoring the "lazy init" to "non-lazy init"
3. It's refactoring the code to avoid relying on the J.5.7 extension in C99.
4. Due to #2 we run into more leaks
5. A dozen or so test files fail due to #2. Are we digressing to fix the
   root cause of the leak(s), or un-marking those tests as passing
   leak-free & losing test coverage?

As https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/ shows we can do #1 as a stand-alone change.

I'm not saying the rest of this isn't worth pursuing, except to point out in https://lore.kernel.org/git/221128.86zgcbl0pe.gmgdl@evledraar.gmail.com/ that #2 can be split off from #3.