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
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -2013,6 +2013,9 @@ internal bool IsShellVirtualDrive(string driveName, out SessionStateScope scope)
// Check to see if the path is relative or absolute
bool isPathForCurrentDrive = false;

// Check to see if the path is to the root of a drive
bool isPathForRootOfDrive = false;

if (IsAbsolutePath(path, out driveName))
{
Dbg.Diagnostics.Assert(
@@ -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 (String.IsNullOrEmpty(path))
{
// path was to the root of a drive such as 'c:'
isPathForRootOfDrive = true;
}
}
}
}
@@ -2111,14 +2120,23 @@ internal bool IsShellVirtualDrive(string driveName, out SessionStateScope scope)
// have access to it.
context.Drive = workingDriveForPath;

string relativePath =
GenerateRelativePath(
workingDriveForPath,
path,
escapeCurrentLocation,
providerInstance,
context);
string relativePath = String.Empty;

if (isPathForRootOfDrive)
{
relativePath = context.Drive.Root;
}
else
{
relativePath =
GenerateRelativePath(
workingDriveForPath,
path,
escapeCurrentLocation,
providerInstance,
context);
}

return relativePath;
}
catch (PSNotSupportedException)
@@ -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!

try {
New-Item -Name $testfolder -Path "TestDrive:\" -ItemType directory > $null
Push-Location -Path "TestDrive:\$testfolder"
New-Item -Name $testfile -Path "TestDrive:\" -ItemType file > $null
$FullyQualifiedFile | Should -Exist
}
finally {
Pop-Location
}
}

It "Should create a folder at the root of the drive while the current working directory is not the root" {
$testfolder2 = "newDirectory2"
$FullyQualifiedFolder2 = Join-Path -Path $tmpDirectory -ChildPath $testfolder2

try {
New-Item -Name $testfolder -Path "TestDrive:\" -ItemType directory > $null
Push-Location -Path "TestDrive:\$testfolder"
New-Item -Name $testfolder2 -Path "TestDrive:\" -ItemType directory > $null
$FullyQualifiedFolder2 | Should -Exist
}
finally {
Pop-Location
}
}
}

# More precisely these tests require SeCreateSymbolicLinkPrivilege.
@@ -186,7 +213,7 @@ Describe "New-Item with links" -Tags @('CI', 'RequireAdminOnWindows') {
}

It "New-Item -ItemType SymbolicLink should understand directory path ending with slash" {
$folderName = [System.IO.Path]::GetRandomFileName()
$folderName = [System.IO.Path]::GetRandomFileName()
$symbolicLinkPath = New-Item -ItemType SymbolicLink -Path "$tmpDirectory/$folderName/" -Value "/bar/"
$symbolicLinkPath | Should -Not -BeNullOrEmpty
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.