Stop globbing of quoted arguments to native commands #5188
Conversation
lzybkr
force-pushed
the
glob_fix
branch
2 times, most recently
from
28e326e to
1534378
Compare
lzybkr
changed the title
lzybkr
force-pushed
the
glob_fix
branch
2 times, most recently
from
a3613a5 to
ad12184
Compare
|
@daxian-dbw - I think this is ready for review. |
|
Must ... have ... this ... fix. :-) |
|
@lzybkr I was out in training last Friday. Will get this done today. |
| foreach (var path in paths) | ||
| { | ||
| _arguments.Append(sep); | ||
| sep = " "; |
There was a problem hiding this comment.
Is sep = " " really needed here? It seems we just need var sep = " " right before the foreach
There was a problem hiding this comment.
Well, it's more tidy - it avoids putting 2 spaces between the previous token - either the command name or an argument.
There was a problem hiding this comment.
Thanks for clarifying #Closed.
| } | ||
| } | ||
| } | ||
| } while (list != null); | ||
| } | ||
| else |
There was a problem hiding this comment.
Now we will expand ~ even if arg is not a bare word. Do we want to expand ~ unconditionally?
There was a problem hiding this comment.
Another question: it seems to me that after this change we don't glob for variables. Is it intentional?
For example, before the change
PS> $var = '*.txt'
PS> /bin/echo $var
a.txt b.txt c.txt
with the change
PS> /bin/echo $var
*.txt
There was a problem hiding this comment.
Thanks for catching, I missed this when reviewing Bruce's changes, but you're right, we don't want to expand ~ unconditionally. I'll fix it.
There was a problem hiding this comment.
And I see bash also expands variables unless quoted, so we should probably match that behavior too.
There was a problem hiding this comment.
@daxian-dbw - updated. If you sign off, I'll merge - I want to squash the last 2 commits and rebase 2 commits instead squashing the 3 commits.
lzybkr
force-pushed
the
glob_fix
branch
2 times, most recently
from
dd4d8ad to
8871dd5
Compare
Previously we used IScriptPosition for context (e.g. error reporting) during parameter binding, but in some cases we want more information, so we'll use the Ast instead. This change just adds the Ast, it doesn't make explicit use of it.
Also fix some minor issues with exceptions being raised when resolving the path - falling back to no glob. Fix: PowerShell#3931 PowerShell#4971
|
🎉 |
Fix globbing so that quoted args are not expanded.
Also Fix some minor issues with exceptions being raised when resolving
the path - falling back to no glob.
The bulk of the change is in the first commit, which passes the
Astornullinstead of theIScriptExtentwhen constructing the parameters and arguments that are used by the parameter binder.Fix #3931,#4971