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

[API Implementation]: DirectoryInfo.CopyTo / Directory.Copy #62375

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2e6ebb0
FileSystem Copy (directory) logic implementation
deeprobin Dec 3, 2021
0367de3
Usage of FileSystem.Copy in static class Directory
deeprobin Dec 3, 2021
3eb9974
Fix return type in Directory.Copy
deeprobin Dec 3, 2021
c019831
Add FileSystem.Copy implementation for DirectoryInfo.CopyTo
deeprobin Dec 3, 2021
05a335c
Ref-Assembly Implementation
deeprobin Dec 12, 2021
e2a24ca
Usage of Full Paths
deeprobin Dec 12, 2021
fe409d6
Add Tests for Directory.Copy
deeprobin Dec 12, 2021
d4ead0f
Add test cases for DirectoryInfo.CopyTo
deeprobin Dec 12, 2021
3021503
Add documentation comments
deeprobin Dec 12, 2021
815aca7
Merge branch 'main' into issue-60903
deeprobin Jan 23, 2022
0298d60
Update ref assembly
deeprobin Jan 23, 2022
5a38e66
Merge branch 'main' into issue-60903
deeprobin Jan 23, 2022
0a1820d
Fix ref assembly
deeprobin Jan 23, 2022
4d9da94
Fix ref assembly
deeprobin Jan 23, 2022
fad47d0
Style change
deeprobin Feb 2, 2022
20404b4
Address feedback from @bartonjs
deeprobin Feb 2, 2022
f430130
Fix tests
deeprobin Feb 2, 2022
4bf462f
Usage of `EnumerationOptions` overload for `EnumerateDirectories`
deeprobin Feb 4, 2022
cdca890
Merge branch 'main' into issue-60903
deeprobin Feb 5, 2022
d207673
Merge branch 'main' into issue-60903
deeprobin Feb 8, 2022
69e7a3b
Fix ref
deeprobin Feb 11, 2022
727997f
Remove `IgnoreInaccessible`
deeprobin Feb 11, 2022
ce89d2d
Remove overload with non-optional parameters
deeprobin Feb 11, 2022
7952a77
Fix mutation of EnumerationOptions instance
deeprobin Feb 12, 2022
42625af
Merge branch 'main' into issue-60903
deeprobin Feb 19, 2022
d8b32cc
Merge branch 'main' into issue-60903
deeprobin Mar 10, 2022
480ea54
Improve documentation
deeprobin Mar 10, 2022
518a734
Use FileSystemEnumerable
deeprobin Mar 10, 2022
6963cb0
Improve documentation
deeprobin Mar 11, 2022
d617da8
Remove redundant using
deeprobin Mar 11, 2022
04843bd
Use FileSystemEnumerable in Copy implementation
deeprobin Mar 11, 2022
08db652
Address feedback (attention: contains a failing test)
deeprobin Mar 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.IO.Enumeration;
using System.Threading;

