Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm
Precedence: bulk
From: Bart Schaefer <schaefer@brasslantern.com>
Message-Id: <160207230544.ZM10065@torch.brasslantern.com>
Date: Sun, 7 Feb 2016 23:05:44 -0800
In-Reply-To: <160207133307.ZM31008@torch.brasslantern.com>
Comments: In reply to Bart Schaefer <schaefer@brasslantern.com>
        "Re: unset "hash[key]" isn't matched with what "key" may be" (Feb  7,  1:33pm)
References: <CAKc7PVBXd-UGwM+_tRSwuw-WL154YaTys6-t3iuzfAsG-RC-YQ@mail.gmail.com> 
	<160207133307.ZM31008@torch.brasslantern.com>
To: Zsh hackers list <zsh-workers@zsh.org>
Subject: Re: unset "hash[key]" isn't matched with what "key" may be
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

On Feb 7,  1:33pm, Bart Schaefer wrote:
}
} On Feb 7,  3:16pm, Sebastian Gniazdowski wrote:
} }                 unset "func[$i]"
} } -zplg-diff-functions:unset:36: func[opp+a\[]: invalid parameter name
} 
} Yep, "unset" is pretty naive about parsing the subscript there.  It wants
} the square brackets in balanced pairs and doesn't consider any kind of
} quoting.

Patch below for consideration.  This doesn't do any additional expansion,
but it has the effect of allowing backslash-quoting -- which therefore
requires that backslashes themselves be doubled, cf. all the caveats
about subscripts behaving as if double-quoted that appear elsewhere.

After this patch, the best way to handle unset of associative array
elements looks something like

  typeset -A ary
  ary[${key}]=value
  ...
  unset "ary[${(b)key}]"

(or use "noglob" instead of double quotes).

This does reflect a change from previous behavior, so we'll want to
consider it carefully.  If we decide to keep the old behavior, just
remove the call to remnulargs().  I think using parse_subscript() intead
of skipcomm() is still the more sensible approach; in fact, this patch
revealed an error in the E01 test script.

} (The -m option also doesn't work for subscripts.)

I haven't attempted to do anything about that, and I'm not sure that it
makes sense to.


diff --git a/Src/builtin.c b/Src/builtin.c
index 63f964d..4c8fbcd 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3350,15 +3350,24 @@ bin_unset(char *name, char **argv, Options ops, int func)
     /* do not glob -- unset the given parameter */
     queue_signals();
     while ((s = *argv++)) {
-	char *ss = strchr(s, '[');
-	char *sse = ss;
+	char *ss = strchr(s, '['), *subscript = 0;
 	if (ss) {
-	    if (skipparens('[', ']', &sse) || *sse) {
-		zerrnam(name, "%s: invalid parameter name", s);
-		returnval = 1;
-		continue;
-	    }
+	    char *sse;
 	    *ss = 0;
+	    if ((sse = parse_subscript(ss+1, 1, ']'))) {
+		*sse = 0;
+		subscript = dupstring(ss+1);
+		*sse = ']';
+		remnulargs(subscript);
+		untokenize(subscript);
+	    }
+	}
+	if ((ss && !subscript) || !isident(s)) {
+	    if (ss)
+		*ss = '[';
+	    zerrnam(name, "%s: invalid parameter name", s);
+	    returnval = 1;
+	    continue;
 	}
 	pm = (Param) (paramtab == realparamtab ?
 		      /* getnode2() to avoid autoloading */
@@ -3376,11 +3385,8 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	} else if (ss) {
 	    if (PM_TYPE(pm->node.flags) == PM_HASHED) {
 		HashTable tht = paramtab;
-		if ((paramtab = pm->gsu.h->getfn(pm))) {
-		    *--sse = 0;
-		    unsetparam(ss+1);
-		    *sse = ']';
-		}
+		if ((paramtab = pm->gsu.h->getfn(pm)))
+		    unsetparam(subscript);
 		paramtab = tht;
 	    } else if (PM_TYPE(pm->node.flags) == PM_SCALAR ||
 		       PM_TYPE(pm->node.flags) == PM_ARRAY) {
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index f270767..40e96af 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -776,7 +776,7 @@
   unsetopt pathdirs
   pathtestdir/findme
   path=($oldpath)
-  unset $oldpath
+  unset oldpath
   rm -rf pdt_topdir pathtestdir
 0:PATH_DIRS option
 >File in upper dir