Skip to content
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

Fix for files/folders not created when given path is drive root (#5228) #6600

Merged
merged 2 commits into from Apr 25, 2018

Conversation

@mcbobke
Copy link
Contributor

mcbobke commented Apr 9, 2018

PR Summary

If the Path argument to New-Item is the root of a drive such as C:\, the LocationGlobber attempts to generate a drive root relative path from the path C:. GetDriveRootRelativePathFromPSPath determines that this is an absolute path, and strips the drive name and colon from the path; originally, this would result in GenerateRelativePath determining that the current working directory should be given as the relative path. Now, a check is made on whether stripping the drive name and colon results in an empty string; if it does, the root of the current PSDrive context.Drive.Root is returned as the relative path.

This fixes #5228.

PR Checklist

@mcbobke mcbobke requested review from anmenaga and BrucePay as code owners Apr 9, 2018
@mcbobke

This comment has been minimized.

Copy link
Contributor Author

mcbobke commented Apr 9, 2018

@iSazonov new PR with branch workon5228.

@daxian-dbw daxian-dbw requested review from adityapatwardhan and removed request for BrucePay Apr 18, 2018
@daxian-dbw

This comment has been minimized.

Copy link
Member

daxian-dbw commented Apr 18, 2018

@adityapatwardhan and @anmenaga, can you please review this PR?

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
It "Should create a file at the root of the drive while the current working directory is not the root" {
$oldLocation = $pwd

New-Item -Name $testfolder -Path $tmpDirectory -ItemType directory

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Please redirect to null using > $null to avoid output of New-Item on console.

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated

New-Item -Name $testfolder -Path $tmpDirectory -ItemType directory
Set-Location -Path $tmpDirectory\$testfolder
New-Item -Name $testfile -Path $tmpDirectory -ItemType file

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Same as above.

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
New-Item -Name $testfolder -Path $tmpDirectory -ItemType directory
Set-Location -Path $tmpDirectory\$testfolder
New-Item -Name $testfile -Path $tmpDirectory -ItemType file
Test-Path $FullyQualifiedFile | Should -BeTrue

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Use Should -Exist instead

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
@@ -117,6 +117,31 @@ Describe "New-Item" -Tags "CI" {
$fileInfo.Target | Should -BeNullOrEmpty
$fileInfo.LinkType | Should -BeExactly "HardLink"
}

It "Should create a file at the root of the drive while the current working directory is not the root" {
$oldLocation = $pwd

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Please use Push-Location and Pop-Location to restore $pwd. Also, use a try .. finally pattern to ensure Pop-Location is applied even if the test fails. Currently if the assert for Should -True fails then Set-Location -Path $oldLocation is never called.

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated

It "Should create a folder at the root of the drive while the current working directory is not the root" {
$testfolder2 = "newDirectory2"
$oldLocation = $pwd

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Same as above.

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
New-Item -Name $testfolder -Path $tmpDirectory -ItemType directory
Set-Location -Path $tmpDirectory\$testfolder
New-Item -Name $testfolder2 -Path $tmpDirectory
Test-Path $FullyQualifiedFolder2 | Should -BeTrue

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

| Should -Exist

test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
Test-Path $FullyQualifiedFolder2 | Should -BeTrue

Set-Location -Path $oldLocation
Remove-Item $FullyQualifiedFolder2

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Do not need to do this. As it is on TestDrive:\. It is cleared automatically after the Describe is finished.

src/System.Management.Automation/namespaces/LocationGlobber.cs Outdated
@@ -2080,6 +2083,12 @@ internal bool IsShellVirtualDrive(string driveName, out SessionStateScope scope)
// this is the default behavior for all windows drives, and all non-filesystem
// drives on non-windows
path = path.Substring(driveName.Length + 1);

if (path == "")

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

Please use String.IsNullOrEmpty(path) instead.

src/System.Management.Automation/namespaces/LocationGlobber.cs Outdated
escapeCurrentLocation,
providerInstance,
context);
string relativePath = "";

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

use String.Empty

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

if (path == "")
{
// path is to the root of a drive such as "C:\" or "C:"

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Apr 18, 2018

Member

The path has to be like c: for this to happen not "c:". Please update comment.

@mcbobke mcbobke force-pushed the mcbobke:workon5228 branch 2 times, most recently Apr 18, 2018
@mcbobke

This comment has been minimized.

Copy link
Contributor Author

mcbobke commented Apr 18, 2018

Changes have been made.

@adityapatwardhan

This comment has been minimized.

Copy link
Member

adityapatwardhan commented Apr 18, 2018

This change can affect a lot of scenarios, please add the [Feature] tag to the last commit so additional tests can be run. https://github.com/PowerShell/PowerShell/blob/6ef94c1dfe912ce18f93c05ce5362bbce2284291/docs/testing-guidelines/testing-guidelines.md#requesting-additional-tests-for-a-pr

@mcbobke mcbobke force-pushed the mcbobke:workon5228 branch Apr 18, 2018
@@ -117,6 +117,33 @@ Describe "New-Item" -Tags "CI" {
$fileInfo.Target | Should -BeNullOrEmpty
$fileInfo.LinkType | Should -BeExactly "HardLink"
}

It "Should create a file at the root of the drive while the current working directory is not the root" {

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Apr 23, 2018

Member

The tests are wrong. They pass even without changes in this PR.

To repro this issue using "TestDrive:", you need to first Set-Location TestDrive:\, and then run New-Item in the context of the TestDrive:\. You cannot just use regular filesystem drive path to repro this.

Executing script .\testPR6600.ps1

  Describing New-Item with links
    [+] Should create a file at the root of the drive while the current working directory is not the root 397ms
    [+] Should create a folder at the root of the drive while the current working directory is not the root 80ms
Tests completed in 477ms
Tests Passed: 2, Failed: 0, Skipped: 0, Pending: 0, Inconclusive: 0

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Apr 23, 2018

Member

Please use [Feature] when adding commits to fix the feedback.

This comment has been minimized.

Copy link
@mcbobke

mcbobke Apr 24, 2018

Author Contributor

Given the previous feedback above, should I use Push-Location to move into the context of TestDrive:\ and then Pop-Location twice at the end of each test?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Apr 24, 2018

Member

Yes, I believe that's how you can repro the issue with TestDrive:\. When using $TestDirve, you are actually using the real filesystem path, which is not a root path at all.
After you write new tests, please make sure that they fail as expected without your fix, but works with your fix.

This comment has been minimized.

Copy link
@mcbobke

mcbobke Apr 24, 2018

Author Contributor

Thank you, I'll be more explicit about TestDrive:\ and will check with my new tests. I'll reply here when I confirm.

This comment has been minimized.

Copy link
@mcbobke

mcbobke Apr 24, 2018

Author Contributor
Executing script .\test\powershell\Modules\Microsoft.PowerShell.Management\New-Item.Tests.ps1
    Describing New-Item
    [-] Should create a file at the root of the drive while the current working directory is not the root 107ms
      Expected path 'C:\Users\mbobke\AppData\Local\Temp\42bafa09-bdae-4f1d-a18f-33def150c3a3\testfile.txt' to exist, but it did not exist.
      127:             $FullyQualifiedFile | Should -Exist
      at <ScriptBlock>, C:\Users\mbobke\Documents\GitKraken\PowerShell\test\powershell\Modules\Microsoft.PowerShell.Management\New-Item.Tests.ps1: line 127

Confirm
The item at
C:\Users\mbobke\AppData\Local\Temp\42bafa09-bdae-4f1d-a18f-33def150c3a3\newDirectory has
children and the Recurse parameter was not specified. If you continue, all children will be
 removed with the item. Are you sure you want to continue?
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"): a
    [-] Should create a folder at the root of the drive while the current working directory is not the root 15.96s
      Expected path 'C:\Users\mbobke\AppData\Local\Temp\42bafa09-bdae-4f1d-a18f-33def150c3a3      ewDirectory2' to exist, but it did not exist.
      144:             $FullyQualifiedFolder2 | Should -Exist
      at <ScriptBlock>, C:\Users\mbobke\Documents\GitKraken\PowerShell\test\powershell\Modules\Microsoft.PowerShell.Management\New-Item.Tests.ps1: line 144

Confirmed that these tests fail without my changes implemented. Waiting for CI/CD to finish with the changes.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Apr 24, 2018

Member

Thanks @mcbobke !
It would be easier to review if you push new commit instead of rebasing the existing one, so that we know where we were and only need to look at the new commits.

Also, please add the [Feature] tag to your commit message so that CI runs extra tests for the file system code change.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Apr 24, 2018

Member

I will add the [Feature] tag for you.

This comment has been minimized.

Copy link
@mcbobke

mcbobke Apr 24, 2018

Author Contributor

Understood, I was under the impression that I should try and keep it to a single commit at all times, noted for the future. Sorry about the confusion about the tag, I thought it needed to be in the first line of the body. Thanks for the help!

[Feature] Checks to see if the root of a PSDrive was given as the Path argument to a parameter that supports it. If so, returns the current context's Drive's Root.
@mcbobke mcbobke force-pushed the mcbobke:workon5228 branch to b101d52 Apr 24, 2018
@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Apr 25, 2018

LGTM.

@mcbobke Thanks for your contribution!

@daxian-dbw daxian-dbw merged commit 35d8de9 into PowerShell:master Apr 25, 2018
4 checks passed
4 checks passed
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@mcbobke

This comment has been minimized.

Copy link
Contributor Author

mcbobke commented Apr 26, 2018

Thank you all for the help and the merge!

rjmholt added a commit that referenced this pull request Apr 27, 2018
…and $PWD is a sub folder of the drive root. (#6600)"

This reverts commit 35d8de9.
rjmholt added a commit to rjmholt/PowerShell that referenced this pull request Apr 30, 2018
…and $PWD is a sub folder of the drive root. (PowerShell#6600)"

This reverts commit 35d8de9.
See issue PowerShell#6749.
daxian-dbw added a commit that referenced this pull request Apr 30, 2018
…or (#6751)

This reverts the commit 35d8de9, which caused a regression in drive behavior where drive paths were not restored. See issue #6749.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.