Patrick Steinhardt <ps@xxxxxx> writes: > my mind had a couple of minutes where it was roaming, and of course it > immediately searched for and chased down the next rabbit hole. The > result is this patch series which fixes a bunch of leaks all over the > place. There isn't really any structure to the leaks that I did fix -- > it's mostly things that I stumbled over. In the end, this series makes > another 56 test suites pass with leak checking enabled, 13 of which have > already been passing without any changes. ... meaning there were coverage gaps? > While most things are unstructured, there are two topics that stand out: > > - Patches 5 to 12 address a shortcoming of our config API. Both > `git_config_string()` and `git_config_pathname()` have a `const char > **` out parameter, but they do in fact transfer memory ownership to > the caller. This resulted in a bunch of memory leaks all over the > place. > > These patches thus refactor a bunch of code and then ultimately > switch the out parameter to become a `char *` I do remember getting hurt by this one relatively recently. Addressing the issue is very much appreciated. > - Patches 16 to 20 have the goal of making git-mv(1) memory leak free. > I had a very hard time understanding how it tracks memory. I think > this wasn't only me, or otherwise there wouldn't be calls to > `UNLEAK()` in there. In any case, I decided to rewrite the arrays to > use a `struct strvec`, which makes tracking and releasing of memory > a ton easier. > > It does come at the cost of more allocations because we may now > duplicate strings that we didn't before. But I think the tradeoff is > worth it because the number of strings we may now duplicate is > bounded by the number of command line arguments anyway. Nice. I have to admit that "git mv" is not one of the best-done code in this project X-<, and improving it with rewriting was long overdue. Thanks.
