Skip to content

Refactor repeated invalid character checks in Path.GetFullPath Unix and Windows - #56568

Merged
stephentoub merged 5 commits into
dotnet:mainfrom
steveberdy:sb-54993
Aug 6, 2021
Merged

Refactor repeated invalid character checks in Path.GetFullPath Unix and Windows#56568
stephentoub merged 5 commits into
dotnet:mainfrom
steveberdy:sb-54993

Conversation

@steveberdy

@steveberdysteveberdy commented Jul 29, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

There were repeated, expensive checks on paths in the Path.GetFullPath method. This has been refactored with a GetFullPathInternal method that contains the shared code between the methods. These repetitions were found on both the Windows and Unix sides, and thus implementations of the GetFullPathInternal method were written for both.

Intended to extend PR #55373

Fixes#54993

@ghostghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jul 29, 2021
@ghost

ghost commented Jul 29, 2021

Copy link
Copy Markdown

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

Issue Details

Summary

Where paths were checked to not be null, empty, or otherwise invalid, calls to Path.GetFullPath() were replaced with Path.GetFullyQualifiedPath(). A Unix implementation of GetFullyQualifiedPath was included as well.

Fixes #54993

Author:steveberdy
Assignees:-
Labels:

area-System.IO

Milestone:-

jkotas
jkotas reviewed Jul 30, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs Outdated
stephentoub
stephentoub reviewed Jul 30, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Directory.cs Outdated
@stephentoubstephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 30, 2021
@steveberdysteveberdy changed the title Replace qualified instances of Path.GetFullPath with Path.GetFullyQualifiedPathReplace non-empty path calls from Path.GetFullPath with Path.GetFullNonEmptyPathJul 31, 2021
jkotas
jkotas reviewed Jul 31, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs Outdated
jkotas
jkotas reviewed Jul 31, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
@karelzkarelz mentioned this pull request Jul 31, 2021
Closed
@steveberdysteveberdy reopened this Aug 2, 2021
@steveberdysteveberdy changed the title Replace non-empty path calls from Path.GetFullPath with Path.GetFullNonEmptyPathRefactored repeated invalid character checks in Path.GetFullPathAug 2, 2021
@steveberdysteveberdy changed the title Refactored repeated invalid character checks in Path.GetFullPathRefactor repeated invalid character checks in Path.GetFullPath Unix and WindowsAug 2, 2021
@jeffhandley

jeffhandley commented Aug 3, 2021

Copy link
Copy Markdown
Member

/azp run runtime

@azure-pipelines

azure-pipelinesBot commented Aug 3, 2021

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@steveberdy
steveberdy requested review from jkotas and stephentoub and removed request for jkotasAugust 3, 2021 13:25
jkotas
jkotas reviewed Aug 4, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
jkotas
jkotas reviewed Aug 4, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
jkotas
jkotas reviewed Aug 4, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
jkotas
jkotas reviewed Aug 4, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs
jkotas
jkotas reviewed Aug 4, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
jkotas
jkotas approved these changes Aug 4, 2021
View reviewed changes

@jkotasjkotas left a comment

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.

LGTM. Thank you!

@jkotas

jkotas commented Aug 5, 2021

Copy link
Copy Markdown
Member

@stephentoub Looks good?

stephentoub
stephentoub approved these changes Aug 5, 2021
View reviewed changes

@stephentoubstephentoub left a comment

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.

LGTM. Thanks.

@stephentoubstephentoub reopened this Aug 5, 2021
@stephentoub

stephentoub commented Aug 5, 2021

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

azure-pipelinesBot commented Aug 5, 2021

Copy link
Copy Markdown
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@stephentoub

stephentoub commented Aug 5, 2021

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

azure-pipelinesBot commented Aug 5, 2021

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@steveberdy

steveberdy commented Aug 6, 2021

Copy link
Copy Markdown
ContributorAuthor

Good to merge?

@stephentoub

stephentoub commented Aug 6, 2021

Copy link
Copy Markdown
Member

System.Net.Security test failure is unrelated.

@stephentoub
stephentoub merged commit 58efa4b into dotnet:mainAug 6, 2021
@steveberdy
steveberdy deleted the sb-54993 branch August 6, 2021 16:59
@adamsitnikadamsitnik removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 11, 2021
@adamsitnikadamsitnik added this to the 6.0.0 milestone Aug 11, 2021
This was referenced Aug 13, 2021
Closed
Closed
@ghostghost locked as resolved and limited conversation to collaborators Sep 10, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffhandleyjeffhandleyjeffhandley approved these changes
@stephentoubstephentoubstephentoub approved these changes
@jkotasjkotasjkotas approved these changes

Assignees

No one assigned

Labels

area-System.IOcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Duplicated path.Contains('\0') check

5 participants

@steveberdy@jeffhandley@jkotas@stephentoub@adamsitnik