On Mon, Nov 28 2022, Jeff King wrote: > 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@xxxxxxxxxxxxxxxxxxx/ 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@xxxxxxxxxxxxxxxxxxx/ that #2 can be split off from #3.
