Implementation of GetFullPath(string path, string basePath) by Anipik · Pull Request #15579 · dotnet/coreclr

Anipik · 2017-12-19T21:13:46Z

Related Test PR- dotnet/corefx#25994
Issue:- https://github.com/dotnet/corefx/issues/25535

jkotas · 2017-12-20T01:05:39Z

src/mscorlib/shared/System/IO/Path.Unix.cs

+                throw new ArgumentNullException(nameof(basePath));
+
+            if (!IsPathFullyQualified(basePath))
+                throw new ArgumentException();


This needs error message.

jkotas · 2017-12-20T01:06:00Z

src/mscorlib/shared/System/IO/Path.Unix.cs


+        public static string GetFullPath(string path, string basePath)
+        {
+


Extra new line

jkotas · 2017-12-20T01:12:09Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+        /// Try to remove relative segments from the given path (without combining with a root).
+        /// </summary>
+        /// <param name="skip">Skip the specified number of characters before evaluating.</param>
+        private static string RemoveRelativeSegments(string path, int skip = 0)


This looks like a exact copy of a method from the Unix-specific. You should move it to Path.cs that is shared between Windows and Unix instead.

danmoseley · 2017-12-20T03:19:51Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            if (basePath == null)
+                throw new ArgumentNullException(nameof(basePath));
+
+            if (!IsPathFullyQualified(basePath))


Missing the string

danmoseley · 2017-12-20T03:20:36Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            {
+                return RemoveRelativeSegments(CombineNoChecks(GetPathRoot(basePath), path.Substring(1)));
+            }
+            else if(length >= 2 && PathInternal.IsValidDriveChar(path[0]) && path[1] == PathInternal.VolumeSeparatorChar && !PathInternal.IsDirectorySeparator(path[2]))


Spacing also length check is off by one

You must be missing a test case of length 2?

Anipik · 2017-12-22T22:17:16Z

@dotnet-bot test this please

danmoseley · 2017-12-27T21:35:05Z

I"ll let @pjanotti review this one

danmoseley · 2017-12-28T18:13:13Z

src/mscorlib/shared/System/IO/Path.cs

+                    }
+
+                    // Skip this character and the next if it's referring to the current directory,
+                    // e.g. "parent/./child" =? "parent/child"


typo =? should be =>

Anipik · 2018-01-02T23:34:50Z

@dotnet-bot test this please

Anipik · 2018-01-03T23:35:53Z

@pjanotti can you please review this PR ?

pjanotti

LGTM, minor thing will be the exceptions when basePath is null or not rooted, even if path is rooted.

If you decide to not do the suggestion related to RemoveRelativeSegments, please, open an issue to track it.

pjanotti · 2018-01-04T19:14:51Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            if (IsPathFullyQualified(path))
+                return GetFullPath(path);
+
+            if (basePath == null)


Any specific reason to not check for basePath == null prior to the statement above? In general I would prefer a more predictable API, i.e.: always throwing if basePath is null, instead of depending on the context.

The only reason I did that was that we were not using basePath if path is fullyQualified. should I reverse them ?

My personal preference is to reverse them and have a consistent behavior regarding the value of basePath not that it is ignored if the path is fully qualified. That said I may be in the minority, @danmosemsft and @jkotas any preference in this regard?

have a consistent behavior regarding the value of basePath not that it is ignored if the path is fully qualified

I agree.

pjanotti · 2018-01-04T22:39:22Z

src/mscorlib/shared/System/IO/Path.Unix.cs

-                StringBuilderCache.Release(sb);
-                return path;
-            }
+            return GetFullPath(CombineNoChecks(basePath, path));


CombineNoChecks is going to cause one allocation, if there are relative segments that will trigger more allocations. One possible way to avoid that is to create a version of RemoveRelativeSegments that takes multiple paths at once so the call to CombineNoChecks can be avoided.

That said perhaps it is better to address it in a separated PR.

pjanotti · 2018-01-08T16:20:59Z

src/mscorlib/shared/System/IO/Path.Unix.cs

-
-                sb.Append(c);
-            }
+            if (!IsPathFullyQualified(basePath))


@Anipik do this check before the IsPathFullyQualified(path) call this way we are ensuring consistent behavior regarding this parameter.

