-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Unblock paths > MAX_PATH without extended syntax. #3001
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,38 @@ | |
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using System; | ||
using System.IO; | ||
using System.Runtime.InteropServices; | ||
using System.Text; | ||
|
||
partial class Interop | ||
{ | ||
partial class mincore | ||
{ | ||
/// <summary> | ||
/// WARNING: This overload does not implicitly handle long paths. | ||
/// </summary> | ||
[DllImport(Libraries.CoreFile_L1, SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
internal unsafe static extern int GetFullPathNameW(char* path, int numBufferChars, char* buffer, IntPtr mustBeZero); | ||
|
||
[DllImport(Libraries.CoreFile_L1, SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
internal static extern int GetFullPathNameW(string path, int numBufferChars, [Out]StringBuilder buffer, IntPtr mustBeZero); | ||
[DllImport(Libraries.CoreFile_L1, EntryPoint = "GetFullPathNameW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
private static extern int GetFullPathNameWPrivate(string path, int numBufferChars, [Out]StringBuilder buffer, IntPtr mustBeZero); | ||
|
||
internal static int GetFullPathNameW(string path, int numBufferChars, [Out]StringBuilder buffer, IntPtr mustBeZero) | ||
{ | ||
bool wasExtended = PathInternal.IsExtended(path); | ||
if (!wasExtended) | ||
{ | ||
path = PathInternal.AddExtendedPathPrefixForLongPaths(path); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we increase the size of buffer by a commensurate amount as we increased the length of path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I'll change that. Good catch. |
||
} | ||
int result = GetFullPathNameWPrivate(path, buffer.Capacity, buffer, mustBeZero); | ||
|
||
if (!wasExtended) | ||
{ | ||
// We don't want to give back \\?\ if we possibly added it ourselves | ||
PathInternal.RemoveExtendedPathPrefix(buffer); | ||
} | ||
return result; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,38 @@ | ||
// Copyright (c) Microsoft. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using System.IO; | ||
using System.Text; | ||
using System.Runtime.InteropServices; | ||
|
||
partial class Interop | ||
{ | ||
partial class mincore | ||
{ | ||
/// <summary> | ||
/// WARNING: This overload does not implicitly handle long paths. | ||
/// </summary> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto on using a different name. Do we expect any code other than the other overload to use these non-extending versions? Let's make them private if possible. |
||
[DllImport(Libraries.CoreFile_L1, SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
internal unsafe static extern int GetLongPathNameW(char* path, char* longPathBuffer, int bufferLength); | ||
|
||
[DllImport(Libraries.CoreFile_L1, SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
internal static extern int GetLongPathNameW(string path, [Out]StringBuilder longPathBuffer, int bufferLength); | ||
[DllImport(Libraries.CoreFile_L1, EntryPoint = "GetLongPathNameW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
private static extern int GetLongPathNameWPrivate(string path, [Out]StringBuilder longPathBuffer, int bufferLength); | ||
|
||
internal static int GetLongPathNameW(string path, [Out]StringBuilder longPathBuffer, int bufferLength) | ||
{ | ||
bool wasExtended = PathInternal.IsExtended(path); | ||
if (!wasExtended) | ||
{ | ||
path = PathInternal.AddExtendedPathPrefixForLongPaths(path); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the previous review, you said we couldn't change AddExtendedPathPrefixForLongPaths to AddExtendedPathPrefixIfNecessary because it would conflict with something in this next PR. I'm not seeing it. What's the conflict? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CreateDirectory is the one doing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spoke offline. Going to use EnsureExtendedPrefix/OverMaxPath in a follow-up. |
||
} | ||
int result = GetLongPathNameWPrivate(path, longPathBuffer, longPathBuffer.Capacity); | ||
|
||
if (!wasExtended) | ||
{ | ||
// We don't want to give back \\?\ if we possibly added it ourselves | ||
PathInternal.RemoveExtendedPathPrefix(longPathBuffer); | ||
} | ||
return result; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,13 +29,16 @@ public void AddExtendedPathPrefixTest(string path, string expected) | |
InlineData(@"\\?", false), | ||
InlineData(@"\\", false), | ||
InlineData(@"//", false), | ||
InlineData(@"\a", true), | ||
InlineData(@"/a", true), | ||
InlineData(@"\", true), | ||
InlineData(@"/", true), | ||
InlineData(@"C:Path", true), | ||
InlineData(@"C:\Path", false), | ||
InlineData(@"\\?\C:\Path", false), | ||
InlineData(@"Path", true), | ||
InlineData(@"X", true)] | ||
[PlatformSpecific(PlatformID.Windows)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an AnyUnix version of this test as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to implement the Unix version of the api first. |
||
public void IsPathRelative(string path, bool expected) | ||
{ | ||
Assert.Equal(expected, PathInternal.IsPathRelative(path)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,11 +127,11 @@ public void IncludeSubdirectories() | |
} | ||
|
||
[Fact] | ||
public void Path_Longer_Than_MaxPath_Throws_Exception() | ||
public void Path_Longer_Than_MaxLongPath_Throws_Exception() | ||
{ | ||
string testDir = GetTestFilePath(); | ||
Directory.CreateDirectory(testDir); | ||
Assert.All((IOInputs.GetPathsLongerThanMaxPath()), (path) => | ||
Assert.All((IOInputs.GetPathsLongerThanMaxLongPath()), (path) => | ||
{ | ||
Assert.Throws<PathTooLongException>(() => Move(testDir, path)); | ||
Assert.Throws<PathTooLongException>(() => Move(path, testDir)); | ||
|
@@ -144,14 +144,26 @@ public void Path_Longer_Than_MaxPath_Throws_Exception() | |
|
||
[Fact] | ||
[PlatformSpecific(PlatformID.Windows)] | ||
public void Path_With_Longer_Than_MaxDirectory_Throws_Exception() | ||
public void Path_With_Longer_Than_MaxDirectory_Succeeds() | ||
{ | ||
string testDir = GetTestFilePath(); | ||
Directory.CreateDirectory(testDir); | ||
Assert.True(Directory.Exists(testDir), "test directory should exist"); | ||
Assert.All((IOInputs.GetPathsLongerThanMaxDirectory()), (path) => | ||
{ | ||
Assert.Throws<PathTooLongException>(() => Move(testDir, path)); | ||
Assert.Throws<PathTooLongException>(() => Move(path, testDir)); | ||
string baseDestinationPath = Path.GetDirectoryName(path); | ||
if (!Directory.Exists(baseDestinationPath)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this check? Doesn't CreateDirectory already check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For troubleshooting when tests fail. If the prereq's aren't there you can rat hole into collateral fallout. |
||
{ | ||
Directory.CreateDirectory(baseDestinationPath); | ||
} | ||
Assert.True(Directory.Exists(baseDestinationPath), "base destination path should exist"); | ||
|
||
Move(testDir, path); | ||
Assert.False(Directory.Exists(testDir), "source directory should exist"); | ||
Assert.True(Directory.Exists(path), "destination directory should exist"); | ||
Move(path, testDir); | ||
Assert.False(Directory.Exists(path), "source directory should exist"); | ||
Assert.True(Directory.Exists(testDir), "destination directory should exist"); | ||
}); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename it then. It's strange to have two overloads so similar and yet with so different behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestions for naming? GetFullPathNameNoLongPath? This one in particular is used for the stack allocating code in PathHelper (all under MAX_PATH).
Like I mentioned, I intend to pass through these P/Invokes and normalize the naming. I want to drop the "W", use "Private" (anything better for that?), add comments on the privates to avoid leaking in other P/Invoke wrappers.