On Sat, Nov 16, 2024 at 12:24:02PM +0100, Patrick Steinhardt wrote: > > * ps/leakfixes-part-10 (2024-11-13) 28 commits > > - t: remove TEST_PASSES_SANITIZE_LEAK annotations > > - test-lib: unconditionally enable leak checking > > - t: remove unneeded !SANITIZE_LEAK prerequisites > > - t: mark some tests as leak free > > - t5601: work around leak sanitizer issue > > - git-compat-util: drop now-unused `UNLEAK()` macro > > - global: drop `UNLEAK()` annotation > > - t/helper: fix leaking commit graph in "read-graph" subcommand > > - builtin/branch: fix leaking sorting options > > - builtin/init-db: fix leaking directory paths > > - builtin/help: fix leaks in `check_git_cmd()` > > - help: fix leaking return value from `help_unknown_cmd()` > > - help: fix leaking `struct cmdnames` > > - help: refactor to not use globals for reading config > > - builtin/sparse-checkout: fix leaking sanitized patterns > > - split-index: fix memory leak in `move_cache_to_base_index()` > > - git: refactor builtin handling to use a `struct strvec` > > - git: refactor alias handling to use a `struct strvec` > > - strvec: introduce new `strvec_splice()` function > > - line-log: fix leak when rewriting commit parents > > - bisect: fix various cases where we leak commit list items > > - bisect: fix leaking commit list items in `check_merge_base()` > > - bisect: fix multiple leaks in `bisect_next_all()` > > - bisect: fix leaking `current_bad_oid` > > - bisect: fix leaking string in `handle_bad_merge_base()` > > - bisect: fix leaking good/bad terms when reading multipe times > > - builtin/blame: fix leaking blame entries with `--incremental` > > - Merge branch 'ps/leakfixes-part-9' into ps/leakfixes-part-10 > > > > Leakfixes. > > > > Will merge to 'next'? > > source: <20241111-b4-pks-leak-fixes-pt10-v2-0-6154bf91f0b0@xxxxxx> > > Rubén's review went through all of the patches and his findings have > been addressed. Yes, this iteration looks good to me. Two thoughts about the merge: First, I'm concerned that we may not have sufficiently documented how contributors should proceed to prevent new leaks when submitting patches, and perhaps avoid some unnecessary noise on the list. I reviewed Documentation/SubmittingPatches and didn't see any mention about it. Perhaps it would be helpful to add a note about SANITIZE=leak. I'm unsure if we want to be explicit about this, though. Second, in the (hopefully) exceptional cases where new series discover old leaks that cannot be fixed within the series itself, for whatever reason, I don't think there is documentation on the use of the SANITIZE_LEAK prerequisite. Its use is not desirable and documenting it could be counterproductive, so perhaps it's better to leave its use suggested on the list when necessary. > The other comment from Peff seemed to only relate to > dropping the use of `UNLEAK()`, so I don't think he had a full look at > the patch series. So personally I don't plan to reroll this, but am not > sure whether this had enough review exposure. I admit that reviewing this series, and reading the previous nine, hasn't been easy. It involved reading code I hadn't seen before. So, while I'm glad to give it my reviewed-by trailer, I share the concern about whether it has had enough review exposure. Finally, I would like to reiterate that I continue to be impressed with the achievement of reaching "test-lib: unconditionally enable leak checking" within the time frame envisioned [1][2]. Thanks. [1] https://lore.kernel.org/git/Zp4gILfskdpc6RUk@tanuki/ [2] https://lore.kernel.org/git/cover.1721995576.git.ps@xxxxxx/
