GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Stop globbing of quoted arguments to native commands by lzybkr · Pull Request #5188 · PowerShell/PowerShell

Conversation

Jason Shirk (lzybkr) commented Oct 21, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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 Ast or null instead of the IScriptExtent when constructing the parameters and arguments that are used by the parameter binder.

Fix #3931,#4971

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rkeithhill and vors reacted with thumbs up emoji

All reactions

👍 2 reactions

Jason Shirk (lzybkr) self-assigned this

Oct 21, 2017

Jason Shirk (lzybkr) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw) and sergei (vors) as code owners

October 21, 2017 02:31

Jason Shirk (lzybkr) mentioned this pull request

Oct 22, 2017

Fix globbing for native commands #4489

Closed

Jason Shirk (lzybkr) changed the title ~~Use Ast for context in parameter binding [testing - do not merge]~~ Stop globbing of quoted arguments to native commands

Oct 22, 2017

Jason Shirk (lzybkr) assigned Dongbo Wang (daxian-dbw) and unassigned Jason Shirk (lzybkr)

Oct 27, 2017

Jason Shirk (lzybkr) commented Oct 27, 2017

Copy link

Copy Markdown

Contributor Author

Dongbo Wang (@daxian-dbw) - I think this is ready for review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Keith Hill (rkeithhill) commented Oct 28, 2017

Copy link

Copy Markdown

Collaborator

Must ... have ... this ... fix. :-)

Happycoil reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Oct 30, 2017

Copy link

Copy Markdown

Member

Jason Shirk (@lzybkr) I was out in training last Friday. Will get this done today.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) previously requested changes

Oct 31, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/engine/NativeCommandParameterBinder.cs Outdated

Dongbo Wang (daxian-dbw) Oct 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sep = " " really needed here? It seems we just need var sep = " " right before the foreach

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's more tidy - it avoids putting 2 spaces between the previous token - either the command name or an argument.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Oct 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying #Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) mentioned this pull request

Oct 31, 2017

globbing for native commands is too agressive #3931

Closed

Dongbo Wang (daxian-dbw) reviewed

Oct 31, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but have one more comment about expanding ~.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/System.Management.Automation/engine/NativeCommandParameterBinder.cs Outdated

Dongbo Wang (daxian-dbw) Oct 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we will expand ~ even if arg is not a bare word. Do we want to expand ~ unconditionally?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Oct 31, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I see bash also expands variables unless quoted, so we should probably match that behavior too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Nov 1, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) dismissed their stale review

October 31, 2017 21:32

Question was answered.

Dongbo Wang (daxian-dbw) approved these changes

Nov 1, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Jason Shirk (@lzybkr) please feel free to merge the PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) added 2 commits

October 31, 2017 23:10


          Use Ast for context in parameter binding

7aa52e0

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.


          Glob native command args only when not quoted

2bcea1d

Also fix some minor issues with exceptions being raised when resolving
the path - falling back to no glob.

Fix: #3931 #4971

Jason Shirk (lzybkr) merged commit 6cbcf5d into PowerShell:master

Nov 1, 2017

Jason Shirk (lzybkr) deleted the glob_fix branch

November 1, 2017 06:34

Jason Shirk (lzybkr) mentioned this pull request

Nov 1, 2017

Native globbing on Unix platforms should pass syntactically-invalid-as-globbing-patterns unquoted literals through as-is #4971

Closed

sergei (vors) commented Nov 2, 2017

Copy link

Copy Markdown

Collaborator

🎉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Michael Klement (mklement0) mentioned this pull request

Oct 2, 2019

Need mechanism to disable globbing #5167

Closed

kzrnm (kzrnm) mentioned this pull request

Oct 17, 2025

Apply quoting for globbing * and ~ in splatting. #26232

Open

3 of 10 tasks

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

sergei (vors)

Awaiting requested review from vors

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Jason Shirk (lzybkr) commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jason Shirk (lzybkr) commented Oct 27, 2017

Uh oh!

Keith Hill (rkeithhill) commented Oct 28, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergei (vors) commented Nov 2, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jason Shirk (lzybkr) commented Oct 21, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) Oct 31, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.