Set-Location should use path with wildcard characters if it exists instead of globbing #5839

SteveL-MSFT · 2018-01-10T01:50:15Z

PR Summary

When InitialSessionState initializes it tries to SetLocation to current working directory,
but if the directory name contains PowerShell wildcard characters, it fails and reverts
to $PSHOME.
The change affects Set-Location in that if the path exists (even if containing wildcard characters), just use it.

Fix #5752

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
[na] User facing Documentation needed
- Issue filed - Issue link:
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affects feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…nt working directory, but if the directory name contains PowerShell wildcard characters, it fails and reverts to $PSHOME. Fix is to suppress wildcard expansion in this case.

iSazonov · 2018-01-10T04:22:56Z

test/powershell/Host/Base-Directory.Tests.ps1

+
+    It "Can start in directory where name contains wildcard characters" {
+        Set-Location -LiteralPath $testPath.FullName
+        & $powershell -noprofile -c get-location | Should BeExactly $testPath.FullName


$powershell don't defined in the Describe block.

Will fix. Broke when I made this test it's own Describe.

lzybkr

The code change is surprising given the PR title. Based on the code, I would have thought that Set-Location was broken. Given that it's not, I'm guessing similar code exists elsewhere and either:
a) that code is not necessary
b) that code is subtly different than this fix

So I'd ask for a little more digging. You might also look for other ways this code is reachable - that might suggest the need for more tests.

SteveL-MSFT · 2018-01-10T18:47:37Z

@lzybkr Changed title a bit to clarify where the issue is. This internal SetLocation method doesn't differentiate between path and literalpath (it's always path). The *-Location cmdlets do end up using this same path but sets SuppressWildcardExpansion to true when -LiteralPath is used.

lzybkr · 2018-01-10T19:10:16Z

With your change - I think Set-Location -Path has a change in behavior. Consider:

mkdir aa
mkdir ba
mkdir [ab]a
Set-Location [ab]a

Before your change, this reports an error, after it works without error.

I think this is desirable - it almost seems like Set-Location never should have had the -LiteralPath parameter in the first place, but I've only thought about this for like a second.

SteveL-MSFT · 2018-01-10T21:23:32Z

@lzybkr you are correct regarding the behavior change in set-location -path and agree it's probably the right thing. I'll add breaking change although I don't think anyone will be impacted.

iSazonov · 2018-01-11T04:11:31Z

If [ab]a is allowed path I'd said that it is not a breaking change, but bug fix.
Also I'd add new explicit test for Set-Location -Path [ab]a.

SteveL-MSFT · 2018-01-11T07:22:55Z

After adding test for Set-Location, it didn't actually do what I thought it would. Apparently the check I had wasn't working as the path was relative and thus didn't "exist". Put the change in the LocationGlobber where the path is resolved to an absolute path first so now Set-Location works as discussed. Appreciate the feedback.

iSazonov · 2018-01-11T08:00:22Z

src/System.Management.Automation/namespaces/LocationGlobber.cs


-            if (!context.SuppressWildcardExpansion)
+            // if the directory contains wildcard characters, but exists, just return it
+            if (!context.SuppressWildcardExpansion && !Utils.NativeDirectoryExists(userPath))


Maybe the comment and logic should be "if the directory exists, just return it"? And wildcard characters should be a next check.

I'm leveraging falling into the else clause which is the same as if wildcard expansion is suppressed. Alternatively, I could rearrange the code so that the suppression/existing happens first which may make it a bit easier to read:

if (context.SuppressWildcardExpansion || Utils.NativeDirectoryExists(userPath)) ...

Need to change this code to handle exceptions during check

handle exceptions during directory exists check

iSazonov

LGTM

iSazonov · 2018-01-12T04:02:14Z

@lzybkr Could you please continue the review?

lzybkr

I sign off.

You should squash the commits and write a good commit message with an explanation of the breaking change so that appears in the git history.

iSazonov · 2018-01-13T08:45:46Z

@SteveL-MSFT Could you please edit PR header and description to address Jason request?

SteveL-MSFT · 2018-01-17T03:26:08Z

Done

KillyMXI · 2018-05-24T00:13:17Z

I'm a bit late to the party, but I hope it's OK now, while 6.1.0 still in preview.

Something with implemented fix still bothers me.
When started playing with v6.1.0-preview.2, I realized that Get-ChildItem is also affected by this change.
And everything else using wildcard expansion now has added conditional branch with try-catch (what kind of performance overhead it might have, btw?)

PS C:\ps_test> Get-ChildItem


    Directory: C:\ps_test


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       24.05.2018      2:03                [ab]a
d-----       24.05.2018      2:03                aa
d-----       24.05.2018      2:03                ba


PS C:\ps_test> Get-ChildItem [ab]a


    Directory: C:\ps_test


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       24.05.2018      2:03                [ab]a


PS C:\ps_test> Get-ChildItem [abc]a


    Directory: C:\ps_test


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       24.05.2018      2:03                aa
d-----       24.05.2018      2:03                ba

