-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix globbing for native commands #4489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Please add a test for something along the lines of echo '11:1' | grep '.*:.'. The : part is important too to avoid interpreting it as a drive latter
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.
This change only impacts expansion of wildcard characters. ':' isn't a wildcard character so I don't see a reason to add this test.
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.
I think the colon is an issue in the FileSystem Provider and should probably be in those tests
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.
Steve - I'm not sure i understand your comment. I'm not testing the file provider here. I'm just making sure that quoting suppresses globbing. Specific issues with the filesystem provider are orthogonal.
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.
Didn't notice that the test didn't involve cmdlets
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.
ast makes storing the extent no longer necessary, right?
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.
I mean - extent should always be ast.Extent, so we could remove the field and use a property instead.
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.
This API is used in many places where the AST is not available e.g. the PowerShell API, F&O. In those cases dummy extents are explicitly passed in. So both the Ast and extent members are required.
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.
We could introduce a dummy ast for the dummy position.
My thinking - if the normal case is a script, we optimize for that case. If the field is typically redundant, we use less memory, generate less code (the push of the extra parameter), and reduce the number of fields gc needs to visit.
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.
I think a dummy AST is overkill. Reworking the API to use dummy extents if the AST is null, etc. is probably the best solution but that's out of scope for this issue.
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.
Adding a new parameter will bloat the generated code significantly - this is an extra parameter for every argument on every command invocation. We should avoid that if there are alternatives.
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.
I'm still not happy with storing 2 references a typical script has a ton of these instances.
What if we use something like:
private object context; // Instead of storing Extent and Ast
// ...
internal Ast ParameterAst { return _parameter?.context as Ast; }
internal IScriptExtent ParameterExtent {
return _parameter?.context as IScriptExtent ?? (_parameter?.context as Ast)?.Extent;
}
// etc.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.
The same applies here regarding ast and extent.
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.
Same response as above - the API is used in places where there is no ast.
|
@BrucePay - please rebase. |
|
@BrucePay - please rebase, and look at the log before force pushing to ensure only your commits appear between master and your branch. |
4ee59c0 to
34f6760
Compare
34f6760 to
a6e5a18
Compare
|
@lzybkr looks like the commits got cleaned up and the CI is passing, any other concerns? |
|
Folks, we could REALLY use this fix! We have scripts that broke when we recently updated from an old alpha to beta.8. ATM we've found no workaround other than to fall back to Bash. |
|
Closing in favor of #5188. |
Fix for #3931. With this change, globbing for native commands will not longer be done if the argument string is quoted (single or double quotes).