On Sat, Sep 23, 2017 at 01:34:51AM +0200, Martin Ågren wrote: > Setting `leak_pending = 1` tells `prepare_revision_walk()` not to > release the `pending` array, and makes that the caller's responsibility. > See 4a43d374f (revision: add leak_pending flag, 2011-10-01) and > 353f5657a (bisect: use leak_pending flag, 2011-10-01). > > Commit 1da1e07c8 (clean up name allocation in prepare_revision_walk, > 2014-10-15) fixed a memory leak in `prepare_revision_walk()` by > switching from `free()` to `object_array_clear()`. However, where we use > the `leak_pending`-mechanism, we're still only calling `free()`. Thanks for digging up the history here. Reviewing those it seems clear that doing a full object_array_clear() is the right thing. > Use `object_array_clear()` instead. Copy some helpful comments from > 353f5657a to the other callers that we update to clarify the memory > responsibilities, and to highlight that the commits are not affected > when we clear the array -- it is indeed correct to both tidy up the > commit flags and clear the object array. > > Document `leak_pending` in revision.h to help future users get this > right. This all looks good to me. Since the main use of "leak_pending" seems to be to clear commit marks, I have a feeling that a better API would have been something like: /* normal setup and use */ init_revisions(&rev); setup_revisions(&rev, ...); prepare_revision_walk(&rev); while (get_revision(&rev)) ...; /* optional, but sensible if there may be other callers */ clear_commit_marks(&rev); /* actually free any held memory */ clear_revisions(&rev); which would imply rev_info holding onto the "old" pending list automatically until we decommission the rev_info in the final step. But that's obviously a much bigger change. What you have here looks like a nice clean fix to the leak problems. -Peff
