[PATCH] bisect: add a --verify flag to `bisect run`

[PATCH] bisect: add a --verify flag to `bisect run`

[Nico Weber via GitGitGadget]

From: Nico Weber <thakis@chromium.org>

If it's passed, then `git bisect run --verify script` will first
check out the good revision and verify that the script passes there,
then check out the bad revision and verify that it fails there,
and only then start the actual `git bisect run script`.

We use `git bisect run` heavily for bisecting bugs in LLVM when using
clang to build Chromium. We sometimes end up with run scripts that are
broken in some way, either by missing the +x bit, or in more subtle
ways, and this adds a simple, low conceptual overhead way to smoke check
the run script before starting a bisect that could run for a day or two.

Signed-off-by: Nico Weber <thakis@chromium.org>
---
    bisect: add a --verify flag to bisect run
    
    If it's passed, then git bisect run --verify script will first check out
    the good revision and verify that the script passes there, then check
    out the bad revision and verify that it fails there, and only then start
    the actual git bisect run script.
    
    We use git bisect run heavily for bisecting bugs in LLVM when using
    clang to build Chromium. We sometimes end up with run scripts that are
    broken in some way, either by missing the +x bit, or in more subtle
    ways, and this adds a simple, low conceptual overhead way to smoke check
    the run script before starting a bisect that could run for a day or two.
    
    Signed-off-by: Nico Weber thakis@chromium.org [thakis@chromium.org]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-741%2Fnico%2Frunverifyforupstreaming-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-741/nico/runverifyforupstreaming-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/741

 Documentation/git-bisect.txt |  12 +++-
 git-bisect.sh                | 110 ++++++++++++++++++++++++++++++++++-
 t/t6030-bisect-porcelain.sh  |  16 +++++
 3 files changed, 134 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index fbb39fbdf5..37658d8ed8 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -26,7 +26,7 @@ on the subcommand:
  git bisect (visualize|view)
  git bisect replay <logfile>
  git bisect log
- git bisect run <cmd>...
+ git bisect run [--verify] <cmd>...
  git bisect help
 
 This command uses a binary search algorithm to find which commit in
@@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the
 command will print out a description of the first bad commit. The
 reference `refs/bisect/bad` will be left pointing at that commit.
 
-
 Bisect reset
 ~~~~~~~~~~~~
 
@@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good
 or bad, you can bisect by issuing the command:
 
 ------------
-$ git bisect run my_script arguments
+$ git bisect run [--verify] my_script arguments
 ------------
 
 Note that the script (`my_script` in the above example) should exit
@@ -376,6 +375,13 @@ ignored.
 This option is particularly useful in avoiding false positives when a merged
 branch contained broken or non-buildable commits, but the merge itself was OK.
 
+--verify::
++
+Before the actual bisect run, check out the current bad revision and
+verify that the script exits with a code between 1 and 127 (inclusive),
+except 125, then check out the current good revision and verify that
+the script exits with code 0. If not, abort the bisect run.
+
 EXAMPLES
 --------
 
diff --git a/git-bisect.sh b/git-bisect.sh
index 2f60fefcfa..99ac01fb55 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -26,7 +26,7 @@ git bisect replay <logfile>
 	replay bisection log.
 git bisect log
 	show bisect log.
-git bisect run <cmd>...
+git bisect run [--verify | --no-verify] <cmd>...
 	use <cmd>... to automatically bisect.
 
 Please use "git help bisect" to get the full man page.'
@@ -38,6 +38,8 @@ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
 _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 TERM_BAD=bad
 TERM_GOOD=good
