On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote: > > (Note that this isn't a pure revert; the previous commit added a test > > showing the regression, so we can now flip it to expect_success). > > Keeping the test is excellent, because it gives us a way to confirm > that a second attempt at a fix is at least as good as the first. > > The only thing that could improve this situation is to add a test > that checks the bug that the previous version introduced, so that > the next round doesn't repeat the mistake. That can be deferred > because it is more important that we get this fix in time for the > next release candidate. Re-reading what I wrote, I think "the previous commit" may be ambiguous. The original commit which introduced the bug (and which we're reverting here) didn't include a test at all. In patch 1/2 of this series (what I'm calling "the previous commit"), I provided a test which shows the regression. And now this revert shows that we fixed it (by flipping from expect_failure to expect_success). So I think I've already done what you're asking (if I understand it correctly). It probably would be a little easier to follow by reverting first, and then adding in the expect_success test on top to future-proof us. But by doing it in the other order, it was easy to see the test demonstrate the behavior before and after the revert. -Peff
