Description
#20689 was merged into blead yesterday, Jan 17. Subsequently @hv wrote in #20689 (comment):
The usual way to "reverse our sense of good and bad commits" is with the
--expect-failoption. While the example you add below is not wrong, I think it risks making things more confusing.I would be inclined to rewrite the example to invert the sense of the
die()and add--expect-fail,
How about this?
$ perl Porting/bisect.pl \
--start=5624cfff8f \
--end=b80b9f7fc6 \
--expect-fail \
-we 'use Scalar::Util; use bigrat; my @w;
local $SIG{__WARN__} = sub { push @w, $_; };
print "mercy\n" if Scalar::Util::looks_like_number(1/9);
die if @w;'
This identifies the same commit which the example added in #20689 did (and which I had previously identified via manual means as the critical commit). However, remember that we're looking for the first "good" commit, i.e., the one where warnings were no longer emitted. This bisection code nonetheless reports:
6853e8af3bcdae27c05a50e71c8ede1ab2b42822 is the first bad commit
This characterization of "good" and "bad" AFAICT comes from git. I believe that as long as Porting/bisect.pl wraps around git bisect good and git bisect bad, we're stuck with it.
... then work out how the surrounding text would have to change to accommodate the new code, then step back and consider whether this entire new section is actually resulting in an improvement of the documentation.
I believe we owe the user of our program some guidance through this confusion. I spent many hours on this problem trying to get bisect.pl to DTRT even after I had already manually identified the critical commit. I've done many bisections over the years and have always found --expect-fail to be the most difficult switch to wrap my head around. So when I finally got an invocation that worked, I didn't then go on to try to get it to work with --expect-fail.
In particular, consider what the documentation would have needed to say for you to consider that
--expect-fail- which is highlighted at lines 7-8 of this 829-line document (as rendered) - could be a possible solution here.
I think that at some point in the documentation we have to clarify for the user the fact that git's sense of "first good" and "first bad" can be very misleading and that we (Perl) can only partially mitigate that misdirection via a discussion of the --expect-fail switch. If you or someone can come up with better language, I'd consider a fresh pull request.
Maybe the answer should be to rename
--expect-fail, or even to replacebisectwith two programs such asbisect-what-fixedandbisect-what-broke.
That, I think, would require a massive overhaul of what is already a massive program. If it were my own creation, I would have extracted most of its code into a module years ago, OO-ified it, given it a proper test suite and only left a short script in bisect.pl. I think that that would also have better accommodated patches such as those which @nwc10 suggested last year in #20016 (not yet merged). But over the years I've sensed opposition to modularizing the code in Porting/ (at least within the context of keeping it in the core distribution).
To sum up:
-
If people like the
--expect-fail ... die ifinvocation above better than what got committed yesterday, we'll put that example in. -
If people can come up with better language clarifying the ambiguities of "first bad commit" and "first good commit" (and perhaps different placement of that language inside the documentation in
Porting/bisect-runner.pl), I'll be open to that.