broken parsing with $((`:`))

broken parsing with $((`:`))

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

looks like zsh doesn't correctly parse this:
$ zsh -c 'echo $((`:`))'
zsh:1: bad math expression: illegal character: \M-]

a little whitespace makes it happy:
$ zsh -c 'echo $(( `:`))'
0
$ zsh -c 'echo $((`:` ))'
0

same for $():
$ zsh -c 'echo $(($(:)))'
zsh:1: bad math expression: illegal character: \M-]
$ zsh -c 'echo $(( $(:)))'
0
$ zsh -c 'echo $(($(:) ))'
0

looks like it's related to the subshell not outputting anything.
if you use 'echo 0' instead of ':', it works out.

$ zsh --version
zsh 5.0.7 (x86_64-pc-linux-gnu)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Tue, 14 Apr 2015 23:05:31 -0400
Mike Frysinger <vapier@gentoo.org> wrote:
> looks like zsh doesn't correctly parse this:
> $ zsh -c 'echo $((`:`))'
> zsh:1: bad math expression: illegal character: \M-]
>
> looks like it's related to the subshell not outputting anything.

Yes, indeed --- there's a funny internal special case for empty strings
that I never quite get my head around.

(Please, God, make the problems with command and math substitution
parsing stop now.)

pws

diff --git a/Src/math.c b/Src/math.c
index c047725..f2c72d5 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -1398,7 +1398,7 @@ matheval(char *s)
     if (!mlevel)
 	outputradix = outputunderscore = 0;
 
-    if (!*s) {
+    if (!*s || *s == Nularg) {
 	x.type = MN_INTEGER;
 	x.u.l = 0;
 	return x;
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index d3176dd..e2dfe56 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -383,4 +383,7 @@
   print ${$(( $1 * 100 ))%%.[0-9]#})
 0:Arithmetic substitution nested in parameter substitution
 >3246
- 
+
+  print $((`:`))
+0:Null string in arithmetic evaluation after command substitution
+>0

Re: broken parsing with $((`:`))

[Bart Schaefer]

On Apr 15, 10:03am, Peter Stephenson wrote:
} 
} Yes, indeed --- there's a funny internal special case for empty strings
} that I never quite get my head around.
} 
} -    if (!*s) {
} +    if (!*s || *s == Nularg) {

I wonder if (a) there are other overlooked cases like this and (b) if it
would be useful to have a #define macro for (c == '\0' || c == Nularg).
A quick grep doesn't find existing cases of that test other than this
new one, but of course if the Nularg part were forgotten, it wouldn't.

I suspect it just isn't that common because Nularg goes away after the
parsing pass.

} (Please, God, make the problems with command and math substitution
} parsing stop now.)

Indeed ... at what point should we consider 5.0.8 to get all these little
edge-case fixes out into the field?

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Wed, 15 Apr 2015 08:13:48 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> } (Please, God, make the problems with command and math substitution
> } parsing stop now.)
> 
> Indeed ... at what point should we consider 5.0.8 to get all these little
> edge-case fixes out into the field?

Soon, I think; the problems are only trickling in somewhat glacially now
so there doesn't seem a lot of point in waiting... does anybody know
anything immediately blocking...?

pws

Potential 3.0.8 [was Re: broken parsing with $((`:`))]

[Bart Schaefer]

On Apr 15,  4:26pm, Peter Stephenson wrote:
} Subject: Re: broken parsing with $((`:`))
}
} On Wed, 15 Apr 2015 08:13:48 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > } (Please, God, make the problems with command and math substitution
} > } parsing stop now.)
} > 
} > Indeed ... at what point should we consider 5.0.8 to get all these little
} > edge-case fixes out into the field?
} 
} Soon, I think; the problems are only trickling in somewhat glacially now
} so there doesn't seem a lot of point in waiting... does anybody know
} anything immediately blocking...?

Dunno if it's "blocking" but it'd be nice to resolve those "exec" with
variable assignments that Jun T. pointed out.

I have the following un-pushed changes (some of which have been posted
in previous articles, some not):

 remove _message from _mkdir
 unquote history words when command line has a quote
 no files to add in compaudit
 compsys.yo tweak "will" -> "will be"
 local TZ in strftime
 quote/quotebreak remove useless cast
 doc vindex pid
 allow clobber of zero-sized files
 maybe-helpful change free() to zfree() in two places
 add backslash to alias expansion matching in _expand
 test for breadth-first glob with (Y1)

Re: Potential 3.0.8 [was Re: broken parsing with $((`:`))]

[Peter Stephenson]

On Wed, 15 Apr 2015 09:23:00 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> I have the following un-pushed changes (some of which have been posted
> in previous articles, some not):
> 
>  remove _message from _mkdir
>  unquote history words when command line has a quote
>  no files to add in compaudit
>  compsys.yo tweak "will" -> "will be"
>  local TZ in strftime
>  quote/quotebreak remove useless cast
>  doc vindex pid
>  allow clobber of zero-sized files
>  maybe-helpful change free() to zfree() in two places
>  add backslash to alias expansion matching in _expand
>  test for breadth-first glob with (Y1)

As far as I can see the only potentially controversial thing there is
"allow clobber of zero-sized files", but I haven't looked back over the
messages.

pws

Pre-3.0.8 consolidated PATCH

[Bart Schaefer]

On Apr 22, 10:01am, Peter Stephenson wrote:
} Subject: Re: Potential 3.0.8 [was Re: broken parsing with $((`:`))]
}
} On Wed, 15 Apr 2015 09:23:00 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > I have the following un-pushed changes (some of which have been posted
} > in previous articles, some not):
} > 
} >  remove _message from _mkdir
} >  unquote history words when command line has a quote
} >  no files to add in compaudit
} >  compsys.yo tweak "will" -> "will be"
} >  local TZ in strftime
} >  quote/quotebreak remove useless cast
} >  doc vindex pid
} >  allow clobber of zero-sized files
} >  maybe-helpful change free() to zfree() in two places
} >  add backslash to alias expansion matching in _expand
} >  test for breadth-first glob with (Y1)
} 
} As far as I can see the only potentially controversial thing there is
} "allow clobber of zero-sized files", but I haven't looked back over the
} messages.

The following patch includes all of the above except that "clobber" one.
This is just to get an article number, I'll do separate commits for each
of the changes.


diff --git a/Completion/Base/Completer/_expand b/Completion/Base/Completer/_expand
index e52144c..3c76e13 100644
--- a/Completion/Base/Completer/_expand
+++ b/Completion/Base/Completer/_expand
@@ -87,7 +87,7 @@ if [[ "$force" = *s* ]] ||
 
   setopt aliases
   eval 'exp=( ${${(e)exp//\\[ 	
-]/ }//(#b)([ 	
+]/ }//(#b)([ 	\\
 ])/\\$match[1]} )' 2>/dev/null
   setopt NO_aliases
 else
diff --git a/Completion/Base/Completer/_history b/Completion/Base/Completer/_history
index 63878ac..cd69ca1 100644
--- a/Completion/Base/Completer/_history
+++ b/Completion/Base/Completer/_history
@@ -51,9 +51,14 @@ ISUFFIX=
 # We skip the first element of historywords so the current word doesn't
 # interfere with the completion
 
+local -a hslice
 while [[ $compstate[nmatches] -eq 0 && beg -lt max ]]; do
+  if [[ -n $compstate[quote] ]]
+  then hslice=( ${(Q)historywords[beg,beg+slice]} )
+  else hslice=( ${historywords[beg,beg+slice]} )
+  fi
   _wanted "$opt" history-words expl 'history word' \
-      compadd -Q -a 'historywords[beg,beg+slice]'
+      compadd -Q -a hslice
   (( beg+=slice ))
 done
 
