| Message ID | 20190808065614.GA209195@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [bc/hash-independent-tests-part-4] t: decrease nesting in test_oid_to_path | expand |
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 &&
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.
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.
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'.
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.
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
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(-)