While this might even make it work as expected more times than before, it has an increased level of surprise. It might make issues caused by the use of -Path instead of -LiteralPath to show up less frequently but will make them harder to trace when they finally show up.
Using wildcards by default and literal path as an option was a major design mistake, imo. This patch makes a step back, but in a way that makes it less predictable. Is it OK to have more "magic"?

I also wonder why it was made in this way instead of just calling Set-Location with -LiteralPath when initializing a session? Working folder is by definition a literal path. It would've kept behavior changes to minimum.

SteveL-MSFT · 2018-05-24T01:20:09Z

@KillyMXI you bring up some good points, can you open a new issue to discuss this?

KillyMXI · 2018-05-24T02:11:38Z

Done #6927

Revert "Set-Location should use path with wildcard characters if it exists instead of globbing (PowerShell#5839)" This reverts commit 7459b54.

Revert "Set-Location should use path with wildcard characters if it exists instead of globbing (#5839)" This reverts commit 7459b54.

SteveL-MSFT assigned iSazonov Jan 10, 2018

SteveL-MSFT requested review from daxian-dbw and lzybkr January 10, 2018 01:50

SteveL-MSFT requested a review from BrucePay as a code owner January 10, 2018 01:50

When InitialSessionState initializes it tries to SetLocation to curre…

e7912fe

…nt working directory, but if the directory name contains PowerShell wildcard characters, it fails and reverts to $PSHOME. Fix is to suppress wildcard expansion in this case.

SteveL-MSFT force-pushed the pwsh-cwd-wildcards branch from 337ce57 to e7912fe Compare January 10, 2018 03:00

iSazonov reviewed Jan 10, 2018

View reviewed changes

fix test

9cfd19c

iSazonov approved these changes Jan 10, 2018

View reviewed changes

lzybkr reviewed Jan 10, 2018

View reviewed changes

SteveL-MSFT changed the title ~~Allow startup directory for runspace to contain wildcard characters~~ Allow startup directory for initialsessionsate to contain wildcard characters Jan 10, 2018

SteveL-MSFT added the Breaking-Change breaking change that may affect users label Jan 10, 2018

address feedback

38e4cf9

SteveL-MSFT requested a review from anmenaga as a code owner January 11, 2018 07:20

iSazonov reviewed Jan 11, 2018

View reviewed changes

[feature]

cc24a84

handle exceptions during directory exists check

SteveL-MSFT force-pushed the pwsh-cwd-wildcards branch from 2265cf6 to cc24a84 Compare January 12, 2018 00:21

iSazonov approved these changes Jan 12, 2018

View reviewed changes

lzybkr approved these changes Jan 12, 2018

View reviewed changes

SteveL-MSFT changed the title ~~Allow startup directory for initialsessionsate to contain wildcard characters~~ Set-Location should use path with wildcard characters if it exists instead of globbing Jan 17, 2018

iSazonov merged commit 7459b54 into PowerShell:master Jan 17, 2018

SteveL-MSFT mentioned this pull request Apr 29, 2018

Fix Set-Location to only use filesystem directory (if exists) only if using FileSystem provider #6654

Closed

11 tasks

SteveL-MSFT deleted the pwsh-cwd-wildcards branch May 24, 2018 01:19

KillyMXI mentioned this pull request May 24, 2018

Changes in wildcard expansion logic #6927

Closed

SteveL-MSFT added a commit to SteveL-MSFT/PowerShell that referenced this pull request Jun 25, 2018

[feature]

7533ba9

Revert "Set-Location should use path with wildcard characters if it exists instead of globbing (PowerShell#5839)" This reverts commit 7459b54.

iSazonov pushed a commit that referenced this pull request Jun 27, 2018

[feature] (#7101)

8829c01

Revert "Set-Location should use path with wildcard characters if it exists instead of globbing (#5839)" This reverts commit 7459b54.

Set-Location should use path with wildcard characters if it exists instead of globbing #5839

Set-Location should use path with wildcard characters if it exists instead of globbing #5839

Uh oh!

Conversation

SteveL-MSFT commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Jan 10, 2018

Uh oh!

lzybkr commented Jan 10, 2018

Uh oh!

SteveL-MSFT commented Jan 10, 2018

Uh oh!

iSazonov commented Jan 11, 2018

Uh oh!

SteveL-MSFT commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov Jan 11, 2018 • 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!

SteveL-MSFT Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jan 12, 2018

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jan 13, 2018

Uh oh!

SteveL-MSFT commented Jan 17, 2018

Uh oh!

KillyMXI commented May 24, 2018

Uh oh!

SteveL-MSFT commented May 24, 2018

Uh oh!

KillyMXI commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SteveL-MSFT commented Jan 10, 2018 •

edited

Loading

SteveL-MSFT commented Jan 11, 2018 •

edited

Loading

iSazonov Jan 11, 2018 •

edited

Loading