2025年5月26日 14:49，Patrick Steinhardt <ps@xxxxxx> 写道： > > This commit message isn't quite a convincing one. As author of a patch > the onus falls on you to explain why the change is sensible, but even > more importantly it also falls on you to explain why it is correct. > > It is of course fine to ask for help and input, but in that case you > should probably mark the patch accordingly, for example with the RFC > tag. Thank you for the suggestion. Please allow me to keep the patch in its current form this time. > We know that `roots_bitmap` is always allocated via `bitmap_new()`, so > it won't ever be a `NULL` pointer and should in theory always be free'd. > Furthermore, we know that the pointer never escapes the local scope, > either. > > The next question would thus be: what does `cascade_pseudo_merges_1()` > do with the bitmap? Are there situations where it does free it for us, > or where it moves ownership of that bitmap? So let's go down the call > chain: > > - `cascade_pseudo_merges_1()` passes it on to > `cascade_pseudo_merges()`. > > - `cascade_pseudo_merges()` passes it on to `apply_pseudo_merge()`. > > `apply_pseudo_merge()` itself then checks whether the pseudo-merge is a > subset of the `roots_bitmap` and, if not, ORs the pseudo-merge into it. I would put this into commit message. I also noticed that `find_objects()` in pack-bitmap.c has similar code but without `if (cascade_pseudo_merges_1)`. > > None of these operations move around ownership or free the bitmap, so > this looks like a true memory leak in case `cascade_pseudo_merges_1()` > returns non-zero. Which would raise another question: when exactly does > it return non-zero, and can we trigger the memory leak via a test? Seems we need to make `bitmap_git->pseudo_merges->v[I]` contains some objects which doesn’t exist in `roots`. I’ll try to figure it out in the next patch. Thanks Lidong