+CURRENT_BISECT_BAD=
+CURRENT_BISECT_GOOD=
 
 bisect_head()
 {
@@ -215,6 +217,7 @@ bisect_replay () {
 		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
 		if test "$git" = "git-bisect"
 		then
+			tail="$rev"
 			rev="$command"
 			command="$bisect"
 		fi
@@ -237,7 +240,112 @@ bisect_replay () {
 	bisect_auto_next
 }
 
+get_current_bisect_bounds () {
+	test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
+	oIFS="$IFS" IFS="$IFS$(printf '\015')"
+	while read git bisect command rev tail
+	do
+		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
+		if test "$git" = "git-bisect"
+		then
+			tail="$rev"
+			rev="$command"
+			command="$bisect"
+		fi
+		get_terms
+		git bisect--helper --check-and-set-terms "$command" "$TERM_GOOD" "$TERM_BAD" || exit
+		get_terms
+		case "$command" in
+		skip)
+			;;
+		start)
+			CURRENT_BISECT_BAD="$rev"
+			CURRENT_BISECT_GOOD="$tail"
+			;;
+		"$TERM_GOOD")
+			CURRENT_BISECT_GOOD="$rev" ;;
+		"$TERM_BAD")
+			CURRENT_BISECT_BAD="$rev" ;;
+		*)
+			die "$(gettext "?? what are you talking about?")" ;;
+		esac
+	done <"$GIT_DIR/BISECT_LOG"
+	IFS="$oIFS"
+}
+
 bisect_run () {
+	verify=
+	while test $# -ne 0
+	do
+		case "$1" in
+		--verify)
+			verify=t
+			;;
+		--no-verify)
+			verify=
+			;;
+	--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
+	if [ -n "$verify" ]; then
+		git rev-parse --verify -q BISECT_HEAD > /dev/null && die "$(gettext "bisect run --verify is incompatible with --no-checkout")"
+
+		get_current_bisect_bounds
+		test -n "$CURRENT_BISECT_BAD" || die "$(gettext "bisect run --verify: no current bad revision")"
+		test -n "$CURRENT_BISECT_GOOD" || die "$(gettext "bisect run --verify: no current good revision")"
+
+		bisected_head=$(bisect_head)
+		rev=$(git rev-parse --verify "$bisected_head") ||
+			die "$(eval_gettext "Bad rev input: \$bisected_head")"
+
+		trap "git checkout -q $rev" 0
+
+		# Check script passes for good rev.
+		command="$@"
+		eval_gettextln "verifying script passes at \$TERM_GOOD rev"
+		eval git checkout -q "$CURRENT_BISECT_GOOD" || die "$(eval_gettext "failed to check out \$TERM_GOOD rev")"
+		"$@"
+		res=$?
+		if [ $res -ne 0 ]
+		then
+			die_with_status $res "$(eval_gettext "aborting: run script fails for \$TERM_GOOD rev")"
+		fi
+
+		# Check script fails orderly for bad rev.
+		command="$@"
+		eval_gettextln "verifying script fails at \$TERM_BAD rev"
+		eval git checkout -q "$CURRENT_BISECT_BAD" || die "$(eval_gettext "failed to check out \$TERM_BAD rev")"
+		"$@"
+		res=$?
+		if [ $res -lt 0 -o $res -ge 128 ]
+		then
+			die "$(eval_gettext "aborting: exit code \$res is < 0 or >= 128")"
+		fi
+		if [ $res -eq 0 ]
+		then
+			die "$(eval_gettext "aborting: run script passes for \$TERM_BAD rev")"
+		fi
+		if [ $res -eq 125 ]
+		then
+			die "$(eval_gettext "aborting: run sript returns 125 (skip) for \$TERM_BAD rev")"
+		fi
+
+		# Check out pre-verify rev again.
+		git checkout -q "$rev"
+		trap '-' 0
+	fi
+
 	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
 
 	test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index b886529e59..acb01dcfff 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -321,6 +321,22 @@ test_expect_success 'bisect run & skip: find first bad' '
 	grep "$HASH6 is the first bad commit" my_bisect_log.txt
 '
 
+test_expect_success 'bisect run --verify: script fails for good rev' '
+	git bisect reset &&
+	git bisect start $HASH7 $HASH1 &&
+	test_must_fail git bisect run --verify false >my_bisect_log3.txt 2>&1 &&
+	test_i18ngrep "aborting: run script fails for good rev" my_bisect_log3.txt
+'
+
+test_expect_success 'bisect run --verify: script passes for bad rev' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect bad $HASH7 &&
+	git bisect good $HASH1 &&
+	test_must_fail git bisect run --verify true >my_bisect_log5.txt 2>&1 &&
+	test_i18ngrep "aborting: run script passes for bad rev" my_bisect_log5.txt
+'
+
 test_expect_success 'bisect skip only one range' '
 	git bisect reset &&
 	git bisect start $HASH7 $HASH1 &&

base-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0
-- 
gitgitgadget

Re: [PATCH] bisect: add a --verify flag to `bisect run`

[Junio C Hamano]

"Nico Weber via GitGitGadget" <gitgitgadget@gmail.com> writes:

> If it's passed, then `git bisect run --verify script` will first
> check out the good revision and verify that the script passes there,

