Skip to content

Conversation

@BrucePay
Copy link
Collaborator

@BrucePay BrucePay commented Aug 4, 2017

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).

@BrucePay BrucePay requested a review from vors August 4, 2017 00:19
@SteveL-MSFT SteveL-MSFT changed the title Brucepay wildcardquoting Fix globbing for native commands Aug 4, 2017
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@lzybkr
Copy link
Contributor

lzybkr commented Sep 7, 2017

@BrucePay - please rebase.

@daxian-dbw daxian-dbw removed their request for review September 11, 2017 18:06
@daxian-dbw daxian-dbw assigned lzybkr and unassigned daxian-dbw Sep 12, 2017
@lzybkr
Copy link
Contributor

lzybkr commented Sep 14, 2017

@BrucePay - please rebase, and look at the log before force pushing to ensure only your commits appear between master and your branch.

@BrucePay BrucePay force-pushed the brucepay-wildcardquoting branch 3 times, most recently from 4ee59c0 to 34f6760 Compare September 18, 2017 17:53
@BrucePay BrucePay force-pushed the brucepay-wildcardquoting branch from 34f6760 to a6e5a18 Compare September 18, 2017 19:34
@SteveL-MSFT
Copy link
Member

@lzybkr looks like the commits got cleaned up and the CI is passing, any other concerns?

@rkeithhill
Copy link
Collaborator

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.

@lzybkr
Copy link
Contributor

lzybkr commented Oct 22, 2017

Closing in favor of #5188.

@lzybkr lzybkr closed this Oct 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants