Re: [RFC/PATCH] Bisect: implement "git bisect run <cmd>..." to automatically bisect.

Christian Couder [off-list ref] writes:

This idea was suggested by Bill Lear
(Message-ID: [off-list ref])
and I think it is a very good one.

Nicely done.

People often find that during bisect they want to have a
near-constant tweaks (e.g., s/#define DEBUG 0/#define DEBUG 1/
in a header file, or "revision that does not have this commit
needs this patch applied to work around other problem this
bisection is not interested in") applied to the revision being
tested.  

To cope with such a situation, after the inner git-bisect finds
the next revision to test, with the "run" script, the user can
apply that tweak before compiling, run the real test, and after
the test decides if the revision (possibly with the needed
tweaks) passed the test, rewind the tree to the pristine state.
Finally the "run" script can exit with the status of the real
test to let "git-bisect run" command loop to know the outcome.

When you write a documentation for this patch, the above issue
should be addressed, as this is often asked on and off the list.

I am not sure if this flexibility/leniency is desirable.  It
certainly allows a sloppily written shell script that exits with
any random small-positive values to report a badness, which may
be handy, but allowing sloppiness might lead to wasted time
after all.  I dunno.  A more strict convention that says the run
script should exit 1 to signal "bad, please continue", 0 for
"good, please continue" and other values for "no good, abort"
might be less error prone.

In any case, the exit status convention needs documentation.

A program that does "exit(-1)" leaves $? = 255 (see exit(3)
manual page, the value is chopped with "& 0377"), so the test
for -ge 128 is certainly good; I do not know if any system gives
negative values, but the check shouldn't hurt.  As a style, I
would have written that as:

	if test $res -lt 0 || test $res -ge 128
        then
        	...

but that is probably a bit old-fashioned.  

+      # Use "git-bisect good" or "git-bisect bad"
+      # depending on run success or failure.
+      # We cannot use bisect_good or bisect_bad functions
+      # because they can exit.
+      if [ $res -gt 0 ]; then
+	  next_bisect='git-bisect bad'
+      else
+	  next_bisect='git-bisect good'
+      fi
+
+      $next_bisect > "$GIT_DIR/BISECT_RUN"

If their exiting, and possibly variable assignments, are the
only problem, you can run that in subshell, can't you?  Like:

	( $next_bisect >"$GIT_DIR/BISECT_RUN" )

+      res=$?

Junio C Hamano a écrit :

[...]

quoted

+      # Check for really bad run error.
+      if [ $res -lt 0 -o $res -ge 128 ]; then
+	  echo >&2 "bisect run failed:"
+	  echo >&2 "exit code $res from '$@' is < 0 or >= 128"
+	  exit $res
+      fi

I am not sure if this flexibility/leniency is desirable.  It
certainly allows a sloppily written shell script that exits with
any random small-positive values to report a badness, which may
be handy, but allowing sloppiness might lead to wasted time
after all.  I dunno.  A more strict convention that says the run
script should exit 1 to signal "bad, please continue", 0 for
"good, please continue" and other values for "no good, abort"
might be less error prone.

Perhaps, but when running "git bisect run make" to automatically find the 
commit that broke the build, it would fail because make will usually return 
2 in case of error. 

It seems that there are few standards for exit code, so whatever convention 
we choose will not work in all cases.

In any case, the exit status convention needs documentation.

Yes, I will work on it.

[...]

quoted

+      # Use "git-bisect good" or "git-bisect bad"
+      # depending on run success or failure.
+      # We cannot use bisect_good or bisect_bad functions
+      # because they can exit.
+      if [ $res -gt 0 ]; then
+	  next_bisect='git-bisect bad'
+      else
+	  next_bisect='git-bisect good'
+      fi
+
+      $next_bisect > "$GIT_DIR/BISECT_RUN"

If their exiting, and possibly variable assignments, are the
only problem, you can run that in subshell, can't you?  Like:

	( $next_bisect >"$GIT_DIR/BISECT_RUN" )

You are right. I will submit an updated patch with this change and some 
documentation.

Thanks,
Christian.