destructive list-expand

destructive list-expand

[Clint Adams]

If I type a command such as

echo ${(M)${(f)"$(<listofthings)"}:#*subthing*}

Then invoke list-expand, the appropriate expansion will be displayed,
and the command line will change to

echo ${(M)${(f)$(<listofthings)}:#*subthing*}

That is, the double quotes disappear, and I become very perplexed
and aggravated until I notice this.

Re: destructive list-expand

[Bart Schaefer]

On May 14, 10:30pm, Clint Adams wrote:
}
} If I type a command such as
} 
} echo ${(M)${(f)"$(<listofthings)"}:#*subthing*}
} 
} Then invoke list-expand, the appropriate expansion will be displayed,
} and the command line will change to
} 
} echo ${(M)${(f)$(<listofthings)}:#*subthing*}

Hmm; this is interesting.

list-expand (C-x g) doesn't use the new completion system, and exhibits
the above-described behavior at least as far back as 3.0.6 and probably
farther.

_list_expansions (C-x d) from _expand_word should be the new completion
system equivalent, but it doesn't appear to work at all. Even just plain
_expand_word (C-x e) beeps at me, but old-style expand-word properly
expands to everything listed by list-expand.

It's a bit hard to tell from the _complete_debug trace, but it looks as
if the double quotes have already been stripped from $PREFIX by the time
_expand is called -- e.g.

: _expand:24:else; word=${(M)${(f)$(<listofthings)}:#*subthing*} 

and then a bit later

: _expand:60:then; exp=: _expand:60:then cmdsubst; print -l CVS ChangeLog
ChangeLog-Release ChangeLog.3.0 Completion Config Doc Etc Functions INSTALL
LICENCE META-FAQ Makefile.in Misc README Src StartupFiles Test Util acconfig.h
aclocal.m4 aczsh.m4 config.guess config.h.in config.log config.sub configure
configure.in full.diff install-sh mkinstalldirs stamp-h.in
: _expand:60:then; exp=( ) 

I don't quite see where the quotes are getting stripped off, unless it's
actually get_comp_string() that's at fault.  (Somebody (hi, Sven) should
update the comment around line 1028 of zle_tricky.c in any event.)

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: destructive list-expand

[Bart Schaefer]

On May 15,  4:48am, Bart Schaefer wrote:
}
} : _expand:24:else; word=${(M)${(f)$(<listofthings)}:#*subthing*} 
} 
} and then a bit later
} 
} : _expand:60:then; exp=: _expand:60:then cmdsubst; print -l CVS ChangeLog

Oops, I cut bits from two different traces there.  The first bit should
have read:

: _expand:24:else; word=${(M)${(f)$(<=(print -l *))}:#*conf*}

} I don't quite see where the quotes are getting stripped off, unless it's
} actually get_comp_string() that's at fault.

Guess what ... around line 1376 of zle_tricky.c ... I'm at a loss for
what that code is meant to be doing or when it is ever the right thing.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: destructive list-expand

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> } I don't quite see where the quotes are getting stripped off, unless it's
> } actually get_comp_string() that's at fault.
> 
> Guess what ... around line 1376 of zle_tricky.c ... I'm at a loss for
> what that code is meant to be doing or when it is ever the right thing.

All this is so disgusting... sigh.

That code is there to turn turn null tokens inside parameter expansions
into their original forms (single or double quotes) so that later code
can use them correctly because they won't be removed in the following
loop.

Unfortunately, this will only be done if we are completing *inside* the
parameter expansion (the parameter name). I'll have to try if it is
correct to change that part of the code to re-install quotes anywhere in
a parameter expansion in the word (well, it sounds right, doesn't it?).

But even with that there's still something fishy with list-expand which
I haven't any further into yet. And with _expand and functions that call
it:

  beta% e=( '${(M)${(f)"$(<x)"}:#*2*}' )
  beta% echo ${(e)e}
   
  beta% echo ${(e)~e}
  111 222 333
  beta%

Oh well. Anyone interested in fixing that?


No patch yet...

Bye
  Sven


-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: destructive list-expand

[Sven Wischnowsky]

I wrote:

> ...
> 
> All this is so disgusting... sigh.

Agreed.

> That code is there to turn turn null tokens inside parameter expansions
> into their original forms (single or double quotes) so that later code
> can use them correctly because they won't be removed in the following
> loop.

The patch makes it keep all quotes *inside* parameter expansions when
*not* completing a parameter name.  For the simple things I tried, this
worked.  And for the case we were discussing, this gives the right
string to the shell code.

> ...
> 
> But even with that there's still something fishy with list-expand which
> I haven't any further into yet. And with _expand and functions that call
> it:
> 
>   beta% e=( '${(M)${(f)"$(<x)"}:#*2*}' )
>   beta% echo ${(e)e}
>    
>   beta% echo ${(e)~e}
>   111 222 333
>   beta%

This is the reason why, even with this patch, the test case still
doesn't work.

That mighty hunk in _expand is a fix for our brace expansion handling. 
Once I got the right string reported to the shell code I had to find out
that the code blindly expanded braces -- even those from a `${foo}'. 
But they were not expanded as a parameter expansion by that code, they
were expanded like a brace expansion (with braceccl set that gave me the
expansions `$f' and `$o').  There is now a loop that protects parameter
expansions from brace expansion (adding one more backslash before their
braces) -- can someone think of a better way?