The subject of the sentence is "git bisect run --verify script", so
"it" in "If it's passed" cannot be any of the "run", "--verify" or
"script".  So what is getting passed to that command???

> then check out the bad revision and verify that it fails there,
> and only then start the actual `git bisect run script`.

Is the intended use case

    - you have *not* run "git bisect" at all, but know two
      revisions, one $bad and one $good

    - you run "git bisect start $bad $good" just once, and then

    - you run "git bisect run [--verify] script"

or does this intend to cover more cases, like

    - you have *not* run "git bisect" at all, but know two
      revisions, one $bad and one $good

    - you run "git bisect start $bad $good", and

    - you iterate manually and use "git bisect good" and/or "git
      bisect bad" to mark points you tested, which advances $bad and
      accumulates good revisions, and after that

    - you run "git bisect run [--verify] script"

If the latter (and I hope it is), it is wrong to assume that the
command can "first check out THE good revision and verify".  There
may have been multiple points in the hsitory where you said "good"
and any of them may fail the script.

> We use `git bisect run` heavily for bisecting bugs in LLVM when using
> clang to build Chromium. We sometimes end up with run scripts that are
> broken in some way, either by missing the +x bit, or in more subtle
> ways, and this adds a simple, low conceptual overhead way to smoke check
> the run script before starting a bisect that could run for a day or two.

Very good intentions.



> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-741%2Fnico%2Frunverifyforupstreaming-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-741/nico/runverifyforupstreaming-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/741
>
>  Documentation/git-bisect.txt |  12 +++-
>  git-bisect.sh                | 110 ++++++++++++++++++++++++++++++++++-
>  t/t6030-bisect-porcelain.sh  |  16 +++++
>  3 files changed, 134 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index fbb39fbdf5..37658d8ed8 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -26,7 +26,7 @@ on the subcommand:
>   git bisect (visualize|view)
>   git bisect replay <logfile>
>   git bisect log
> - git bisect run <cmd>...
> + git bisect run [--verify] <cmd>...
>   git bisect help
>  
>  This command uses a binary search algorithm to find which commit in
> @@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the
>  command will print out a description of the first bad commit. The
>  reference `refs/bisect/bad` will be left pointing at that commit.
>  
> -
>  Bisect reset
>  ~~~~~~~~~~~~
>  
> @@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good
>  or bad, you can bisect by issuing the command:
>  
>  ------------
> -$ git bisect run my_script arguments
> +$ git bisect run [--verify] my_script arguments
>  ------------
>  
>  Note that the script (`my_script` in the above example) should exit
> @@ -376,6 +375,13 @@ ignored.
>  This option is particularly useful in avoiding false positives when a merged
>  branch contained broken or non-buildable commits, but the merge itself was OK.
>  
> +--verify::
> ++
> +Before the actual bisect run, check out the current bad revision and
> +verify that the script exits with a code between 1 and 127 (inclusive),
> +except 125, then check out the current good revision and verify that
> +the script exits with code 0. If not, abort the bisect run.
> +
>  EXAMPLES
>  --------
>  
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 2f60fefcfa..99ac01fb55 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -26,7 +26,7 @@ git bisect replay <logfile>
>  	replay bisection log.
>  git bisect log
>  	show bisect log.
> -git bisect run <cmd>...
> +git bisect run [--verify | --no-verify] <cmd>...
>  	use <cmd>... to automatically bisect.
>  
>  Please use "git help bisect" to get the full man page.'
> @@ -38,6 +38,8 @@ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
>  _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>  TERM_BAD=bad
>  TERM_GOOD=good
> +CURRENT_BISECT_BAD=
> +CURRENT_BISECT_GOOD=
>  
>  bisect_head()
>  {
> @@ -215,6 +217,7 @@ bisect_replay () {
>  		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
>  		if test "$git" = "git-bisect"
>  		then
> +			tail="$rev"
>  			rev="$command"
>  			command="$bisect"
>  		fi
> @@ -237,7 +240,112 @@ bisect_replay () {
>  	bisect_auto_next
>  }
>  
> +get_current_bisect_bounds () {
> +	test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
> +	oIFS="$IFS" IFS="$IFS$(printf '\015')"
> +	while read git bisect command rev tail
> +	do
> +		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
> +		if test "$git" = "git-bisect"
> +		then
> +			tail="$rev"
> +			rev="$command"
> +			command="$bisect"
> +		fi
> +		get_terms
> +		git bisect--helper --check-and-set-terms "$command" "$TERM_GOOD" "$TERM_BAD" || exit
> +		get_terms
> +		case "$command" in
> +		skip)
> +			;;
> +		start)
> +			CURRENT_BISECT_BAD="$rev"
> +			CURRENT_BISECT_GOOD="$tail"
> +			;;
> +		"$TERM_GOOD")
> +			CURRENT_BISECT_GOOD="$rev" ;;
> +		"$TERM_BAD")
> +			CURRENT_BISECT_BAD="$rev" ;;
> +		*)
> +			die "$(gettext "?? what are you talking about?")" ;;
> +		esac
> +	done <"$GIT_DIR/BISECT_LOG"
> +	IFS="$oIFS"
> +}
> +
>  bisect_run () {
> +	verify=
> +	while test $# -ne 0
> +	do
> +		case "$1" in
> +		--verify)
> +			verify=t
> +			;;
> +		--no-verify)
> +			verify=
> +			;;
> +	--)
> +			shift
> +			break
> +			;;
> +		-*)
> +			usage
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done
> +
> +	if [ -n "$verify" ]; then
> +		git rev-parse --verify -q BISECT_HEAD > /dev/null && die "$(gettext "bisect run --verify is incompatible with --no-checkout")"
> +
> +		get_current_bisect_bounds
> +		test -n "$CURRENT_BISECT_BAD" || die "$(gettext "bisect run --verify: no current bad revision")"
> +		test -n "$CURRENT_BISECT_GOOD" || die "$(gettext "bisect run --verify: no current good revision")"
> +
> +		bisected_head=$(bisect_head)
> +		rev=$(git rev-parse --verify "$bisected_head") ||
> +			die "$(eval_gettext "Bad rev input: \$bisected_head")"
> +
> +		trap "git checkout -q $rev" 0
> +
> +		# Check script passes for good rev.
> +		command="$@"
> +		eval_gettextln "verifying script passes at \$TERM_GOOD rev"
> +		eval git checkout -q "$CURRENT_BISECT_GOOD" || die "$(eval_gettext "failed to check out \$TERM_GOOD rev")"
> +		"$@"
> +		res=$?
> +		if [ $res -ne 0 ]
> +		then
> +			die_with_status $res "$(eval_gettext "aborting: run script fails for \$TERM_GOOD rev")"
> +		fi
> +
> +		# Check script fails orderly for bad rev.
> +		command="$@"
> +		eval_gettextln "verifying script fails at \$TERM_BAD rev"
> +		eval git checkout -q "$CURRENT_BISECT_BAD" || die "$(eval_gettext "failed to check out \$TERM_BAD rev")"
> +		"$@"
> +		res=$?
> +		if [ $res -lt 0 -o $res -ge 128 ]
> +		then
> +			die "$(eval_gettext "aborting: exit code \$res is < 0 or >= 128")"
> +		fi
> +		if [ $res -eq 0 ]
> +		then
> +			die "$(eval_gettext "aborting: run script passes for \$TERM_BAD rev")"
> +		fi
> +		if [ $res -eq 125 ]
> +		then
> +			die "$(eval_gettext "aborting: run sript returns 125 (skip) for \$TERM_BAD rev")"
> +		fi
> +
> +		# Check out pre-verify rev again.
> +		git checkout -q "$rev"
> +		trap '-' 0
> +	fi
> +
>  	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
>  
>  	test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index b886529e59..acb01dcfff 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -321,6 +321,22 @@ test_expect_success 'bisect run & skip: find first bad' '
>  	grep "$HASH6 is the first bad commit" my_bisect_log.txt
>  '
>  
> +test_expect_success 'bisect run --verify: script fails for good rev' '
> +	git bisect reset &&
> +	git bisect start $HASH7 $HASH1 &&
> +	test_must_fail git bisect run --verify false >my_bisect_log3.txt 2>&1 &&
> +	test_i18ngrep "aborting: run script fails for good rev" my_bisect_log3.txt
> +'
> +
> +test_expect_success 'bisect run --verify: script passes for bad rev' '
> +	git bisect reset &&
> +	git bisect start &&
> +	git bisect bad $HASH7 &&
> +	git bisect good $HASH1 &&
> +	test_must_fail git bisect run --verify true >my_bisect_log5.txt 2>&1 &&
> +	test_i18ngrep "aborting: run script passes for bad rev" my_bisect_log5.txt
> +'
> +
>  test_expect_success 'bisect skip only one range' '
>  	git bisect reset &&
>  	git bisect start $HASH7 $HASH1 &&
>
> base-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0

