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

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   

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   

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

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