Bye
  Sven

Index: Completion/Base/Completer/_expand
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Base/Completer/_expand,v
retrieving revision 1.6
diff -u -r1.6 _expand
--- Completion/Base/Completer/_expand	2001/05/09 12:06:10	1.6
+++ Completion/Base/Completer/_expand	2001/05/15 13:37:00
@@ -55,8 +55,31 @@
 
 if [[ "$force" = *s* ]] ||
    zstyle -T ":completion:${curcontext}:" substitute; then
-  [[ ! -o ignorebraces && "${#${exp}//[^\{]}" = "${#${exp}//[^\}]}" ]] &&
-      eval exp\=\( ${${(q)exp}:gs/\\{/\{/:gs/\\}/\}/} \) 2>/dev/null
+
+###  We once used this:
+###
+###  [[ ! -o ignorebraces && "${#${exp}//[^\{]}" = "${#${exp}//[^\}]}" ]] &&
+###      eval exp\=\( ${${(q)exp}:gs/\\{/\{/:gs/\\}/\}/} \) 2>/dev/null
+###
+###  instead of the following loop to expand braces.  But that made
+###  parameter expressions such as ${foo} be expanded like brace
+###  expansions, too (and with braceccl set...).
+
+   if [[ ! -o ignorebraces && "${#${exp}//[^\{]}" = "${#${exp}//[^\}]}" ]]; then
+     local otmp
+
+     tmp=${(q)word}
+     while [[ $#tmp != $#otmp ]]; do
+       otmp=$tmp
+       tmp=${tmp//(#b)\\\$\\\{(([^\{\}]|\\\\{|\\\\})#)([^\\])\\\}/\\$\\\\{${match[1]}${match[3]}\\\\}}
+     done
+     eval exp\=\( ${tmp:gs/\\{/\{/:gs/\\}/\}/} \) 2>/dev/null
+   fi
+
+###  There's a bug: spaces resulting from brace expansion are quoted in
+###  the following expression, too.  We don't want that, but I have no
+###  idea how to fix it.
+
   eval 'exp=( ${${(e)exp//\\[ 	
 ]/ }//(#b)([ 	
 ])/\\$match[1]} )' 2>/dev/null
Index: Src/Zle/zle_tricky.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_tricky.c,v
retrieving revision 1.25
diff -u -r1.25 zle_tricky.c
--- Src/Zle/zle_tricky.c	2001/05/08 08:14:34	1.25
+++ Src/Zle/zle_tricky.c	2001/05/15 13:37:01
@@ -1020,13 +1020,10 @@
      * the previously massaged command line using the lexer.  It stores *
      * each token in each command (commands being regarded, roughly, as *
      * being separated by tokens | & &! |& || &&).  The loop stops when *
