{"thread":{"id":"52562","subject":"[PATCH] bisect run: allow '--' as an options terminator","startedAt":"2020-01-04T03:46:07Z","lastAt":"2020-01-07T01:35:02Z","messageCount":4,"participants":["Stephen Oberholtzer","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389211","messageId":"20200104034551.23658-1-stevie@qrpff.net","threadId":"52562","inReplyTo":null,"subject":"[PATCH] bisect run: allow '--' as an options terminator","fromName":"Stephen Oberholtzer","fromEmail":"stevie@qrpff.net","sentAt":"2020-01-04T03:45:51Z","receivedAt":"2020-01-04T03:46:07Z","isPatch":true,"sender":{"key":"stevie@qrpff.net","avatar":null},"body":"The 'bisect run' feature doesn't currently accept any options, but\nit could, and that means a '--' option.\n\nAdditionally, this adds an actual test script for bisect run - it\ncreates a repository with a failed build, then attempts to bisect\nthe offending commit.\n\nSigned-off-by: Stephen Oberholtzer \n---\n Documentation/git-bisect.txt | 2 +-\n git-bisect.sh | 19 ++++++-\n t/t6071-bisect-run.sh | 96 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 115 insertions(+), 2 deletions(-)\n create mode 100755 t/t6071-bisect-run.sh\n\ndiff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\nindex 7586c5a843..e72353e157 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 \n git bisect log\n- git bisect run ...\n+ git bisect run [--] ...\n git bisect help\n \n This command uses a binary search algorithm to find which commit in\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex efee12b8b1..46085651e1 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -26,7 +26,7 @@ git bisect replay \n \treplay bisection log.\n git bisect log\n \tshow bisect log.\n-git bisect run ...\n+git bisect run [--] ...\n \tuse ... to automatically bisect.\n \n Please use \"git help bisect\" to get the full man page.'\n@@ -238,6 +238,23 @@ bisect_replay () {\n bisect_run () {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n+\t# check for options\n+\t# (there aren't any supported yet, unless you count \"--\")\n+\twhile test -n \"$1\"\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t--)\n+\t\t\tshift\n+\t\t\tbreak ;;\n+\t\t-*)\n+\t\t\toption=\"$1\" # \\$1 is not expanded by eval_gettext\n+\t\t\tdie \"$(eval_gettext \"bisect run: invalid option: \\$option\")\" ;;\n+\t\t*)\n+\t\t\tbreak ;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\n \ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n \n \twhile true\ndiff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh\nnew file mode 100755\nindex 0000000000..e173ca18b3\n--- /dev/null\n+++ b/t/t6071-bisect-run.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='Tests git bisect run'\n+\n+exec 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\" || exit 1\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+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\" || exit 1\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\" || exit 1\n+done\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+\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+# 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+# now we have to undo the bisect run\n+test_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9'\n+\n+test_expect_success \"running bisection\" \"\n+\tgit bisect run $success_option make &&\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+\n+test_done\n-- \n2.20.1\n\n"},{"id":"389213","messageId":"xmqq1rsfddkq.fsf@gitster-ct.c.googlers.com","threadId":"52562","inReplyTo":"20200104034551.23658-1-stevie@qrpff.net","subject":"Re: [PATCH] bisect run: allow '--' as an options terminator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-04T04:56:37Z","receivedAt":"2020-01-04T04:56:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Oberholtzer writes:\n\n> The 'bisect run' feature doesn't currently accept any options, but\n> it could, and that means a '--' option.\n\nSorry, but the above description does not make much sense to me. I\ndo agree up to the \"but it could\" part with the above sentence, but\ndouble-dash is hardly a logical consequence of it, at least to me.\n\n> - git bisect run ...\n> + git bisect run [--] ...\n\nThe only reason why you might want '--' disambiguator is if you have\na that happens to begin with a dash (i.e. making it possible\nto be confused as an unrecognised option), but I do not think it is\nunreasonable at all if we declare that you cannot feed a cmd that\nbegins with a dash. On a rare occasion like that, the user could\neven do the \"prefix with \"sh -c'\" you alluded to in the other thread\nto escape/quote the leading dash, if the user really wanted to use\nsuch a strange command.\n\n> Additionally, this adds an actual test script for bisect run - it\n> creates a repository with a failed build, then attempts to bisect\n> the offending commit.\n\nI do not think I have seen enough to justify addition of \"--\", but\naddition of tests by itself may have a value, and I'd prefer to see\nit as its own patch. But I see hits for\n\n\tgit grep 'git bisect run' t/\n\nalready in t6030, so \"adds an actual test script\", while it is not\ntelling a lie, may be giving a false impression that it is adding\nsomething new that has been missing.\n\nSo, I dunno.\n\n> diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh\n> new file mode 100755\n> index 0000000000..e173ca18b3\n> --- /dev/null\n> +++ b/t/t6071-bisect-run.sh\n> @@ -0,0 +1,96 @@\n> +#!/bin/sh\n> +\n> +test_description='Tests git bisect run'\n> +\n> +exec +. ./test-lib.sh\n> +\n> +{ test_expect_success 'Setting up repo for \"git bisect run\" tests.' \"$(cat)\" ; } <<'SETUP'\n\nYuck. I have to say this is trying to be too clever and cute than\nits worth. Doesn't it defeat test-lint, for example?\n\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\nMore than one Documentation/CodingGuidelines violation on a single\nline X-<.\n\n> +\techo \"$x\" >path0 &&\n> +\tgit update-index --replace -- path0 &&\n> +\tgit commit -q -m \"working commit $x\" &&\n> +\tgit tag \"working$x\" || exit 1\n> +done &&\n> +# break the makefile\n> +sed -i.bak -e 's/true/false/' Makefile &&\n\n\"sed -i\" is unportable, isn't it? Avoid it.\n\n> ...\n> +SETUP\n> +\n> +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'\n\nLay it out like so for readability:\n\n\ttest_expect_success \"setup first bisect\" '\n\t\tgit bisect start &&\n\t\t...\n\t'\n\nIn general, read and mimic existing tests that have been cleaned up\nrecently (\"git log t/\" is your friend).\n\n> +\n> +test_expect_failure() {\n\nDo not override what we provide in test-lib and test-lib-functions.\nThose who are familiar with the test framework depend on these names\nto work the way they are used to.\n\nLearn what are given by the test framework to you already before\ntrying to invent your own convention---that would hurt everybody,\nincluding the current set of reviewers and future developers who\nneed to fix what you wrote.\n\n> +\tshift\n> +\ttest_must_fail \"$@\"\n> +}\n\nThe remainder of the file not reviewed (yet).\n\nThanks.\n"},{"id":"389300","messageId":"CAD_xR9faremhsGP4G65vHj=cvskUbK1rpPrQisa2_4AD2AGNYw@mail.gmail.com","threadId":"52562","inReplyTo":"xmqq1rsfddkq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] bisect run: allow '--' as an options terminator","fromName":"Stephen Oberholtzer","fromEmail":"stevie@qrpff.net","sentAt":"2020-01-06T23:29:35Z","receivedAt":"2020-01-06T23:30:00Z","isPatch":true,"sender":{"key":"stevie@qrpff.net","avatar":null},"body":"Junio,\n\nSorry about that sloppy test script; that'll teach me to try cleaning\nup a patch late at night.\n\nOn Fri, Jan 3, 2020 at 11:56 PM Junio C Hamano wrote:\n\n\n> I do not think I have seen enough to justify addition of \"--\",\n\nThat's fine; I was just trying to be thorough (also, it was easy to\ntest.) I was taught: if you accept any -options, honor -- as well. If\nyou're not concerned about that, that's fine with me.\n\n> but\n> addition of tests by itself may have a value, and I'd prefer to see\n> it as its own patch. But I see hits for\n>\n> git grep 'git bisect run' t/\n>\n> already in t6030, so \"adds an actual test script\", while it is not\n> telling a lie, may be giving a false impression that it is adding\n> something new that has been missing.\n\nMmh, I didn't see those 'bisect run' tests in there; clearly, I didn't\nlook hard enough. I was distracted by several factors, including the\nfilename (t/t6030-bisect-porcelain.sh: I associated \"porcelain\" with\n\"output intended to be parsed\". Since run doesn't have any meaningful\nparse-able output, I did not expect to find it there.) and the fact\nthat bisect apparently has all of the test cases in one single script,\nwhen (for example) rev-list currently has 23 different test scripts.\n\n> > +exec \n> All of our test scripts are designed to be used unattended with the\n> use of test_expect_* harness helpers. Can you tell us why this new\n> file alone needs to dissociate the input to _all_ its subproesses by\n> doing this, while others do not have to?\n\nIronically, that line was copied straight out of the top of t6030.\n\n>\n> > +. ./test-lib.sh\n> > +\n> > +{ test_expect_success 'Setting up repo for \"git bisect run\" tests.' \"$(cat)\" ; } <<'SETUP'\n>\n> Yuck. I have to say this is trying to be too clever and cute than\n> its worth. Doesn't it defeat test-lint, for example?\n\nActually, it doesn't. test-lint is too devious :-o\n\nI was trying to adapt the script that I initially wrote to set up a\nrepo for manual testing. A heredoc was the easiest way to achieve\nthat without having to worry too much about escaping.\n\n\n\n> Thanks.\n\nNo, thank you, for putting up with me :)\n\nAnyway, it sounds like: (a) the feature my patch adds isn't wanted or\nneeded, and (b) the functionality I *thought* I was adding test\ncoverage for already has test coverage, so we can kill this particular\nthread entirely.\n\n\n--\n-- Stevie-O\nReal programmers use COPY CON PROGRAM.EXE\n"},{"id":"389309","messageId":"xmqqtv586oci.fsf@gitster-ct.c.googlers.com","threadId":"52562","inReplyTo":"CAD_xR9faremhsGP4G65vHj=cvskUbK1rpPrQisa2_4AD2AGNYw@mail.gmail.com","subject":"Re: [PATCH] bisect run: allow '--' as an options terminator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T01:34:53Z","receivedAt":"2020-01-07T01:35:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Oberholtzer writes:\n\n>> I do not think I have seen enough to justify addition of \"--\",\n>\n> That's fine; I was just trying to be thorough (also, it was easy to\n> test.) I was taught: if you accept any -options, honor -- as well. If\n> you're not concerned about that, that's fine with me.\n\nAhh, that indeed is the crucial piece of information that was\nmissing. My review was about \"Do we really *need* it?\", but if you\nare doing so from following a rule/dogma/principle, that changes the\nequation quite a bit.\n\nI do not think this project officially subscribes to the \"anywhere\n-option is taken should accept '--' as the end of options marker\"\nschool, but because most modern command line processors use\nparse_options() API which gets \"--\" for free, we can consider\nourselves practically accepting it already. \n\nAnd adopting such a rule/dogma/principle frees us from having to\nthink about each individual case and helps us being consistent, so\nit is not necessarily a bad thing as long as the cost of following\nthe rule/dogma/principle is not too onerous. At that point, what\nneeds to be reviewed instead becomes to \"does this new code follow\nthe rule, and is it not bending backwards to do so?\"\n\nSo, I do not have strong objection against \"bisect run -- \".\nIt was, as I said in the original review, that it was so unclear\nwhy double-dash is a logical consequence of accepting options,\nbecause it was left unsaid. It would have been an easy sell if\nthis were a part of a patch that actually adds new option(s) and\nexplained perhaps like so:\n\n\tTeach 'bisect run' to take --foo and --bar options, that\n\ttell the command to do Foo and Bar. While at it, teach it\n\tthe common convention that \"--\" is used to terminate the\n\tlist of options. Nobody sane may try to use that\n\tbegins with a dash, but supporting \"--\" everywhere we accept\n\ta dashed option is good for consistency.\n\nThanks.\n\n"}]}