Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C

> Pranit Bauva <pranit.bauva@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).

> 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").

>>>> +     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?

> 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.