-     * the end of the command containing the cursor is reached.  It's a *
-     * simple way to do things, but suffers from an inability to        *
-     * distinguish actual command arguments from, for example,          *
-     * filenames in redirections.  (But note that code elsewhere checks *
-     * if we are completing *in* a redirection.)  The only way to fix   *
-     * this would be to pass the command line through the parser too,   *
-     * and get the arguments that way.  Maybe in 3.1...                 */
+     * the end of the command containing the cursor is reached.  What   *
+     * makes this messy is checking for things like redirections, loops *
+    * and whatnot. */
+
     do {
 	lincmd = ((incmdpos && !ins && !incond) || (oins == 2 && i == 2) ||
 		  (ins == 3 && i == 1));
@@ -1343,6 +1340,19 @@
 		*p = '"';
 	    else if (*p == Snull)
 		*p = '\'';
+    } else {
+        int level = 0;
+
+        for (p = s; *p; p++) {
+            if (level && *p == Snull)
+                *p = '\'';
+            else if (level && *p == Dnull)
+                *p = '"';
+            else if (*p == String && p[1] == Inbrace)
+                level++;
+            else if (*p == Outbrace)
+                level--;
+        }
     }
     if ((*s == Snull || *s == Dnull) && !has_real_token(s + 1)) {
 	char *q = (*s == Snull ? "'" : "\""), *n = tricat(qipre, q, "");
@@ -1673,11 +1683,18 @@
 {
     int ret = 1, first = 1;
     LinkList vl;
-    char *ss;
+    char *ss, *ts;
 
     pushheap();
     vl = newlinklist();
     ss = dupstring(s);
+    /* get_comp_string() leaves these quotes unchanged when they are
+     * inside parameter expansions. */
+    for (ts = ss; *ts; ts++)
+        if (*ts == '"')
+            *ts = Dnull;
+        else if (*ts == '\'')
+            *ts = Snull;
     addlinknode(vl, ss);
     prefork(vl, 0);
     if (errflag)

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: destructive list-expand

[Peter Stephenson]

Sven wrote:
> The patch makes it keep all quotes *inside* parameter expansions when
> *not* completing a parameter name.  For the simple things I tried, this
> worked.  And for the case we were discussing, this gives the right
> string to the shell code.

Are we sure the zle_tricky.c change doesn't cause some knock-on problem,
e.g. in old-style completion?  Lots of people who install 4.0 for the first
time will still be using that to begin with.  I don't think most of us
have been testing this much, but under the present circumstances it's
important it still works at least as well as before, unfortunately.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: destructive list-expand

[Sven Wischnowsky]

Peter Stephenson wrote:

> Sven wrote:
> > The patch makes it keep all quotes *inside* parameter expansions when
> > *not* completing a parameter name.  For the simple things I tried, this
> > worked.  And for the case we were discussing, this gives the right
> > string to the shell code.
> 
> Are we sure the zle_tricky.c change doesn't cause some knock-on problem,
> e.g. in old-style completion? 

The change affects only completion of words with quotes in them and
that's something that has been changed before (I mean in 3.1.x).  I also
tried some of the cases with old-style completion (as with new-style)
and didn't find any immediate problems.

Of course, it's hard to `be sure' here.

Bye
  Sven


-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: destructive list-expand

[Bart Schaefer]

On May 15,  4:18pm, Sven Wischnowsky wrote:
} Subject: Re: destructive list-expand
}
} Peter Stephenson wrote:
} 
} > Sven wrote:
} > > The patch makes it keep all quotes *inside* parameter expansions when
} > > *not* completing a parameter name.  For the simple things I tried, this
} > > worked.  And for the case we were discussing, this gives the right
} > > string to the shell code.

This now gives the right thing for me with list-expand, but still fails
on _list_expansions (or _expand_word).  The command line isn't de-quoted
in either case, but _expand_word still just feeps at me.

schaefer<506> echo ${(M)${(f)"$(<=(print -l *))"}:#*conf*}<TAB>
No matches for `file' or `corrections'
schaefer<506> echo ${(M)${(f)"$(<=(print -l *))"}:#*conf*}<C-x g>
acconfig.h    config.h.in   config.sub    configure.in  
config.guess  config.log    configure     

Then there's this problem -- move the quotes outside the parameter:

schaefer<507> echo "${(M)${(f)$(<=(print -l *))}:#*conf*}"<C-x g>
[expansion elided]
schaefer<507> echo ${(M)${(f)$(<=(print -l *))}:#*conf*}"

Now the leading quote has been removed, but the trailing quote is there.
(They used to both disappear.)

} > Are we sure the zle_tricky.c change doesn't cause some knock-on problem,
} > e.g. in old-style completion? 

Obviously we're not.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: destructive list-expand

[Bart Schaefer]

On May 15,  2:41pm, Bart Schaefer wrote:
} Subject: Re: destructive list-expand
}
} schaefer<507> echo "${(M)${(f)$(<=(print -l *))}:#*conf*}"<C-x g>
} [expansion elided]
} schaefer<507> echo ${(M)${(f)$(<=(print -l *))}:#*conf*}"
} 
} Now the leading quote has been removed, but the trailing quote is there.
} (They used to both disappear.)

Just to be clear ... both of them disappearing is wrong too, of course.

Why is it ever correct to delete quotes _anywhere_?

Here's an even worse error:

