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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 15, 2019, 13:08 UTC
Message-ID: <nycvar.QRO.7.76.6.1904151503390.44@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Sun, 14 Apr 2019, Junio C Hamano wrote:

Show 34 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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

Ævar rightfully pointed out that I introduced a new paradigm, so I switched to imitating the exact way `test-lib.sh` handles similar cases. Which is the `isset` method.

Sure, I could do something differently, like `test -z`. But why? I think it is better to stay consistent with the existing checks.

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

Yes.

Let's assume that there is a developer, or even a team (think e.g. VFS for Git), working on some long-running features that should be eventually contributed to Git, but that are still in flux, and through a merge with the current Git version, they get this change, and it breaks 50 of their test scripts.

This is the scenario I wanted to help, however unlikely that is ;-)

Of course, eventually they'd want to fix their test scripts. But maybe right now is not such a great time, so let's let them override the new behavior.

Show 16 quoted lines

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

Thank you! Dscho