diff --git a/Completion/Unix/Command/_mkdir b/Completion/Unix/Command/_mkdir
index b5f7519..58d6c74 100644
--- a/Completion/Unix/Command/_mkdir
+++ b/Completion/Unix/Command/_mkdir
@@ -61,7 +61,7 @@ case "$state" in
       [[ $variant == zsh && ${#${${words[2,-1]}:#-*}} -gt 0 ]]; then
       _wanted directories expl \
         'parent directory (alternatively specify name of directory)' \
-        _path_files -/ && ret=0 || _message 'name of directory'
+        _path_files -/ && ret=0
     fi
     ;;
 esac
diff --git a/Completion/compaudit b/Completion/compaudit
index 00125e4..fb0463c 100644
--- a/Completion/compaudit
+++ b/Completion/compaudit
@@ -71,6 +71,7 @@ if [[ -n $_compdir ]]; then
       _i_addfiles=(${_compdir}/*(/^M))
     fi
     for _i_line in {1..$#_i_addfiles}; do
+      (( $_i_line )) || break
       _i_file=${_i_addfiles[$_i_line]}
       [[ -d $_i_file && -z ${fpath[(r)$_i_file]} ]] ||
         _i_addfiles[$_i_line]=
diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index 45afe24..b699502 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -354,7 +354,7 @@ tt(all): no further completion is attempted; a string
 containing the substring tt(patterns): no pattern completion functions
 will be called; a string containing tt(default): the
 function for the `tt(-default-)' context will not be called, but
-functions defined for commands will
+functions defined for commands will be.
 )
 kindex(-math-, completion context)
 item(tt(-math-))(
diff --git a/Doc/Zsh/mod_system.yo b/Doc/Zsh/mod_system.yo
index d6b3765..7101e37 100644
--- a/Doc/Zsh/mod_system.yo
+++ b/Doc/Zsh/mod_system.yo
@@ -181,10 +181,12 @@ A readonly associative array.  The keys are:
 
 startitem()
 item(tt(pid))(
+vindex(pid, sysparams)
 Returns the process ID of the current process, even in subshells.  Compare
 tt($$), which returns the process ID of the main shell process.
 )
 item(tt(ppid))(
+vindex(ppid, sysparams)
 Returns the process ID of the parent of the current process, even in
 subshells.  Compare tt($PPID), which returns the process ID of the parent
 of the main shell process.
diff --git a/Src/Modules/datetime.c b/Src/Modules/datetime.c
index 00ebd2b..63a04dc 100644
--- a/Src/Modules/datetime.c
+++ b/Src/Modules/datetime.c
@@ -94,7 +94,7 @@ reverse_strftime(char *nam, char **argv, char *scalar, int quiet)
 }
 
 static int
-bin_strftime(char *nam, char **argv, Options ops, UNUSED(int func))
+output_strftime(char *nam, char **argv, Options ops, UNUSED(int func))
 {
     int bufsize, x;
     char *endptr = NULL, *scalar = NULL, *buffer;
@@ -145,6 +145,25 @@ bin_strftime(char *nam, char **argv, Options ops, UNUSED(int func))
     return 0;
 }
 
+static int
+bin_strftime(char *nam, char **argv, Options ops, int func)
+{
+    int result = 1;
+    char *tz = getsparam("TZ");
+
+    startparamscope();
+    if (tz && *tz) {
+	Param pm = createparam("TZ", PM_LOCAL|PM_SCALAR|PM_EXPORTED);
+	if (pm)
+	    pm->level = locallevel; /* because createparam() doesn't */
+	setsparam("TZ", ztrdup(tz));
+    }
+    result = output_strftime(nam, argv, ops, func);
+    endparamscope();
+
+    return result;
+}
+
 static zlong
 getcurrentsecs(UNUSED(Param pm))
 {
diff --git a/Src/compat.c b/Src/compat.c
index 09b3d6a..b3a8b06 100644
--- a/Src/compat.c
+++ b/Src/compat.c
@@ -443,7 +443,7 @@ zgetcwd(void)
 	ret = getcwd(cwdbuf, PATH_MAX);
 	if (ret)
 	    ret = dupstring(ret);
-	free(cwdbuf);
+	zfree(cwdbuf, PATH_MAX);
 #endif /* GETCWD_CALLS_MALLOC */
     }
 #endif /* HAVE_GETCWD */
diff --git a/Src/hist.c b/Src/hist.c
index 185d0a0..bd03c4f 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2255,10 +2255,10 @@ getargs(Histent elist, int arg1, int arg2)
 }
 
 /**/
-int
+static int
 quote(char **tr)
 {
-    char *ptr, *rptr, **str = (char **)tr;
+    char *ptr, *rptr, **str = tr;
     int len = 3;
     int inquotes = 0;
 
@@ -2299,7 +2299,7 @@ quote(char **tr)
 static int
 quotebreak(char **tr)
 {
-    char *ptr, *rptr, **str = (char **)tr;
+    char *ptr, *rptr, **str = tr;
     int len = 3;
 
     for (ptr = *str; *ptr; ptr++, len++)
diff --git a/Src/init.c b/Src/init.c
index 3e41fb9..102276a 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1409,7 +1409,7 @@ source(char *s)
 	retflag = 0;
     scriptname = old_scriptname;
     scriptfilename = old_scriptfilename;
-    free(cmdstack);
+    zfree(cmdstack, CMDSTACKSZ);
     cmdstack = ocs;
     cmdsp = ocsp;
 
diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 217ce7c..3e2095a 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -548,6 +548,7 @@
  (){ print "Negated:" $@:t } glob.tmp/dir*(Y1^Y)
  (){ print "Sorting:" $@:t } glob.tmp/dir*(Y4On)
  (){ [[ $#@ -eq 1 ]] && print Globs before last path component } glob.tmp/dir?/subdir(NY1)
+ (){ [[ $1 == glob.tmp/a ]] } glob.tmp/**/a(Y1) && print Breadth first
  (){ [[ $#@ -eq 0 ]] && print Respects qualifiers } glob.tmp/dir*(NY1.)
  (print -- *(Y)) 2>/dev/null || print "Argument required"
 0:short-circuit modifier
@@ -558,6 +559,7 @@
 >Negated: dir1 dir2 dir3 dir4
 >Sorting: dir4 dir3 dir2 dir1
 >Globs before last path component
+>Breadth first
 >Respects qualifiers
 >Argument required
 

-- 
Barton E. Schaefer

Re: Pre-3.0.8 consolidated PATCH

[Axel Beckert]

Hi Bart,

On Sat, Apr 25, 2015 at 10:14:58AM -0700, Bart Schaefer wrote:
> Subject: Pre-3.0.8 consolidated PATCH

Just to be sure: You're talking about an upcoming zsh _5_.0.8 release
in your e-mail subjects and not of some _3_.0.8 release of some other,
so far not mentioned software, right?

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://abe.noone.org/ (Web)

FIVE-ZERO-EIGHT (was Re: Pre-3.0.8 consolidated PATCH)

[Bart Schaefer]

On Apr 25,  8:29pm, Axel Beckert wrote:
}
} Just to be sure: You're talking about an upcoming zsh _5_.0.8 release
} in your e-mail subjects and not of some _3_.0.8 release of some other,
} so far not mentioned software, right?

Oops, yes.  I've made that same typo twice now.  I am referring to zsh
5.0.8.

bug with expansion and backslashes

[Oliver Kiddle]

On 25 Apr, Bart wrote:
> } >  add backslash to alias expansion matching in _expand
> --- a/Completion/Base/Completer/_expand
>    setopt aliases
>    eval 'exp=( ${${(e)exp//\\[ 	
> -]/ }//(#b)([ 	
> +]/ }//(#b)([ 	\\
>  ])/\\$match[1]} )' 2>/dev/null

