| Message ID | 20200104034551.23658-1-stevie@qrpff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bisect run: allow '--' as an options terminator | expand |
Stephen Oberholtzer <stevie@qrpff.net> writes: > The 'bisect run' feature doesn't currently accept any options, but > it could, and that means a '--' option. Sorry, but the above description does not make much sense to me. I do agree up to the "but it could" part with the above sentence, but double-dash is hardly a logical consequence of it, at least to me. > - git bisect run <cmd>... > + git bisect run [--] <cmd>... The only reason why you might want '--' disambiguator is if you have a <cmd> that happens to begin with a dash (i.e. making it possible to be confused as an unrecognised option), but I do not think it is unreasonable at all if we declare that you cannot feed a cmd that begins with a dash. On a rare occasion like that, the user could even do the "prefix with "sh -c'" you alluded to in the other thread to escape/quote the leading dash, if the user really wanted to use such a strange command. > Additionally, this adds an actual test script for bisect run - it > creates a repository with a failed build, then attempts to bisect > the offending commit. I do not think I have seen enough to justify addition of "--", but addition of tests by itself may have a value, and I'd prefer to see it as its own patch. But I see hits for git grep 'git bisect run' t/ already in t6030, so "adds an actual test script", while it is not telling a lie, may be giving a false impression that it is adding something new that has been missing. So, I dunno. > diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh > new file mode 100755 > index 0000000000..e173ca18b3 > --- /dev/null > +++ b/t/t6071-bisect-run.sh > @@ -0,0 +1,96 @@ > +#!/bin/sh > + > +test_description='Tests git bisect run' > + > +exec </dev/null All of our test scripts are designed to be used unattended with the use of test_expect_* harness helpers. Can you tell us why this new file alone needs to dissociate the input to _all_ its subproesses by doing this, while others do not have to? > +. ./test-lib.sh > + > +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP' Yuck. I have to say this is trying to be too clever and cute than its worth. Doesn't it defeat test-lint, for example? > +echo '.DEFAULT: dummy > +.PHONY: dummy > +dummy: > + true > +' > Makefile && > +make && > +echo '0' >path0 && > +git update-index --add -- Makefile path0 && > +git commit -q -m 'initial commit' && > +git tag working0 && > +# make some commits that don't cause problems > +for x in `test_seq 1 20`; do More than one Documentation/CodingGuidelines violation on a single line X-<. > + echo "$x" >path0 && > + git update-index --replace -- path0 && > + git commit -q -m "working commit $x" && > + git tag "working$x" || exit 1 > +done && > +# break the makefile > +sed -i.bak -e 's/true/false/' Makefile && "sed -i" is unportable, isn't it? Avoid it. > ... > +SETUP > + > +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9' Lay it out like so for readability: test_expect_success "setup first bisect" ' git bisect start && ... ' In general, read and mimic existing tests that have been cleaned up recently ("git log t/" is your friend). > + > +test_expect_failure() { Do not override what we provide in test-lib and test-lib-functions. Those who are familiar with the test framework depend on these names to work the way they are used to. Learn what are given by the test framework to you already before trying to invent your own convention---that would hurt everybody, including the current set of reviewers and future developers who need to fix what you wrote. > + shift > + test_must_fail "$@" > +} The remainder of the file not reviewed (yet). Thanks.
Junio, Sorry about that sloppy test script; that'll teach me to try cleaning up a patch late at night. On Fri, Jan 3, 2020 at 11:56 PM Junio C Hamano <gitster@pobox.com> wrote: > I do not think I have seen enough to justify addition of "--", That's fine; I was just trying to be thorough (also, it was easy to test.) I was taught: if you accept any -options, honor -- as well. If you're not concerned about that, that's fine with me. > but > addition of tests by itself may have a value, and I'd prefer to see > it as its own patch. But I see hits for > > git grep 'git bisect run' t/ > > already in t6030, so "adds an actual test script", while it is not > telling a lie, may be giving a false impression that it is adding > something new that has been missing. Mmh, I didn't see those 'bisect run' tests in there; clearly, I didn't look hard enough. I was distracted by several factors, including the filename (t/t6030-bisect-porcelain.sh: I associated "porcelain" with "output intended to be parsed". Since run doesn't have any meaningful parse-able output, I did not expect to find it there.) and the fact that bisect apparently has all of the test cases in one single script, when (for example) rev-list currently has 23 different test scripts. > > +exec </dev/null > > All of our test scripts are designed to be used unattended with the > use of test_expect_* harness helpers. Can you tell us why this new > file alone needs to dissociate the input to _all_ its subproesses by > doing this, while others do not have to? Ironically, that line was copied straight out of the top of t6030. > > > +. ./test-lib.sh > > + > > +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP' > > Yuck. I have to say this is trying to be too clever and cute than > its worth. Doesn't it defeat test-lint, for example? Actually, it doesn't. test-lint is too devious :-o I was trying to adapt the script that I initially wrote to set up a repo for manual testing. A heredoc was the easiest way to achieve that without having to worry too much about escaping. <more advice on readability and portability snipped> > Thanks. No, thank you, for putting up with me :) Anyway, it sounds like: (a) the feature my patch adds isn't wanted or needed, and (b) the functionality I *thought* I was adding test coverage for already has test coverage, so we can kill this particular thread entirely. -- -- Stevie-O Real programmers use COPY CON PROGRAM.EXE
Stephen Oberholtzer <stevie@qrpff.net> writes: >> I do not think I have seen enough to justify addition of "--", > > That's fine; I was just trying to be thorough (also, it was easy to > test.) I was taught: if you accept any -options, honor -- as well. If > you're not concerned about that, that's fine with me. Ahh, that indeed is the crucial piece of information that was missing. My review was about "Do we really *need* it?", but if you are doing so from following a rule/dogma/principle, that changes the equation quite a bit. I do not think this project officially subscribes to the "anywhere -option is taken should accept '--' as the end of options marker" school, but because most modern command line processors use parse_options() API which gets "--" for free, we can consider ourselves practically accepting it already. And adopting such a rule/dogma/principle frees us from having to think about each individual case and helps us being consistent, so it is not necessarily a bad thing as long as the cost of following the rule/dogma/principle is not too onerous. At that point, what needs to be reviewed instead becomes to "does this new code follow the rule, and is it not bending backwards to do so?" So, I do not have strong objection against "bisect run -- <cmd>". It was, as I said in the original review, that it was so unclear why double-dash is a logical consequence of accepting options, because it was left unsaid. It would have been an easy sell if this were a part of a patch that actually adds new option(s) and explained perhaps like so: Teach 'bisect run' to take --foo and --bar options, that tell the command to do Foo and Bar. While at it, teach it the common convention that "--" is used to terminate the list of options. Nobody sane may try to use <cmd> that begins with a dash, but supporting "--" everywhere we accept a dashed option is good for consistency. Thanks.
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt index 7586c5a843..e72353e157 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 [--] <cmd>... git bisect help This command uses a binary search algorithm to find which commit in diff --git a/git-bisect.sh b/git-bisect.sh index efee12b8b1..46085651e1 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 [--] <cmd>... use <cmd>... to automatically bisect. Please use "git help bisect" to get the full man page.' @@ -238,6 +238,23 @@ bisect_replay () { bisect_run () { git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit + # check for options + # (there aren't any supported yet, unless you count "--") + while test -n "$1" + do + case "$1" in + --) + shift + break ;; + -*) + option="$1" # \$1 is not expanded by eval_gettext + die "$(eval_gettext "bisect run: invalid option: \$option")" ;; + *) + break ;; + esac + shift + done + test -n "$*" || die "$(gettext "bisect run failed: no command provided.")" while true diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh new file mode 100755 index 0000000000..e173ca18b3 --- /dev/null +++ b/t/t6071-bisect-run.sh @@ -0,0 +1,96 @@ +#!/bin/sh + +test_description='Tests git bisect run' + +exec </dev/null + +. ./test-lib.sh + +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP' +echo '.DEFAULT: dummy +.PHONY: dummy +dummy: + true +' > Makefile && +make && +echo '0' >path0 && +git update-index --add -- Makefile path0 && +git commit -q -m 'initial commit' && +git tag working0 && +# make some commits that don't cause problems +for x in `test_seq 1 20`; do + echo "$x" >path0 && + git update-index --replace -- path0 && + git commit -q -m "working commit $x" && + git tag "working$x" || exit 1 +done && +# break the makefile +sed -i.bak -e 's/true/false/' Makefile && +rm -f Makefile.bak && +! make && +git update-index --replace -- Makefile && +git commit -q -m "First broken commit" && +git tag broken0 && +# make some more commits that still FTBFS +for x in `test_seq 1 20`; do + echo "$x" >path0 && + git update-index --replace -- path0 && + git commit -q -m "broken build $x" && + git tag "broken$x" || exit 1 +done && +# repair it +git checkout working0 -- Makefile && +make && +git update-index --replace -- Makefile && +git commit -q -m "First repaired commit" && +git tag fixed0 && +# make some more commits with the bugfix +for x in `test_seq 1 20`; do + echo "$x" >path0 && + git update-index --replace -- path0 && + git commit -q -m "fixed build $x" && + git tag "fixed$x" || exit 1 +done +SETUP + +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9' + +test_expect_failure() { + shift + test_must_fail "$@" +} + +# okay, let's do some negative testing + +OLDPATH="$PATH" + +PATH="$PATH:." + +test_expect_success 'setup this-is-not-a-valid-option' ' + echo "#/bin/sh" > --this-is-not-a-valid-option && + chmod a+x -- --this-is-not-a-valid-option && + --this-is-not-a-valid-option' + +test_expect_failure 'git bisect run: reject unrecognized options' git bisect run --this-is-not-a-valid-option + +PATH="$OLDPATH" + +# finally, verify that '--' is honored (note that will mess things up and require a bisect reset) +PATH="$PATH:." + +test_expect_success 'git bisect run: honor --' 'git bisect run -- --this-is-not-a-valid-option' + +PATH="$OLDPATH" + +# now we have to undo the bisect run +test_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9' + +test_expect_success "running bisection" " + git bisect run $success_option make && + # we should have determined that broken0 is the first bad version + test_cmp_rev broken0 refs/bisect/bad && + # and that version should be the one checked out + test_cmp_rev broken0 HEAD +" + +test_done
The 'bisect run' feature doesn't currently accept any options, but it could, and that means a '--' option. Additionally, this adds an actual test script for bisect run - it creates a repository with a failed build, then attempts to bisect the offending commit. Signed-off-by: Stephen Oberholtzer <stevie@qrpff.net> --- Documentation/git-bisect.txt | 2 +- git-bisect.sh | 19 ++++++- t/t6071-bisect-run.sh | 96 ++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 2 deletions(-) create mode 100755 t/t6071-bisect-run.sh