Re: [PATCH] bisect: add a --verify flag to `bisect run`

[Junio C Hamano]

"Nico Weber via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nico Weber <thakis@chromium.org>
>
> If it's passed, then `git bisect run --verify script` will first
> check out the good revision and verify that the script passes there,
> then check out the bad revision and verify that it fails there,
> and only then start the actual `git bisect run script`.
>
> We use `git bisect run` heavily for bisecting bugs in LLVM when using
> clang to build Chromium. We sometimes end up with run scripts that are
> broken in some way, either by missing the +x bit, or in more subtle
> ways, and this adds a simple, low conceptual overhead way to smoke check
> the run script before starting a bisect that could run for a day or two.

In our log message, we tend NOT to say "This commit does X" or "X is
done", because such a statement is often insufficient to illustrate
if the commit indeed does X, and explain why it is a good thing to
do X in the first place.

Instead, we 

 - first explain that the current system does not do X (in present
   tense, so we do NOT say "previously we did not do X"), then

 - explain why doing X would be a good thing, and finally

 - give an order to the codebase to start doing X.


For this change, it might look like this:

  The script given to the `git bisect run script` may broken in some
  way.  [Explain why it is bad that 'script' is not run on the known
  bad or good revisions here in the current code.  The script is run
  to test on the midway commit anyway and the lack of +x bit would
  be noticed just as quickly as you retest a known good or bad
  revision, so this 'feature' appears as a pure overhead to cause
  extra checkouts and builds for no great additional benefit, at
  least to me, and I tried but cannot make a good case for you
  here].

  As we already know at least one bad revision and good revision(s)
  when the command is run, we can sanity check the script by running
  on these revisions and checking if it produces expected results.

  Introduce `--verify` option to the `git bisect run` command for
  this purpose.  It would cost checking out extra revisions and
  retesting them but gives a basic sanity check for the script, to
  save a potentially long "bisect" session that may span days.

> @@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the
>  command will print out a description of the first bad commit. The
>  reference `refs/bisect/bad` will be left pointing at that commit.
>  
> -
>  Bisect reset
>  ~~~~~~~~~~~~

Why this removal?  Just checking to make sure that the document does
not use double-blank lines between sections, in which case this
removal is a bad idea.  In any case, this change does not have
anything to do with the `--verify` option, so leave it out---we can
review a documentation clean-up patch separately if you want.

> @@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good
>  or bad, you can bisect by issuing the command:
>  
>  ------------
> -$ git bisect run my_script arguments
> +$ git bisect run [--verify] my_script arguments
>  ------------
>  
>  Note that the script (`my_script` in the above example) should exit
> @@ -376,6 +375,13 @@ ignored.
>  This option is particularly useful in avoiding false positives when a merged
>  branch contained broken or non-buildable commits, but the merge itself was OK.
>  
> +--verify::
> ++
> +Before the actual bisect run, check out the current bad revision and
> +verify that the script exits with a code between 1 and 127 (inclusive),
> +except 125, then check out the current good revision and verify that
> +the script exits with code 0. If not, abort the bisect run.
> +

"current bad revision" is OK, there is no concenpt of "current good
revision", but "the last revision marked as good" is a better way to
phrase what the above paragraph may be trying to address.

> +get_current_bisect_bounds () {
> +	test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
> +	oIFS="$IFS" IFS="$IFS$(printf '\015')"

What is this CR about?  Is anybody manually futzing with DOS line endings?

> +	while read git bisect command rev tail
> +	do

There are bisect/bad ref and bisect/good-* refs that lets you figure
out exactly where the unknown territories are, but because you are
only trying to find one bad and one good rev to use as a sample, you
do not have to enumerate all of them.

But I doubt that reading the bisect log a good way to do this.  If
you only want one good and one bad rev to test, why not peek into
the implementation of bisect_next and find

    git show-ref --hash --verify refs/bisect/$TERM_BAD
    git for-each-ref --format="%(objectname" "refs/bisect/$TERM_GOOD-*"

that can easily be mimicked?

> +		start)
> +			CURRENT_BISECT_BAD="$rev"
> +			CURRENT_BISECT_GOOD="$tail"
> +			;;
> +		"$TERM_GOOD")
> +			CURRENT_BISECT_GOOD="$rev" ;;
> +		"$TERM_BAD")
> +			CURRENT_BISECT_BAD="$rev" ;;

