[PATCH] bisect run: allow '--' as an options terminator

[PATCH] bisect run: allow '--' as an options terminator

[Stephen Oberholtzer]

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

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
-- 
2.20.1

Re: [PATCH] bisect run: allow '--' as an options terminator

[Junio C Hamano]

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.

Re: [PATCH] bisect run: allow '--' as an options terminator

[Stephen Oberholtzer]

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

Re: [PATCH] bisect run: allow '--' as an options terminator

[Junio C Hamano]

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.