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

[Junio C Hamano <gitster@pobox.com>]

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.