Re: [RFC] shell: local x=$1 may need to quote the RHS

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Jan 25, 2022 at 09:53:25PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> 
> > Yeah; bisecting dash with your example script pointed me at cbb71a8
> > (eval: Add assignment built-in support again, 2018-05-19), which indeed
> > appears in v0.5.11 (and all subsequent versions).
> >
> > cbb71a8 points at release 0.3.8-15, which predates Git (and a tag
> > pointing at it was never created, since it's behind the big "initial
> > import" commit at the beginning of dash.git's history). But skimming
> > ChangeLog.O, we see:
> >
> >     * Removed assignment builtins since it is at best undefined by the
> >       SuS and also can't be implemented consistently.
> >
> > So this probably didn't work at all between that 0.3.8-15 up until v0.5.11.
> >
> >> The manual page for recent dash may need an update.
> >> Can you perhaps file a bug on their documentation?
> >
> > Yes, I agree that dash.1 is out-of-date after cbb71a8. Konstantin:
> > please feel free to use any of this if it's helpful to you in creating a
> > bug report for the dash folks.
> 
> I doubt that we can write off dash v0.5.10 as too old to matter, as
> the tagger date seems to be mid 2020 at 
> 
> https://kernel.googlesource.com/pub/scm/utils/dash/dash/+/refs/tags/v0.5.11

I agree, and v0.5.10 is widely used, as it is the default shell in
Ubuntu 20.04 LTS.

> So here is a bit wider "grep" output, looking for
> 
> $ git grep '^[ 	]*local[	 ].*=' \*.sh
> 
> to reject _any_ assignment on the same line as "local", but I
> manually excluded the ones that are not meant to be run with dash.
> I obviously excluded the one in t0000 (try_local_xy) that is a
> weather-balloon for this exact issue.
> 
> According to this thread, these would not work correctly on dash
> older than 0.5.11 and needs fixing by splitting declaration of
> variables as locals and assignment of their initial values.

I don't think that's necessary.  Whatever that (apparently horribly
outdated) documentation might state, I think what really matters is
what dash actually does.

The patch below puts a bit more strain on our `local` weatherballoon
test:

  - It checks assignments from a parameter expansion and command
    substitution as well, not only simple string assignments.

  - The expanded values contain spaces and are quoted (to avoid the
    field splitting issue that started this thread [1]).

  - Creates a local variable with a name that doesn't hide an already
    existing variable in the global scope, to see whether it becomes
    global.

It succeeds with all tagged dash versions, the oldest being v0.5.2
tagged on 2005-01-31.  I think that's more than old enough to say that
it's not necessary to put local variable declaration and assignment on
different lines, and we can declare multiple local variables on one
line.

Note that NetBSD 8.1's /bin/sh need quotes on the RHS in similar
assignments as well.

[1] We've already run into this issue in ebee5580ca
    (parallel-checkout: avoid dash local bug in tests, 2021-06-06),
    but apparently didn't think through what else might be affected by
    that bug.


diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 03fead95e7..196106fdf5 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -22,8 +22,8 @@ modification *should* take notice and update the test vectors here.
 . "$TEST_DIRECTORY"/lib-subtest.sh
 
 try_local_xy () {
-	local x="local" y="alsolocal" &&
-	echo "$x $y"
+	local x="local" y="${no_such_var-also local}" z="$(echo "third local")" &&
+	echo "$x $y $z"
 }
 
 # Check whether the shell supports the "local" keyword. "local" is not
@@ -37,11 +37,11 @@ try_local_xy () {
 test_expect_success 'verify that the running shell supports "local"' '
 	x="notlocal" &&
 	y="alsonotlocal" &&
-	echo "local alsolocal" >expected1 &&
+	echo "local also local third local" >expected1 &&
 	try_local_xy >actual1 &&
 	test_cmp expected1 actual1 &&
-	echo "notlocal alsonotlocal" >expected2 &&
-	echo "$x $y" >actual2 &&
+	echo "notlocal alsonotlocal z_unset" >expected2 &&
+	echo "$x $y ${z-z_unset}" >actual2 &&
 	test_cmp expected2 actual2
 '