Pranit Bauva [off-list ref] 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").
quoted
quoted
+ 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.
Thanks.
Hey Junio,
On Wed, Aug 31, 2016 at 1:03 AM, Junio C Hamano [off-list ref] wrote:
Pranit Bauva [off-list ref] writes:
quoted
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.
quoted
quoted
quoted
+ 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