Hey Junio, On Wed, Aug 31, 2016 at 1:03 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Pranit Bauva <pranit.bauva@xxxxxxxxx> 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. Regards, Pranit Bauva