I just noticed that expansion was broken when I tried to expand *\~
(exiftran backs up files with a trailing ~ and I wanted to clean them up)
git bisect led me to 33ff5d06 which is the patch above.

What's not clear to me is what problem this patch was trying to solve.

To reproduce, the following seems to be enough:
  zsh -f
  autoload -U compinit ; compinit
  zstyle ':completion:*::::' completer _expand _complete
  bindkey '^I' complete-word
  setopt extendedglob

Oliver

Re: bug with expansion and backslashes

[Bart Schaefer]

On Aug 16,  1:14am, Oliver Kiddle wrote:
} Subject: bug with expansion and backslashes
}
} On 25 Apr, Bart wrote:
} > } >  add backslash to alias expansion matching in _expand
} > --- a/Completion/Base/Completer/_expand
} >    setopt aliases
} >    eval 'exp=( ${${(e)exp//\\[ 	
} > -]/ }//(#b)([ 	
} > +]/ }//(#b)([ 	\\
} >  ])/\\$match[1]} )' 2>/dev/null
} 
} I just noticed that expansion was broken when I tried to expand *\~
} (exiftran backs up files with a trailing ~ and I wanted to clean them up)
} git bisect led me to 33ff5d06 which is the patch above.
} 
} What's not clear to me is what problem this patch was trying to solve.

