Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

Junio C Hamano <gitster@pobox.com> writes:

I'm obviously biaised since I already spent time on the "bisect terms" idea, and I would hate to see my work and Antoine & Louis' thrown away. But I have to admit that I do like the idea of "git bisect" figuring out which commit is the old and which commit is the new one.

It's much easier to implement after the series. I'm currently forbidding redefining "good" as "bad" and vice versa, but that's just to avoid confusion and because I didn't test this case, but it should just work. So, essentially an implementation of this "guess who's old and who's new" could be: if we find a good commit that is an ancestor of an old commit, swap the terms in BISECT_TERMS. When this happens before we started to set any refs, this should do the trick. In general, we should rename the good-$sha1 reference to good and the bad to bad-$sha1 (there are corner-cases where the user already specified several good-$sha1 refs, in which case we would need to discard some of them).

I'm getting out of Git time budget, so I can't be the one doing this, at least not soon.

So, one option is to take the series as-is, and wait for someone to implement the "guess who's old and who's new" later on top of it. One drawback would be that we'd end up having the not-so-useful feature "bisect terms" in the user-interface. At least, I am now convinced that hardcoding the pair old/new is not needed. In the short term, we can have "git bisect start --name-old foo --name-new bar" which avoids the "One needs to remember in which order to give terms" issue we used to have, so we don't need to clutter the user-interface with many ways to do the same thing. OTOH, the "bisect terms" feature wouldn't be completely useless: not everything is good or bad, so leaving the option to the user to tag "slow/fast", "present/absent", ... still makes sense.

So, my proposal would be to remove the "old/new" patch from the series, and keep the other patches.

What do you think?

Ideally, the whole code would be ported to use old/new, but the more I read the code the more I agree with Christian actually: we still have many more instances of good/bad in the code (e.g. functions check_good_are_ancestors_of_bad, for_each_good_bisect_ref, ...), so having just name_new/name_old would make the code very inconsistant. It's easier to read the code thinking "good revs are old, bad revs are recent; maybe some magic swapped the terms but I don't need to worry about this" for now.