Feature request: 'git bisect run' option to redundantly check start and end commits

Feature request: 'git bisect run' option to redundantly check start and end commits

[Ed Avis]

When kicking off a 'git bisect run', I have manually chosen good and bad commits, but I would like to double check that the command given to run really does succeed for the good commit and fail for the bad one.  Of course I can switch to those commits and run it manually, but mistakes can happen.  It's frustrating to set up a bisection and then find at the end that the first bad commit is the one immediately after 'good', because the command string just failed every time.  An optional startup check of the two endpoints would only be a small slowdown in most cases, but could save a lot of time.

% git bisect start
% git bisect good abcde
% git bisect bad bcdef
% git bisect run --check-endpoints my gnarly command
Checking initial 'bad' commit bcdef
running my gnarly command
command fails, as expected
Checking initial 'good' commit abcde
running my gnarly command
Error: the command fails for the initial 'good' commit

I think that it should check the 'bad' commit first, on the assumption that a failing command usually finishes quicker than a successful one.

(In my case, what went wrong is that I had put quotation marks around 'my gnarly command' when passing it on the command line.  I was thinking of git rebase --exec, which requires a single quoted argument for the command to run, whereas git bisect run takes it as separate words.  I think that an option to recheck the two endpoints before starting the bisection would be a useful idiot-proofing and make git bisect a nicer tool.  If nothing else, it's good to be reassured that the command you gave works, since when debugging an unexplained problem, you can start to doubt these things...)

--
Ed Avis <ed.avis@qmaw.com>
Please ignore any confidentiality stuff below this point.

This email and any files transmitted with it are CONFIDENTIAL and are intended solely for the use of the individual(s) or entity to whom they are addressed. Any unauthorized copying, disclosure or distribution of the material within this email is strictly forbidden. Any views or opinions presented within this email are solely those of the author and do not necessarily represent those of QMA Wadhwani LLP (QMAW) unless otherwise specifically stated. An electronic message is not binding on its sender. Any message referring to a binding agreement must be confirmed in writing and duly signed. If you have received this email in error, please notify the sender immediately and delete the original. Telephone, electronic and other communications and conversations with QMAW and/or its associated persons may be recorded and retained. QMAW is authorized and regulated by the Financial Conduct Authority. QMAW (registered in England No. OC303168) has its registered office at 9th Floor Orion House, 5 Upper St Martin's Lane, London, WC2H 9EA.

Please note that your personal information may be stored and processed in any country where we have facilities or in which we engage service providers. If you provide personal information to us by email or otherwise, you consent to the transfer of that information to countries outside of your country of residence and these countries may have different data protection rules than your country.’

To learn about our privacy policies, please use this link<https://www.pgim.com/disclaimer/privacy-center> to read the PGIM Privacy Notice.

RE: Feature request: 'git bisect run' option to redundantly check start and end commits

[Felipe Contreras]

Ed Avis wrote:
> When kicking off a 'git bisect run', I have manually chosen good and
> bad commits, but I would like to double check that the command given
> to run really does succeed for the good commit and fail for the bad
> one.  Of course I can switch to those commits and run it manually, but
> mistakes can happen.  It's frustrating to set up a bisection and then
> find at the end that the first bad commit is the one immediately after
> 'good', because the command string just failed every time.  An
> optional startup check of the two endpoints would only be a small
> slowdown in most cases, but could save a lot of time.

I like this idea.

I for one have to think twice if I should do the extra check manually or
not, and the biggest reason why I usually don't is because it would not
be automated.

With an option like that I would have no excuse.

Here's a quick patch to implement such feature (it doesn't apply
directly, it's mostly for human eyes).

diff --git a/git-bisect.sh b/git-bisect.sh
index 1f3f6e9fc5..e8adeab008 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -109,13 +109,7 @@ bisect_replay () {
 	git bisect--helper --bisect-auto-next || exit
 }
 
-bisect_run () {
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
-
-	test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
-
-	while true
-	do
+run_command () {
 	command="$@"
 	eval_gettextln "running \$command"
 	"$@"
@@ -140,6 +134,41 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 	else
 		state="$TERM_GOOD"
 	fi
+}
+
+run_check () {
+	rev=$1
+	term=$2
+	shift 2
+	git checkout $rev
+	run_command "$@"
+	echo "# recheck $term ($state): $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
+	[[ "$state" != "$term" ]] &&
+	die "$(eval_gettext "check failed, expected \$rev to be \$term")"
+}
+
+bisect_run () {
+	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
+
+	if [ "$1" = "--recheck" ]
+	then
+		shift
+		test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
+
+		rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
+		run_check $rev "$TERM_BAD" "$@"
+
+		rev=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
+		run_check $rev "$TERM_GOOD" "$@"
+
+		git bisect--helper --bisect-next || exit
+	else
+		test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
+	fi
+
+	while true
+	do
+		run_command "$@"
 
 		git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
 		res=$?

-- 
Felipe Contreras

Re: Feature request: 'git bisect run' option to redundantly check start and end commits

[Christian Couder]

On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Ed Avis wrote:
> > When kicking off a 'git bisect run', I have manually chosen good and
> > bad commits, but I would like to double check that the command given
> > to run really does succeed for the good commit and fail for the bad
> > one.  Of course I can switch to those commits and run it manually, but
> > mistakes can happen.  It's frustrating to set up a bisection and then
> > find at the end that the first bad commit is the one immediately after
> > 'good', because the command string just failed every time.  An
> > optional startup check of the two endpoints would only be a small
> > slowdown in most cases, but could save a lot of time.
>
> I like this idea.

I like this idea too.

> I for one have to think twice if I should do the extra check manually or
> not, and the biggest reason why I usually don't is because it would not
> be automated.
>
> With an option like that I would have no excuse.
>
> Here's a quick patch to implement such feature (it doesn't apply
> directly, it's mostly for human eyes).

There has been a lot of work over the years to port code from shell in
git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,
if you plan to implement this feature, if you could do it directly in
builtin/bisect--helper.c.

Thanks,
Christian.

Re: Feature request: 'git bisect run' option to redundantly check start and end commits

[Felipe Contreras]

Christian Couder wrote:
> On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > Here's a quick patch to implement such feature (it doesn't apply
> > directly, it's mostly for human eyes).
> 
> There has been a lot of work over the years to port code from shell in
> git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,
> if you plan to implement this feature, if you could do it directly in
> builtin/bisect--helper.c.

Yeah, I'm aware and I might be willing to do that--which probably would
require moving the whole of "bisect run" to C--if there was an
indication that such a patch would actually be merged.

Cheers.

-- 
Felipe Contreras

Re: Feature request: 'git bisect run' option to redundantly check start and end commits

[Christian Couder]

On Fri, Dec 18, 2020 at 6:15 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Christian Couder wrote:
> > On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > > Here's a quick patch to implement such feature (it doesn't apply
> > > directly, it's mostly for human eyes).
> >
> > There has been a lot of work over the years to port code from shell in
> > git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,
> > if you plan to implement this feature, if you could do it directly in
> > builtin/bisect--helper.c.
>
> Yeah, I'm aware and I might be willing to do that--which probably would
> require moving the whole of "bisect run" to C--if there was an
> indication that such a patch would actually be merged.

I think it would likely be merged, as it would just be finishing the
porting git bisect to C saga.

There has been work on this by a number of GSoC students and Outreachy
interns. I am adding Miriam in Cc who is the last one to have been
working on this. I think her most current work is:

https://gitlab.com/mirucam/git/-/commits/git-bisect-work4.3.1/

which contains a commit to port bisect_run to C.

I am not sure if she was planning on continuing the port by sending
this to the mailing list. Hopefully she will reply to this thread.

Best,
Christian.

Re: Feature request: 'git bisect run' option to redundantly check start and end commits

[Felipe Contreras]

Christian Couder wrote:
> On Fri, Dec 18, 2020 at 6:15 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Christian Couder wrote:
> > > On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras
> > > <felipe.contreras@gmail.com> wrote:
> > > > Here's a quick patch to implement such feature (it doesn't apply
> > > > directly, it's mostly for human eyes).
> > >
> > > There has been a lot of work over the years to port code from shell in
> > > git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,
> > > if you plan to implement this feature, if you could do it directly in
> > > builtin/bisect--helper.c.
> >
> > Yeah, I'm aware and I might be willing to do that--which probably would
> > require moving the whole of "bisect run" to C--if there was an
> > indication that such a patch would actually be merged.
> 
> I think it would likely be merged, as it would just be finishing the
> porting git bisect to C saga.

My experience tells me otherwise. We'll see.

-- 
Felipe Contreras

Re: Feature request: 'git bisect run' option to redundantly check start and end commits

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Dec 18, 2020 at 6:15 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>
>> > There has been a lot of work over the years to port code from shell in
>> > git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,
>> > if you plan to implement this feature, if you could do it directly in
>> > builtin/bisect--helper.c.
>>
>> Yeah, I'm aware and I might be willing to do that--which probably would
>> require moving the whole of "bisect run" to C--if there was an
>> indication that such a patch would actually be merged.
>
> I think it would likely be merged, as it would just be finishing the
> porting git bisect to C saga.

I agree that the issue such a patch series would try to address
(i.e. "reimplement 'bisect run' in C") may be worth solving, and I
do not offhand see why it would become a reason of not merging.

But the "cause" is mere prerequisite to consider a particular
iteration of such a series for inclusion.  I think it is
irresponsible to say "would likely" before seeing any patch to base
our assessment on.

If the design is poorly done (e.g. it does not solve the problem it
aims to solve well, it makes it hard to explain to end users what is
going on [*1*], or makes a poor design choice and paints us in a
corner we cannot easily escape from, ...), or if the execution is
poorly done (e.g. it is unmaintainable, it breaks other things by
mistake, it makes future changes harder, ...), it would not be
possible for the project to take the series.

Until reviewers comments aimed to help by pointing out these defects
are addressed, that is.  And if the series stops getting updated to
address such defects, it may end up getting not merged at all.


[Footnote]

*1* This obviously applies only for a series that makes externally
    visible behaviour change.  "rewrite 'bisect run' in C" may not
    have end-user visible effect (and if it does, such a regression
    is a reason to say "this is not yet ready").