schaefer<510> echo '${(@M)${(f)$(<=(print -l *))}:#*conf*}'<C-x g>
schaefer<510> echo ${(@M)${(f)$(<=(print -l *))}:#*conf*}

Now both *single* quotes are gone.  Bad news.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: destructive list-expand

[Sven Wischnowsky]

Bart Schaefer wrote:

> On May 15,  4:18pm, Sven Wischnowsky wrote:
> } Subject: Re: destructive list-expand
> }
> } Peter Stephenson wrote:
> } 
> } > Sven wrote:
> } > > The patch makes it keep all quotes *inside* parameter expansions when
> } > > *not* completing a parameter name.  For the simple things I tried, this
> } > > worked.  And for the case we were discussing, this gives the right
> } > > string to the shell code.
> 
> This now gives the right thing for me with list-expand, but still fails
> on _list_expansions (or _expand_word).  The command line isn't de-quoted
> in either case, but _expand_word still just feeps at me.
> 
> schaefer<506> echo ${(M)${(f)"$(<=(print -l *))"}:#*conf*}<TAB>
> No matches for `file' or `corrections'
> schaefer<506> echo ${(M)${(f)"$(<=(print -l *))"}:#*conf*}<C-x g>
> acconfig.h    config.h.in   config.sub    configure.in  
> config.guess  config.log    configure     

Yes, I pointed that out and the reason for it: _expand uses

      eval 'exp=( ${${(e)exp//\\[ 	
    ]/ }//(#b)([ 	
    ])/\\$match[1]} )' 2>/dev/null

But that doesn't work here:

    beta% echo ${(M)${(f)"$(<=(print -l *))"}:#*conf*}
    acconfig.h conf config.cache config.guess config.h config.h.in config.log config.moduls config.status config.sub configure configure.in sconf
    beta% foo='${(M)${(f)"$(<=(print -l *))"}:#*conf*}'
    beta% echo ${(e)foo}
 
    beta% echo ${(e)~foo}
    CVS ChangeLog ChangeLog-Release ChangeLog.3.0 ChangeLog~ Completion Config Doc Etc Functions INSTALL LICENCE META-FAQ Makefile Makefile.in Misc README Src StartupFiles Test Util a acconfig.h aclocal.m4 aczsh.m4 conf config.cache config.guess config.h config.h.in config.log config.modules config.status config.sub configure configure.in core install-sh mkinstalldirs sconf so_locations stamp-h stamp-h.in

I don't know if this is a bug (it looks like one, in a certain sense),
nor do I know how to fix this (at least without looking at the parameter
code again).

> Then there's this problem -- move the quotes outside the parameter:
> 
> schaefer<507> echo "${(M)${(f)$(<=(print -l *))}:#*conf*}"<C-x g>
> [expansion elided]
> schaefer<507> echo ${(M)${(f)$(<=(print -l *))}:#*conf*}"
> 
> Now the leading quote has been removed, but the trailing quote is there.
> (They used to both disappear.)

The real reason for this was that my change didn't look out for
Qstring's, only for String tokens, fix below.

> } > Are we sure the zle_tricky.c change doesn't cause some knock-on problem,
> } > e.g. in old-style completion? 
> 
> Obviously we're not.

Yes, but it was already broken before. Still, I offer to back out the
last patch and this one if you or Peter decide that you prefer it.


In another mail:

> On May 15,  2:41pm, Bart Schaefer wrote:
> } Subject: Re: destructive list-expand
> }
> } schaefer<507> echo "${(M)${(f)$(<=(print -l *))}:#*conf*}"<C-x g>
> } [expansion elided]
> } schaefer<507> echo ${(M)${(f)$(<=(print -l *))}:#*conf*}"
> } 
> } Now the leading quote has been removed, but the trailing quote is there.
> } (They used to both disappear.)
> 
> Just to be clear ... both of them disappearing is wrong too, of course.

Yes.

> Why is it ever correct to delete quotes _anywhere_?

