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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from Nov 26, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34b193e
Fix #664
Oct 31, 2020
47b00e1
Merge branch 'main' into feature/Fix664
fgreinacher Nov 3, 2020
90749f8
Apply suggestions from code review
fgreinacher Nov 21, 2020
b959bcd
Merge branch 'main' into feature/Fix664
fgreinacher Nov 25, 2020
b171048
Merge branch 'main' into feature/Fix664
fgreinacher Nov 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/System.IO.Abstractions.TestingHelpers/MockFileSystem.cs
View file
Open in desktop
Expand Up @@ -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


lock (files)
{
var affectedPaths = files.Keys
.Where(p => StringOperations.StartsWith(p, sourcePath))
.Where(p => PathStartsWith(p, sourcePathSequence))
.ToList();

foreach (var path in affectedPaths)
Expand All @@ -249,6 +251,25 @@ public void MoveDirectory(string sourcePath, string destPath)
files.Remove(path);
}
}

bool PathStartsWith(string path, string[] minMatch)
{
var pathSequence = path.Split(new[] {Path.DirectorySeparatorChar}, StringSplitOptions.RemoveEmptyEntries);
fgreinacher marked this conversation as resolved.
Show resolved Hide resolved
if (pathSequence.Length < minMatch.Length)
{
return false;
}

for (var i = 0; i < minMatch.Length; i++)
{
if (!StringOperations.Equals(minMatch[i], pathSequence[i]))
{
return false;
}
}

return true;
}
}

public void RemoveFile(string path)
Expand Down
21 changes: 21 additions & 0 deletions tests/System.IO.Abstractions.TestingHelpers.Tests/MockDirectoryTests.cs
View file
Open in desktop
Expand Up @@ -1461,6 +1461,27 @@ public void MockDirectory_Move_ShouldMoveDirectoryWithReadOnlySubDirectory()
Assert.IsTrue(fileSystem.FileExists(destSubDirName));
}

[Test]
public void MockDirectory_Move_ShouldOnlyMoveDirAndFilesWithinDir()
{
// Arrange
var fileSystem = new MockFileSystem(new Dictionary<string, MockFileData>
{
{XFS.Path(@"c:\source\dummy"), new MockDirectoryData()},
{XFS.Path(@"c:\source\dummy\content.txt"), new MockFileData(new byte[] {0})},
{XFS.Path(@"c:\source\dummy.txt"), new MockFileData(new byte[] {0})},
{XFS.Path(@"c:\source\dummy2"), new MockDirectoryData()},
{XFS.Path(@"c:\destination"), new MockDirectoryData()},
});

// Act
fileSystem.Directory.Move(@"c:\source\dummy", @"c:\destination\dummy");
fgreinacher marked this conversation as resolved.
Show resolved Hide resolved

// Assert
Assert.That(fileSystem.FileExists(@"c:\source\dummy.txt"), Is.True);
fgreinacher marked this conversation as resolved.
Show resolved Hide resolved
Assert.That(fileSystem.Directory.Exists(@"c:\source\dummy2"), Is.True);
fgreinacher marked this conversation as resolved.
Show resolved Hide resolved
}

[Test]
public void MockDirectory_GetCurrentDirectory_ShouldReturnValueFromFileSystemConstructor()
{
Expand Down