Look at workers/32186 which refers back to users/18247 and its thread.

It has to do with glob-expansion of file names that contain a character
that needs to be protected with a backslash, particularly file names
actually containing backslashes and/or quote marks.

(In other words, if you had attempted to complete "*~" without having
the backslash in there already, it would have worked and inserted the
backslash for you.)

I have no objection to backing out that change again, although it works
as expected when extendedglob is NOT set.

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Wed, 15 Apr 2015 08:13:48 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Apr 15, 10:03am, Peter Stephenson wrote:
> } 
> } Yes, indeed --- there's a funny internal special case for empty strings
> } that I never quite get my head around.
> } 
> } -    if (!*s) {
> } +    if (!*s || *s == Nularg) {
> 
> I wonder if (a) there are other overlooked cases like this and (b) if it
> would be useful to have a #define macro for (c == '\0' || c == Nularg).
> A quick grep doesn't find existing cases of that test other than this
> new one, but of course if the Nularg part were forgotten, it wouldn't.

Just looked and the typical tests for Nularg are:

- We get it as a single character so just ignore it because there's a '\0'
  next which the following code will handle normally;

- We look for Nularg in a string and skip over the character because
  ditto (we could have made the math.c cases follow this form);

- In a few cases we explicitly check to see if there was a '\0'
  after the Nularg --- possibly me being cautious because I don't really
  know what's going on; I think that's actually unnecessary because
  Nularg means the whole string is empty.

(- Not directly relevant but in prompt.c we look to see if there's a
Nularg we can backup over, which is probably really cool.)

pws

Re: broken parsing with $((`:`))

[Bart Schaefer]

On Apr 15,  4:37pm, Peter Stephenson wrote:
} Subject: Re: broken parsing with $((`:`))
}
} > } +    if (!*s || *s == Nularg) {
} > 
} 
} - We look for Nularg in a string and skip over the character because
}   ditto (we could have made the math.c cases follow this form);

Hmm.

Src/pattern.c:571:           * Empty metafied strings have an initial Nularg.

Maybe we NEED to have the math.c case follow this form.  *s == Nularg
is not the same as !*s if there can be something non-null as s[1].

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Wed, 15 Apr 2015 09:13:35 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Hmm.
> 
> Src/pattern.c:571:           * Empty metafied strings have an initial Nularg.
> 
> Maybe we NEED to have the math.c case follow this form.  *s == Nularg
> is not the same as !*s if there can be something non-null as s[1].

I think "empty" means there *is* nothing after the Nularg.

But I may update math.c just to be like the other cases anyway; I won't
bother posting.

pws