Think about *completion*, not expansion. There we *need* to remove the
quotes at some time to be able to complete `"foo<TAB>' and things like
that. We had quite a bit of discussion about all this quoting stuff
which I don't want to revive. The result was that we want to try to keep
starting single and double quotes if possible and otherwise use what
could probably be called a normalised quoted form (backslashes instead
of single or double quotes). I had several very miserable days to at
least reach the state we have now.

> Here's an even worse error:
> 
> schaefer<510> echo '${(@M)${(f)$(<=(print -l *))}:#*conf*}'<C-x g>
> schaefer<510> echo ${(@M)${(f)$(<=(print -l *))}:#*conf*}
> 
> Now both *single* quotes are gone.  Bad news.

The patch below just blindly changes the command line back to the
original when expansion didn't change anything (because we were only
listing or because the word couldn't be expanded).


Anyway, here's the patch which hopefully fixes at least most problems.
The expansion stuff in _expand is not yet handled.


And all this is really ugly to work on. That's why I said I would like
to move more stuff into shell code, which might make this much easier to
fix.


Bye
  Sven

Index: Src/Zle/zle_tricky.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_tricky.c,v
retrieving revision 1.26
diff -u -r1.26 zle_tricky.c
--- Src/Zle/zle_tricky.c	2001/05/15 13:52:23	1.26
+++ Src/Zle/zle_tricky.c	2001/05/16 10:22:22
@@ -783,8 +783,20 @@
 			}
 		}
 		ret = docompletion(s, lst, lincmd);
-	    } else if (ret)
-		clearlist = 1;
+            } else {
+                if (ret)
+                    clearlist = 1;
+                if (!strcmp(ol, (char *)line)) {
+                    /* We may have removed some quotes. For completion, other
+                     * parts of the code re-install them, but for expansion
+                     * we have to do it here. */
+                    cs = 0;
+                    foredel(ll);
+                    spaceinline(origll);
+                    memcpy(line, origline, origll);
+                    cs = origcs;
+                }
+            }
 	} else
 	    /* Just do completion. */
 	    ret = docompletion(s, lst, lincmd);
@@ -1348,7 +1360,7 @@
                 *p = '\'';
             else if (level && *p == Dnull)
                 *p = '"';
-            else if (*p == String && p[1] == Inbrace)
+            else if ((*p == String || *p == Qstring) && p[1] == Inbrace)
                 level++;
             else if (*p == Outbrace)
                 level--;
@@ -1722,9 +1734,15 @@
 	goto end;
     }
     if (lst == COMP_LIST_EXPAND) {
-	/* Only the list of expansions was requested. */
-	ret = listlist(vl);
-	showinglist = 0;
+	/* Only the list of expansions was requested. Restore the 
+         * command line. */
+        cs = 0;
+        foredel(ll);
+        spaceinline(origll);
+        memcpy(line, origline, origll);
+        cs = origcs;
+        ret = listlist(vl);
+        showinglist = 0;
 	goto end;
     }
     /* Remove the current word and put the expansions there. */

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: destructive list-expand

[Peter Stephenson]

Sven wrote:
> > } > Are we sure the zle_tricky.c change doesn't cause some knock-on problem
> ,
> > } > e.g. in old-style completion? 
> > 
> > Obviously we're not.
> 
> Yes, but it was already broken before. Still, I offer to back out the
> last patch and this one if you or Peter decide that you prefer it.

I'm perfectly happy with the change, I'm just worried in case it tickles
something else.  If it doesn't affect anything other than quotes, then I'm
not concerned, since I realise that's always been hairy.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: destructive list-expand

[Sven Wischnowsky]

I wrote:

> ...
> 
> Yes, I pointed that out and the reason for it: _expand uses
> 
>       eval 'exp=( ${${(e)exp//\\[ 	
>     ]/ }//(#b)([ 	
>     ])/\\$match[1]} )' 2>/dev/null
> 
> But that doesn't work here:
> 
>     beta% echo ${(M)${(f)"$(<=(print -l *))"}:#*conf*}
>     acconfig.h conf config.cache config.guess config.h config.h.in config.log config.moduls config.status config.sub configure configure.in sconf
>     beta% foo='${(M)${(f)"$(<=(print -l *))"}:#*conf*}'
>     beta% echo ${(e)foo}
>  
>     beta% echo ${(e)~foo}
>     CVS ChangeLog ChangeLog-Release ChangeLog.3.0 ChangeLog~ Completion Config Doc Etc Functions INSTALL LICENCE META-FAQ Makefile Makefile.in Misc README Src StartupFiles Test Util a acconfig.h aclocal.m4 aczsh.m4 conf config.cache config.guess config.h config.h.in config.log config.modules config.status config.sub configure configure.in core install-sh mkinstalldirs sconf so_locations stamp-h stamp-h.in


Part of the reason may probably be fixed by this (not to be committed
until some knowledgeable person comments):

Index: Src/subst.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
retrieving revision 1.17
diff -u -r1.17 subst.c
--- Src/subst.c	2001/04/28 17:38:01	1.17
+++ Src/subst.c	2001/05/16 12:39:51
@@ -720,9 +720,13 @@
 
     if (!(err ? parsestr(s) : parsestrnoerr(s))) {
 	if (!single) {
+            int qt = 0;
+
 	    for (; *s; s++)
-		if (*s == Qstring)
+		if (!qt && *s == Qstring)
 		    *s = String;
+                else if (*s == Dnull)
+                    qt = !qt;
 	}
 	return 0;
     }


That loop is in subst_parse_str() and turns all Qstring tokens into
String, even the one inside those double quotes in the parameter
expression. That makes the inner substitution return a string instead
of an array.

The patch leaves all Qstring's inside Dnull's unchanged.

The other part of the problem (not tackled by the patch) is that the
pattern after `:#...' is not tokenized. In paramsubst() is some code
that tokenizes the pattern, but only if it things the whole parameter
expansion is inside quotes. But the result of a ${(e)...} is not
considered to be in double quotes exactly because of the loop above.
There must be some reason for this because, of course, we never ever
even think about doing things that are not needed. Ahem.

