Hey Junio,

On Wed, Aug 31, 2016 at 1:03 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Pranit Bauva <pranit.ba...@gmail.com> writes:
>> This is a very tricky one. I have purposely not included this after a
>> lot of testing. I have hand tested with the original git and with this
>> branch. The reason why anyone wouldn't be able to catch this is
>> because its not covered in the test suite. I am including a patch with
>> this as an attachment (because I am behind a proxy right now but don't
>> worry I will include this as a commit in the next series). The
>> original behaviour of git does not clean the bisect state when this
>> situation occurs.
> "We sometimes clean and we sometimes don't and this follows the
> original" may be a valid justification but it is not a very useful
> explanation.  The real issue is if not cleaning is intended (and if
> so why; otherwise, if it is clear that it is simply forgotten, we
> can just fix it in the series as a follow-up step).

I think we do want to recover from this "bad merge base" state and for
that not cleaning up is essential. The original behaviour of git seems
natural to me.

> If not cleaning in some cases (but not others) is the right thing,
> at least there needs an in-code comment to warn others against
> "fixing" the lack of cleanups (e.g. "don't clean state here, because
> the caller still wants to see what state we were for this and that
> reason").

I will mention it in the comments.

>>>> +     if (bisect_next_check(terms, terms->term_good.buf))
>>>> +             return -1;
>>> Mental note.  The "autostart" in the original is gone.  Perhaps it
>>> is done by next_check in this code, but we haven't seen that yet.
>> This will be added back again in a coming patch[1].
> In other words, this series is broken at this step and the breakage
> stay with the codebase until a later step?

Broken though it passes the test suite.

> I do not know if reordering the patches in the series is enough to
> fix that, or if it is even worth to avoid such a temporary breakage.
> It depends on how serious the circularity is, but I would understand
> if it is too hard and not worth the effort (I think in a very early
> review round some people advised against the bottom-up rewrite
> because they anticipated this exact reason).  At least the omission
> (and later resurrection) needs to be mentioned in the log so that
> people understand what is going on when they later need to bisect.

bisect_autostart() call from bisect_next() was introduced in the
earliest version of git-bisect in the commit 8cc6a0831 but it isn't
really a very big deal and I think it would be OK to skip it for a
very little while as any which ways the series in the end would fix
it. It is important to mention this in the commit message and I will
do it.

Pranit Bauva

Reply via email to