Repository navigation
Stop globbing of quoted arguments to native commands - #5188
Conversation
Jason Shirk (lzybkr)
changed the title
Jason Shirk (lzybkr)
assigned
Dongbo Wang (daxian-dbw) and unassigned
Jason Shirk (lzybkr)
|
Dongbo Wang (@daxian-dbw) - I think this is ready for review. |
|
Must ... have ... this ... fix. :-) |
|
Jason Shirk (@lzybkr) I was out in training last Friday. Will get this done today. |
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.
Dongbo Wang (daxian-dbw)
left a comment
Dongbo Wang (daxian-dbw)
left a comment
There was a problem hiding this comment.
LGTM, but have one more comment about expanding ~.
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.
Dongbo Wang (@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.
There was a problem hiding this comment.
LGTM. Jason Shirk (@lzybkr) please feel free to merge the PR.
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.
|
🎉 |
4 participants
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