These variables are better called LAST_BISECT_(GOOD/BAD), as current
set of good revs have multiple commits in it.  CURRENT_BISECT_BAD is
OK as-is when taken by itself, but as a contrasting pair to
LAST_BISECT_GOOD, renaming it to LAST_BISECT_BAD would make more
sense.

In any case, I am not sure why we need to read thru the log, futzing
with $IFS and all that.

>  bisect_run () {
> +	verify=
> +	while test $# -ne 0
> +	do
> +		case "$1" in
> +		--verify)
> +			verify=t
> +			;;
> +		--no-verify)
> +			verify=
> +			;;
> +	--)

Funny indentation.

> +			shift
> +			break
> +			;;


> +		-*)
> +			usage
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done
> +
> +	if [ -n "$verify" ]; then

Style (Documentation/CodingGuidelines):

 - We prefer "test" over "[ ... ]".

 - Do not write control structures on a single line with semicolon.
   "then" should be on the next line for if statements, and "do"
   should be on the next line for "while" and "for".

> +		git rev-parse --verify -q BISECT_HEAD > /dev/null && die "$(gettext "bisect run --verify is incompatible with --no-checkout")"

 - Overlong line (cut after && _without_ adding trailing backslash)

 - Redirection operators should be written with space before, but no
   space after them.  In other words, write 'echo test >"$file"'
   instead of 'echo test> $file' or 'echo test > $file'.

> +
> +		get_current_bisect_bounds
> +		test -n "$CURRENT_BISECT_BAD" || die "$(gettext "bisect run --verify: no current bad revision")"
> +		test -n "$CURRENT_BISECT_GOOD" || die "$(gettext "bisect run --verify: no current good revision")"

Dotto.

> +		bisected_head=$(bisect_head)
> +		rev=$(git rev-parse --verify "$bisected_head") ||
> +			die "$(eval_gettext "Bad rev input: \$bisected_head")"
> +
> +		trap "git checkout -q $rev" 0
> +
> +		# Check script passes for good rev.
> +		command="$@"

What effect do we expect out of this assignment to $command?

> +		eval_gettextln "verifying script passes at \$TERM_GOOD rev"
> +		eval git checkout -q "$CURRENT_BISECT_GOOD" || die "$(eval_gettext "failed to check out \$TERM_GOOD rev")"
> +		"$@"
> +		res=$?
> +		if [ $res -ne 0 ]
> +		then
> +			die_with_status $res "$(eval_gettext "aborting: run script fails for \$TERM_GOOD rev")"
> +		fi
> +
> +		# Check script fails orderly for bad rev.
> +		command="$@"

Ditto.

> +		eval_gettextln "verifying script fails at \$TERM_BAD rev"
> +		eval git checkout -q "$CURRENT_BISECT_BAD" || die "$(eval_gettext "failed to check out \$TERM_BAD rev")"
> +		"$@"
> +		res=$?
> +		if [ $res -lt 0 -o $res -ge 128 ]


 - We do not write our "test" command with "-a" and "-o" and use "&&"
   or "||" to concatenate multiple "test" commands instead,...

Re: [PATCH] bisect: add a --verify flag to `bisect run`

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> For this change, it might look like this:
>
>   The script given to the `git bisect run script` may broken in some
>   way.  [Explain why it is bad that 'script' is not run on the known
>   bad or good revisions here in the current code.  The script is run
>   to test on the midway commit anyway and the lack of +x bit would
>   be noticed just as quickly as you retest a known good or bad
>   revision, so this 'feature' appears as a pure overhead to cause
>   extra checkouts and builds for no great additional benefit, at
>   least to me, and I tried but cannot make a good case for you
>   here].

After thinking about this a bit, I think the reason why you want is
because once "bisect run" starts to run, you cannot tell between the
script that failed because it itself is buggy and the script that
noticed a breakage in the revision being tested.  So you may waste a
whole night if you start a long bisection session and then leave
work after seeing a few first failures.

But I still am skeptical if it is worth the overhead of checking out
a large working tree to a different revision (like Chromium), only
to sanity check the script.  If I were in such a situation, what I
would do would be to first manually run 'script' outside "git bisect
run" on the revision that happens to be checked out (i.e. the one
that "git bisect" asked me to test) before throwing the script at
"git bisect run".  Sure, I won't know if the revision yet to be
tested is good or bad, so there is no yardstick to tell if the
script is producing an expected result, but didn't I have the same
issue when declaring the "current" bad commit was bad and the
"current" good commit was good?