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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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

> I _think_ bulk of Antoine and Matthieu's work can be salvaged/reused
> to implement the proposal,

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?

> but now it would be more clear that $name_good and $name_bad is a bad
> way to name internal variables and files in $GIT_DIR. The inferred 'ah
> you are hunting for regression' mode would call old ones 'bad' and new
> ones 'good', they have to be given value neutral names, e.g. $name_old
> and $name_new.

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/