git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Denton Liu" <liu.denton@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
Date: Sun, 14 Apr 2019 11:35:58 +0900
Message-ID: <xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <a27d316855a833aa1726fc20c905dc40e41adf2f.1555061837.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Fri, 12 Apr 2019 02:37:24 -0700 (PDT)")

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



  reply	other threads:[~2019-04-14  2:36 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-25 18:14 [PATCH 0/8] Do not use abbreviated options in tests Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 1/8] tests (rebase): spell out the `--keep-empty` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 2/8] tests (rebase): spell out the `--force-rebase` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match` Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 4/8] t5531: avoid using an abbreviated option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 5/8] tests (push): do not abbreviate the `--follow-tags` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 6/8] tests (status): spell out the `--find-renames` option in full Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 8/8] tests: disallow the use of abbreviated options (by default) Johannes Schindelin via GitGitGadget
2019-03-25 18:35   ` Denton Liu
2019-03-25 20:26     ` Ævar Arnfjörð Bjarmason
2019-04-12  8:48       ` Johannes Schindelin
2019-04-12  8:50     ` Johannes Schindelin
2019-03-25 19:47   ` Ævar Arnfjörð Bjarmason
2019-04-12  8:59     ` Johannes Schindelin
2019-03-25 20:23 ` [PATCH 0/2] allow for configuring option abbreviation + fix Ævar Arnfjörð Bjarmason
2019-03-25 20:23 ` [PATCH 1/2] parse-options: allow for configuring option abbreviation Ævar Arnfjörð Bjarmason
2019-03-25 21:23   ` Eric Sunshine
2019-03-25 22:47     ` Ævar Arnfjörð Bjarmason
2019-03-26  4:14       ` Duy Nguyen
2019-03-26  6:28         ` Ævar Arnfjörð Bjarmason
2019-03-26  7:13           ` Duy Nguyen
2019-03-26 11:00             ` Ævar Arnfjörð Bjarmason
2019-04-01 10:47     ` Junio C Hamano
2019-04-12  9:06       ` Johannes Schindelin
2019-03-25 20:23 ` [PATCH 2/2] parse-options: don't emit "ambiguous option" for aliases Ævar Arnfjörð Bjarmason
2019-04-17 12:44   ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2019-04-17 16:04     ` Duy Nguyen
2019-04-18  0:48       ` Junio C Hamano
2019-04-18  9:29         ` Duy Nguyen
2019-04-19  4:39           ` Junio C Hamano
2019-04-22 12:22             ` [PATCH] " Nguyễn Thái Ngọc Duy
2019-04-22 12:34               ` Duy Nguyen
2019-04-29 10:05               ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2019-05-07  3:43                 ` Junio C Hamano
2019-05-07 11:58                   ` Duy Nguyen
2019-04-02  0:58 ` [PATCH 0/8] Do not use abbreviated options in tests Junio C Hamano
2019-04-12  9:37 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 1/8] tests (rebase): spell out the `--keep-empty` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match` Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 2/8] tests (rebase): spell out the `--force-rebase` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 4/8] t5531: avoid using an abbreviated option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 5/8] tests (push): do not abbreviate the `--follow-tags` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 6/8] tests (status): spell out the `--find-renames` option in full Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default) Johannes Schindelin via GitGitGadget
2019-04-14  2:35     ` Junio C Hamano [this message]
2019-04-15  2:55       ` Junio C Hamano
2019-04-15 13:09         ` Johannes Schindelin
2019-04-15 13:45           ` Junio C Hamano
2019-04-17 12:07             ` Johannes Schindelin
2019-04-15 13:08       ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git