I'm not sure how to fix this. We don't want the string from a ${(e)...}
being completely tokenized (that's the ~-modifier's job), but having the
patterns used inside parameter expansions be tokenized would be the
right thing, I think, wouldn't it? But where and when exactly would that
have to happen? So that it doesn't interfere with whatever made us add
that loop above?


Bye
  Sven


-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Quoting and ${(e)param} (was Re: destructive list-expand)

[Bart Schaefer]

On May 16,  2:49pm, Sven Wischnowsky wrote:
} Subject: Re: destructive list-expand
}
} Part of the reason may probably be fixed by this (not to be committed
} until some knowledgeable person comments):
} 
} Index: Src/subst.c
} ===================================================================
} RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
} retrieving revision 1.17
} diff -u -r1.17 subst.c
} --- Src/subst.c	2001/04/28 17:38:01	1.17
} +++ Src/subst.c	2001/05/16 12:39:51
} @@ -720,9 +720,13 @@
}  
}      if (!(err ? parsestr(s) : parsestrnoerr(s))) {
}  	if (!single) {
} +            int qt = 0;
} +
}  	    for (; *s; s++)
} -		if (*s == Qstring)
} +		if (!qt && *s == Qstring)
}  		    *s = String;
} +                else if (*s == Dnull)
} +                    qt = !qt;
}  	}
}  	return 0;
}      }
} 
} 
} That loop is in subst_parse_str() and turns all Qstring tokens into
} String, even the one inside those double quotes in the parameter
} expression. That makes the inner substitution return a string instead
} of an array.
} 
} The patch leaves all Qstring's inside Dnull's unchanged.

Probably the Qstring should change to String when (err == 0), as in
that case parsestrnoerr() will not have complained about unmatched
quotes.  Or will that mean that there are no Qstring to begin with?
Hrm.
 
} The other part of the problem (not tackled by the patch) is that the
} pattern after `:#...' is not tokenized. In paramsubst() is some code
} that tokenizes the pattern, but only if it things the whole parameter
} expansion is inside quotes.

I believe that's because in the normal [non-(e)] case, the pattern will
already have been tokenized when the expansion is not in quotes, so it
would be redundant to tokenize it again.  I'm pretty far from sure about
this, though.

But, not tokenized where?  parsestr() untokenizes and re-tokenizes its
whole argument ... as if it were in double quotes ...

} But the result of a ${(e)...} is not
} considered to be in double quotes exactly because of the loop above.

Right, that loop must be attempting to undo the implicit double-quoting
from parsestr().  It just doesn't undo enough of it.

} I'm not sure how to fix this.

It looks to me as though the only "right" way is to create a new flavor
of parsestr() that parses as if NOT in quotes, and call the appropriate
one from subst_parse_str() (which means passing in `qt' and not just
`single', I suspect).

Does anyone remember anything else that might bear on this?  Peter?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Quoting and ${(e)param} (was Re: destructive list-expand)

[Sven Wischnowsky]

I probably should have said some more.

Actually, the current state really isn't that far away from the right
thing. The (e) flag should make only the $-expansions from the value be
done on the result. Because of that parsing the string as in double
quotes is the right thing. Using parse_subst_string() or adding the
`parsestr()-as-if-not-in-quotes' Bart suggested would make glob pattern
be expanded, too.