namespace System.IO
{
Expand Down Expand Up @@ -350,5 +351,13 @@ public static FileSystemInfo CreateSymbolicLink(string path, string pathToTarget
FileSystem.VerifyValidPath(linkPath, nameof(linkPath));
return FileSystem.ResolveLinkTarget(linkPath, returnFinalTarget, isDirectory: true);
}

public static bool Copy(string sourcePath, string destinationPath, bool recursive, bool skipExistingFiles = true, CancellationToken cancellationToken = default)
{
string fullSourcePath = Path.GetFullPath(sourcePath);
string fullDestinationPath = Path.GetFullPath(destinationPath);

return FileSystem.Copy(sourcePath, destinationPath, recursive, skipExistingFiles, cancellationToken);
}
jozkee marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.IO.Enumeration;
using System.Threading;

namespace System.IO
{
Expand Down Expand Up @@ -233,5 +234,15 @@ public void Delete(bool recursive)
FileSystem.RemoveDirectory(FullPath, recursive);
Invalidate();
}

public bool CopyTo(string destinationPath, bool recursive, bool skipExistingFiles = true,
CancellationToken cancellationToken = default)
Comment on lines +233 to +234
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
public bool CopyTo(string destinationPath, bool recursive, bool skipExistingFiles = true,
CancellationToken cancellationToken = default)
public bool CopyTo(string destinationPath, bool recursive, bool skipExistingFiles = true, CancellationToken cancellationToken = default)

{
string fullSourcePath = FullPath;
string fullDestinationPath = Path.GetFullPath(destinationPath);

return FileSystem.Copy(fullSourcePath, fullDestinationPath, recursive, skipExistingFiles,
cancellationToken);
Comment on lines +239 to +240
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
return FileSystem.Copy(fullSourcePath, fullDestinationPath, recursive, skipExistingFiles,
cancellationToken);
return FileSystem.Copy(fullSourcePath, fullDestinationPath, recursive, skipExistingFiles, cancellationToken);

}
}
jozkee marked this conversation as resolved.
Show resolved Hide resolved
}
54 changes: 54 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
deeprobin marked this conversation as resolved.
Show resolved Hide resolved
using System.Collections.Generic;
using System.Threading;

namespace System.IO
{
internal static partial class FileSystem
Expand All @@ -20,5 +24,55 @@ internal static void VerifyValidPath(string path, string argName)
throw new ArgumentException(SR.Argument_InvalidPathChars, argName);
}
}

internal static bool Copy(string sourcePath, string destinationPath, bool recursive,
Copy link
Member

Choose a reason for hiding this comment

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

nit: rename this method to CopyDirectory, and add full int the argument names to indicate the caller must provide the method with full paths.

bool skipExistingFiles, CancellationToken cancellationToken)
{
var searchOption = recursive ? SearchOption.AllDirectories : SearchOption.TopDirectoryOnly;
IEnumerable<string> directoryEnumeration;

try
{
directoryEnumeration = Directory.EnumerateDirectories(sourcePath, "*", searchOption);
}
catch (IOException)
Copy link
Member

Choose a reason for hiding this comment

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

There's probably more to catch than IOExceptions here, isn't it? e.g DirectoryNotFoundException, UnauthorizedAccessException (see the Exceptions section in https://docs.microsoft.com/en-us/dotnet/api/system.io.directory.enumeratedirectories?view=net-6.0.).
Or maybe we should catch all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now updated this (See #62375 (comment)). I'm not sure if I have the right behavior for the exceptions though.

Can you take a look if there is anything that needs to be narrowed down.

{
// Return false if Enumeration is not possible
return false;
deeprobin marked this conversation as resolved.
Show resolved Hide resolved
deeprobin marked this conversation as resolved.
Show resolved Hide resolved
}

foreach (string enumeratedDirectory in directoryEnumeration)
{
string newDirectoryPath = enumeratedDirectory.Replace(sourcePath, destinationPath);
Directory.CreateDirectory(newDirectoryPath);

cancellationToken.ThrowIfCancellationRequested();
}

foreach (string enumeratedFile in Directory.GetFiles(sourcePath, "*.*", searchOption))
deeprobin marked this conversation as resolved.
Show resolved Hide resolved
{
if (skipExistingFiles && File.Exists(enumeratedFile))
{
cancellationToken.ThrowIfCancellationRequested();
continue;
}

try
{
CopyFile(enumeratedFile, enumeratedFile.Replace(sourcePath, destinationPath), true);
}
catch (IOException ex)
deeprobin marked this conversation as resolved.
Show resolved Hide resolved
{
// Return false for read failures
return ex.HResult == Interop.Errors.ERROR_ACCESS_DENIED ? false :
// Otherwise rethrow
throw ex;
deeprobin marked this conversation as resolved.
Show resolved Hide resolved
}

cancellationToken.ThrowIfCancellationRequested();
}

return false;
}
}
}