Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Directory move fix #40570

Merged
merged 36 commits into from
Nov 6, 2019
Merged

Directory move fix #40570

merged 36 commits into from
Nov 6, 2019

Conversation

Jlalond
Copy link
Contributor

@Jlalond Jlalond commented Aug 25, 2019

Fix for #40013

Allowing the moving of directories with different casing.

Thus if in a case-sensitive file system renaming Directory .\foo to .\FOO should still be allowed.

But still preventing a directory having .\foo .\fOO .\FOO

As I am no expert on inter-opting with the OS I look forward to feedback

(And I went out on a bit of a limb with the design of this change)

@Jlalond
Copy link
Contributor Author

Jlalond commented Aug 26, 2019

@JeremyKuhne after digging through my logs, it looks like it's failing because when I expect the interop to return me FILE_ALREADY_EXISTS it silently continues or no-ops (Hard to tell through the lenses of a unit test). Do we want to build something platform specific for these *NIX builds, or is this potentially an issue with the interop code?

@JeremyKuhne
Copy link
Member

Do we want to build something platform specific for these *NIX builds, or is this potentially an issue with the interop code?

You'll have to investigate to see why it fails on Unix. As a general principle we want behavior to be identical.

@JeremyKuhne JeremyKuhne added this to the 5.0 milestone Aug 26, 2019
@Jlalond
Copy link
Contributor Author

Jlalond commented Aug 26, 2019

@JeremyKuhne on it.

@Jlalond
Copy link
Contributor Author

Jlalond commented Aug 30, 2019

@JeremyKuhne / @danmosemsft could I get a review on this?

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Mostly nits, but I believe there is some missing test coverage and a possible regression with root casing.

src/System.IO.FileSystem/src/System/IO/Directory.cs Outdated Show resolved Hide resolved
src/System.IO.FileSystem/src/System/IO/Directory.cs Outdated Show resolved Hide resolved
src/System.IO.FileSystem/src/System/IO/Directory.cs Outdated Show resolved Hide resolved
src/System.IO.FileSystem/src/System/IO/Directory.cs Outdated Show resolved Hide resolved
src/System.IO.FileSystem/tests/Directory/Move.cs Outdated Show resolved Hide resolved
@JeremyKuhne
Copy link
Member

cc: @carlossanlop

@Jlalond
Copy link
Contributor Author

Jlalond commented Sep 22, 2019

Hey @JeremyKuhne, Implemented your suggestions. I actually got really tired one night and commited moronic code, but serendipitously I think I found some odd behavior. On all our Linux builds moving foo into a directory with FOO works, except Debug builds for x64 and Release for Redhat. I found that pretty funky, is that expected behavior? (Also I should be good for another PR to destroy my confidence whenever you're ready :D )

@Jlalond
Copy link
Contributor Author

Jlalond commented Oct 17, 2019

@carlossanlop @JeremyKuhne I switched over to checking if the dir's were case variants, and deferring to the OS for the paths. Could I get a review, and sorry this PR seems to be going on forever, but this was much more nuanced than I originally realized.

@Jlalond
Copy link
Contributor Author

Jlalond commented Oct 29, 2019

@carlossanlop Does my rework make sense to you? At what point should I just give up 🤣

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Overall looks good. I pinged @JeremyKuhne to answer your question confirming the additional unit test. If that gets addressed, we can approve.

Edit: @Jlalond Jeremy answered your question: #40570 (comment)

@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your corefx repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants