Update Copy-Item to copy directory attributes #13622

Joe-Zer0 · 2020-09-11T19:51:05Z

PR Summary

This change updates Copy-Item to also copy Directory attributes. There are changes to 3 distinct scenarios. Copying a folder locally, copying a folder to a remote session, and copying a folder from a remote session.

PR Context

#13552

PR Checklist

microsoft-cla · 2020-09-11T19:51:21Z

All CLA requirements met.

iSazonov · 2020-09-12T17:18:19Z

@Joe-Zer0 Please add in Summary all that is changed in the PR - this helps reviewers.
Also please add a test/tests for local scenario.

Joe-Zer0 · 2020-09-14T22:33:22Z

I added a unit test. But I made a mess of the commit's. I am new to rebaseing. I was attempting to add this change to the existing commit. I believe that is the desired action for this repository. I would appreciate a point in the right direction to clean them up. Thanks!

iSazonov · 2020-09-15T12:13:56Z

I would appreciate a point in the right direction to clean them up. Thanks!

You have no need to rebase until maintainers ask you. You should add new commits.

To clean you should revert the rebase. If you can not do this you can close the PR and restart with new branch and PR.

Joe-Zer0 · 2020-09-15T15:15:09Z

I have fix the git commit's to show 2 commit's: the original, and the addition of the unit test. I have also updated the summary to mention the 3 different scenarios that were updated by the changes.

test/powershell/Modules/Microsoft.PowerShell.Management/Copy-Item.Tests.ps1

Joe-Zer0 · 2020-09-18T13:13:12Z

Please let me know if there is anything else I need to be doing. Thanks!

src/System.Management.Automation/namespaces/FileSystemProvider.cs

test/powershell/Modules/Microsoft.PowerShell.Management/Copy-Item.Tests.ps1

Joe-Zer0 · 2020-09-21T12:32:37Z

I have made the requested changes. I was able to add a test for copying TO and FROM a remote session. To help me learn, can you explain why you wanted one parameter as a named parameter, but not the other one?

iSazonov · 2020-09-23T04:05:12Z

To help me learn, can you explain why you wanted one parameter as a named parameter, but not the other one?

Common practice in C# use self-descriptive names everywhere. If you use a constant like 123 a coder reader doesn't understand what is 123. It is better to define a named const and use it:

    const int DefaultDelay = 123;
    stopTime = startTime + DefaultDelay;

If we say about constants in arguments it is better to use names too as named arguments:

    Create(Capacity: 100);

So we get more readable code.

Joe-Zer0 · 2020-09-28T14:08:51Z

Please let me know what else needs to be done for this PR. Thanks!

PaulHigin

Added some comments, but my concern is if this new behavior might be a breaking change. Are there scenarios where setting directory attributes can fail?

