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

deeprobin · 2021-12-03T23:53:34Z

Proposal implementation of #60903 (closes #60903)

Current state of implementation

Implementation
Ref Assembly
Documentation comments
Test Cases

ghost · 2021-12-03T23:53:41Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

See #60903

Current state of implementation

Implementation
Ref Assembly
Documentation comments
Test Cases

Author:	deeprobin
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs

deeprobin · 2022-01-12T06:12:24Z

Oh conflicts - I'll fix this the next days

Symbai · 2022-01-12T09:51:49Z

Oh conflicts - I'll fix this the next days

Maybe first ping the right people again to make sure the implementation is correct or not?! Because as long as its marked as draft, I doubt anyone from the .NET team looks at it.

deeprobin · 2022-01-12T10:02:23Z

Oh conflicts - I'll fix this the next days

Maybe first ping the right people again to make sure the implementation is correct or not?! Because as long as its marked as draft, I doubt anyone from the .NET team looks at it.

Yeah.
@bartonjs @adamsitnik @carlossanlop 😄

Symbai · 2022-01-21T08:27:49Z

@bartonjs Could you please say something about the implementation? The longer nobody of the .NET team takes a look the higher the chance everyone forgot was has been discussed and probably nobody except me wants to watch the review video. @deeprobin From all the pings we did not even a single one has reacted. Maybe doing you changes and removing the DRAFT helps, so this one at least needs to get reviewed.

deeprobin · 2022-01-21T08:36:34Z

@Symbai

Maybe doing you changes and removing the DRAFT helps, so this one at least needs to get reviewed.

I'll look into it this afternoon (CET) and then mark the PR as "Ready for Review".

deeprobin · 2022-01-23T14:29:10Z

@Symbai It took a little longer for me. I have now marked it as "Ready for Review".

It's quite possible that the tests are not passing. But I think that's not so bad (for now).

iSazonov · 2022-01-26T07:53:31Z

Why not use SHFileOperationW P/Invoke on Windows?
Perhaps there is something like on Unix too (libc?). /cc @tmds

Symbai · 2022-01-26T08:28:13Z

Why not use SHFileOperationW P/Invoke on Windows? Perhaps there is something like on Unix too (libc?). /cc @tmds

If you would have watched the video where the API got approved you would see that this WinAPI wouldn't fit into how this API should be implemented.

deeprobin · 2022-02-01T20:19:48Z

Due to health problems, I was in the hospital the last few days and could not continue working on this (and several other issues).

I hope that I still bring the tests to pass in the course of this week.

Co-Authored-By: David Cantú <16040868+Jozkee@users.noreply.github.com>

src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs

Jozkee · 2022-03-07T20:59:52Z

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs

+
+                directoryEnumeration = Directory.EnumerateDirectories(sourcePath, "*", enumerationOptions);
+            }
+            catch (IOException)


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?

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.

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs

danmoseley · 2022-03-09T20:24:27Z

@deeprobin it would be great to drain some of your several open PR's. could you take a look at feedback here?

deeprobin · 2022-03-09T21:06:21Z

@deeprobin it would be great to drain some of your several open PR's. could you take a look at feedback here?

As I wrote above, I will have a look at it the days. Currently I am still busy with a few changes to my nuint/nint Intrinsics overloads PR. But I think that I still manage to look at this PR this week.

The SafeFileHandle PR is completely finished from the implementation in my opinion. I have also requested a review from you there.
I think we can merge that first as soon as at least Jozkee and you have approved.

Co-Authored-By: David Cantú <16040868+Jozkee@users.noreply.github.com>

src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs

src/libraries/System.IO.FileSystem/tests/Directory/Copy.cs

Jozkee · 2022-03-14T19:51:53Z

src/libraries/System.IO.FileSystem/tests/Directory/Copy.cs

+
+            // False because we only want to copy top level files (= non-recursive)
+            Assert.False(File.Exists(destThirdLevelFilePath));
+        }


You need to add tests for read and write errors, and for the simpler Directory.Copy overload.

In my last commit, I added these. However, the two tests that check for it will fail.

I don't know exactly why but I guess it's still due to the problem that I don't know exactly how to clearly distinguish the read and write failures....

Maybe you can find another approach.

Feel free to cancel the CI, I'm sure it won't pass.

src/libraries/System.IO.FileSystem/tests/DirectoryInfo/CopyTo.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs

Co-Authored-By: Tom Deseyn <tom.deseyn@gmail.com> Co-Authored-By: David Cantú <16040868+jozkee@users.noreply.github.com>

tmds · 2022-03-16T09:43:53Z

src/libraries/System.Runtime/ref/System.Runtime.cs

@@ -10246,6 +10246,8 @@ public sealed partial class BufferedStream : System.IO.Stream
    }
    public static partial class Directory
    {
+        public static bool Copy(string sourcePath, string destinationPath, bool recursive) { throw null; }


@Jozkee @adamsitnik @bartonjs @danmoseley @stephentoub this API fails in two different ways: by returning false, or by throwing IOException.

Can the user act upon this returing false in some meaningful way?
The false hides more information which would be conveyed in an IOException, like what path failed to read and why.
Splitting the error cases also makes the implementation more complex.

Can we change the signature to void and always throw IOException?

@tmds: The API review video is still available, I recommend you watch it. The idea of returning false instead of throwing is/was that it should copy everything which can be copied and not immediately fail on the first exception. I believe this is a good decision, especially because when it fails even if we know what file/path has failed we still don't know what has been copied and what not. So the information you are looking for is only useful in a very limited way.

@Symbai it is an attempt to provide a best-effort copy where the user has no idea what has been copied and why the copy failed. That means it is hard to handle the false case.

I think it would be better to let the user roll their own best-effort copy and nail the semantics as they want them.

Most users for this API won't be looking for a best-effort copy, so this API forces them to deal with a false path they don't want.

Well it has been discussed in API review and this is how it got approved and so it should get implemented. As for the pro and cons and why (arguments) please watch the API review video. There were also example situations discussed where returning false makes more sense than throwing. Also please don't speak for "Most users", as I for once don't agree.

@Symbai I watched the API review. I think the bool-return got added under the false presumption it was needed to know everything was copied.
Also in the review it gets mentioned there shouldn't be a ton of options, and if you want more specific behavior, you can do it enumerate yourself.
This bool return value represents the option 'SkipReadErrors' which every user of this API is now forced to handle as a specific case.

If it returns false then now I have to do something more complicated

Do what? How do you know what went wrong and what to do differently next time / how to fix it so it doesn't happen again?

With no retry policy/error handler callback/etc, the method feels, to me, too trivial to be worth adding if it throws on the first failure (except for a few degenerate cases, like "your destination is an illegal path").

I agree with that. I also think it's not worth adding if it only does some of what you asked it to do and happily returns false with no additional information. If we want to throw an exception after it's completed copying whatever it can, that's fine; the API review discussed either throwing the first exception or the last exception or an aggregate... any of those seems better than returning false (though with an aggregate I'd worry about memory impact if lots of things in the copy failed).

Could we have a policy so we can satisfy several behaviors, including return false on errors and throw?
Something like:

public enum DirectoryCopyErrorPolicy { Warn, // Best effort to copy, but return false if something failed. Throw, // throws an aggregate exception. ThrowFirst, // throws the first exception it gets. }

I don't remember if we discussed this in API review.

Maybe the issue should go back to API review? And we should close this PR for now?

Or it should get implemented on how it was reviewed and improvements/changes are tracked in a separate issue. Wouldn't even be the first time.

I think this should get back to APi review.

tmds · 2022-03-16T09:51:52Z

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs

@@ -14,6 +18,58 @@ internal static void VerifyValidPath(string path, string argName)
            }
        }

