{"thread":{"id":"54338","subject":"[PATCH] bisect: add a --verify flag to `bisect run`","startedAt":"2020-10-03T18:33:59Z","lastAt":"2020-10-03T19:35:01Z","messageCount":4,"participants":["Nico Weber via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406869","messageId":"pull.741.git.1601750003858.gitgitgadget@gmail.com","threadId":"54338","inReplyTo":null,"subject":"[PATCH] bisect: add a --verify flag to `bisect run`","fromName":"Nico Weber via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-03T18:33:23Z","receivedAt":"2020-10-03T18:33:59Z","isPatch":true,"sender":{"key":"thakis@chromium.org","avatar":null},"body":"From: Nico Weber <thakis@chromium.org>\n\nIf it's passed, then `git bisect run --verify script` will first\ncheck out the good revision and verify that the script passes there,\nthen check out the bad revision and verify that it fails there,\nand only then start the actual `git bisect run script`.\n\nWe use `git bisect run` heavily for bisecting bugs in LLVM when using\nclang to build Chromium. We sometimes end up with run scripts that are\nbroken in some way, either by missing the +x bit, or in more subtle\nways, and this adds a simple, low conceptual overhead way to smoke check\nthe run script before starting a bisect that could run for a day or two.\n\nSigned-off-by: Nico Weber <thakis@chromium.org>\n---\n    bisect: add a --verify flag to bisect run\n    \n    If it's passed, then git bisect run --verify script will first check out\n    the good revision and verify that the script passes there, then check\n    out the bad revision and verify that it fails there, and only then start\n    the actual git bisect run script.\n    \n    We use git bisect run heavily for bisecting bugs in LLVM when using\n    clang to build Chromium. We sometimes end up with run scripts that are\n    broken in some way, either by missing the +x bit, or in more subtle\n    ways, and this adds a simple, low conceptual overhead way to smoke check\n    the run script before starting a bisect that could run for a day or two.\n    \n    Signed-off-by: Nico Weber thakis@chromium.org [thakis@chromium.org]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-741%2Fnico%2Frunverifyforupstreaming-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-741/nico/runverifyforupstreaming-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/741\n\n Documentation/git-bisect.txt |  12 +++-\n git-bisect.sh                | 110 ++++++++++++++++++++++++++++++++++-\n t/t6030-bisect-porcelain.sh  |  16 +++++\n 3 files changed, 134 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\nindex fbb39fbdf5..37658d8ed8 100644\n--- a/Documentation/git-bisect.txt\n+++ b/Documentation/git-bisect.txt\n@@ -26,7 +26,7 @@ on the subcommand:\n  git bisect (visualize|view)\n  git bisect replay <logfile>\n  git bisect log\n- git bisect run <cmd>...\n+ git bisect run [--verify] <cmd>...\n  git bisect help\n \n This command uses a binary search algorithm to find which commit in\n@@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the\n command will print out a description of the first bad commit. The\n reference `refs/bisect/bad` will be left pointing at that commit.\n \n-\n Bisect reset\n ~~~~~~~~~~~~\n \n@@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good\n or bad, you can bisect by issuing the command:\n \n ------------\n-$ git bisect run my_script arguments\n+$ git bisect run [--verify] my_script arguments\n ------------\n \n Note that the script (`my_script` in the above example) should exit\n@@ -376,6 +375,13 @@ ignored.\n This option is particularly useful in avoiding false positives when a merged\n branch contained broken or non-buildable commits, but the merge itself was OK.\n \n+--verify::\n++\n+Before the actual bisect run, check out the current bad revision and\n+verify that the script exits with a code between 1 and 127 (inclusive),\n+except 125, then check out the current good revision and verify that\n+the script exits with code 0. If not, abort the bisect run.\n+\n EXAMPLES\n --------\n \ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 2f60fefcfa..99ac01fb55 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -26,7 +26,7 @@ git bisect replay <logfile>\n \treplay bisection log.\n git bisect log\n \tshow bisect log.\n-git bisect run <cmd>...\n+git bisect run [--verify | --no-verify] <cmd>...\n \tuse <cmd>... to automatically bisect.\n \n Please use \"git help bisect\" to get the full man page.'\n@@ -38,6 +38,8 @@ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n+CURRENT_BISECT_BAD=\n+CURRENT_BISECT_GOOD=\n \n bisect_head()\n {\n@@ -215,6 +217,7 @@ bisect_replay () {\n \t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n \t\tif test \"$git\" = \"git-bisect\"\n \t\tthen\n+\t\t\ttail=\"$rev\"\n \t\t\trev=\"$command\"\n \t\t\tcommand=\"$bisect\"\n \t\tfi\n@@ -237,7 +240,112 @@ bisect_replay () {\n \tbisect_auto_next\n }\n \n+get_current_bisect_bounds () {\n+\ttest -s \"$GIT_DIR/BISECT_LOG\" || die \"$(gettext \"We are not bisecting.\")\"\n+\toIFS=\"$IFS\" IFS=\"$IFS$(printf '\\015')\"\n+\twhile read git bisect command rev tail\n+\tdo\n+\t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n+\t\tif test \"$git\" = \"git-bisect\"\n+\t\tthen\n+\t\t\ttail=\"$rev\"\n+\t\t\trev=\"$command\"\n+\t\t\tcommand=\"$bisect\"\n+\t\tfi\n+\t\tget_terms\n+\t\tgit bisect--helper --check-and-set-terms \"$command\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n+\t\tget_terms\n+\t\tcase \"$command\" in\n+\t\tskip)\n+\t\t\t;;\n+\t\tstart)\n+\t\t\tCURRENT_BISECT_BAD=\"$rev\"\n+\t\t\tCURRENT_BISECT_GOOD=\"$tail\"\n+\t\t\t;;\n+\t\t\"$TERM_GOOD\")\n+\t\t\tCURRENT_BISECT_GOOD=\"$rev\" ;;\n+\t\t\"$TERM_BAD\")\n+\t\t\tCURRENT_BISECT_BAD=\"$rev\" ;;\n+\t\t*)\n+\t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n+\t\tesac\n+\tdone <\"$GIT_DIR/BISECT_LOG\"\n+\tIFS=\"$oIFS\"\n+}\n+\n bisect_run () {\n+\tverify=\n+\twhile test $# -ne 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t--verify)\n+\t\t\tverify=t\n+\t\t\t;;\n+\t\t--no-verify)\n+\t\t\tverify=\n+\t\t\t;;\n+\t--)\n+\t\t\tshift\n+\t\t\tbreak\n+\t\t\t;;\n+\t\t-*)\n+\t\t\tusage\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\n+\tif [ -n \"$verify\" ]; then\n+\t\tgit rev-parse --verify -q BISECT_HEAD > /dev/null && die \"$(gettext \"bisect run --verify is incompatible with --no-checkout\")\"\n+\n+\t\tget_current_bisect_bounds\n+\t\ttest -n \"$CURRENT_BISECT_BAD\" || die \"$(gettext \"bisect run --verify: no current bad revision\")\"\n+\t\ttest -n \"$CURRENT_BISECT_GOOD\" || die \"$(gettext \"bisect run --verify: no current good revision\")\"\n+\n+\t\tbisected_head=$(bisect_head)\n+\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n+\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n+\n+\t\ttrap \"git checkout -q $rev\" 0\n+\n+\t\t# Check script passes for good rev.\n+\t\tcommand=\"$@\"\n+\t\teval_gettextln \"verifying script passes at \\$TERM_GOOD rev\"\n+\t\teval git checkout -q \"$CURRENT_BISECT_GOOD\" || die \"$(eval_gettext \"failed to check out \\$TERM_GOOD rev\")\"\n+\t\t\"$@\"\n+\t\tres=$?\n+\t\tif [ $res -ne 0 ]\n+\t\tthen\n+\t\t\tdie_with_status $res \"$(eval_gettext \"aborting: run script fails for \\$TERM_GOOD rev\")\"\n+\t\tfi\n+\n+\t\t# Check script fails orderly for bad rev.\n+\t\tcommand=\"$@\"\n+\t\teval_gettextln \"verifying script fails at \\$TERM_BAD rev\"\n+\t\teval git checkout -q \"$CURRENT_BISECT_BAD\" || die \"$(eval_gettext \"failed to check out \\$TERM_BAD rev\")\"\n+\t\t\"$@\"\n+\t\tres=$?\n+\t\tif [ $res -lt 0 -o $res -ge 128 ]\n+\t\tthen\n+\t\t\tdie \"$(eval_gettext \"aborting: exit code \\$res is < 0 or >= 128\")\"\n+\t\tfi\n+\t\tif [ $res -eq 0 ]\n+\t\tthen\n+\t\t\tdie \"$(eval_gettext \"aborting: run script passes for \\$TERM_BAD rev\")\"\n+\t\tfi\n+\t\tif [ $res -eq 125 ]\n+\t\tthen\n+\t\t\tdie \"$(eval_gettext \"aborting: run sript returns 125 (skip) for \\$TERM_BAD rev\")\"\n+\t\tfi\n+\n+\t\t# Check out pre-verify rev again.\n+\t\tgit checkout -q \"$rev\"\n+\t\ttrap '-' 0\n+\tfi\n+\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n \ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex b886529e59..acb01dcfff 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -321,6 +321,22 @@ test_expect_success 'bisect run & skip: find first bad' '\n \tgrep \"$HASH6 is the first bad commit\" my_bisect_log.txt\n '\n \n+test_expect_success 'bisect run --verify: script fails for good rev' '\n+\tgit bisect reset &&\n+\tgit bisect start $HASH7 $HASH1 &&\n+\ttest_must_fail git bisect run --verify false >my_bisect_log3.txt 2>&1 &&\n+\ttest_i18ngrep \"aborting: run script fails for good rev\" my_bisect_log3.txt\n+'\n+\n+test_expect_success 'bisect run --verify: script passes for bad rev' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect bad $HASH7 &&\n+\tgit bisect good $HASH1 &&\n+\ttest_must_fail git bisect run --verify true >my_bisect_log5.txt 2>&1 &&\n+\ttest_i18ngrep \"aborting: run script passes for bad rev\" my_bisect_log5.txt\n+'\n+\n test_expect_success 'bisect skip only one range' '\n \tgit bisect reset &&\n \tgit bisect start $HASH7 $HASH1 &&\n\nbase-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0\n-- \ngitgitgadget\n"},{"id":"406870","messageId":"xmqqlfgn87fy.fsf@gitster.c.googlers.com","threadId":"54338","inReplyTo":"pull.741.git.1601750003858.gitgitgadget@gmail.com","subject":"Re: [PATCH] bisect: add a --verify flag to `bisect run`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-03T18:56:17Z","receivedAt":"2020-10-03T18:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nico Weber via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> If it's passed, then `git bisect run --verify script` will first\n> check out the good revision and verify that the script passes there,\n\nThe subject of the sentence is \"git bisect run --verify script\", so\n\"it\" in \"If it's passed\" cannot be any of the \"run\", \"--verify\" or\n\"script\".  So what is getting passed to that command???\n\n> then check out the bad revision and verify that it fails there,\n> and only then start the actual `git bisect run script`.\n\nIs the intended use case\n\n    - you have *not* run \"git bisect\" at all, but know two\n      revisions, one $bad and one $good\n\n    - you run \"git bisect start $bad $good\" just once, and then\n\n    - you run \"git bisect run [--verify] script\"\n\nor does this intend to cover more cases, like\n\n    - you have *not* run \"git bisect\" at all, but know two\n      revisions, one $bad and one $good\n\n    - you run \"git bisect start $bad $good\", and\n\n    - you iterate manually and use \"git bisect good\" and/or \"git\n      bisect bad\" to mark points you tested, which advances $bad and\n      accumulates good revisions, and after that\n\n    - you run \"git bisect run [--verify] script\"\n\nIf the latter (and I hope it is), it is wrong to assume that the\ncommand can \"first check out THE good revision and verify\".  There\nmay have been multiple points in the hsitory where you said \"good\"\nand any of them may fail the script.\n\n> We use `git bisect run` heavily for bisecting bugs in LLVM when using\n> clang to build Chromium. We sometimes end up with run scripts that are\n> broken in some way, either by missing the +x bit, or in more subtle\n> ways, and this adds a simple, low conceptual overhead way to smoke check\n> the run script before starting a bisect that could run for a day or two.\n\nVery good intentions.\n\n\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-741%2Fnico%2Frunverifyforupstreaming-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-741/nico/runverifyforupstreaming-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/741\n>\n>  Documentation/git-bisect.txt |  12 +++-\n>  git-bisect.sh                | 110 ++++++++++++++++++++++++++++++++++-\n>  t/t6030-bisect-porcelain.sh  |  16 +++++\n>  3 files changed, 134 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\n> index fbb39fbdf5..37658d8ed8 100644\n> --- a/Documentation/git-bisect.txt\n> +++ b/Documentation/git-bisect.txt\n> @@ -26,7 +26,7 @@ on the subcommand:\n>   git bisect (visualize|view)\n>   git bisect replay <logfile>\n>   git bisect log\n> - git bisect run <cmd>...\n> + git bisect run [--verify] <cmd>...\n>   git bisect help\n>  \n>  This command uses a binary search algorithm to find which commit in\n> @@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the\n>  command will print out a description of the first bad commit. The\n>  reference `refs/bisect/bad` will be left pointing at that commit.\n>  \n> -\n>  Bisect reset\n>  ~~~~~~~~~~~~\n>  \n> @@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good\n>  or bad, you can bisect by issuing the command:\n>  \n>  ------------\n> -$ git bisect run my_script arguments\n> +$ git bisect run [--verify] my_script arguments\n>  ------------\n>  \n>  Note that the script (`my_script` in the above example) should exit\n> @@ -376,6 +375,13 @@ ignored.\n>  This option is particularly useful in avoiding false positives when a merged\n>  branch contained broken or non-buildable commits, but the merge itself was OK.\n>  \n> +--verify::\n> ++\n> +Before the actual bisect run, check out the current bad revision and\n> +verify that the script exits with a code between 1 and 127 (inclusive),\n> +except 125, then check out the current good revision and verify that\n> +the script exits with code 0. If not, abort the bisect run.\n> +\n>  EXAMPLES\n>  --------\n>  \n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 2f60fefcfa..99ac01fb55 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -26,7 +26,7 @@ git bisect replay <logfile>\n>  \treplay bisection log.\n>  git bisect log\n>  \tshow bisect log.\n> -git bisect run <cmd>...\n> +git bisect run [--verify | --no-verify] <cmd>...\n>  \tuse <cmd>... to automatically bisect.\n>  \n>  Please use \"git help bisect\" to get the full man page.'\n> @@ -38,6 +38,8 @@ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n>  _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n>  TERM_BAD=bad\n>  TERM_GOOD=good\n> +CURRENT_BISECT_BAD=\n> +CURRENT_BISECT_GOOD=\n>  \n>  bisect_head()\n>  {\n> @@ -215,6 +217,7 @@ bisect_replay () {\n>  \t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n>  \t\tif test \"$git\" = \"git-bisect\"\n>  \t\tthen\n> +\t\t\ttail=\"$rev\"\n>  \t\t\trev=\"$command\"\n>  \t\t\tcommand=\"$bisect\"\n>  \t\tfi\n> @@ -237,7 +240,112 @@ bisect_replay () {\n>  \tbisect_auto_next\n>  }\n>  \n> +get_current_bisect_bounds () {\n> +\ttest -s \"$GIT_DIR/BISECT_LOG\" || die \"$(gettext \"We are not bisecting.\")\"\n> +\toIFS=\"$IFS\" IFS=\"$IFS$(printf '\\015')\"\n> +\twhile read git bisect command rev tail\n> +\tdo\n> +\t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n> +\t\tif test \"$git\" = \"git-bisect\"\n> +\t\tthen\n> +\t\t\ttail=\"$rev\"\n> +\t\t\trev=\"$command\"\n> +\t\t\tcommand=\"$bisect\"\n> +\t\tfi\n> +\t\tget_terms\n> +\t\tgit bisect--helper --check-and-set-terms \"$command\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> +\t\tget_terms\n> +\t\tcase \"$command\" in\n> +\t\tskip)\n> +\t\t\t;;\n> +\t\tstart)\n> +\t\t\tCURRENT_BISECT_BAD=\"$rev\"\n> +\t\t\tCURRENT_BISECT_GOOD=\"$tail\"\n> +\t\t\t;;\n> +\t\t\"$TERM_GOOD\")\n> +\t\t\tCURRENT_BISECT_GOOD=\"$rev\" ;;\n> +\t\t\"$TERM_BAD\")\n> +\t\t\tCURRENT_BISECT_BAD=\"$rev\" ;;\n> +\t\t*)\n> +\t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n> +\t\tesac\n> +\tdone <\"$GIT_DIR/BISECT_LOG\"\n> +\tIFS=\"$oIFS\"\n> +}\n> +\n>  bisect_run () {\n> +\tverify=\n> +\twhile test $# -ne 0\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t--verify)\n> +\t\t\tverify=t\n> +\t\t\t;;\n> +\t\t--no-verify)\n> +\t\t\tverify=\n> +\t\t\t;;\n> +\t--)\n> +\t\t\tshift\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\t-*)\n> +\t\t\tusage\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\n> +\tif [ -n \"$verify\" ]; then\n> +\t\tgit rev-parse --verify -q BISECT_HEAD > /dev/null && die \"$(gettext \"bisect run --verify is incompatible with --no-checkout\")\"\n> +\n> +\t\tget_current_bisect_bounds\n> +\t\ttest -n \"$CURRENT_BISECT_BAD\" || die \"$(gettext \"bisect run --verify: no current bad revision\")\"\n> +\t\ttest -n \"$CURRENT_BISECT_GOOD\" || die \"$(gettext \"bisect run --verify: no current good revision\")\"\n> +\n> +\t\tbisected_head=$(bisect_head)\n> +\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n> +\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n> +\n> +\t\ttrap \"git checkout -q $rev\" 0\n> +\n> +\t\t# Check script passes for good rev.\n> +\t\tcommand=\"$@\"\n> +\t\teval_gettextln \"verifying script passes at \\$TERM_GOOD rev\"\n> +\t\teval git checkout -q \"$CURRENT_BISECT_GOOD\" || die \"$(eval_gettext \"failed to check out \\$TERM_GOOD rev\")\"\n> +\t\t\"$@\"\n> +\t\tres=$?\n> +\t\tif [ $res -ne 0 ]\n> +\t\tthen\n> +\t\t\tdie_with_status $res \"$(eval_gettext \"aborting: run script fails for \\$TERM_GOOD rev\")\"\n> +\t\tfi\n> +\n> +\t\t# Check script fails orderly for bad rev.\n> +\t\tcommand=\"$@\"\n> +\t\teval_gettextln \"verifying script fails at \\$TERM_BAD rev\"\n> +\t\teval git checkout -q \"$CURRENT_BISECT_BAD\" || die \"$(eval_gettext \"failed to check out \\$TERM_BAD rev\")\"\n> +\t\t\"$@\"\n> +\t\tres=$?\n> +\t\tif [ $res -lt 0 -o $res -ge 128 ]\n> +\t\tthen\n> +\t\t\tdie \"$(eval_gettext \"aborting: exit code \\$res is < 0 or >= 128\")\"\n> +\t\tfi\n> +\t\tif [ $res -eq 0 ]\n> +\t\tthen\n> +\t\t\tdie \"$(eval_gettext \"aborting: run script passes for \\$TERM_BAD rev\")\"\n> +\t\tfi\n> +\t\tif [ $res -eq 125 ]\n> +\t\tthen\n> +\t\t\tdie \"$(eval_gettext \"aborting: run sript returns 125 (skip) for \\$TERM_BAD rev\")\"\n> +\t\tfi\n> +\n> +\t\t# Check out pre-verify rev again.\n> +\t\tgit checkout -q \"$rev\"\n> +\t\ttrap '-' 0\n> +\tfi\n> +\n>  \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n>  \n>  \ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index b886529e59..acb01dcfff 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -321,6 +321,22 @@ test_expect_success 'bisect run & skip: find first bad' '\n>  \tgrep \"$HASH6 is the first bad commit\" my_bisect_log.txt\n>  '\n>  \n> +test_expect_success 'bisect run --verify: script fails for good rev' '\n> +\tgit bisect reset &&\n> +\tgit bisect start $HASH7 $HASH1 &&\n> +\ttest_must_fail git bisect run --verify false >my_bisect_log3.txt 2>&1 &&\n> +\ttest_i18ngrep \"aborting: run script fails for good rev\" my_bisect_log3.txt\n> +'\n> +\n> +test_expect_success 'bisect run --verify: script passes for bad rev' '\n> +\tgit bisect reset &&\n> +\tgit bisect start &&\n> +\tgit bisect bad $HASH7 &&\n> +\tgit bisect good $HASH1 &&\n> +\ttest_must_fail git bisect run --verify true >my_bisect_log5.txt 2>&1 &&\n> +\ttest_i18ngrep \"aborting: run script passes for bad rev\" my_bisect_log5.txt\n> +'\n> +\n>  test_expect_success 'bisect skip only one range' '\n>  \tgit bisect reset &&\n>  \tgit bisect start $HASH7 $HASH1 &&\n>\n> base-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0\n"},{"id":"406871","messageId":"xmqqeemf8633.fsf@gitster.c.googlers.com","threadId":"54338","inReplyTo":"pull.741.git.1601750003858.gitgitgadget@gmail.com","subject":"Re: [PATCH] bisect: add a --verify flag to `bisect run`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-03T19:25:36Z","receivedAt":"2020-10-03T19:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nico Weber via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Nico Weber <thakis@chromium.org>\n>\n> If it's passed, then `git bisect run --verify script` will first\n> check out the good revision and verify that the script passes there,\n> then check out the bad revision and verify that it fails there,\n> and only then start the actual `git bisect run script`.\n>\n> We use `git bisect run` heavily for bisecting bugs in LLVM when using\n> clang to build Chromium. We sometimes end up with run scripts that are\n> broken in some way, either by missing the +x bit, or in more subtle\n> ways, and this adds a simple, low conceptual overhead way to smoke check\n> the run script before starting a bisect that could run for a day or two.\n\nIn our log message, we tend NOT to say \"This commit does X\" or \"X is\ndone\", because such a statement is often insufficient to illustrate\nif the commit indeed does X, and explain why it is a good thing to\ndo X in the first place.\n\nInstead, we \n\n - first explain that the current system does not do X (in present\n   tense, so we do NOT say \"previously we did not do X\"), then\n\n - explain why doing X would be a good thing, and finally\n\n - give an order to the codebase to start doing X.\n\n\nFor this change, it might look like this:\n\n  The script given to the `git bisect run script` may broken in some\n  way.  [Explain why it is bad that 'script' is not run on the known\n  bad or good revisions here in the current code.  The script is run\n  to test on the midway commit anyway and the lack of +x bit would\n  be noticed just as quickly as you retest a known good or bad\n  revision, so this 'feature' appears as a pure overhead to cause\n  extra checkouts and builds for no great additional benefit, at\n  least to me, and I tried but cannot make a good case for you\n  here].\n\n  As we already know at least one bad revision and good revision(s)\n  when the command is run, we can sanity check the script by running\n  on these revisions and checking if it produces expected results.\n\n  Introduce `--verify` option to the `git bisect run` command for\n  this purpose.  It would cost checking out extra revisions and\n  retesting them but gives a basic sanity check for the script, to\n  save a potentially long \"bisect\" session that may span days.\n\n> @@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the\n>  command will print out a description of the first bad commit. The\n>  reference `refs/bisect/bad` will be left pointing at that commit.\n>  \n> -\n>  Bisect reset\n>  ~~~~~~~~~~~~\n\nWhy this removal?  Just checking to make sure that the document does\nnot use double-blank lines between sections, in which case this\nremoval is a bad idea.  In any case, this change does not have\nanything to do with the `--verify` option, so leave it out---we can\nreview a documentation clean-up patch separately if you want.\n\n> @@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good\n>  or bad, you can bisect by issuing the command:\n>  \n>  ------------\n> -$ git bisect run my_script arguments\n> +$ git bisect run [--verify] my_script arguments\n>  ------------\n>  \n>  Note that the script (`my_script` in the above example) should exit\n> @@ -376,6 +375,13 @@ ignored.\n>  This option is particularly useful in avoiding false positives when a merged\n>  branch contained broken or non-buildable commits, but the merge itself was OK.\n>  \n> +--verify::\n> ++\n> +Before the actual bisect run, check out the current bad revision and\n> +verify that the script exits with a code between 1 and 127 (inclusive),\n> +except 125, then check out the current good revision and verify that\n> +the script exits with code 0. If not, abort the bisect run.\n> +\n\n\"current bad revision\" is OK, there is no concenpt of \"current good\nrevision\", but \"the last revision marked as good\" is a better way to\nphrase what the above paragraph may be trying to address.\n\n> +get_current_bisect_bounds () {\n> +\ttest -s \"$GIT_DIR/BISECT_LOG\" || die \"$(gettext \"We are not bisecting.\")\"\n> +\toIFS=\"$IFS\" IFS=\"$IFS$(printf '\\015')\"\n\nWhat is this CR about?  Is anybody manually futzing with DOS line endings?\n\n> +\twhile read git bisect command rev tail\n> +\tdo\n\nThere are bisect/bad ref and bisect/good-* refs that lets you figure\nout exactly where the unknown territories are, but because you are\nonly trying to find one bad and one good rev to use as a sample, you\ndo not have to enumerate all of them.\n\nBut I doubt that reading the bisect log a good way to do this.  If\nyou only want one good and one bad rev to test, why not peek into\nthe implementation of bisect_next and find\n\n    git show-ref --hash --verify refs/bisect/$TERM_BAD\n    git for-each-ref --format=\"%(objectname\" \"refs/bisect/$TERM_GOOD-*\"\n\nthat can easily be mimicked?\n\n> +\t\tstart)\n> +\t\t\tCURRENT_BISECT_BAD=\"$rev\"\n> +\t\t\tCURRENT_BISECT_GOOD=\"$tail\"\n> +\t\t\t;;\n> +\t\t\"$TERM_GOOD\")\n> +\t\t\tCURRENT_BISECT_GOOD=\"$rev\" ;;\n> +\t\t\"$TERM_BAD\")\n> +\t\t\tCURRENT_BISECT_BAD=\"$rev\" ;;\n\nThese variables are better called LAST_BISECT_(GOOD/BAD), as current\nset of good revs have multiple commits in it.  CURRENT_BISECT_BAD is\nOK as-is when taken by itself, but as a contrasting pair to\nLAST_BISECT_GOOD, renaming it to LAST_BISECT_BAD would make more\nsense.\n\nIn any case, I am not sure why we need to read thru the log, futzing\nwith $IFS and all that.\n\n>  bisect_run () {\n> +\tverify=\n> +\twhile test $# -ne 0\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t--verify)\n> +\t\t\tverify=t\n> +\t\t\t;;\n> +\t\t--no-verify)\n> +\t\t\tverify=\n> +\t\t\t;;\n> +\t--)\n\nFunny indentation.\n\n> +\t\t\tshift\n> +\t\t\tbreak\n> +\t\t\t;;\n\n\n> +\t\t-*)\n> +\t\t\tusage\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\n> +\tif [ -n \"$verify\" ]; then\n\nStyle (Documentation/CodingGuidelines):\n\n - We prefer \"test\" over \"[ ... ]\".\n\n - Do not write control structures on a single line with semicolon.\n   \"then\" should be on the next line for if statements, and \"do\"\n   should be on the next line for \"while\" and \"for\".\n\n> +\t\tgit rev-parse --verify -q BISECT_HEAD > /dev/null && die \"$(gettext \"bisect run --verify is incompatible with --no-checkout\")\"\n\n - Overlong line (cut after && _without_ adding trailing backslash)\n\n - Redirection operators should be written with space before, but no\n   space after them.  In other words, write 'echo test >\"$file\"'\n   instead of 'echo test> $file' or 'echo test > $file'.\n\n> +\n> +\t\tget_current_bisect_bounds\n> +\t\ttest -n \"$CURRENT_BISECT_BAD\" || die \"$(gettext \"bisect run --verify: no current bad revision\")\"\n> +\t\ttest -n \"$CURRENT_BISECT_GOOD\" || die \"$(gettext \"bisect run --verify: no current good revision\")\"\n\nDotto.\n\n> +\t\tbisected_head=$(bisect_head)\n> +\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n> +\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n> +\n> +\t\ttrap \"git checkout -q $rev\" 0\n> +\n> +\t\t# Check script passes for good rev.\n> +\t\tcommand=\"$@\"\n\nWhat effect do we expect out of this assignment to $command?\n\n> +\t\teval_gettextln \"verifying script passes at \\$TERM_GOOD rev\"\n> +\t\teval git checkout -q \"$CURRENT_BISECT_GOOD\" || die \"$(eval_gettext \"failed to check out \\$TERM_GOOD rev\")\"\n> +\t\t\"$@\"\n> +\t\tres=$?\n> +\t\tif [ $res -ne 0 ]\n> +\t\tthen\n> +\t\t\tdie_with_status $res \"$(eval_gettext \"aborting: run script fails for \\$TERM_GOOD rev\")\"\n> +\t\tfi\n> +\n> +\t\t# Check script fails orderly for bad rev.\n> +\t\tcommand=\"$@\"\n\nDitto.\n\n> +\t\teval_gettextln \"verifying script fails at \\$TERM_BAD rev\"\n> +\t\teval git checkout -q \"$CURRENT_BISECT_BAD\" || die \"$(eval_gettext \"failed to check out \\$TERM_BAD rev\")\"\n> +\t\t\"$@\"\n> +\t\tres=$?\n> +\t\tif [ $res -lt 0 -o $res -ge 128 ]\n\n\n - We do not write our \"test\" command with \"-a\" and \"-o\" and use \"&&\"\n   or \"||\" to concatenate multiple \"test\" commands instead,...\n\n"},{"id":"406872","messageId":"xmqqa6x385nj.fsf@gitster.c.googlers.com","threadId":"54338","inReplyTo":"xmqqeemf8633.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bisect: add a --verify flag to `bisect run`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-03T19:34:56Z","receivedAt":"2020-10-03T19:35:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> For this change, it might look like this:\n>\n>   The script given to the `git bisect run script` may broken in some\n>   way.  [Explain why it is bad that 'script' is not run on the known\n>   bad or good revisions here in the current code.  The script is run\n>   to test on the midway commit anyway and the lack of +x bit would\n>   be noticed just as quickly as you retest a known good or bad\n>   revision, so this 'feature' appears as a pure overhead to cause\n>   extra checkouts and builds for no great additional benefit, at\n>   least to me, and I tried but cannot make a good case for you\n>   here].\n\nAfter thinking about this a bit, I think the reason why you want is\nbecause once \"bisect run\" starts to run, you cannot tell between the\nscript that failed because it itself is buggy and the script that\nnoticed a breakage in the revision being tested.  So you may waste a\nwhole night if you start a long bisection session and then leave\nwork after seeing a few first failures.\n\nBut I still am skeptical if it is worth the overhead of checking out\na large working tree to a different revision (like Chromium), only\nto sanity check the script.  If I were in such a situation, what I\nwould do would be to first manually run 'script' outside \"git bisect\nrun\" on the revision that happens to be checked out (i.e. the one\nthat \"git bisect\" asked me to test) before throwing the script at\n\"git bisect run\".  Sure, I won't know if the revision yet to be\ntested is good or bad, so there is no yardstick to tell if the\nscript is producing an expected result, but didn't I have the same\nissue when declaring the \"current\" bad commit was bad and the\n\"current\" good commit was good?  \n"}]}