On Thu 18-11-21 17:49:48, Taylor Blau wrote: > On Thu, Nov 18, 2021 at 05:49:13PM +0100, Jan Kara wrote: > > > The first part of the series improves some tests so that they accept > > other valid decisions for bisection points. This is needed because to > > make it easier to share some logic between normal and stochastic > > bisection, I needed to slightly change some bits for normal bisection > > and then since commit weights will be computed in a somewhat different > > order, also chosen bisection points are sometimes different. > > I have only looked through a couple of the first half of your patches, > but I'm not sure I understand why non-stochastic bisection needs to > change at all in order to support stochastic bisection. > > In other words, if we're tweaking all of these tests to allow picking > equivalent bisection points, why can't we simply leave them alone? It > would be nice if normal bisection didn't change as a result of adding a > new feature on top. The big part of why results for normal bisection change are the changes in "bisect: Reorganize commit weight computation" to function do_find_bisection() where previously we didn't call approx_halfway() on the commits at the end of chain (looks like unintended omission) while after my changes we call approx_halfway() for all commits. And I have reorganized do_find_bisection() because I reuse it for stochastic bisection as well and the code is IMO easier to understand after reorganization so it is still comprehensible after I add there complexity of stochastic bisection. I understand the churn in the tests is unwelcome but long-term it seems like a low enough cost to pay for more maintainable code. But if git maintainers think otherwise I can try keeping classic bisection code decisions without changes. Just let me know what you prefer. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