+        internal static bool Copy(string sourcePath, string destinationPath, bool recursive,


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

Jozkee · 2022-04-15T21:19:07Z

Closing based on extraofficial API suggestions. Will bring the discussion back to the issue (#60903).
We can re-open this PR after we reach consensus there.

deeprobin added 4 commits December 4, 2021 00:40

FileSystem Copy (directory) logic implementation

2e6ebb0

Usage of FileSystem.Copy in static class Directory

0367de3

Fix return type in Directory.Copy

3eb9974

Add FileSystem.Copy implementation for DirectoryInfo.CopyTo

c019831

dotnet-issue-labeler bot added the area-System.IO label Dec 3, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 3, 2021

AraHaan reviewed Dec 4, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs Outdated Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs Outdated Show resolved Hide resolved

deeprobin changed the title ~~API proposal implementation: DirectoryInfo.Copy / Directory.Copy~~ API proposal implementation: DirectoryInfo.CopyTo / Directory.Copy Dec 12, 2021

deeprobin added 5 commits December 12, 2021 17:27

Ref-Assembly Implementation

05a335c

Usage of Full Paths

e2a24ca

Add Tests for Directory.Copy

fe409d6

Add test cases for DirectoryInfo.CopyTo

d4ead0f

Add documentation comments

3021503

deeprobin changed the title ~~API proposal implementation: DirectoryInfo.CopyTo / Directory.Copy~~ [API Implementation]: DirectoryInfo.CopyTo / Directory.Copy Jan 20, 2022

deeprobin added 5 commits January 23, 2022 10:29

Merge branch 'main' into issue-60903

815aca7

Update ref assembly

0298d60

Merge branch 'main' into issue-60903

5a38e66

Fix ref assembly

0a1820d

Fix ref assembly

4d9da94

deeprobin marked this pull request as ready for review January 23, 2022 14:28

deeprobin and others added 5 commits February 11, 2022 18:05

Fix ref

69e7a3b

Co-Authored-By: David Cantú <16040868+Jozkee@users.noreply.github.com>

Remove IgnoreInaccessible

727997f

Remove overload with non-optional parameters

ce89d2d

Fix mutation of EnumerationOptions instance

7952a77

Merge branch 'main' into issue-60903

42625af

Jozkee requested changes Mar 7, 2022

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Mar 7, 2022

deeprobin and others added 6 commits March 10, 2022 21:18

Merge branch 'main' into issue-60903

d8b32cc

Improve documentation

480ea54

Co-Authored-By: David Cantú <16040868+Jozkee@users.noreply.github.com>

Use FileSystemEnumerable

518a734

Improve documentation

6963cb0

Co-Authored-By: David Cantú <16040868+Jozkee@users.noreply.github.com>

Remove redundant using

d617da8

Use FileSystemEnumerable in Copy implementation

04843bd

Jozkee requested changes Mar 14, 2022

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 14, 2022

tmds reviewed Mar 15, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs Outdated Show resolved Hide resolved

tmds reviewed Mar 15, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs Outdated Show resolved Hide resolved

tmds reviewed Mar 15, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs Outdated Show resolved Hide resolved

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 15, 2022

Address feedback (attention: contains a failing test)

08db652

Co-Authored-By: Tom Deseyn <tom.deseyn@gmail.com> Co-Authored-By: David Cantú <16040868+jozkee@users.noreply.github.com>

tmds reviewed Mar 16, 2022

View reviewed changes

adamsitnik mentioned this pull request Mar 16, 2022

Relative Path and x-plat I/O papercuts #64890

Open

Jozkee closed this Apr 15, 2022

Jozkee mentioned this pull request Apr 15, 2022

[API Proposal]: DirectoryInfo.Copy / Directory.Copy #60903

Open

ghost locked as resolved and limited conversation to collaborators May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

deeprobin commented Dec 3, 2021 •

edited

Loading

ghost commented Dec 3, 2021

Current state of implementation

deeprobin commented Jan 12, 2022

Symbai commented Jan 12, 2022 •

edited

Loading

deeprobin commented Jan 12, 2022

Symbai commented Jan 21, 2022

deeprobin commented Jan 21, 2022 •

edited

Loading

deeprobin commented Jan 23, 2022

iSazonov commented Jan 26, 2022

Symbai commented Jan 26, 2022

deeprobin commented Feb 1, 2022

Jozkee Mar 7, 2022

deeprobin Mar 11, 2022

danmoseley commented Mar 9, 2022

deeprobin commented Mar 9, 2022

Jozkee Mar 14, 2022

deeprobin Mar 15, 2022

tmds Mar 16, 2022

Symbai Mar 16, 2022

tmds Mar 16, 2022

Symbai Mar 16, 2022

tmds Mar 16, 2022

stephentoub Mar 28, 2022 •

edited

Loading

Jozkee Apr 1, 2022

tmds Apr 14, 2022

Symbai Apr 14, 2022

deeprobin Apr 14, 2022

tmds Mar 16, 2022

Jozkee commented Apr 15, 2022

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

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

Conversation

deeprobin commented Dec 3, 2021 • edited Loading

Current state of implementation

ghost commented Dec 3, 2021

Current state of implementation

deeprobin commented Jan 12, 2022

Symbai commented Jan 12, 2022 • edited Loading

deeprobin commented Jan 12, 2022

Symbai commented Jan 21, 2022

deeprobin commented Jan 21, 2022 • edited Loading

deeprobin commented Jan 23, 2022

iSazonov commented Jan 26, 2022

Symbai commented Jan 26, 2022

deeprobin commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Mar 9, 2022

deeprobin commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jozkee commented Apr 15, 2022

deeprobin commented Dec 3, 2021 •

edited

Loading

Symbai commented Jan 12, 2022 •

edited

Loading

deeprobin commented Jan 21, 2022 •

edited

Loading

stephentoub Mar 28, 2022 •

edited

Loading