Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not move unrelated files with same prefix #665

Merged
merged 5 commits into from Nov 26, 2020

Conversation

meisenring
Copy link
Contributor

My attempt to fix #664

I'm not very familiar with expected behavior in unix file systems but I guess it covers those as well?

Apologies if this is prematurely.

@@ -236,10 +236,12 @@ public void MoveDirectory(string sourcePath, string destPath)
sourcePath = FixPath(sourcePath);
destPath = FixPath(destPath);

var sourcePathSequence = sourcePath.Split(new[] {Path.DirectorySeparatorChar}, StringSplitOptions.RemoveEmptyEntries);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also split on Path.AltDirectorySeparatorChar ?
E.g. the underlying question is, what happens when you do Directory.Move("c:/foo/bar", "c:/baz");
If it is supported on the real filessystem the mockFilessystem should also support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FixPath on the lines above (236 and 237) ensures only Path.DirectorySeparatorChar is in the path (Path.AltDirectorySeparatorChar is replaced).

Should it split on Path.AltDirectorySeparatorChar anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I wasn't aware of that. In that case, it is not needed.
What threw me off is that I couldn't find any tests which ensure that paths with Path.AltDirectorySeparatorChar are actually handeled correctly, at least I couldn't find any when glancing over the code.
Pls don't take this the wrong way, this is not a request for you to add those tests!
It is more or less a question if those tests for Path.AltDirectorySeparatorChar exist at all, e.g. at a central place or how else it is ensured that this works

@fgreinacher
Copy link
Contributor

fgreinacher commented Nov 3, 2020

Thanks for your contribution! Changes look alright to me at a first look, happy to do a deeper review as soon as tests are passing.

@fgreinacher fgreinacher changed the title Fix #664 fix: do not move unrelated files with same prefix Nov 26, 2020
@fgreinacher fgreinacher merged commit 9885122 into TestableIO:main Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestingHelpers: Directory.Move operates on files with a related name
3 participants