Jeff King <peff@xxxxxxxx> writes: >> Drop the `else` branch entirely as it seems to be a no-op anyway. >> Another option might be to instead call `free_commit_list()` on `list`, >> which is the modified version of `commit_list` and thus wouldn't cause a >> double free. But as mentioned, I couldn't come up with any case where a >> passed-in non-NULL list becomes empty, so this shouldn't be necessary. >> And if it ever does become necessary we'd notice anyway via the leak >> sanitizer. > > Nicely explained. > >> Interestingly enough we did not have a single test exercising this >> branch: all tests pass just fine even when replacing it with a call to >> `BUG()`. Add a test that exercises it. > > Seems reasonable. That test will end up passing an empty list into > find_bisection(), because everything is UNINTERESTING, and the revision > machinery's limit_list() removes the UNINTERESTING elements from the > revs->commits list. > > I wondered if a topology more like this: > > one > / > base--two > > could produce something interesting from "rev-list --bisect ^one two". > But no, we still end up removing all of the uninteresting commits before > we hit find_bisection(). And anyway, "two" is obviously going to be the > output, so we know "best" won't be NULL and it won't hit your new code. > > So I agree there doesn't seem to be a way to trigger the new code that > isn't just a noop. > > Thanks for fixing this! Thanks, both of you. Will queue and merge to 'next'.
