Code review comment for lp:~amanica/bzr/325618_log_returns_too_much

Ian Clatworthy (ian-clatworthy) wrote :

Hi Marius,

This is looking a much better approach. The big issue right now is that the new test passes *without* the code change which means that the new test isn't triggering the bug you're fixing, right?

Also, I'm not 100% sold on the proposed change yet though for a few reasons:

* I'd prefer to keep the term left_parent (as opposed to mainline_stop_rev) because a user can log a "development line" which isn't the mainline, e.g. log -ra.b.x..a.b.y

* The inner if statement can go at the same level as the outer one???

* Do we need to test reached_top_revision_id in the inner if clause???

Does all that make sense?

FWIW, I'm about to look at a similar bug in log so I may end up using your code to do that. Perhaps we can pool our efforts and solve this batch of bugs together in coming days?

review: Needs Fixing

« Back to merge proposal