On Fri, Feb 22, 2019 at 09:49:44AM -0800, Junio C Hamano wrote: > > Even though bisect might be driven by scripts, there's no reason to > > consider this part of the output as machine-readable (if anything, the > > initial "$hash is the first bad commit" might be parsed, but we won't > > touch that here). Let's make it prettier and more informative for a > > human reading the output. > > Sounds very sensible. One potential point that makes me worried is > this move might tempt people to make the output even larger (e.g. a > full diff with "--patch" is overkill if done unconditionally). Yeah, I agree that would be overkill. What I have here isn't actually much bigger. It mostly trades one line of --raw for one line of --stat. The big change is that we actually bother to recurse, but I think the old behavior was just buggy. At any rate, I think we can discuss such a patch if and when it comes up. > > If we do care about the change in exit code from bisect, then it > > probably does make sense to go with an external process. Then it can > > happily die on the corruption, while bisect continues with the rest of > > the high-level operation. I'm not sure it really matters much, though. > > Once your repository is corrupted, all bets are off. It's nice that we > > can bisect in such a state at all. > > This is about showing the very final message after finding which one > is the culprit. Is there any other "clean-up" action we need to do > after showing the message? I do not care too much about the exit > code from the bisection, but if dying from diff-tree can interfere > with such a clean-up, that would bother me a lot more, and at that > point, given especially that this is not a performance sensitive > thing at all (it is not even invoked log(n) times---just once at the > end), moving to external process may make it a lot simpler and > cleaner. Thanks, I had a vague feeling along these lines, but you nicely put it into words. As far as I can tell, no, we're not missing any important cleanup in that process; it looks like the only call to show_diff_tree() then calls exit(10) immediately after. However, that does change our exit code, which git-bisect.sh then propagates instead of writing the entry into the BISECT_LOG. I'm still not convinced this is really worth caring about, as it implies a corrupt repo. But if we did want to fix it, then I think the external diff-tree would still be the right thing (since it's hard for git-bisect.sh to tell the different between this death and another one). -Peff