pjanotti · 2018-01-08T16:22:14Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            if (IsPathFullyQualified(path))
+                return GetFullPath(path);
+
+            if (!IsPathFullyQualified(basePath))


Move to be before IsPathFullyQualified(path)

pjanotti · 2018-01-09T00:03:50Z

It looks good to me.

JeremyKuhne · 2018-01-17T23:31:07Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+
+            int length = path.Length;
+
+            if (PathInternal.IsDevice(basePath))


if [](start = 12, length = 2)

You need to check the inputs for embedded nulls at this point and throw as above. We don't ever want to give back "c:\Foo\\0bar".

JeremyKuhne · 2018-01-17T23:39:07Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            }
+            else if (length >= 2 && PathInternal.IsValidDriveChar(path[0]) && path[1] == PathInternal.VolumeSeparatorChar)
+            {
+                if (length == 2 || !PathInternal.IsDirectorySeparator(path[2]))


This line should just be an assert, it should never not be true.

what if basepath = "c:\foo" and path = "d:\bar\tmp.." Wont then we will require this pathway ?

It would be fully qualified and shouldn't have gotten past the earlier check. @"d:bar\tmp" would get through, and that is our special case (drive relative).

JeremyKuhne · 2018-01-17T23:39:37Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+
+            if (PathInternal.IsDevice(basePath))
+            {
+                return RemoveRelativeSegments(CombineNoChecks(basePath, path), PathInternal.GetRootLength(basePath));


Add some comment lines to your cases- this stuff is hard to follow without them.

JeremyKuhne · 2018-01-17T23:41:08Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            {
+                if (length == 2 || !PathInternal.IsDirectorySeparator(path[2]))
+                {
+                    if (GetPathRoot(path) == GetPathRoot(basePath))


if (GetPathRoot(path) == GetPathRoot(basePath)) [](start = 20, length = 47)

This should use the span overloads to avoid the allocations.

JeremyKuhne · 2018-01-17T23:44:42Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+                {
+                    if (GetPathRoot(path) == GetPathRoot(basePath))
+                    {
+                        return RemoveRelativeSegments(CombineNoChecks(basePath, path.Substring(2)), basePath.Length);


CombineNoChecks [](start = 54, length = 15)

We should get rid of the extra allocations in our RemoveRelativeSegments impl (in all places). Use Span to prepare all of the inputs.

Sorry, I meant CombineNoChecks() should take Span.

In reply to: 162213527 [](ancestors = 162213527)

CombineNoChecks dont have a span implementation

It does in CoreFX. Need to port it and use it. :)

should I remove the internal string overload for combineNoChecks ?

The existing ones in Path can be replaced where the implementation is already done. You can create another issue to deal with the others (such as the array version). (And feel free to take the issue if you want.)

JeremyKuhne

🕐

Anipik · 2018-01-19T00:17:02Z

@JeremyKuhne can you take another look. I have added the span overloads but it wont be working until the other span PR dont get merged. Can you also review the tests PR also

JeremyKuhne · 2018-01-19T01:50:32Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+                throw new ArgumentException(SR.Arg_BasePathNotFullyQualified);
+
+            if (basePath.Contains('\0'))
+                throw new ArgumentException(SR.Arg_BasePathNotFullyQualified);


Should be InvalidPathChars || it with the other

JeremyKuhne · 2018-01-19T01:58:19Z

src/mscorlib/shared/System/IO/Path.cs

+        }
+
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]


I'm not really sure how the JIT will handle the inlining here. I'm a little worried that we end up with a ton of copies. I don't think they're necessary, I'd remove them.

JeremyKuhne · 2018-01-19T03:17:33Z

src/mscorlib/shared/System/IO/Path.Unix.cs

+        public static string GetFullPath(string path, string basePath)
        {
-            bool flippedSeparator = false;
+            if (basePath == null)


Sorry, missed that you aren't checking path for null here- should check that as well. Check it before basePath.

Can you review the tests for both the PR ?

Anipik · 2018-01-31T23:37:46Z

@dotnet-bot test this please

… done

danmoseley · 2018-02-01T04:30:40Z

@Anipik you'll want to make sure you add tests as needed to cover Jeremy's feedback above.

JeremyKuhne · 2018-02-01T18:51:56Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+                {
+                    // No matching root, root to specified drive
+                    // "D:Foo" and "C:\Bar" => "D:Foo"
+                    // "D:Foo" and "\\?\C:\Bar" => "\\?\C:\Bar"


Whoops. I wrote the comments wrong. Should be => "D:\Foo" and => "\\?\D:\Foo"

JeremyKuhne · 2018-02-01T18:57:33Z

src/mscorlib/shared/System/IO/Path.cs

-            if (path1.Length == 0)
-                return path2;
+            if (second.Length == 0)
+                return new string(first);


You should create a follow up issue to try and get rid of the new string's that you introduced in these helpers. Better to deal with it separately as they should be corner cases.

okay I will do that . should I open to change the implementation for the NormalizeDirectorypath to not use stringbuilder ?

Having issues to track pending work is never a bad thing. :)

JeremyKuhne

As stated, we should create a follow up issue to look for opportunities to remove more of the potential allocations.

This API is supposed to mimick giving you the same results you would get after setting the current directory and then calling GetFullPath(). You can create test data that you run in proc with the new API and then remote after setting the current directory and calling GetFullPath. I wouldn't block on getting those written, but I would definitely write them. Note that device path tests can't be tested via setting the current working directory. That should be the same set of local test data, sticking \?\ on the base path.

JeremyKuhne · 2018-02-02T19:10:15Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+                path.Slice(0, path.Length - 1) :
+                path;
+
+        internal static bool IsUnc(ReadOnlySpan<char> path) =>


UNCs are in three formats. \\Server\Share, \\?\UNC\Server\Share, or \\.\UNC\Server\Share. Anything that begins with \\ and is not followed by . or ? is a UNC. If it is followed by . or ? it needs to have at least \UNC\ to be a UNC. Technically a UNC also requires at least two segments (for the server and share) past the prefix to be valid but we don't care about that here.

JeremyKuhne · 2018-02-02T19:11:28Z

src/mscorlib/shared/System/IO/Path.Windows.cs

        /// <summary>Gets whether the system is case-sensitive.</summary>
        internal static bool IsCaseSensitive { get { return false; } }
+
+        internal static ReadOnlySpan<char> GetVolumeName(ReadOnlySpan<char> path)


Should add a comment as to what this returns.

JeremyKuhne · 2018-02-02T19:13:07Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            if (!PathInternal.IsDevice(path))
+                return TrimEndingDirectorySeparator(root); // e.g. "C:"
+
+            if (IsUnc(path))


Need if (IsUnc(path)) then a check for IsDevicePath() to handle both UNC cases.

JeremyKuhne · 2018-02-02T19:16:05Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+
+            // 3 cases: UNC ("\\server\share"), Device ("\\?\C:\"), or Dos ("C:\")
+            ReadOnlySpan<char> root = GetPathRoot(path);
+            if (!PathInternal.IsDevice(path))


You want server\share out of any type of UNC, in device syntax or not. This would return \\server\share.

You could, in theory, pass back \\server\share for all UNC types. Doing so, however, would require allocating in the device unc format. As directory separators aren't valid inside a name, it makes trimming it on both ends workable.

Anipik · 2018-02-02T22:55:36Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+        /// </summary>
        internal static ReadOnlySpan<char> GetVolumeName(ReadOnlySpan<char> path)
        {
            if (!IsPathFullyQualified(path))


@JeremyKuhne Can you explain why we are doing this check ?

Volume name has no meaning if you don't know where your path is. That said, there is a corner case that I forgot about, and that is drive relative. You know the volume definitively, even if it isn't qualified.

If the path starts with just \ or doesn't have a drive letter with : results from this would have no meaning. GetPathRoot() might be enough??

okay understood

JeremyKuhne · 2018-02-03T02:00:10Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+        /// </summary>
+        internal static ReadOnlySpan<char> GetVolumeName(ReadOnlySpan<char> path)
+        {
+            if (!IsPathFullyQualified(path))


As mentioned, this probably isn't correct for c:. Just calling GetPathRoot() and bailing if it is empty may give you the desired behavior.

@JeremyKuhne

if (!IsPathFullyQualified(path)) return getPathroot(path)

do you mean like this ?

No,

var root = GetPathRoot(path); if (root.Length == 0) return root;

Remove

if (!IsPathFullyQualified(path)) return getPathroot(path)

JeremyKuhne · 2018-02-03T02:02:30Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            if (!isDevice && StringSpanHelpers.Equals(path.Slice(0, 2), @"\\") )
+                return 2;
+            else if (isDevice && path.Length >= 8
+                && (StringSpanHelpers.Equals(path.Slice(0, 8), PathInternal.UncExtendedPathPrefix)


As you know it is a device, you can just slice and check for UNC\ in one step.

JeremyKuhne · 2018-02-03T02:02:56Z

src/mscorlib/shared/System/IO/Path.Windows.cs

+            {
+                return TrimEndingDirectorySeparator(root.Slice(4)); // Cut from "\\?\C:\" to "C:"
+            }
+            {


Why the block here?

…oreclr#15579) GetFullPathAPI Overload Commit migrated from dotnet/coreclr@2ede044

Anipik requested review from JeremyKuhne, danmoseley and jkotas December 19, 2017 21:14

Anipik mentioned this pull request Dec 19, 2017

Tests For GetFullPath(string path,string basePath) dotnet/corefx#25994

Closed

jkotas suggested changes Dec 20, 2017

View reviewed changes

danmoseley reviewed Dec 20, 2017

View reviewed changes

jkotas requested a review from pjanotti December 20, 2017 04:30

danmoseley reviewed Dec 28, 2017

View reviewed changes

jkotas added the area-System.IO label Dec 29, 2017

pjanotti approved these changes Jan 4, 2018

View reviewed changes

pjanotti reviewed Jan 8, 2018

View reviewed changes

JeremyKuhne reviewed Jan 17, 2018

View reviewed changes

JeremyKuhne suggested changes Jan 17, 2018

View reviewed changes

JeremyKuhne reviewed Jan 19, 2018

View reviewed changes

Anipik added 2 commits January 31, 2018 17:54

Get Full path new api

e666449

Path is specific drive completed and Half of device and Current drive…

280147f

… done

getfullpath used in case of removerelativesegments

8222016

JeremyKuhne reviewed Feb 1, 2018

View reviewed changes

JeremyKuhne approved these changes Feb 1, 2018

View reviewed changes

Anipik added 3 commits February 1, 2018 11:31

Comment corrected

2282b6d

matching root case corrected

d48b554

GetVolumeName Added

97f57ea

JeremyKuhne reviewed Feb 2, 2018

View reviewed changes

VolumeName corrected for UNC and Device Paths

672fb51

Anipik commented Feb 2, 2018

View reviewed changes

JeremyKuhne reviewed Feb 3, 2018

View reviewed changes

Anipik added 6 commits February 5, 2018 12:15

Minor changes

c96e708

Changed public to internal

0ce7125

Merge branch 'master' into FullPath

920a7a5

Data brace enclosed

c44efda

Remove IspathFullyQualified

ebb1271

public to internal

d3f013b

JeremyKuhne approved these changes Feb 5, 2018

View reviewed changes

Anipik merged commit 2ede044 into dotnet:master Feb 6, 2018

Anipik mentioned this pull request Feb 13, 2018

Tests For GetFullPath(string path,string basePath) dotnet/corefx#27085

Merged

Anipik deleted the FullPath branch March 24, 2018 01:35

peterhuene mentioned this pull request Aug 28, 2018

Expand relative paths for CLI arguments dotnet/cli#9892

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Implementation of GetFullPath(string path, string basePath) (dotnet/c…

61d0429

…oreclr#15579) GetFullPathAPI Overload Commit migrated from dotnet/coreclr@2ede044

AnakinRaW mentioned this pull request Jan 28, 2024

[Question] Case sensitivity for GetFullPath(string, string) with drive relative paths dotnet/runtime#97615

Closed


		public static string GetFullPath(string path, string basePath)
		{


		int length = path.Length;

		if (PathInternal.IsDevice(basePath))

Conversation

Anipik commented Dec 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Dec 22, 2017

Uh oh!

danmoseley commented Dec 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jan 2, 2018

Uh oh!

Anipik commented Jan 3, 2018

Uh oh!

pjanotti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjanotti commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne left a comment

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pjanotti left a comment •

edited

Loading

Anipik commented Jan 19, 2018 •

edited

Loading