FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Do not reject Windows' reserved device names on non-Windows platforms. by jeffbi · Pull Request #3252 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Do not reject Windows' reserved device names on non-Windows platforms. - #3252

Merged
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
jeffbi:reserved-device-names
Mar 7, 2017
ConversationCommits (2)ChecksFiles changed
Merged

Do not reject Windows' reserved device names on non-Windows platforms.#3252
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
jeffbi:reserved-device-names

Conversation

jeffbi commented Mar 3, 2017

Copy link
Copy Markdown

Fixes #3221

Steve Lee (SteveL-MSFT) requested changes Mar 5, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

Steve Lee (SteveL-MSFT) Mar 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is there a more specific check that it failed because of reserved name vs something else?

jeffbi Mar 6, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The current code throws an IOException with a localized string as its message, and uses "CopyError" ("MoveError"/"RenameError") in the error record.

We could modify the error ID in the error record to say something like "CopyToReservedNameError", with "MoveTo" and "RenameTo" versions, if that wouldn't badly impact existing user code.

Ilya (iSazonov) Mar 6, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is in common function PathIsReservedDeviceName
We could make new specific IOException but is it worth?

Steve Lee (SteveL-MSFT) Mar 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Probably not worth it then.

Dongbo Wang (daxian-dbw) reviewed Mar 5, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

Dongbo Wang (daxian-dbw) Mar 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

$tempFile seems not necessary in these tests. $testFile is created in BeforeEach and removed in AfterEach, so you can use testFile directly here.

jeffbi Mar 6, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For Move-Item and Rename-Item the $tempFile was to be able to restore $testFile each time through the foreach across device names. I can certainly use New-Item to create a new $testFile each iteration if you prefer.

Dongbo Wang (daxian-dbw) Mar 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, right. It's in a loop. Ignore my comment. #Closed

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

Dongbo Wang (daxian-dbw) Mar 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How about just Test-Path $deviceName | Should Be $true?

jeffbi Mar 6, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I was just keeping consistency with existing tests. I'm happy to make this change. Should I do so for the pre-existing tests as well?

Dongbo Wang (daxian-dbw) Mar 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I see. You can submit a new PR later to address that particular issue. #Closed

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

Dongbo Wang (daxian-dbw) Mar 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ditto here. #Closed

Ilya (iSazonov) reviewed Mar 5, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

Ilya (iSazonov) Mar 5, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I suppose that's enough:

Copy-Item -Path $testFile -Destination $deviceName -Force -ErrorAction SilentlyContinue
Test-Path $deviceName | Should Be $true

The same for tests below.

Dongbo Wang (daxian-dbw) Mar 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

jeffbi It would be great if you can address this comment, but it doesn't block merging this PR in case you want to address it later for all existing tests in this file. Please let me know.

jeffbi Mar 7, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've addressed Ilya (@iSazonov)'s comments here. I'll put off changing the previously existing tests.

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

Ilya (iSazonov) Mar 5, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe more specifically:

"Copy-Item succeeds on Unix with Windows reserved device names"

jeffbi Mar 6, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, I agree. I'll change that.

Ilya (iSazonov) reviewed Mar 7, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated
It "Move-Item on Unix succeeds with Windows reserved device names" -Skip:($IsWindows) {
foreach ($deviceName in $reservedNames)
{
Copy-Item -Path $testFile -Destination $tempFile -Force

Ilya (iSazonov) Mar 7, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe really use New-Item here? The code will be more clear.

Ilya (iSazonov) Mar 7, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#Closed.

Dongbo Wang (daxian-dbw) merged commit 97be759 into PowerShell:master Mar 7, 2017
jeffbi deleted the reserved-device-names branch March 7, 2017 21:12
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Do not reject Windows' reserved device names on non-Windows platforms. (
97879db 
PowerShell#3252)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Dongbo Wang (daxian-dbw) daxian-dbw left review comments
Ilya (iSazonov) iSazonov left review comments
Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL