[bc/hash-independent-tests-part-4] t: decrease nesting in test_oid_to_path
diff mbox series

Message ID 20190808065614.GA209195@google.com
State New
Headers show
Series
  • [bc/hash-independent-tests-part-4] t: decrease nesting in test_oid_to_path
Related show

Commit Message

Jonathan Nieder Aug. 8, 2019, 6:56 a.m. UTC
t1410.3 ("corrupt and checks") fails when run using dash versions
before 0.5.8, with a cryptic message:

	mv: cannot stat '.git/objects//e84adb2704cbd49549e52169b4043871e13432': No such file or directory

The function generating that path:

	test_oid_to_path () {
		echo "${1%${1#??}}/${1#??}"
	}

which is supposed to produce a result like

	12/3456789....

But a dash bug[*] causes it to instead expand to

	/3456789...

The stream of symbols that makes up this function is hard for humans
to follow, too.  The complexity mostly comes from the repeated use of
the expression ${1#??} for the basename of the loose object.  Use a
variable instead --- nowadays, the dialect of shell used by Git
permits local variables, so this is cheap.

An alternative way to work around [*] is to remove the double-quotes
around test_oid_to_path's return value.  That makes the expression
easier for dash to read, but harder for humans.  Let's prefer the
rephrasing that's helpful for humans, too.

Noticed by building on Ubuntu trusty, which uses dash 0.5.7.

[*] Fixed by v0.5.8~13 ("[EXPAND] Propagate EXP_QPAT in subevalvar, 2013-08-23).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jeff King Aug. 8, 2019, 9:37 a.m. UTC | #1
On Wed, Aug 07, 2019 at 11:56:14PM -0700, Jonathan Nieder wrote:

> But a dash bug[*] causes it to instead expand to
> 
> 	/3456789...
> 
> The stream of symbols that makes up this function is hard for humans
> to follow, too.  The complexity mostly comes from the repeated use of
> the expression ${1#??} for the basename of the loose object.

Yeah, both seem like good reasons to change this (and the patch looks
good to me).

> Use a variable instead --- nowadays, the dialect of shell used by Git
> permits local variables, so this is cheap.

I don't recall whether we ever made a conscious decision about that.
Commit 01d3a526ad (t0000: check whether the shell supports the "local"
keyword, 2017-10-26) put in a test balloon about 2 years ago. Since then
several other uses have crept in to test-lib-function.sh.

But I think we can probably call the experiment a success at this point.
Maybe we'd want to do this, as well:

-- >8 --
Subject: [PATCH] t0000: reword comments for "local" test

Commit 01d3a526ad (t0000: check whether the shell supports the "local"
keyword, 2017-10-26) added a test to gather data on whether people run
the test suite with shells that don't support "local".

After almost two years, nobody has complained, and several other uses
have cropped up in test-lib-functions.sh. Let's declare it acceptable to
use.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0000-basic.sh | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index e89438e619..9ca0818cbe 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -25,16 +25,14 @@ try_local_x () {
 	echo "$x"
 }
 
-# This test is an experiment to check whether any Git users are using
-# Shells that don't support the "local" keyword. "local" is not
+# Check whether the shell supports the "local" keyword. "local" is not
 # POSIX-standard, but it is very widely supported by POSIX-compliant
-# shells, and if it doesn't cause problems for people, we would like
-# to be able to use it in Git code.
+# shells, and we rely on it within Git's test framework.
 #
-# For now, this is the only test that requires "local". If your shell
-# fails this test, you can ignore the failure, but please report the
-# problem to the Git mailing list <git@vger.kernel.org>, as it might
-# convince us to continue avoiding the use of "local".
+# If your shell fails this test, the results of other tests may be
+# unreliable. You may wish to report the problem to the Git mailing
+# list <git@vger.kernel.org>, as it could cause us to reconsider
+# relying on "local".
 test_expect_success 'verify that the running shell supports "local"' '
 	x="notlocal" &&
 	echo "local" >expected1 &&
brian m. carlson Aug. 8, 2019, 11:25 a.m. UTC | #2
On 2019-08-08 at 06:56:14, Jonathan Nieder wrote:
> t1410.3 ("corrupt and checks") fails when run using dash versions
> before 0.5.8, with a cryptic message:
> 
> 	mv: cannot stat '.git/objects//e84adb2704cbd49549e52169b4043871e13432': No such file or directory
> 
> The function generating that path:
> 
> 	test_oid_to_path () {
> 		echo "${1%${1#??}}/${1#??}"
> 	}
> 
> which is supposed to produce a result like
> 
> 	12/3456789....
> 
> But a dash bug[*] causes it to instead expand to
> 
> 	/3456789...
> 
> The stream of symbols that makes up this function is hard for humans
> to follow, too.  The complexity mostly comes from the repeated use of
> the expression ${1#??} for the basename of the loose object.  Use a
> variable instead --- nowadays, the dialect of shell used by Git
> permits local variables, so this is cheap.
> 
> An alternative way to work around [*] is to remove the double-quotes
> around test_oid_to_path's return value.  That makes the expression
> easier for dash to read, but harder for humans.  Let's prefer the
> rephrasing that's helpful for humans, too.
> 
> Noticed by building on Ubuntu trusty, which uses dash 0.5.7.

This seems like a sane, well-reasoned fix. I don't know if we care about
building on Ubuntu trusty (since it is EOL), but if we do, then we
should definitely take this patch.

I agree it makes things easier to follow as well, which is also nice,
and it preserves the shell-only nature that's so desirable on Windows.
brian m. carlson Aug. 8, 2019, 11:35 a.m. UTC | #3
On 2019-08-08 at 09:37:33, Jeff King wrote:
> On Wed, Aug 07, 2019 at 11:56:14PM -0700, Jonathan Nieder wrote:
> -- >8 --
> Subject: [PATCH] t0000: reword comments for "local" test
> 
> Commit 01d3a526ad (t0000: check whether the shell supports the "local"
> keyword, 2017-10-26) added a test to gather data on whether people run
> the test suite with shells that don't support "local".
> 
> After almost two years, nobody has complained, and several other uses
> have cropped up in test-lib-functions.sh. Let's declare it acceptable to
> use.

I'm also in favor of this patch. To my knowledge, the only FLOSS
POSIX-compliant shell that doesn't support "local" is AT&T ksh, so there
are a lot of alternatives that folks have (bash, dash, posh, zsh, mksh,
pdksh, busybox sh, etc.) in case they have a less capable shell.
SZEDER Gábor Aug. 8, 2019, 12:58 p.m. UTC | #4
On Thu, Aug 08, 2019 at 11:35:38AM +0000, brian m. carlson wrote:
> On 2019-08-08 at 09:37:33, Jeff King wrote:
> > On Wed, Aug 07, 2019 at 11:56:14PM -0700, Jonathan Nieder wrote:
> > -- >8 --
> > Subject: [PATCH] t0000: reword comments for "local" test
> > 
> > Commit 01d3a526ad (t0000: check whether the shell supports the "local"
> > keyword, 2017-10-26) added a test to gather data on whether people run
> > the test suite with shells that don't support "local".
> > 
> > After almost two years, nobody has complained, and several other uses
> > have cropped up in test-lib-functions.sh. Let's declare it acceptable to
> > use.
> 
> I'm also in favor of this patch. To my knowledge, the only FLOSS
> POSIX-compliant shell that doesn't support "local" is AT&T ksh, so there
> are a lot of alternatives that folks have (bash, dash, posh, zsh, mksh,
> pdksh, busybox sh, etc.) in case they have a less capable shell.

From these shells:

  - dash and Bash work; no surprise there, being the default shells in
    some of the most popular Linux distros and GfW.

  - mksh works as well (just don't hit ctrl-C while running a test
    involving a daemon with '--verbose-log').

  - Zsh seems to be fundamentally incapable to run our test
    framework.

  - BusyBox sh is fine for the most part, but there are a few test
    failures, where we rely on some options or capabilities of various
    system commands that are not supported by the corresponding
    BusyBox builtins.

  - posh can successfully run a lot of test scripts, but there are a
    lot of failures as well, it skips a few test scripts that it
    shouldn't (claiming that Perl and Bash are not available?!), and
    t0000 aborts with a 'bug in test framework' error.  What's most
    worrying is that it reported a few "known breakage fixed" cases as
    well.

  - pdksh seems to be unavailable in my Linux distro, but since its
    name contains the "ksh" substring I'm sceptical.

So there are not that many alternatives.

Having said that, I'm in favor of using 'local'.
Junio C Hamano Aug. 8, 2019, 5:22 p.m. UTC | #5
Jeff King <peff@peff.net> writes:

> On Wed, Aug 07, 2019 at 11:56:14PM -0700, Jonathan Nieder wrote:
>
>> But a dash bug[*] causes it to instead expand to
>> 
>> 	/3456789...
>> 
>> The stream of symbols that makes up this function is hard for humans
>> to follow, too.  The complexity mostly comes from the repeated use of
>> the expression ${1#??} for the basename of the loose object.
>
> Yeah, both seem like good reasons to change this (and the patch looks
> good to me).

Good spottig.  Thanks.

> +# Check whether the shell supports the "local" keyword. "local" is not
> +# POSIX-standard, but it is very widely supported by POSIX-compliant
> +# shells, and we rely on it within Git's test framework.
> +#
> +# If your shell fails this test, the results of other tests may be
> +# unreliable. You may wish to report the problem to the Git mailing
> +# list <git@vger.kernel.org>, as it could cause us to reconsider
> +# relying on "local".
>  test_expect_success 'verify that the running shell supports "local"' '
>  	x="notlocal" &&
>  	echo "local" >expected1 &&

Updated text reads well.  Leaving the test in is still a very good
idea.

Thanks.

Patch
diff mbox series

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 7860491660..de58e8b502 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1337,7 +1337,8 @@  test_oid () {
 # Insert a slash into an object ID so it can be used to reference a location
 # under ".git/objects".  For example, "deadbeef..." becomes "de/adbeef..".
 test_oid_to_path () {
-	echo "${1%${1#??}}/${1#??}"
+	local basename=${1#??}
+	echo "${1%$basename}/$basename"
 }
 
 # Choose a port number based on the test script's number and store it in