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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2019, 02:35 UTC
Message-ID: <xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <a27d316855a833aa1726fc20c905dc40e41adf2f.1555061837.git.gitgitgadget@gmail.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,

Show 11 quoted lines

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

Show 6 quoted lines

> +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.

Show 14 quoted lines

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