Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +	disallow_abbreviated_options =
> +		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);

... which means that the value of the environment variable follows
the usual "true, yes and 1 all activate it"; very good,

> diff --git a/t/README b/t/README
> index 656288edce..9ed3051a1c 100644
> --- a/t/README
> +++ b/t/README
> @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
>  fetch-pack to not request sideband-all (even if the server advertises
>  sideband-all).
>  
> +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
> +the default when running tests), errors out when an abbreviated option
> +is used.

OK.

> +test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
> +	env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> +		test-tool parse-options --ye &&
> +	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
> +		test-tool parse-options --ye
> +'

The feature is activated for all tests unless otherwise noted, and
the above marked the places that need to be "otherwise noted" in a
reasonable way.  Good.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 562c57e685..f1a0fea4e1 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -57,6 +57,13 @@ fi
>  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
>  export PERL_PATH SHELL_PATH
>  
> +# Disallow the use of abbreviated options in the test suite by default
> +if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
> +then
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> +fi

If the original environment has it as flase, as it is set, the
substitution will yield "isset" which is not an empty string, so we
assign true.

If the original environment is not set, or set to an empty, however,
the substitution will yield an empty string, so we won't touch the
variable.

I am not sure in what situation the above behaviour becomes useful.

Do you mean more like

	if test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS"
	then
		... assignment ...
	fi

IOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false
as a sign that the developer for whatever readon wants the disambiguation
code in parse-options to kick in for all uses and allow a shortened option
names?

If on the other hand you are protecting our tests against those
who casually have the environment set to false, because they know
some of the scripts they use are sloppy *and* for whatever reason
they anticipate that someday we will start to disallow abbrevated
options by default?  If so, an unconditional assignment of true
would be more appropriate.

I think I can agree with either of the two positions (i.e. we let
those who explicitly want to decline do so, or we unconditionally
make sure we catch issues in our tests), and I do not think of a
third position that are different from these two and that would make
sense.  Between the two, I'd probably vote for the latter if I was
pressed, but even then that is not a very strong preference.

Thanks.  I very much like the premise of this series, and the above
hunk stood out in the range-diff in 0/8.