Fix += operator regressions (re: fae8862c) (#270)

The bugfix for BUG_CMDSPASGN backported in commit fae8862c caused
two regressions with the += operator:

1. The += operator did not append to variables. Reproducer:
     $ integer foo=3
     $ foo+=2 command eval 'echo $foo'
     2

2. The += operator ignored the readonly attribute, modifying readonly
   variables in the same manner as above. Reproducer
     $ readonly bar=str
     $ bar+=ing command eval 'echo $bar'
     ing

Both of the regressions above were caused by nv_putval() failing to
clone the variable from the previous scope into the invocation-local
scope. As a result, 'foo+=2' was effectively 0 + 2 (since ksh didn't
clone 3). The first regression was noticed during the development of
ksh93v-, so to fix both bugs I've backported the bugfix for the
regression from the ksh93v- 2013-10-10 alpha version:
https://www.mail-archive.com/ast-users@lists.research.att.com/msg00369.html

src/cmd/ksh93/sh/name.c:
- To fix both of the bugs above, find the variable to modify with
  nv_search(), then clone it into the invocation local scope. To
  fix the readonly bug as well, this is done before the NV_RDONLY
  check (otherwise np will be missing that attribute and be
  incorrectly modified in the invocation-local scope).
- Update a nearby comment describing what sh_assignok() does (per this
  comment: https://github.com/ksh93/ksh/pull/249#issuecomment-811381759)

src/cmd/ksh93/tests/builtins.sh:
- Add regression tests for both of the now fixed regressions,
  loosely based on the regression tests in ksh93v-.

NEWS

@@ -3,6 +3,17 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2021-04-11:
+
+- Fixed two related regressions introduced on 2020-06-16:
+  1. The += assignment failed to append the value of variables when used
+     in an invocation-local scope. The following should print '5', but
+     the regression resulted in '3' being printed instead:
+       $ integer foo=2; foo+=3 command eval 'echo $foo'
+       3
+  2. Any += assignment used in an invocation-local scope could modify
+     readonly variables.
+
 2021-04-10:
 
 - Fixed: the internal count of the recursion level for arithmetic expressions

src/cmd/ksh93/include/version.h

@@ -20,7 +20,7 @@
 
 #define SH_RELEASE_FORK	"93u+m"		/* only change if you develop a new ksh93 fork */
 #define SH_RELEASE_SVER	"1.0.0-alpha"	/* semantic version number: https://semver.org */
-#define SH_RELEASE_DATE	"2021-04-10"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2021-04-11"	/* must be in this format for $((.sh.version)) */
 #define SH_RELEASE_CPYR	"(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK
 
 /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */

src/cmd/ksh93/sh/name.c

@@ -1601,15 +1601,23 @@ void nv_putval(register Namval_t *np, const char *string, int flags)
 #if SHOPT_FIXEDARRAY
 	Namarr_t	*ap;
 #endif /* SHOPT_FIXEDARRAY */
-	if(!(flags&NV_RDONLY) && nv_isattr (np, NV_RDONLY))
+	/*
+	 * When we are in an invocation-local scope and we are using the
+	 * += operator, clone the variable from the previous scope.
+	 */
+	if((flags&NV_APPEND) && nv_isnull(np) && shp->var_tree->view)
+	{
+		Namval_t *mp = nv_search(np->nvname,shp->var_tree->view,0);
+		if(mp)
+			nv_clone(mp,np,0);
+	}
+	/* Don't alter readonly variables */
+	if(!(flags&NV_RDONLY) && nv_isattr(np,NV_RDONLY))
 	{
 		errormsg(SH_DICT,ERROR_exit(1),e_readonly, nv_name(np));
 		UNREACHABLE();
 	}
-	/*
+	/* Create a local scope when inside of a virtual subshell */
-	 * The following could cause the shell to fork if assignment
-	 * would cause a side effect
-	 */
 	shp->argaddr = 0;
 	if(shp->subshell && !nv_local && !(flags&NV_RDONLY))
 		np = sh_assignok(np,1);

src/cmd/ksh93/tests/builtins.sh

@@ -740,10 +740,37 @@ printf '\\\000' | read -r -d ''
 [[ $REPLY == $'\\' ]] || err_exit "read -r -d '' ignores -r"
 
 # ======
-# Preceding a special builtin with 'command' should disable its special properties
+# BUG_CMDSPASGN: Preceding a special builtin with 'command' should disable its special properties.
 foo=BUG command eval ':'
 [[ $foo == BUG ]] && err_exit "'command' fails to disable the special properties of special builtins"
 
+# Regression that occurred after fixing the bug above: the += operator didn't work correctly.
+# https://www.mail-archive.com/ast-users@lists.research.att.com/msg00369.html
+unset foo
+integer foo=1
+exp=4
+got=$(foo+=3 command eval 'echo $foo')
+[[ $exp == $got ]] || err_exit "[1]: += assignment for environment variables doesn't work with 'command special_builtin'" \
+	"(expected $exp, got $got)"
+foo+=3 command eval 'test $foo'
+(( foo == 1 )) || err_exit "environment isn't restored after 'command special_builtin'" \
+	"(expected 1, got $foo)"
+got=$(foo+=3 eval 'echo $foo')
+[[ $exp == $got ]] || err_exit "+= assignment for environment variables doesn't work with builtins" \
+	"(expected $exp, got $got)"
+
+unset foo
+exp=barbaz
+got=$(foo=bar; foo+=baz command eval 'echo $foo')
+[[ $exp == $got ]] || err_exit "[2]: += assignment for environment variables doesn't work with 'command special_builtin'" \
+	"(expected $exp, got $got)"
+
+# Attempting to modify a readonly variable with the += operator should fail
+exp=2
+got=$(integer -r foo=2; foo+=3 command eval 'echo $foo' 2> /dev/null)
+[[ $? == 0 ]] && err_exit "+= assignment modifies readonly variables" \
+	"(expected $exp, got $got)"
+
 # ======
 # 'whence -f' should ignore functions
 foo_bar() { true; }