PaulHigin · 2020-09-28T17:43:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -3730,6 +3732,7 @@ private void CopyItemFromRemoteSession(string path, string destinationPath, bool
                                    destinationPath,
                                    force,
                                    recurse,
+                                    ItemInfo,


We should pass in either the ItemInfo hash table itself or properties within the ItemInfo here, but not both.

yah, i'm also concerned that we're converting the attributes to strings and then back to attributes

PaulHigin · 2020-09-28T17:43:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -4048,6 +4052,7 @@ private void CopyFileInfoItem(FileInfo file, string destinationPath, bool force,
            string destination,
            bool force,
            bool recurse,
+            Hashtable srcDir,


Should be named 'srcDirInfo', or just pass in attributes (see above comment).

PaulHigin · 2020-09-28T17:43:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -4067,7 +4072,9 @@ private void CopyFileInfoItem(FileInfo file, string destinationPath, bool force,
            {
                // Create destinationPath directory. This will fail if the directory already exists
                // and Force is not selected.
-                CreateDirectory(destination, false);
+                var destDir = CreateDirectory(destination, streamOutput: true);
+                foreach(var attribute in ((string)srcDir["Attributes"]).Split(','))


Error handling? Probably should use 'TryParse' here.

PaulHigin · 2020-09-28T17:43:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                CreateDirectory(destination, false);
+                var destDir = CreateDirectory(destination, streamOutput: true);
+                foreach(var attribute in ((string)srcDir["Attributes"]).Split(','))
+                    destDir.Attributes |= Enum.Parse<System.IO.FileAttributes>(attribute.Trim());


Please add comment to clarify that this applies the attribute immediately to the newly created directory.

PaulHigin · 2020-09-28T17:43:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                CreateDirectory(destination, false);
+                var destDir = CreateDirectory(destination, streamOutput: true);
+                foreach(var attribute in ((string)srcDir["Attributes"]).Split(','))
+                    destDir.Attributes |= Enum.Parse<System.IO.FileAttributes>(attribute.Trim());


Error handling? Can this fail?

PaulHigin · 2020-09-28T17:43:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -9210,6 +9224,7 @@ function PSCreateDirectoryOnRemoteSession
                    if ($force)
                    {{
                        Microsoft.PowerShell.Management\New-Item $createDirectoryPath -ItemType Directory -Force | Out-Null
+                        (Get-Item $createDirectoryPath -Force).Attributes = $attributes


If dir path already exists, should new attributes be checked and applied?

I believe the intent of the Force flag is to complete the copy operation regardless of initial state. So the new attributes should be applied.

Joe-Zer0 · 2020-10-05T16:49:03Z

When copying an item from a session, I am no longer converting FileAttributes to a string. I was unable to pass FileAttributes through in the hashtable, I kept receiving "Copy-Item: Unable to cast object of type 'System.Management.Automation.PSObject' to type 'System.IO.FileAttributes'.". I was able to pass it through as byte however. This removes the string manipulation that was the concern for several of the comments. I also updated the parameters for CopyDirectoryFromRemoteSession to pass through the Hashtable. Please let me know what other changes need to be made. Thanks!

src/System.Management.Automation/namespaces/FileSystemProvider.cs

PaulHigin

Overall LGTM. I am still not sure how setting directory attributes can fail, and if it does whether that should be a terminating or non-terminating error. But since the committee is Ok with this going in to next preview version as 'experimental', I think it is Ok as is and we can change if needed.

iSazonov · 2020-10-08T06:11:31Z

@Joe-Zer0 Please make the feature experimental. You can find examples by "ExperimentalFeature" keyword in the repo.
In short:

add new feature in static ExperimentalFeature()
wrap new code with if (ExperimentalFeature.IsEnable("feature name"))
skip new tests if the new feature is disabled.

test/powershell/Modules/Microsoft.PowerShell.Management/Copy-Item.Tests.ps1

Joe-Zer0 · 2020-10-13T17:51:12Z

What are next steps?

iSazonov · 2020-10-14T04:08:17Z

What are next steps?

Waiting @daxian-dbw ...

msftbot · 2020-10-21T14:00:06Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Joe-Zer0 · 2020-11-09T19:27:55Z

Is there any update on this?

iSazonov · 2020-11-10T03:54:53Z

I hope MSFT team will review community PRs after PowerShell 7.1 release in weeks.

msftbot · 2020-11-17T14:00:07Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

daxian-dbw

The remote side could be an older version of PowerShell, so checking ExperimentalFeature.IsEnabled(...) on the remote side won't work.

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                    if (ExperimentalFeature.IsEnabled("PSCopyItemDirectoryAttributes")) {
+                        destDir.Attributes = directory.Attributes;
+                    }


Please follow the existing formatting in this file

Suggested change

if (ExperimentalFeature.IsEnabled("PSCopyItemDirectoryAttributes")) {

destDir.Attributes = directory.Attributes;

}

if (ExperimentalFeature.IsEnabled("PSCopyItemDirectoryAttributes"))

{

destDir.Attributes = directory.Attributes;

}

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                CreateDirectory(destination, false);
+                // Create destinationPath directory and apply attributes of source directory.
+                // This will fail if the directory already exists and Force is not selected.
+                var destDir = CreateDirectory(destination, streamOutput: true);


It was CreateDirectory(destination, false) before the change. Why did you change to true?

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                if (ExperimentalFeature.IsEnabled("PSCopyItemDirectoryAttributes")) {
+                    destDir.Attributes = sourceDirectoryAttributes;
+                }


Same here, please follow the existing formatting in this file.

Suggested change

if (ExperimentalFeature.IsEnabled("PSCopyItemDirectoryAttributes")) {

destDir.Attributes = sourceDirectoryAttributes;

}

if (ExperimentalFeature.IsEnabled("PSCopyItemDirectoryAttributes"))

{

destDir.Attributes = sourceDirectoryAttributes;

}

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+
+            [System.IO.FileAttributes] $attributes,


Suggested change

[System.IO.FileAttributes] $attributes,

[System.IO.FileAttributes] $attributes,

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -9649,11 +9670,12 @@ function PSGetPathItems
                    return $op
                }}

-                $items = @(Microsoft.PowerShell.Management\Get-Item -Path $getPathItems | Microsoft.PowerShell.Core\ForEach-Object {{
+                $items = @(Microsoft.PowerShell.Management\Get-Item -Path $getPathItems -Force | Microsoft.PowerShell.Core\ForEach-Object {{


Why is -Force added?

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                     }}
                }})

-                $directories = @(Microsoft.PowerShell.Management\Get-ChildItem -Path $getPathDir -Directory | Microsoft.PowerShell.Core\ForEach-Object {{
+                $directories = @(Microsoft.PowerShell.Management\Get-ChildItem -Path $getPathDir -Directory -Force | Microsoft.PowerShell.Core\ForEach-Object {{


ditto, why is -Force added?

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                        if (ExperimentalFeature.IsEnabled('PSCopyItemDirectoryAttributes'))
+                        {{
+                            (Get-Item $createDirectoryPath -Force).Attributes = $attributes
+                        }}


When the remote session is an older version of PowerShell, ExperimentalFeature.IsEnabled(PSCopyItemDirectoryAttributes) won't work. I think you will have to decide whether to set the attribute depending on if $attribute is null or not-null. Currently $attributes is of a enum type, so I guess you cannot just pass in null when the feature is disabled.

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                    if (ExperimentalFeature.IsEnabled('PSCopyItemDirectoryAttributes'))
+                    {{
+                        (Get-Item $createDirectoryPath -Force).Attributes = $attributes
+                    }}


Same here, when the remote side is an older version of PS, ExperimentalFeature.IsEnabled('PSCopyItemDirectoryAttributes') won't work.

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

        {
            ps.AddCommand(CopyFileRemoteUtils.PSCopyToSessionHelperName);
            ps.AddParameter("createDirectoryPath", destination);
+            ps.AddParameter("attributes", sourceDirectory.Attributes);


To put this feature under experimental, you will have to guard this line in a if (ExperimentalFeature.IsEnabled(....)).

daxian-dbw · 2021-11-01T23:50:49Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -8886,6 +8894,10 @@ internal static class CopyFileRemoteUtils

            [string] $createDirectoryPath,

+            [Parameter(ParameterSetName=""PSCreateDirectoryOnRemoteSession"", Mandatory=$true)]


To make the feature experimental, you will need to decide whether to pass in -Attributes based on if the experimental feature is turned on, so this cannot be Mandatory.

msftbot · 2021-11-26T23:00:57Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

pull-request-quantifier · 2021-12-07T06:48:26Z

This PR has 106 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +74 -32
Percentile : 41.2%

Total files changed: 3

Change summary by file extension:
.cs : +50 -32
.ps1 : +24 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 👌 👎 (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw · 2021-12-07T17:27:05Z

@iSazonov Please don't open this PR. It's been marked as "Waiting on Author" for 25 days, and the author didn't get to it. So, the bot closed it as part of the housekeeping. The author can re-open when ready to work on it again.

Update Copy-Item to copy directory attributes

1ca4a22

Joe-Zer0 requested a review from anmenaga as a code owner Sep 11, 2020

msftbot bot assigned daxian-dbw Sep 11, 2020

iSazonov requested a review from PaulHigin Sep 12, 2020

iSazonov added the CL-General label Sep 12, 2020

Joe-Zer0 force-pushed the issue-13552 branch from 24f7bec to a54ab8e Sep 15, 2020

Add Copy-Item unit test

98807a6

Joe-Zer0 force-pushed the issue-13552 branch from a54ab8e to 98807a6 Sep 15, 2020

iSazonov reviewed Sep 15, 2020

View changes

test/powershell/Modules/Microsoft.PowerShell.Management/Copy-Item.Tests.ps1 Outdated Show resolved Hide resolved

Update attribute used in unit test

4b796ac

iSazonov reviewed Sep 18, 2020

View changes

pull request changes

9016d67

PaulHigin requested changes Sep 28, 2020

View changes

msftbot bot added the Waiting on Author label Sep 28, 2020

do not convert FileAttributes to string

86e792d

msftbot bot removed the Waiting on Author label Oct 5, 2020

iSazonov reviewed Oct 5, 2020

View changes

src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated Show resolved Hide resolved

iSazonov reviewed Oct 5, 2020

View changes

src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated Show resolved Hide resolved

update initializations to use var

72d9413

PaulHigin approved these changes Oct 7, 2020

View changes

use int instead of byte to transfer enum

61906be

add Experimental Feature flag

69cf1fe

iSazonov reviewed Oct 8, 2020

View changes

test/powershell/Modules/Microsoft.PowerShell.Management/Copy-Item.Tests.ps1 Outdated Show resolved Hide resolved

use skip for tests

c1ba444

iSazonov approved these changes Oct 9, 2020

View changes

msftbot bot added the Review - Needed label Oct 21, 2020

msftbot bot removed the Review - Needed label Nov 10, 2020

msftbot bot added the Review - Needed label Nov 17, 2020

daxian-dbw requested changes Nov 1, 2021

View changes

msftbot bot added Waiting on Author and removed Review - Needed labels Nov 1, 2021

msftbot bot added the Stale label Nov 26, 2021

msftbot bot closed this Dec 7, 2021

iSazonov reopened this Dec 7, 2021

pull-request-quantifier bot added the Medium label Dec 7, 2021

msftbot bot removed the Stale label Dec 7, 2021

daxian-dbw closed this Dec 7, 2021

Feb	MAR	Apr
	11
2021	2022	2023


	[System.IO.FileAttributes] $attributes,
	[System.IO.FileAttributes] $attributes,

		@@ -8886,6 +8894,10 @@ internal static class CopyFileRemoteUtils

		[string] $createDirectoryPath,

		[Parameter(ParameterSetName=""PSCreateDirectoryOnRemoteSession"", Mandatory=$true)]

PowerShell / PowerShell Public

Update Copy-Item to copy directory attributes #13622

Update Copy-Item to copy directory attributes #13622

Conversation

Joe-Zer0 commented Sep 11, 2020 • edited

PR Summary

PR Context

PR Checklist

microsoft-cla bot commented Sep 11, 2020 • edited

iSazonov commented Sep 12, 2020

Joe-Zer0 commented Sep 14, 2020

iSazonov commented Sep 15, 2020

Joe-Zer0 commented Sep 15, 2020

Joe-Zer0 commented Sep 18, 2020

Joe-Zer0 commented Sep 21, 2020

iSazonov commented Sep 23, 2020 • edited

Joe-Zer0 commented Sep 28, 2020

PaulHigin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joe-Zer0 commented Oct 5, 2020

PaulHigin left a comment

iSazonov commented Oct 8, 2020

Joe-Zer0 commented Oct 13, 2020

iSazonov commented Oct 14, 2020

msftbot bot commented Oct 21, 2020

Joe-Zer0 commented Nov 9, 2020

iSazonov commented Nov 10, 2020

msftbot bot commented Nov 17, 2020

daxian-dbw left a 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

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

msftbot bot commented Nov 26, 2021

pull-request-quantifier bot commented Dec 7, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw commented Dec 7, 2021 • edited

Joe-Zer0 commented Sep 11, 2020 •

edited

microsoft-cla bot commented Sep 11, 2020 •

edited

iSazonov commented Sep 23, 2020 •

edited

daxian-dbw commented Dec 7, 2021 •

edited