There are only two problems: we have to handle the case when the
expansion stored in the parameter value (the one we have to expand after
(e) has done its work) needs to expand to more than one word. In that
case we have to selectively `de-quote' some of the string -- and that's
what the loop we're discussing does. It just de-quotes too many
Qstring's, namely those inside quoted nested expansions. That would be
fixed by the patch I sent (and that makes me like that patch quite a bit).

The other problem is the tokenization of pattern *inside* parameter
expansions only (it isn't a problem if the parameter we are using the
(e) flag on contains $(..) or $((..)) expansions -- both of them take
care of tokenization themselves).

As Bart correctly pointed out (I knew that and probably should have
explained it some more), there are two things playing together.
subst_parse_string() tokenizes as if in double quotes (which, as I said
above, I think is correct), which also means that the patterns inside
parameter expansions are not tokenized. Normally if one does `"${x$*}"',
paramsubst() will take of that because the `$' is turned into a Qstring
token, so paramsubst() knows that the pattern isn't tokenized and does
it itself. But if that string comes from subst_parse_string(), the
Qstring will be turned into a String token, but the pattern will not be
tokenized -- and hence paramsubst() will not tokenize it itself.

Completely tokenizing the string resulting from a (e)ed parameter
expansion isn't an option, because that would also tokenize patterns
outside of parameter expansions -- we get globbing which we don't want
to have there, that's the job of `${~x}'. At least, it would be quite a
serious change if we modified this to do, e.g., globbing if the `${(e)x}'
isn't in quotes and only the other expansions if one uses `"${(e)x}"'.

So, subst_parse_string() has to do the de-quoting to be able to take
into account the way the expansion with the (e) was quoted or not.

I think all this could be solved if we find a way to tell the
paramsubst() doing the expansion in the value of the (e)ed parameter
that it has to tokenize the pattern even if there is a String token and
not a Qstring token. But currently we don't have a way to do that, or at
least none I can think of. It's problematic, because the first
paramsubst() just passes the resulting words back to it's calling
function which then re-examines them, expanding the now tokenized
expansions.

So the easiest solution would be to just make paramsubst() always
tokenize the pattern strings, not only if it knows that the parameter
expansion is in quotes. I was fearing that this might result in some
quoting problems (having to double backslashes or some such), but it
seems to work, including handling of parameter expansions inside
patterns.

