{"thread":{"id":"52555","subject":"[RFC PATCH] bisect run: allow inverting meaning of exit code","startedAt":"2020-01-03T04:39:40Z","lastAt":"2020-01-07T01:43:07Z","messageCount":8,"participants":["Stephen Oberholtzer","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389189","messageId":"20200103043027.4537-1-stevie@qrpff.net","threadId":"52555","inReplyTo":null,"subject":"[RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Stephen Oberholtzer","fromEmail":"stevie@qrpff.net","sentAt":"2020-01-03T04:30:28Z","receivedAt":"2020-01-03T04:39:40Z","isPatch":true,"sender":{"key":"stevie@qrpff.net","avatar":null},"body":"NOTE: This is obviously not ready for merging; I just wanted to\nget feedback.\n\nIn particular, I expect some bikeshedding on the specific option\nnames (-r, --invert, --expect).  I'm probably going to change\n`--expect` to `--success`, in fact.\n\nIf we can come to a consensus on the names (and, of course, on the\nfeature itself), I'll clean up the tests, remove the debug output,\nupdate the documentation, then resubmit.\n\n>8------------------------------------------------------8<\n\n\nIf your automated test naturally yields zero for _old_/_good_,\n1-124 or 126-127 for _new_/_bad_, then you're set.\n\nIf that logic is reversed, however, it's a bit more of a pain: you\ncan't just stick a `sh -c !` in front of your command, because that\ndoesn't account for exit codes 125 or 129-255.\n\nThis commit enhances `git bisect run` as follows:\n\n* '--' can be used as an option list terminator,\n  just as everywhere else.\n\n* The treatment of the exit code can be selected via an option:\n\n  - No option, of course, treats 0 as _old_/_good_\n  - `-r` (for reverse) treats 0 as _new_/_bad_\n  - `--invert` is the long form for `-r`\n  - `--expect=<term>` treats 0 as <term>\n\nYou're not allowed to specify more than one expectation.\n\nNote that this lets one specify `--expect=good` as an explicit\nselection of the default behavior.  This is intentional.\n\nSigned-off-by: Stephen Oberholtzer <stevie@qrpff.net>\n---\n git-bisect.sh         |  33 +++++++++-\n t/t6071-bisect-run.sh | 142 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 172 insertions(+), 3 deletions(-)\n create mode 100755 t/t6071-bisect-run.sh\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex efee12b8b1..dbeb213846 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 [--expect=<term> | -r | --invert] [--] <cmd>...\n \tuse <cmd>... to automatically bisect.\n \n Please use \"git help bisect\" to get the full man page.'\n@@ -238,6 +238,31 @@ bisect_replay () {\n bisect_run () {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n+\tSUCCESS_TERM=$TERM_GOOD\n+\tFAIL_TERM=$TERM_BAD\n+\tINVERT_SET=\n+\twhile [ \"$1\" != \"${1#-}\" ]; do\n+\t\tcase \"$1\" in\n+\t\t--)\n+\t\t\tshift\n+\t\t\tbreak ;;\n+\t\t--expect=$TERM_GOOD)\n+\t\t\t[ -z \"$INVERT_SET\" ] || die \"$(gettext \"bisect run: multiple expect options specified\")\"\n+\t\t\tINVERT_SET=1 ;;\n+\t\t-r|--invert|--expect=$TERM_BAD)\n+\t\t\t[ -z \"$INVERT_SET\" ] || die \"$(gettext \"bisect run: multiple expect options specified\")\"\n+\t\t\tSUCCESS_TERM=$TERM_BAD\n+\t\t\tFAIL_TERM=$TERM_GOOD\n+\t\t\tINVERT_SET=1 ;;\n+\t\t--expect=*)\n+\t\t\t# how to localize part 2?\n+\t\t\tdie \"$(printf \"$(gettext \"bisect run: invalid --expect value, use --expect=%s or --expect=%s\")\" \"$TERM_GOOD\" \"$TERM_BAD\")\" ;;\n+\t\t*)\n+\t\t\tdie \"$(printf \"$(gettext \"bisect run: invalid option: %s\")\" \"$1\")\" ;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\n \ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n \n \twhile true\n@@ -262,11 +287,13 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\tstate='skip'\n \t\telif [ $res -gt 0 ]\n \t\tthen\n-\t\t\tstate=\"$TERM_BAD\"\n+\t\t\tstate=\"$FAIL_TERM\"\n \t\telse\n-\t\t\tstate=\"$TERM_GOOD\"\n+\t\t\tstate=\"$SUCCESS_TERM\"\n \t\tfi\n \n+\t\techo \"exit code $res means this commit is $state\"\n+\n \t\t# We have to use a subshell because \"bisect_state\" can exit.\n \t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n \t\tres=$?\ndiff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh\nnew file mode 100755\nindex 0000000000..2708e0f854\n--- /dev/null\n+++ b/t/t6071-bisect-run.sh\n@@ -0,0 +1,142 @@\n+# verify that unrecognized options are rejected by 'git bisect run'\n+#!/bin/sh\n+\n+# the linter's not smart enough to handle set -e\n+GIT_TEST_CHAIN_LINT=0\n+\n+test_description='Tests git bisect run'\n+\n+exec </dev/null\n+\n+. ./test-lib.sh\n+\n+{ test_expect_success 'Setting up repo for \"git bisect run\" tests.' \"$(cat)\" ; } <<'SETUP'\n+(\n+# I don't know how they managed it, but the git test engine\n+# somehow ignores the effect of 'set -e'.\n+set -eu || exit 1\n+# set -e canary\n+false\n+# hopefully, next year, we get -o pipefail!\n+echo '.DEFAULT: dummy\n+.PHONY: dummy\n+dummy:\n+\ttrue\n+' > Makefile\n+make\n+echo '0' >path0\n+git update-index --add -- Makefile path0\n+git commit -q -m 'initial commit'\n+git tag working0\n+# make some commits that don't cause problems\n+for x in `test_seq 1 20`; do\n+\techo \"$x\" >path0\n+\tgit update-index --replace -- path0\n+\tgit commit -q -m \"working commit $x\"\n+\tgit tag \"working$x\"\n+done\n+# break the makefile\n+sed -i.bak -e 's/true/false/' Makefile\n+rm -f Makefile.bak\n+! make\n+git update-index --replace -- Makefile\n+git commit -q -m \"First broken commit\"\n+git tag broken0\n+# make some more commits that still FTBFS\n+echo \"exit code was $?; flags are $-\"\n+for x in `test_seq 1 20`; do\n+\techo \"$x\" >path0\n+\tgit update-index --replace -- path0\n+\tgit commit -q -m \"broken build $x\"\n+\tgit tag \"broken$x\"\n+done\n+# repair it\n+git checkout working0 -- Makefile\n+make\n+git update-index --replace -- Makefile\n+git commit -q -m \"First repaired commit\"\n+git tag fixed0\n+# make some more commits with the bugfix\n+for x in `test_seq 1 20`; do\n+\techo \"$x\" >path0\n+\tgit update-index --replace -- path0\n+\tgit commit -q -m \"fixed build $x\"\n+\tgit tag \"fixed$x\"\n+done\n+#sh -c 'bash -i <> /dev/tty >&0 2>&1'\n+)\n+\n+SETUP\n+\n+test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'\n+\n+test_expect_failure() {\n+\tshift\n+\t#echo arguments are \"$*\"\n+\ttest_must_fail \"$@\"\n+}\n+\n+# okay, let's do some negative testing\n+\n+OLDPATH=\"$PATH\"\n+\n+PATH=\"$PATH:.\"\n+\n+test_expect_success 'setup this-is-not-a-valid-option' '\n+ echo \"#/bin/sh\" > --this-is-not-a-valid-option &&\n+ chmod a+x -- --this-is-not-a-valid-option &&\n+ --this-is-not-a-valid-option'\n+\n+test_expect_failure 'git bisect run: reject unrecognized options' git bisect run --this-is-not-a-valid-option\n+\n+PATH=\"$OLDPATH\"\n+\n+test_expect_failure 'git bisect run: reject invalid values for --expect'  git bisect run --expect=invalid make\n+\n+# okay, all of these settings are mutually exclusive (for sanity's sake, even with themselves)\n+for a in --expect=bad --expect=good -r --invert; do\n+\tfor b in --expect=bad --expect=good -r --invert; do\n+\t\ttest_expect_failure 'git bisect run: reject multiple --expect options'  git bisect run $a $b make\n+\tdone\n+done\n+\n+# finally, verify that '--' is honored (note that will mess things up and require a bisect reset)\n+PATH=\"$PATH:.\"\n+\n+test_expect_success 'git bisect run: honor --' 'git bisect run -- --this-is-not-a-valid-option'\n+\n+PATH=\"$OLDPATH\"\n+\n+for expect_syntax in '' --expect=good; do\n+\n+\t# now we have to undo the bisect run\n+\ttest_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9'\n+\n+\ttest_expect_success \"running bisection ($expect_syntax)\" \"\n+git bisect run $expect_syntax make &&\n+git log --oneline &&\n+\t# we should have determined that broken0 is the first bad version\n+\ttest_cmp_rev broken0 refs/bisect/bad &&\n+\t# and that version should be the one checked out\n+\ttest_cmp_rev broken0 HEAD\n+\"\n+done\n+\n+\n+# NOW, test the reverse:  find when we fixed it again\n+\n+for expect_syntax in -r --invert --expect=fixed; do\n+\n+\ttest_expect_success 'restarting bisection' 'git bisect reset && git bisect start --term-old=broken --term-new=fixed && git bisect broken broken20 && git bisect fixed fixed20'\n+\ttest_expect_success \"running bisection ($expect_syntax)\" \"\n+\t\tgit bisect run $expect_syntax make &&\n+\t\tgit log --oneline &&\n+\ttest_cmp_rev fixed0 refs/bisect/fixed &&\n+\ttest_cmp_rev fixed0 HEAD\n+\t\"\n+done\n+\n+test_expect_failure 'sanity check error message with custom terms' git bisect run --expect=invalid make\n+\n+\n+test_done\n-- \n2.20.1\n\n"},{"id":"389207","messageId":"20200104012230.GD130883@google.com","threadId":"52555","inReplyTo":"20200103043027.4537-1-stevie@qrpff.net","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-04T01:22:30Z","receivedAt":"2020-01-04T01:22:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Christian Couder, bisect expert)\nHi!\n\nStephen Oberholtzer wrote:\n\n> NOTE: This is obviously not ready for merging; I just wanted to\n> get feedback.\n\nThanks for writing.  I like where you're going.\n\n> In particular, I expect some bikeshedding on the specific option\n> names (-r, --invert, --expect).  I'm probably going to change\n> `--expect` to `--success`, in fact.\n>\n> If we can come to a consensus on the names (and, of course, on the\n> feature itself), I'll clean up the tests, remove the debug output,\n> update the documentation, then resubmit.\n>\n> >8------------------------------------------------------8<\n>\n> If your automated test naturally yields zero for _old_/_good_,\n> 1-124 or 126-127 for _new_/_bad_, then you're set.\n>\n> If that logic is reversed, however, it's a bit more of a pain: you\n> can't just stick a `sh -c !` in front of your command, because that\n> doesn't account for exit codes 125 or 129-255.\n>\n> This commit enhances `git bisect run` as follows:\n>\n> * '--' can be used as an option list terminator,\n>   just as everywhere else.\n\nCould this part go in a separate commit?  That way, it can go in\nmore quickly while we figure out the rest. :)\n\n> * The treatment of the exit code can be selected via an option:\n>\n>   - No option, of course, treats 0 as _old_/_good_\n>   - `-r` (for reverse) treats 0 as _new_/_bad_\n>   - `--invert` is the long form for `-r`\n>   - `--expect=<term>` treats 0 as <term>\n\nInitial thoughts:\n\n- it's not immediately clear to me that this is common enough\n  to need the short-and-sweet name \"-r\".  Could it have a long\n  form only?\n\n- I think I agree with you that \"git bisect run --expect=5\" might\n  be more clearly written as \"git bisect run --success=5\".  Or even\n  something explicitly referring to exit status, like\n  --success-status=5.\n\n- This has an interesting relationship to the \"alternate terms\"\n  feature (--term-old / --term-new).  I don't know if there's a\n  good way to make that more explicit --- maybe just some notes\n  with examples in the relevant parts of the manpage?\n\n- the name --invert doesn't make it obvious to me what it is\n  inverting: good versus bad, ones complement of the status\n  code, revision range passed to \"git bisect start\"?\n\n  I'm even tempted to call it something like '-!', to make the\n  allusion to ! in shells more explicit.  (But that's probably not a\n  great idea, since quoting ! correctly in interactive shells can be\n  difficult.)\n\n  Are there other commands we can try to make this consistent with?\n  \"find\" supports arbitrary expressions involving '!' and '-not'.\n  \"git grep\" has --invert-match: perhaps a name --invert-status\n  would be clear enough?\n\n> You're not allowed to specify more than one expectation.\n\nUsual convention would be \"last specified option wins\".\n\n> Note that this lets one specify `--expect=good` as an explicit\n> selection of the default behavior.  This is intentional.\n>\n> Signed-off-by: Stephen Oberholtzer <stevie@qrpff.net>\n> ---\n>  git-bisect.sh         |  33 +++++++++-\n>  t/t6071-bisect-run.sh | 142 ++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 172 insertions(+), 3 deletions(-)\n>  create mode 100755 t/t6071-bisect-run.sh\n\nAs you said, this needs docs.  Writing docs often helps make the UI a\nbit better since it forces one to think about the various ways a tool\nwould be used in practice.\n\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index efee12b8b1..dbeb213846 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 [--expect=<term> | -r | --invert] [--] <cmd>...\n>  \tuse <cmd>... to automatically bisect.\n>  \n>  Please use \"git help bisect\" to get the full man page.'\n> @@ -238,6 +238,31 @@ bisect_replay () {\n>  bisect_run () {\n>  \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n>  \n> +\tSUCCESS_TERM=$TERM_GOOD\n> +\tFAIL_TERM=$TERM_BAD\n> +\tINVERT_SET=\n> +\twhile [ \"$1\" != \"${1#-}\" ]; do\n\nMight be simpler to do 'while true', and in the *) case to break.\n\n> +\t\tcase \"$1\" in\n> +\t\t--)\n> +\t\t\tshift\n> +\t\t\tbreak ;;\n> +\t\t--expect=$TERM_GOOD)\n> +\t\t\t[ -z \"$INVERT_SET\" ] || die \"$(gettext \"bisect run: multiple expect options specified\")\"\n> +\t\t\tINVERT_SET=1 ;;\n> +\t\t-r|--invert|--expect=$TERM_BAD)\n> +\t\t\t[ -z \"$INVERT_SET\" ] || die \"$(gettext \"bisect run: multiple expect options specified\")\"\n> +\t\t\tSUCCESS_TERM=$TERM_BAD\n> +\t\t\tFAIL_TERM=$TERM_GOOD\n> +\t\t\tINVERT_SET=1 ;;\n> +\t\t--expect=*)\n> +\t\t\t# how to localize part 2?\n> +\t\t\tdie \"$(printf \"$(gettext \"bisect run: invalid --expect value, use --expect=%s or --expect=%s\")\" \"$TERM_GOOD\" \"$TERM_BAD\")\" ;;\n\nIt's more idiomatic to use eval_gettext here.  See\n\"git grep -e die -- '*.sh'\" for some examples.\n\n> +\t\t*)\n> +\t\t\tdie \"$(printf \"$(gettext \"bisect run: invalid option: %s\")\" \"$1\")\" ;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\n>  \ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n>  \n>  \twhile true\n> @@ -262,11 +287,13 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n>  \t\t\tstate='skip'\n>  \t\telif [ $res -gt 0 ]\n>  \t\tthen\n> -\t\t\tstate=\"$TERM_BAD\"\n> +\t\t\tstate=\"$FAIL_TERM\"\n>  \t\telse\n> -\t\t\tstate=\"$TERM_GOOD\"\n> +\t\t\tstate=\"$SUCCESS_TERM\"\n>  \t\tfi\n>  \n> +\t\techo \"exit code $res means this commit is $state\"\n> +\n>  \t\t# We have to use a subshell because \"bisect_state\" can exit.\n>  \t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n>  \t\tres=$?\n> diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh\n> new file mode 100755\n> index 0000000000..2708e0f854\n> --- /dev/null\n> +++ b/t/t6071-bisect-run.sh\n> @@ -0,0 +1,142 @@\n> +# verify that unrecognized options are rejected by 'git bisect run'\n> +#!/bin/sh\n> +\n> +# the linter's not smart enough to handle set -e\n> +GIT_TEST_CHAIN_LINT=0\n> +\n> +test_description='Tests git bisect run'\n> +\n> +exec </dev/null\n> +\n> +. ./test-lib.sh\n> +\n> +{ test_expect_success 'Setting up repo for \"git bisect run\" tests.' \"$(cat)\" ; } <<'SETUP'\n\nTests put the commands to be passed to test_expect_success in a\nsingle-quoted argument, with explicit &&-chaining.  (\"set -e\" has\nenough exceptions that it hasn't been worth using for this.  The test\nharness is able to detect a missing \"&&\" so this is less error-prone\nthan it sounds.)  See t/README and any recently added test scripts in\nt/ (try \"git log -p --diff-filter=A -- t\") for more details.\n\nThanks for a clear and pleasant description of the problem being\nsolved.  I would like to say \"here's a simple shell construct to use\ninstead of ! that makes this all redundant\" but lacking that, for what\nit's worth this set of new options seems like a good idea to me. :)\n\nSincerely,\nJonathan\n\n(the rest left unsnipped for reference)\n> +(\n> +# I don't know how they managed it, but the git test engine\n> +# somehow ignores the effect of 'set -e'.\n> +set -eu || exit 1\n> +# set -e canary\n> +false\n> +# hopefully, next year, we get -o pipefail!\n> +echo '.DEFAULT: dummy\n> +.PHONY: dummy\n> +dummy:\n> +\ttrue\n> +' > Makefile\n> +make\n> +echo '0' >path0\n> +git update-index --add -- Makefile path0\n> +git commit -q -m 'initial commit'\n> +git tag working0\n> +# make some commits that don't cause problems\n> +for x in `test_seq 1 20`; do\n> +\techo \"$x\" >path0\n> +\tgit update-index --replace -- path0\n> +\tgit commit -q -m \"working commit $x\"\n> +\tgit tag \"working$x\"\n> +done\n> +# break the makefile\n> +sed -i.bak -e 's/true/false/' Makefile\n> +rm -f Makefile.bak\n> +! make\n> +git update-index --replace -- Makefile\n> +git commit -q -m \"First broken commit\"\n> +git tag broken0\n> +# make some more commits that still FTBFS\n> +echo \"exit code was $?; flags are $-\"\n> +for x in `test_seq 1 20`; do\n> +\techo \"$x\" >path0\n> +\tgit update-index --replace -- path0\n> +\tgit commit -q -m \"broken build $x\"\n> +\tgit tag \"broken$x\"\n> +done\n> +# repair it\n> +git checkout working0 -- Makefile\n> +make\n> +git update-index --replace -- Makefile\n> +git commit -q -m \"First repaired commit\"\n> +git tag fixed0\n> +# make some more commits with the bugfix\n> +for x in `test_seq 1 20`; do\n> +\techo \"$x\" >path0\n> +\tgit update-index --replace -- path0\n> +\tgit commit -q -m \"fixed build $x\"\n> +\tgit tag \"fixed$x\"\n> +done\n> +#sh -c 'bash -i <> /dev/tty >&0 2>&1'\n> +)\n> +\n> +SETUP\n> +\n> +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'\n> +\n> +test_expect_failure() {\n> +\tshift\n> +\t#echo arguments are \"$*\"\n> +\ttest_must_fail \"$@\"\n> +}\n> +\n> +# okay, let's do some negative testing\n> +\n> +OLDPATH=\"$PATH\"\n> +\n> +PATH=\"$PATH:.\"\n> +\n> +test_expect_success 'setup this-is-not-a-valid-option' '\n> + echo \"#/bin/sh\" > --this-is-not-a-valid-option &&\n> + chmod a+x -- --this-is-not-a-valid-option &&\n> + --this-is-not-a-valid-option'\n> +\n> +test_expect_failure 'git bisect run: reject unrecognized options' git bisect run --this-is-not-a-valid-option\n> +\n> +PATH=\"$OLDPATH\"\n> +\n> +test_expect_failure 'git bisect run: reject invalid values for --expect'  git bisect run --expect=invalid make\n> +\n> +# okay, all of these settings are mutually exclusive (for sanity's sake, even with themselves)\n> +for a in --expect=bad --expect=good -r --invert; do\n> +\tfor b in --expect=bad --expect=good -r --invert; do\n> +\t\ttest_expect_failure 'git bisect run: reject multiple --expect options'  git bisect run $a $b make\n> +\tdone\n> +done\n> +\n> +# finally, verify that '--' is honored (note that will mess things up and require a bisect reset)\n> +PATH=\"$PATH:.\"\n> +\n> +test_expect_success 'git bisect run: honor --' 'git bisect run -- --this-is-not-a-valid-option'\n> +\n> +PATH=\"$OLDPATH\"\n> +\n> +for expect_syntax in '' --expect=good; do\n> +\n> +\t# now we have to undo the bisect run\n> +\ttest_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9'\n> +\n> +\ttest_expect_success \"running bisection ($expect_syntax)\" \"\n> +git bisect run $expect_syntax make &&\n> +git log --oneline &&\n> +\t# we should have determined that broken0 is the first bad version\n> +\ttest_cmp_rev broken0 refs/bisect/bad &&\n> +\t# and that version should be the one checked out\n> +\ttest_cmp_rev broken0 HEAD\n> +\"\n> +done\n> +\n> +\n> +# NOW, test the reverse:  find when we fixed it again\n> +\n> +for expect_syntax in -r --invert --expect=fixed; do\n> +\n> +\ttest_expect_success 'restarting bisection' 'git bisect reset && git bisect start --term-old=broken --term-new=fixed && git bisect broken broken20 && git bisect fixed fixed20'\n> +\ttest_expect_success \"running bisection ($expect_syntax)\" \"\n> +\t\tgit bisect run $expect_syntax make &&\n> +\t\tgit log --oneline &&\n> +\ttest_cmp_rev fixed0 refs/bisect/fixed &&\n> +\ttest_cmp_rev fixed0 HEAD\n> +\t\"\n> +done\n> +\n> +test_expect_failure 'sanity check error message with custom terms' git bisect run --expect=invalid make\n> +\n> +\n> +test_done\n> -- \n> 2.20.1\n> \n"},{"id":"389209","messageId":"xmqqftgvdhpz.fsf@gitster-ct.c.googlers.com","threadId":"52555","inReplyTo":"20200103043027.4537-1-stevie@qrpff.net","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-04T03:27:04Z","receivedAt":"2020-01-04T03:27:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Oberholtzer <stevie@qrpff.net> writes:\n\n> If your automated test naturally yields zero for _old_/_good_,\n> 1-124 or 126-127 for _new_/_bad_, then you're set.\n>\n> If that logic is reversed, however, it's a bit more of a pain: you\n> can't just stick a `sh -c !` in front of your command, because that\n> doesn't account for exit codes 125 or 129-255.\n\nHmm.\n\nNo off-the-shelf tool I know of exits 125 to signal \"I dunno\", so if\nyou have to care about the special status 125, the \"command\" you are\ndriving with \"git bisect run\" must be something that was written\nspecifically to match what \"git bisect\" expects by knowing that 125\nis a special code to declare that the commit's goodness cannot be\ndetermined.  Now, what's the reason why this \"command\" written\nspecifically to be used with \"git bisect\", which even knows the\nspecial 125 convention, yields \"this is good\" in the wrong polarity?\n\nThe only realistic reason I can think of is when the user is hunting\nfor a commit that fixed what has long been broken.  In such a use\ncase, commits in the older part of the history would not pass the\ntest (i.e. the exit status of the script would be non-zero) while\ncommits in the newer part of the history would.  \n\n> -git bisect run <cmd>...\n> +git bisect run [--expect=<term> | -r | --invert] [--] <cmd>...\n>  \tuse <cmd>... to automatically bisect.\n\nI'd suggest dropping \"-r\", which has little connection to \"--invert\".\n\n> @@ -238,6 +238,31 @@ bisect_replay () {\n>  bisect_run () {\n>  \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n>  \n> +\tSUCCESS_TERM=$TERM_GOOD\n> +\tFAIL_TERM=$TERM_BAD\n> +\tINVERT_SET=\n> +\twhile [ \"$1\" != \"${1#-}\" ]; do\n\nLet's not make the style violations even worse.  Ideally, a\npreliminary clean-up patch to fix the existing ones before the main\npatch would be a good idea (cf. Documentation/CodingGuidelines).\n\nIt is more efficient and conventional (hence easier to read) to\nreuse the single \"case\" you would need to write anyway for the loop\ncontrol, i.e.\n\n\twhile :\n\tdo\n\t\tcase \"$1\" in\n                --) ... ;;\n                --invert | ... ) ... ;;\n                -*) die \"unknown command ;;\n\t\t*) break ;;\n\t\tesac\n\tdone\n\n> +\t\t--expect=*)\n> +\t\t\t# how to localize part 2?\n\nUsing things like \"%2$s\", you mean?\n\nAs I alluded earlier, it is unclear how this new feature should\ninteract with the \"we use 'xyzzy' to mean 'good', and 'frotz' to\nmean 'bad'\" feature.  One part of me would want to say that when\nrunning bisect with good and bad swapped, we should reverse the\npolarity of \"bisect run\" script automatically, but the rest of me\nof course would say that it would not necessarily be a good idea,\nand it is of course a huge backward incompatible change, so anything\nautomatic is a no go.\n\n> @@ -262,11 +287,13 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n>  \t\t\tstate='skip'\n>  \t\telif [ $res -gt 0 ]\n>  \t\tthen\n> -\t\t\tstate=\"$TERM_BAD\"\n> +\t\t\tstate=\"$FAIL_TERM\"\n>  \t\telse\n> -\t\t\tstate=\"$TERM_GOOD\"\n> +\t\t\tstate=\"$SUCCESS_TERM\"\n>  \t\tfi\n>  \n> +\t\techo \"exit code $res means this commit is $state\"\n\nIs this a leftover debugging output?\n\nIn any case, I wonder why something along the line of the attached\npatch is not sufficient and it needs this much code.\n\n git-bisect.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex efee12b8b1..7fc4f9bd8f 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -247,6 +247,15 @@ bisect_run () {\n \t\t\"$@\"\n \t\tres=$?\n \n+\t\tif test -n \"$invert_run_status\"\n+\t\tthen\n+\t\t\tcase \"$res\" in\n+\t\t\t0)\tres=1 ;;\n+\t\t\t125)\t;;\n+\t\t\t*)\tres=0 ;;\n+\t\t\tesac\n+\t\tfi\n+\n \t\t# Check for really bad run error.\n \t\tif [ $res -lt 0 -o $res -ge 128 ]\n \t\tthen\n"},{"id":"389215","messageId":"CAD_xR9ch4M3i0tPgR1uOvkC=xpXo2nPHA71A1nwmZqtb_YCMsw@mail.gmail.com","threadId":"52555","inReplyTo":"20200104012230.GD130883@google.com","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Stephen Oberholtzer","fromEmail":"stevie@qrpff.net","sentAt":"2020-01-04T05:37:02Z","receivedAt":"2020-01-04T05:44:15Z","isPatch":true,"sender":{"key":"stevie@qrpff.net","avatar":null},"body":"Jonathan,\n\nThanks for your feedback!\n\nOn Fri, Jan 3, 2020 at 8:22 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> (+cc: Christian Couder, bisect expert)\n(noted)\n\n> >\n> > * '--' can be used as an option list terminator,\n> >   just as everywhere else.\n>\n> Could this part go in a separate commit?  That way, it can go in\n> more quickly while we figure out the rest. :)\n\nDone!  That one was simple enough I didn't tag it RFC.\n\n>\n> Initial thoughts:\n>\n> - it's not immediately clear to me that this is common enough\n>   to need the short-and-sweet name \"-r\".  Could it have a long\n>   form only?\n\nOf course; I was just trying to be thorough.\n\n> - I think I agree with you that \"git bisect run --expect=5\" might\n>   be more clearly written as \"git bisect run --success=5\".  Or even\n>   something explicitly referring to exit status, like\n>   --success-status=5.\n\nThat's not quite what I had here (although it touches on another idea\nI've been mulling over.)\nMy patch adds \"--success=<term>\", meaning\n\ngit bisect run --success=foo ./myscript.sh\n\ndoes: run myscript.sh, and if the script succeeds -- that is, exits\nwith status 0 -- then do 'git bisect foo'; otherwise, do whatever the\nother one is.\n\nWIthout any specialized options, the behavior today is equivalent to\n\"--success=good\" (where status 0 means 'git bisect good', otherwise\n'git bisect bad'.)\n\n>\n> - This has an interesting relationship to the \"alternate terms\"\n>   feature (--term-old / --term-new).  I don't know if there's a\n>   good way to make that more explicit --- maybe just some notes\n>   with examples in the relevant parts of the manpage?\n\nI actually had this detail spelled out in the blurb I had been writing\nin Documentation/git-bisect.txt:\n\n+The \"good/bad\" interpretation can be reversed by specifying -r,\n+--invert, or --success=bad (or, if the bisect was started with\n+--term-bad specified, the appropriate string.) In such circumstances,\n+an exit code of 0 is taken to mean 'bad', while an exit code of 1-124\n+or 126-127 is treated as 'good'.  The meaning of all other exit codes\n+is unaffected by this option.\n\nThat's why my initial version called it \"expect\": my thinking was that\nI was \"expecting\" the script to say that the version was bad, rather\nthan that it was good.\n\n> - the name --invert doesn't make it obvious to me what it is\n>   inverting: good versus bad, ones complement of the status\n>   code, revision range passed to \"git bisect start\"?\n>\n>   Are there other commands we can try to make this consistent with?\n>   \"find\" supports arbitrary expressions involving '!' and '-not'.\n>   \"git grep\" has --invert-match: perhaps a name --invert-status\n>   would be clear enough?\n\n\"--invert-status\" would work for me (or \"--negate-status\", perhaps?)\n\n>\n> > You're not allowed to specify more than one expectation.\n>\n> Usual convention would be \"last specified option wins\".\n\nThat's fine, I can make such a thing work.\n\n> As you said, this needs docs.  Writing docs often helps make the UI a\n> bit better since it forces one to think about the various ways a tool\n> would be used in practice.\n\nI'd started writing them even before you replied; I didn't want to\nwrite *too* much and have to do a lot of replacing on option names.\n\n> > +     while [ \"$1\" != \"${1#-}\" ]; do\n>\n> Might be simpler to do 'while true', and in the *) case to break.\n\nI went with `test -n \"$1\"` after Junio pointed me at the coding guidelines :)\n\n> It's more idiomatic to use eval_gettext here.  See\n> \"git grep -e die -- '*.sh'\" for some examples.\n\nYeah, I hadn't yet learned about about eval_gettext before sending the\npatch.  Once I saw it referenced in other code, it was easy to make\nhappen.\n\n> \"set -e\" has\n> enough exceptions that it hasn't been worth using for this.\n\nExceptions? Like what? As my comment pointed out, I don't know how the\ntest suite managed to make 'set -e' _not_ work (although I can see\nthat it has, in fact, managed this.) In fact, I suspect the fact that\nit managed it is actually a bug in dash.\n\n> The test\n> harness is able to detect a missing \"&&\" so this is less error-prone\n> than it sounds.)\n\n'set -e' should be functionally equivalent to using '&&' between all\npipelines, except it also handles loops.\n(also, all of those &&s are awkward to write.)\n\n> Thanks for a clear and pleasant description of the problem being\n> solved.  I would like to say \"here's a simple shell construct to use\n> instead of ! that makes this all redundant\" but lacking that, for what\n> it's worth this set of new options seems like a good idea to me. :)\n\nWell, yeah, if there was a simple shell construct to use instead of !,\nI wouldn't have bothered to code this up in the first place :)\n\nHere's what got me started on this stuff in the first place:\n\nI was trying to bisect an kernel issue I was having. However, I had\ntwo problems: (a) the affected machine is not powerful enough to build\na kernel in less than a day, and (b) my most reliable way to detect\nthe issue had to be initiated from an external system.\n\nI wrote a script that would: build the kernel, install it onto\naffected machine, boot new kernel on affected machine, and run some\ntests to see if the problem occurs.\n\nThere were four possible outcomes from each attempt:\n\n1. Kernel fails to build or boot: correct action is 'git bisect skip'\n(this was mapped to exit code 125)\n    (note that, since there was no way to automatically determine if\nthe kernel booted correctly, that situation actually manifested as\ncase 4)\n2. Problem is not present: correct action is 'git bisect good' (mapped\nto exit code 0)\n3. Problem is present: correct action is 'git bisect bad' (mapped to\nexit code 99)\n    (also: need to reboot the machine back to a working kernel.)\n4. Something unexpected went wrong: correct action is to hit the\nemergency stop and let me fix it manually (mapped to exit code 255)\n\nSituation #4 probably happened about five or six times, in all. (One\nmemorable instance was during the 16th or 17th revision, when the\ntarget machine ran out of disk space while copying the new kernel\nover.  A couple of times, ssh had an error trying to connect.  And\nseveral times, for reasons beyond my understanding, the 255 failsafe\nkicked in on exit code 99.)\n\nI had to map a _lot_ of failure cases to 255. In essence, I did this:\n(That's why I used 99 to indicate failure; it was unlikely to produce\na false positive.)  I used a bunch of ultimately-not-very-reliable\ntechniques to try and map all exit codes other than 0, 99, or 125 to\n255.  I'm thinking of adding a \"--fail-status=\" option, which would\nlet you specify the explicit status code or codes to treat as 'bad',\nwith all other nonzero codes being hard errors and causing 'run' to\nbail immediately.  Such a thing would have made several aspects of my\nscript far less error-prone.\n\nIt took most of a day (much better than most of a month) but I found\nthe offending commit.\n\nLater, I noticed that, in one of the recent -rc builds, the problem no\nlonger seemed to manifest.  But then the question became: was the\nproblem truly fixed, or was the problem being masked otherwise?\nThat's when I discovered that properly adjusting the exit status from\nmy script was... complicated: I needed to reverse the behavior for\ncases 2 and 3 above, but leave the behavior for cases 1 and 4\nunchanged. And I figured that, if I had this problem, someone else\nsurely did as well.\n\n-- \n-- Stevie-O\nReal programmers use COPY CON PROGRAM.EXE\n"},{"id":"389216","messageId":"CAD_xR9fUxDTvwmAsfH-6=buRP+UmwBHhQJSV+T3paUOy-S1CGw@mail.gmail.com","threadId":"52555","inReplyTo":"xmqqftgvdhpz.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Stephen Oberholtzer","fromEmail":"stevie@qrpff.net","sentAt":"2020-01-04T06:22:21Z","receivedAt":"2020-01-04T06:22:44Z","isPatch":true,"sender":{"key":"stevie@qrpff.net","avatar":null},"body":"Junio,\n\nThank you for your feedback! Several of your concerns were already\nraised by Jonathan Nieder; I'm going to focus on the specifics you\nbrought up that Jonathan didn't, to avoid repeating myself too much.\n\nOn Fri, Jan 3, 2020 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Hmm.\n>\n> No off-the-shelf tool I know of exits 125 to signal \"I dunno\", so if\n> you have to care about the special status 125, the \"command\" you are\n> driving with \"git bisect run\" must be something that was written\n> specifically to match what \"git bisect\" expects by knowing that 125\n> is a special code to declare that the commit's goodness cannot be\n> determined.  Now, what's the reason why this \"command\" written\n> specifically to be used with \"git bisect\", which even knows the\n> special 125 convention, yields \"this is good\" in the wrong polarity?\n>\n> The only realistic reason I can think of is when the user is hunting\n> for a commit that fixed what has long been broken.  In such a use\n> case, commits in the older part of the history would not pass the\n> test (i.e. the exit status of the script would be non-zero) while\n> commits in the newer part of the history would.\n\nThat's nearly exactly what happened: after bisecting the problem to\nfind when it was introduced, I found that an updated version did not\nfail the test.  I wanted to bisect the \"fix\" in order to review the\nchange and verify that it did, in fact, fix the problem -- as opposed\nto change things so my test didn't fail (as it turned out, it was the\nlatter: rather than fix the issue, the \"fix\" commit simply turned the\nbugged feature off by default.)\n\n> I'd suggest dropping \"-r\", which has little connection to \"--invert\".\n\nI was simply trying to be thorough, so if it doesn't need a short\nname, that's fine by me.  (And it's probably going to be\n--invert-status.)\n\n> Let's not make the style violations even worse.\n\nJonathan pointed that one out; you can see my corrected version in my\n(at his suggestion) split-out version that simply adds support for\n'run -- cmd'\n\n> > +                     # how to localize part 2?\n>\n> Using things like \"%2$s\", you mean?\n\nHah, yeah, that was a leftover comment from before I came up with the\n'$(printf $(gettext))' trick.  (Which is now eval_gettext.)\n\n> As I alluded earlier, it is unclear how this new feature should\n> interact with the \"we use 'xyzzy' to mean 'good', and 'frotz' to\n> mean 'bad'\" feature. One part of me would want to say that when\n> running bisect with good and bad swapped, we should reverse the\n> polarity of \"bisect run\" script automatically, but the rest of me\n> of course would say that it would not necessarily be a good idea,\n> and it is of course a huge backward incompatible change, so anything\n> automatic is a no go.\n\nThis new feature works just fine with that (in fact, the last round of\ntests in the test script explicitly covers this interaction.)\nWith \"--invert-status\" it doesn't matter what your names are, and with\n\"--success=<term>\" it honors whatever terms you specified.\n\n> > +             echo \"exit code $res means this commit is $state\"\n>\n> Is this a leftover debugging output?\n\nYep, missed that one during my cursory review before uploading (since\ntest scripts hide their output by default, I didn't realize I'd left\nit in there.)\n\n> In any case, I wonder why something along the line of the attached\n> patch is not sufficient and it needs this much code.\n>\n>  git-bisect.sh | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index efee12b8b1..7fc4f9bd8f 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -247,6 +247,15 @@ bisect_run () {\n>                 \"$@\"\n>                 res=$?\n>\n> +               if test -n \"$invert_run_status\"\n> +               then\n> +                       case \"$res\" in\n> +                       0)      res=1 ;;\n> +                       125)    ;;\n> +                       *)      res=0 ;;\n> +                       esac\n> +               fi\n> +\n\nUnfortunately, that doesn't behave properly.\nAs far as 'git bisect run' is concerned, there are four distinct sets\nof status codes:\n\n1. Test passed (translated to 'git bisect good') -- status 0\n2. Test failed (translated to 'git bisect bad') -- status 1-124 or 126-127\n3. Untestable (translated to 'git bisect skip') -- status 125\n4. Malfunction (causes 'git bisect run' to halt immediately, leaving\nthe bisection incomplete) -- anything else\n\nWhat needs to happen is that status code lists for \"test passed\" (#1)\nand \"test failed\" (#2) are swapped; even when bisecting a fix, #3\n(untestable) and #4 (malfunction) remain unchanged.  Your patch remaps\ncase #4 to case #1.\n\n(I'm actually going to put together a patch to allow the user to pare\ndown the exit code list for #2 to a specific list, to make 'run' less\ndicey in the face of complex test requirements.)\n\n-- \n-- Stevie-O\nReal programmers use COPY CON PROGRAM.EXE\n"},{"id":"389258","messageId":"xmqqy2ul9yg0.fsf@gitster-ct.c.googlers.com","threadId":"52555","inReplyTo":"CAD_xR9fUxDTvwmAsfH-6=buRP+UmwBHhQJSV+T3paUOy-S1CGw@mail.gmail.com","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-06T01:16:15Z","receivedAt":"2020-01-06T01:16:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Oberholtzer <stevie@qrpff.net> writes:\n\n>> In any case, I wonder why something along the line of the attached\n>> patch is not sufficient and it needs this much code.\n>> ...\n> Unfortunately, that doesn't behave properly.\n> As far as 'git bisect run' is concerned, there are four distinct sets\n> ...\n> What needs to happen is that status code lists for \"test passed\" (#1)\n> and \"test failed\" (#2) are swapped; even when bisecting a fix, #3\n> (untestable) and #4 (malfunction) remain unchanged.  Your patch remaps\n> case #4 to case #1.\n\nYeah, I know.  I didn't mean to give you a perfect solution and that\nwas why I said \"along the line of...\".  I know I ignored the 128 and\nabove, as I usually trust that our contributors are competent enough\nto be able to fill in the missing details given an outline.\n\nThe key takeaway I wanted you to notice was that a single case\nstatement that maps the exit code external command would give us\nwould look sufficient, without any of the {SUCCESS,FAIL}_TERM magic\nyou had in your version, which indicates that there is more than the\nsimple \"using a run script to find where a bug was fixed can be done\nby swapping exit code\" going on.  And it is quite unclear why that\nis needed either from the patch or the text that accompanied the\npatch.\n\nThanks.\n\n\n\n\n"},{"id":"389306","messageId":"CAD_xR9dGNKdVNzFgFUaZCgJetpW5tXxb8wERovdjbc=1jS-KxA@mail.gmail.com","threadId":"52555","inReplyTo":"xmqqy2ul9yg0.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Stephen Oberholtzer","fromEmail":"stevie@qrpff.net","sentAt":"2020-01-07T00:10:00Z","receivedAt":"2020-01-07T00:10:23Z","isPatch":true,"sender":{"key":"stevie@qrpff.net","avatar":null},"body":"Junio,\n\nOn Sun, Jan 5, 2020 at 8:16 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Yeah, I know.  I didn't mean to give you a perfect solution and that\n> was why I said \"along the line of...\".  I know I ignored the 128 and\n> above, as I usually trust that our contributors are competent enough\n> to be able to fill in the missing details given an outline.\n>\n> The key takeaway I wanted you to notice was that a single case\n> statement that maps the exit code external command would give us\n> would look sufficient, without any of the {SUCCESS,FAIL}_TERM magic\n> you had in your version, which indicates that there is more than the\n> simple \"using a run script to find where a bug was fixed can be done\n> by swapping exit code\" going on.  And it is quite unclear why that\n> is needed either from the patch or the text that accompanied the\n> patch.\n\n\nIn this particular instance at least, I'm not competent enough to come\nup with a clean, portable way to create a single case statement that\nhandles the final condition.\nThe issue I'm having is that case-esac blocks do string matching, not\ninteger value matching, so I don't know how to replicate the current\nbehavior without a case pattern that looks like\n[12]|1[3-9]|1[01][0-9]|12[0-467]|1[3-9]|2[0-9]|[3-9].\n\n\n\n-- \n-- Stevie-O\nReal programmers use COPY CON PROGRAM.EXE\n"},{"id":"389311","messageId":"xmqqpnfw6nz1.fsf@gitster-ct.c.googlers.com","threadId":"52555","inReplyTo":"CAD_xR9dGNKdVNzFgFUaZCgJetpW5tXxb8wERovdjbc=1jS-KxA@mail.gmail.com","subject":"Re: [RFC PATCH] bisect run: allow inverting meaning of exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T01:42:58Z","receivedAt":"2020-01-07T01:43:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Oberholtzer <stevie@qrpff.net> writes:\n\n> ... a clean, portable way to create a single case statement that\n> handles the final condition.\n\nHmm, don't we want to say (1) original 0 (success) maps to 1 (2)\noriginal 125 maps to 125 (3) anything below 128 in the original maps\nto 0 and (4) everything else is left intact?  Perhaps something as\nsimple like this, taking advantage of the fact that $? won't have\nanything other than digits?\n\n    ... do something ...\n    status=$?\n\n    case $status in\n    0) status=1 ;;\n    125) : as-is ;;\n    ? | ?? | 1[01]? | 12[0-7]) status=0;;\n    *) : as-is ;;\n    esac\n\nI am still unclear where the need for {SUCCESS_FAIL}_TERM magic\ncomes from, though.\n"}]}