| Message ID | cover-00.10-00000000000-20221230T071741Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | sequencer API & users: fix widespread leaks | expand |
Hi Ævar On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote: > This series fixes various widespread leaks in the sequencer and its > users (rebase, revert, cherry-pick). As a result 18 tests become > leak-free in their entirety. > > The main change is the 3/10 here, where we introduce a > replay_opts_release() to free the "struct replay_opts". The rest is > then either refactorings to be able to call that destructor > (e.g. "return" to "goto cleanup"), or other miscellanious adjacent > leaks. > > This is a follow-up to the discussion ending at [1], as noted there > the recent ff84d031a9d (Merge branch 'pw/rebase-no-reflog-action', > 2022-11-23) ended up introducing a leak because of the disfunctional > lack of a destructor (or rather, the current logic being tied up in > sequencer_remove_state(). Thanks for working on this. Separating out replay_opts_release() from sequencer_remove_state() enables us to free struct replay_opts properly. I've left some comments, the patches I haven't commented on all looked fine. Best Wishes Phillip > This can be queued and graduated independently of the other concurrent > leak series I've submitted[2]. When the two are combined we'll end up > passing more tests, i.e. both topics combined get us over the finish > line for some of them, but neither one is enough in isolation. > > But that's OK, we just won't opt them into the "linux-leaks" > testing. I'll submit a follow-up similar to [3] at some later date to > mark them as passing. I think that's a better trade-off than making > these two depend on one another. > > 1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/ > 2. https://lore.kernel.org/git/cover-v2-00.20-00000000000-20221230T020341Z-avarab@gmail.com/ > 3. https://lore.kernel.org/git/patch-v2-01.20-3de29c6d75f-20221230T020341Z-avarab@gmail.com/ > > Ævar Arnfjörð Bjarmason (10): > rebase: use "cleanup" pattern in do_interactive_rebase() > sequencer.c: split up sequencer_remove_state() > rebase & sequencer API: fix get_replay_opts() leak in "rebase" > builtin/revert.c: refactor run_sequencer() return pattern > builtin/revert.c: fix common leak by using replay_opts_release() > builtin/revert.c: move free-ing of "revs" to replay_opts_release() > builtin/rebase.c: fix "options.onto_name" leak > sequencer.c: always free() the "msgbuf" in do_pick_commit() > builtin/rebase.c: free() "options.strategy_opts" > commit.c: free() revs.commit in get_fork_point() > > builtin/rebase.c | 19 +++++----- > builtin/revert.c | 40 +++++++++++---------- > commit.c | 1 + > sequencer.c | 48 +++++++++++++++++--------- > sequencer.h | 1 + > t/t3405-rebase-malformed.sh | 1 + > t/t3412-rebase-root.sh | 1 + > t/t3416-rebase-onto-threedots.sh | 1 + > t/t3419-rebase-patch-id.sh | 1 + > t/t3423-rebase-reword.sh | 1 + > t/t3425-rebase-topology-merges.sh | 2 ++ > t/t3431-rebase-fork-point.sh | 1 + > t/t3432-rebase-fast-forward.sh | 1 + > t/t3437-rebase-fixup-options.sh | 1 + > t/t3438-rebase-broken-files.sh | 2 ++ > t/t3501-revert-cherry-pick.sh | 1 + > t/t3502-cherry-pick-merge.sh | 1 + > t/t3503-cherry-pick-root.sh | 1 + > t/t3506-cherry-pick-ff.sh | 1 + > t/t3511-cherry-pick-x.sh | 1 + > t/t7402-submodule-rebase.sh | 1 + > t/t9106-git-svn-commit-diff-clobber.sh | 1 - > t/t9164-git-svn-dcommit-concurrent.sh | 1 - > 23 files changed, 82 insertions(+), 47 deletions(-) >
Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for working on this. Separating out replay_opts_release() from > sequencer_remove_state() enables us to free struct replay_opts > properly. I've left some comments, the patches I haven't commented on > all looked fine. Ditto. I agree with the review comments I so far saw on the list, and this topic needs a bit more work, but it is great to aim for allowing us freeing replay_opts properly. Thanks, all.