Just so that everyone interested can easily play with it, I'll add a
patch below containing both changes (just #if'ed out the test).

The tests at least still work for me...

Bye
  Sven

Index: Src/subst.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
retrieving revision 1.17
diff -u -r1.17 subst.c
--- Src/subst.c	2001/04/28 17:38:01	1.17
+++ Src/subst.c	2001/05/17 09:03:40
@@ -720,9 +720,13 @@
 
     if (!(err ? parsestr(s) : parsestrnoerr(s))) {
 	if (!single) {
+            int qt = 0;
+
 	    for (; *s; s++)
-		if (*s == Qstring)
+		if (!qt && *s == Qstring)
 		    *s = String;
+                else if (*s == Dnull)
+                    qt = !qt;
 	}
 	return 0;
     }
@@ -1483,7 +1487,11 @@
 	case '#':
 	case Pound:
 	case '/':
+#if 0
 	    if (qt) {
+#else
+            {
+#endif
 		int one = noerrs, oef = errflag, haserr;
 
 		if (!quoteerr)

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Quoting and ${(e)param} (was Re: destructive list-expand)

[Bart Schaefer]

(Hello, it's nearly 3am here and I'm waiting for my Windows machine to
repair its disk ... if I leave it alone I can't answer the dialogs that it
throws up periodically, and not answering causes it to lock up and have to
be reset, which of course only makes matters worse ...)

On May 17, 11:03am, Sven Wischnowsky wrote:
}
} Actually, the current state really isn't that far away from the right
} thing. The (e) flag should make only the $-expansions from the value be
} done on the result. Because of that parsing the string as in double
} quotes is the right thing. Using parse_subst_string() or adding the
} `parsestr()-as-if-not-in-quotes' Bart suggested would make glob pattern
} be expanded, too.

OK, that's obviously not ideal ...

} So the easiest solution would be to just make paramsubst() always
} tokenize the pattern strings, not only if it knows that the parameter
} expansion is in quotes. I was fearing that this might result in some
} quoting problems (having to double backslashes or some such), but it
} seems to work, including handling of parameter expansions inside
} patterns.

Yes, it's pretty obvious that it should work:  parse_subst_str() does
an untokenize() before it lexes the string, so it won't matter if the
string is or is not already tokenized.  All that your patch is doing is
removing an optimization.

Consequently I think it would be OK to include it (possibly with the #if
replaced by an explanatory comment).  It'll slow down expansion of un-
quoted parameters slightly in the name of correctness, a tradeoff I made
several times in my subscript-parsing changes.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH: Re: Quoting and ${(e)param} (was Re: destructive list-expand)

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> } So the easiest solution would be to just make paramsubst() always
> } tokenize the pattern strings, not only if it knows that the parameter
> } expansion is in quotes. I was fearing that this might result in some
> } quoting problems (having to double backslashes or some such), but it
> } seems to work, including handling of parameter expansions inside
> } patterns.
> 
> Yes, it's pretty obvious that it should work:  parse_subst_str() does
> an untokenize() before it lexes the string, so it won't matter if the
> string is or is not already tokenized.  All that your patch is doing is
> removing an optimization.

Yes, I was hoping to be able somehow to keep that optimization. Sigh.

> Consequently I think it would be OK to include it (possibly with the #if
> replaced by an explanatory comment).  It'll slow down expansion of un-
> quoted parameters slightly in the name of correctness, a tradeoff I made
> several times in my subscript-parsing changes.

Ok, then, here's the final version of the patch.

Bye
  Sven

Index: Src/subst.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
retrieving revision 1.17
diff -u -r1.17 subst.c
--- Src/subst.c	2001/04/28 17:38:01	1.17
+++ Src/subst.c	2001/05/18 12:36:31
@@ -720,9 +720,13 @@
 
     if (!(err ? parsestr(s) : parsestrnoerr(s))) {
 	if (!single) {
+            int qt = 0;
+
 	    for (; *s; s++)
-		if (*s == Qstring)
+		if (!qt && *s == Qstring)
 		    *s = String;
+                else if (*s == Dnull)
+                    qt = !qt;
 	}
 	return 0;
     }
@@ -1483,7 +1487,14 @@
 	case '#':
 	case Pound:
 	case '/':
-	    if (qt) {
+            /* This once was executed only `if (qt) ...'. But with that
+             * patterns in a expansion resulting from a ${(e)...} aren't
+             * tokenized even though this function thinks they are (it thinks
+             * they are because subst_parse_string() turns Qstring tokens
+             * into String tokens and for unquoted parameter expansions the
+             * lexer normally does tokenize patterns inside parameter
+             * expansions). */
+            {
 		int one = noerrs, oef = errflag, haserr;
 
 		if (!quoteerr)

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: destructive list-expand

[Bart Schaefer]

On May 16, 12:24pm, Sven Wischnowsky wrote:
} Subject: Re: destructive list-expand
}
} Bart Schaefer wrote:
} 
} > This now gives the right thing for me with list-expand, but still fails
} > on _list_expansions (or _expand_word).
} 
} Yes, I pointed that out and the reason for it: _expand uses
} 
}       eval 'exp=( ${${(e)exp//\\[ 	
}     ]/ }//(#b)([ 	
}     ])/\\$match[1]} )' 2>/dev/null

Could we change that to:

	eval "exp=( $exp )" 2>/dev/null
	eval 'exp=( ${$exp//\\[
	]/ }//(#b)([
	])/\\$match[1]} )' 2>/dev/null

Or is that going to break something else?  (It fixes the specific example
we've been using, but I haven't tried any other examples e.g. that really
do have carriage-returns or other backslashed-whitespace in the results.)

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Quoting and ${(e)param} (was Re: destructive list-expand)

[Bart Schaefer]

On May 16, 11:32pm, Peter Stephenson wrote:
} Subject: Re: Quoting and ${(e)param} (was Re: destructive list-expand)
}
} "Bart Schaefer" wrote:
} > On May 16,  2:49pm, Sven Wischnowsky wrote:
} > }  	    for (; *s; s++)
} > } -		if (*s == Qstring)
} > } +		if (!qt && *s == Qstring)
} > }  		    *s = String;
} > } +                else if (*s == Dnull)
} > } +                    qt = !qt;
} > 
} > Does anyone remember anything else that might bear on this?  Peter?
} 
} No, it wasn't me.  Unfortunately the CVS change is when I updated Akira's
} CVS archive with a whole pile of changes, so all I know is that it happened
} between 15th April 1999 and 6th April 2000.

I have this notation in my personal CVS repository for a revision that
covers the entire body of subst_parse_str():

Sven: 9763: Fix quoting behavior of ${(e)...} substitutions.

Looks like before that patch it used parse_subst_str() instead.  So it
may be that what we need is to call parsestr() when the outer expansion is
quoted, and parse_subst_str() when it is not?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net