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

Do not Path.Join user directory if it's "C:" #94587

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Nov 9, 2023

Fixes #68503

On netfx we were concatenating user directories ending with volume separator : instead of joining with a path separator \. That is not the case in .NET [core]. This change brings that back as it is incorrect to join paths like "C:" with a separator in between the enumerated entries as the meaning of "relative to drive's CWD" gets lost.

@jozkee jozkee added this to the 9.0.0 milestone Nov 9, 2023
@jozkee jozkee self-assigned this Nov 9, 2023
@ghost
Copy link

ghost commented Nov 9, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #68503

On netfx we were special-casing joining user directories ending with volume separator. That is not the case in .NET [core]. This change brings that back as it is incorrect to join paths like "C:" with a separator in between the enumerated entries as the meaning of "relative to drive's CWD" gets lost.

Author: Jozkee
Assignees: Jozkee
Labels:

area-System.IO

Milestone: 9.0.0

Add ConditionalFact for RemoteExecutor
@jozkee jozkee added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 21, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 21, 2023
@ghost
Copy link

ghost commented Nov 21, 2023

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Overall it LGTM, I just need to make sure it's always a single letter. Thank you for fixing this bug @jozkee !

ReadOnlySpan<char> relativePath,
ReadOnlySpan<char> fileName)
{
if (originalRootDirectory.Length == 2 && originalRootDirectory[1] == Path.VolumeSeparatorChar)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for a Windows drive to have more than one letter?

Copy link
Member

Choose a reason for hiding this comment

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

Not as far as I know and I expect that would break assumptions elsewhere if it could.

Copy link
Member

Choose a reason for hiding this comment

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

When A-Z alphabets are all assigned, user either needs to switch to UNC paths via GUIDs \\?\Volume{GUID} to access the next device, or mount it as directory (via Disk management). That device would not show up in explorer by default.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@jozkee thank you!

@jozkee
Copy link
Member Author

jozkee commented Nov 27, 2023

CI errors are #95298.

@jozkee jozkee merged commit 1af3963 into dotnet:main Nov 27, 2023
172 of 176 checks passed
@jozkee jozkee deleted the getentries-cwd branch November 28, 2023 16:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
@jozkee
Copy link
Member Author

jozkee commented Jan 17, 2024

Breaking change doc. issue: dotnet/docs#39186

@jozkee jozkee removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Error .Net Core 5) "Directory.GetDirectories("D:")" returns not existingDirectory
4 participants