Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

JeremyKuhne
Copy link
Member

Some paths are not creatable in Windows without special syntax. Notably paths with trailing spaces and periods. As GetFullPath() (and GetFullPathName()) trim these we would lose the correct file name as we passed the path around. With the enumeration changes we now populate FileInfo correctly- this change allows the other methods to work when wrapped around such a path.

Also remove some CAS related comments.

cc: @jkotas, @Anipik, @danmosemsft, @pjanotti

Some paths are not creatable in Windows without special syntax. Notably paths with trailing spaces and periods. As GetFullPath() (and GetFullPathName()) trim these we would lose the correct file name as we passed the path around. With the enumeration changes we now populate FileInfo correctly- this change allows the other methods to work when wrapped around such a path.
@JeremyKuhne JeremyKuhne added this to the 2.1.0 milestone Mar 7, 2018
@JeremyKuhne JeremyKuhne self-assigned this Mar 7, 2018
@danmoseley danmoseley requested a review from Anipik March 7, 2018 04:01
//
/// <summary>
/// Copies an existing file to a new file.
/// If <paramref name="overwrite"/> is false, and exception will be
Copy link
Member

Choose a reason for hiding this comment

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

typo and->an

//
// Your application must have Delete permission to the target file.
//
// or a file that is memory mapped.
Copy link
Member

Choose a reason for hiding this comment

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

Nit, "On NT" -> "On Windows" ?

}


// Tests if a file exists. The result is true if the file
Copy link
Member

Choose a reason for hiding this comment

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

If you feel like changing it, "if" -> "whether"

FileInfo[] files = directory.GetFiles();
Assert.Equal(2, files.Length);

FileInfo destination = new FileInfo(Path.Join(directory.FullName, GetTestFilePath()));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add "DeleteTestFilePath" to FileCleanupTestBase? Dispose can call it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you're suggesting here.

Copy link
Member

Choose a reason for hiding this comment

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

Disregard.

string fullPath = Path.GetFullPath(path);

FileSystem.DeleteFile(fullPath);
FileSystem.DeleteFile(Path.GetFullPath(path));
Copy link
Member

Choose a reason for hiding this comment

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

Do we always call GetFullPath immediately on paths passed in to IO API? This is presumably in case current directory changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we always call GetFullPath(). The current directory thing would impact things, and in some cases we need to know that the path is normalized. Historically I'm sure it was more about CAS and the preemptive checks we made in GFP than anything else.

Remove NotSupported and SecurityException from Exists as these are no longer thrown.
Add a test fix I hadn't staged correctly.
DirectoryInfo directory = Directory.CreateDirectory(GetTestFilePath());
string fileOne = Path.Join(directory.FullName, "Trailing space ");
string fileTwo = Path.Join(directory.FullName, "Trailing period.");
File.Create(@"\\?\" + fileOne).Dispose();
Copy link

Choose a reason for hiding this comment

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

why are we using dispose here ?

Copy link
Member

Choose a reason for hiding this comment

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

It gives you a Filestream.

Copy link
Member Author

Choose a reason for hiding this comment

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

To close the file handle right away so it doesn't stay locked past this point.

@JeremyKuhne
Copy link
Member Author

@dotnet-bot test NETFX x86 Release Build

Appears to have hung up running System.Net.Http.Functional.Tests

@danmoseley
Copy link
Member

@dotnet-bot test NETFX x86 Release Build please (hang now fixed)

@JeremyKuhne JeremyKuhne merged commit 41a2120 into dotnet:master Mar 7, 2018
@JeremyKuhne JeremyKuhne deleted the trimmedsupport branch March 7, 2018 18:14
caesar-chen pushed a commit to caesar-chen/corefx that referenced this pull request Mar 8, 2018
* Support trimmed paths in FileInfo

Some paths are not creatable in Windows without special syntax. Notably paths with trailing spaces and periods. As GetFullPath() (and GetFullPathName()) trim these we would lose the correct file name as we passed the path around. With the enumeration changes we now populate FileInfo correctly- this change allows the other methods to work when wrapped around such a path.

* Tweak comments.
Remove NotSupported and SecurityException from Exists as these are no longer thrown.
Add a test fix I hadn't staged correctly.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Support trimmed paths in FileInfo

Some paths are not creatable in Windows without special syntax. Notably paths with trailing spaces and periods. As GetFullPath() (and GetFullPathName()) trim these we would lose the correct file name as we passed the path around. With the enumeration changes we now populate FileInfo correctly- this change allows the other methods to work when wrapped around such a path.

* Tweak comments.
Remove NotSupported and SecurityException from Exists as these are no longer thrown.
Add a test fix I hadn't staged correctly.


Commit migrated from dotnet/corefx@41a2120
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants