Replace string-compare-based test for copying to same file with more … #3441

jeffbi · 2017-03-28T00:51:30Z

Fix #1930.

Rather than relying on case-insensitive string compares of source and destination paths, use operating system calls to determine whether two paths refer to the same file. This solves not only the case-insensitivity issue but also allows the cmdlet to operate properly if the destination is a hard or symbolic link to the source.

The Windows side is implemented in C#. The Unix side is implemented partially in native code.

daxian-dbw · 2017-03-28T01:07:32Z

@jeffbi I updated the description as this seems the fix for #1930, but please correct it if I'm wrong.
Could you please add more description about how your fix works? It will be helpful for reviewing the changes.

The last comment of #1930 shows:

edit: fixed, thanks @iSazonov

@SteveL-MSFT, do you mean that bug had already been fixed by @iSazonov?

jeffbi · 2017-03-28T01:27:53Z

@daxian-dbw Yes, it is a fix for #1930. I had that at the end of the pull-request title, but it looks like it got clipped off.

I've updated the description with more information.

I believe @SteveL-MSFT's comment referred to fixing the wording on an earlier comment.

iSazonov · 2017-03-28T04:29:39Z

@daxian-dbw

@SteveL-MSFT, do you mean that bug had already been fixed by @iSazonov?

It was just a typo.

iSazonov · 2017-03-28T04:33:25Z

I tested

Test-Path /etc/sysctl.conf
Test-Path /etc/sysctl.CONF

on WSL and Test-Path is case-sensitive. Do we really need additional code here?

SteveL-MSFT · 2017-03-28T05:45:50Z

@daxian-dbw yeah, the 'fixed' statement referred to a typo in the comment @iSazonov caught, nit that the issue was fixed

jeffbi · 2017-03-28T07:38:33Z

@iSazonov The goal of the original code was to ensure that the cmdlet did not attempt to copy a file onto itself, and it did so by doing a case-insensitive compare of the two normalized paths. That worked fine on FAT volumes, on NTFS volumes under Windows in its default mode, and on HFS+ volumes on OS X. In your example, it would consider /etc/sysctl.conf and /etc/sysctl.CONF to be the same file and would not allow the copy. On a case-sensitive file system such as ext under Linux, or NTFS under either Linux or OS X, those two paths are generally not the same file, so the copy should be allowed to continue, which is the complaint in issue #1930. I'm fairly confident that if you ran copy-item /etc/sysctl.conf /etc/sysctl.CONF under WSL it would fail with an error saying it cannot copy the file onto itself.

Then consider links. If file A is a file on a volume, and file B is a hard or symbolic link to file A, copying A to B would actually be copying A onto itself, which the cmdlet does not allow but cannot be detected by merely comparing two path strings.

The new code determines whether two paths refer to the same file (the goal of the original code) regardless of what the paths appear to be. It works on HFS+, ext, FAT, and NTFS file systems. For NTFS file systems, it does the right thing under Windows, where the volume is case-insensitive, and on Mac/Linux where the volume is case-sensitive. And it works for hard and symbolic links on those file systems that support them.

iSazonov · 2017-03-28T14:27:45Z

My question was only whether we can use existing code without new low level code?
I believe yes.
Currently .Net Core works uniformly on all platforms with hard and soft links. (We can even remove this code for Unix and perhaps even more code can be replaced with .Net Core code there)

Name           : powershell
Length         : 0
CreationTime   : 3/21/2017 11:18:52 AM
LastWriteTime  : 3/21/2017 11:18:52 AM
LastAccessTime : 3/21/2017 11:18:52 AM
Mode           : -a---l
LinkType       : SymbolicLink
Target         : C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\debug\powershell.exe
VersionInfo    : File:             C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\debug\powershell

Here Target contains true value for soft and hard links. We can use Mode or LinkType to detect links.
Another option is [Microsoft.PowerShell.Commands.InternalSymbolicLinkLinkCodeMethods]::GetLinkType

So here we can make the code more simple.

We should create a new API to compare path and place it in engen\Utils.cs or PathUtils.cs

jeffbi · 2017-03-28T20:39:30Z

This works differently in Linux.

PS> new-item junk.txt -value "stuff"
PS> get-item j* | select name,mode,attributes,length,linktype,target
Name       : junk.txt
Mode       : ------
Attributes : Normal
Length     : 5
LinkType   : 
Target     : 

PS> new-item -itemtype symboliclink -path junk-sym.txt -value junk.txt 
PS> new-item -itemtype hardlink -path junk-hard.txt -value junk.txt
PS> get-item j* | select name,mode,attributes,length,linktype,target
Name       : junk-hard.txt
Mode       : -----l
Attributes : Normal
Length     : 5
LinkType   : HardLink
Target     : 

Name       : junk-sym.txt
Mode       : -----l
Attributes : ReparsePoint
Length     : 24
LinkType   : SymbolicLink
Target     : {/home/jeff/junk/junk.txt}

Name       : junk.txt
Mode       : -----l
Attributes : Normal
Length     : 5
LinkType   : HardLink
Target     :

Here, the symbolic link points to its target, but the hard link does not. Also, once the hard link was created, the original file became a hard link as well. Linux (or the ext file system) does not distinguish between a hard link and the "original" file.

A slightly off-topic question: Note that the symbolic link's attributes property says "ReparsePoint". That makes sense for Windows, but does it for Linux?

iSazonov · 2017-03-29T09:10:18Z

@jeffbi Sorry for mesh in my previous comment.

This works differently in Linux.

I believe we should fix this locally - behavior on all platforms must be the same. So my suggestion is don't block the PR, continue the code review and open new Issue (if you confirm that we have inconsistency in file provider with soft/hard links on different platforms).

jeffbi · 2017-03-30T21:47:47Z

Is this being reviewed, or is it awaiting something from me?

iSazonov · 2017-03-31T04:56:13Z

I originally thought that we already have full compliance with links on all platforms as associated problems has been closed both locally and in CoreFX. As you pointed out it is not. I propose to open a new issue and collect the inconsistencies there. Anyway we need to fix it now or later. I need your help with this on Unix.
I suppose this is the preferred way to reduce the places where we use low level API, PInvoke and "If $Platform".
If we can fix Target, then it will be a more fundamental and useful solution and this will simplify many things including this PR (yes?). If it will be difficult to fix (Although I am assuming that your low-level code would simply be used in another API), we will take the PR "as is". Confirm if I don't wrong again and you agree with the road map.

@daxian-dbw Could you please review my thoughts?

jeffbi · 2017-04-03T21:53:21Z

@iSazonov I don't think we can "fix" Target, at least for Unix. Short of walking the file system, like the UNIX find command does, I don't believe there is a way to discover all the hard links to a given inode. An inode has a count of links that refer to it, but not a list of the actual links.

My low-level code does not attempt to find all paths to an inode, just whether two paths point to the same inode. There is an open request issue in corefx (dotnet/corefx#10120) to implement this in corefx.

In PowerShell 5.1 on my Windows 10 box, the Target property contains a collection of other hard links to the same file. On PowerShell for CoreCLR, Target is empty. The code for discovering the links on Windows is #if-ed out, with a comment saying the Windows API functions FindFirstFileName and FindNextFileName are not supported in CoreCLR. Is that no longer the case? I re-enabled that code, using the appropriate DLL name from here, and I'm able to populate Target on CoreCLR. That code, though, does have another instance of case-insensitive path comparison, which is what started this PR in the first place.

I have come across a few other issues that are related to links, one in New-Item and two that directly affect #621 (the latter two are, I believe, corefx issues: see dotnet/corefx#17843 and dotnet/corefx#17844). Do we want to create a single issue as a catch-all for link-related items?

iSazonov · 2017-04-04T04:20:11Z

@jeffbi Great ** 2! 😄 Thanks!
I see that:

We cannot fix Targets on Unix (at least effectively). Now we leave it as is and wait CoreFX solution.
We should open new Issue to fix targets on PowerShell Core on Windows to get the same behavior as Windows PowerShell (for NTFS).
We would open new Meta-Issue with your research results. (Although it is necessary to comprehend the already open ones )

Sorry for the delay of the PR.

iSazonov

Please add tests.

iSazonov · 2017-04-04T04:27:41Z

src/libpsl-native/src/issamefile.cpp

@@ -0,0 +1,47 @@
+//! @file isdirectory.cpp
+//! @author Andrew Schwartzmeyer <andschwa@microsoft.com>
+//! @brief returns if the path is a directory


Please renew the comments.

iSazonov · 2017-04-04T04:30:08Z

src/libpsl-native/src/issamefile.cpp

+//! char* is marshaled as an LPStr, which on Linux is UTF-8.
+//! @endparblock
+//!
+//! @param[in] path_one


path_two?

iSazonov · 2017-04-04T04:34:45Z

src/libpsl-native/src/issamefile.cpp

+//! char* is marshaled as an LPStr, which on Linux is UTF-8.
+//! @endparblock
+//!
+//! @retval true if directory, false otherwise


Please correct the comment.

iSazonov · 2017-04-04T04:41:41Z

src/libpsl-native/src/issamefile.cpp

+//!
+//! @retval true if directory, false otherwise
+//!
+bool IsSameFile(const char* path_one, const char* path_two)


I believe it works for both files and directories. Parameters is "path" too. So maybe rename to IsSamePath?

I agree that this could use a better name, since it does work for both files and directories, but I'm not sure that IsSamePath is the best choice. I avoided IsSamePath because two different paths can arrive at the same destination. I went with IsSameFile just because the docs for the stat function and the GetFileInformationByHandle both just use the term "file" even though they both work for files and directories.

Maybe good comments (on high level API too) and/or IsSameTarget or something like that?

Renamed to IsSameFileSystemItem

iSazonov · 2017-04-04T04:45:53Z

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

+#endif
+        }
+
+        internal static bool WinIsSameFile(string pathOne, string pathTwo)


The same as for IsSameFile - maybe rename to WinIsSamePath

Renamed to IsSameFileSystemItem

iSazonov · 2017-04-04T04:50:17Z

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

+            var access = FileDesiredAccess.GenericRead;
+            var share = FileShareMode.Read;
+            var creation = FileCreationDisposition.OpenExisting;
+            var attributes = FileAttributes.BackupSemantics;    // consider FileAttributes.PosixSemantics as well


Please remove the comment or make it full and clear.

iSazonov · 2017-04-04T05:47:10Z

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

+            var attributes = FileAttributes.BackupSemantics;    // consider FileAttributes.PosixSemantics as well
+            var handleOne = CreateFile(pathOne, access, share, IntPtr.Zero, creation, attributes, IntPtr.Zero);
+
+            using (SafeFileHandle sfOne = new SafeFileHandle(handleOne, true))


Can we use NativeMethods.CreateFile here?

Not really, but that code could have been a lot better. I've updated it.

iSazonov · 2017-04-04T05:52:43Z

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

            {
                String error = StringUtil.Format(FileSystemProviderStrings.CopyError, path);
                Exception e = new IOException(error);
                WriteError(new ErrorRecord(e, "CopyError", ErrorCategory.WriteError, path));
                return;
            }
-


Unneeded formatting. Please renew the comments.

iSazonov · 2017-04-04T05:56:30Z

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

                {
                    String error = StringUtil.Format(FileSystemProviderStrings.CopyError, destinationPath);
                    Exception e = new IOException(error);
                    WriteError(new ErrorRecord(e, "CopyError", ErrorCategory.WriteError, destinationPath));
                    return;
                }
-


Unneeded formatting. Please renew the comments.

jeffbi · 2017-04-14T17:18:51Z

@daxian-dbw This has been waiting for approval/merge of PR #3509 because the Pester tests should be using New-Item to create directory symbolic links.

I can get around that by shelling out to cmd.exe to run the mklink program, but that would mean revisiting the test script once the PR has been merged. Perhaps I could open a new issue to update the test.

Would this be acceptable?

daxian-dbw · 2017-04-14T20:49:08Z

@jeffbi Thanks for letting me know. I noticed that @iSazonov has signed off #3509. I will quickly go through it and merge the PR if I don't spot anything obvious.

jeffbi · 2017-04-14T20:58:58Z

@daxian-dbw I think there are discussions required for #3509 regarding the proper way to create symlinks in various circumstances under Windows, and that that's why approval/merging of that PR is taking a while.

daxian-dbw · 2017-04-14T21:23:59Z

@jeffbi #3509 has been merged. Please sync and carry on with your test changes for this PR.

…accurate, cross-platform test (#1930) Changes per code review * Cleaned up comments in .cpp file * Renamed IsSameFile to IsSameFileSystemItem * Removed comment about PosixSemantics * Added PosixSemantics to CreateFile call * Provide additional information in exception when paths point to same destination. * Added tests

iSazonov · 2017-04-17T07:27:07Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+                $principal = New-Object System.Security.Principal.WindowsPrincipal($WinId)
+                $admin = [System.Security.Principal.WindowsBuiltInRole]::Administrator
+
+                return $principal.IsInRole($admin)


Usually we set -Tags @('CI', 'RequireAdminOnWindows').

iSazonov · 2017-04-17T07:32:47Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+                catch
+                {
+                                  # we expected the copy to fail and it did. make sure it failed the right way

            
+                    $_.FullyQualifiedErrorId | Should Be "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"


Please use ShouldBeErrorId

$exc = { ... } | ShouldBeErrorId ... $exc.Exception | Should BeOfType System.IO.IOException ...

iSazonov · 2017-04-17T07:35:43Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+                    {
+                        # we expected the copy to succeed. make sure it did
+                        Test-Path $testCase.Destination | Should Be $true
+                        Remove-Item -Path $testCase.Destination -Force -ErrorAction SilentlyContinue


If previous Should Be is failed the cleanup code will be unreacheable.
Please move the cleanup code to AfterEach.

iSazonov · 2017-04-17T07:42:44Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+        # Test the ability to avoid an item copying onto itself
+        function TestSelfCopy($testCase)
+        {
+            It "$($testCase.Name)" -Skip:($testCase.SkipIf) {


With moving to 'RequireAdminOnWindows' I believe we can use It "" -TestCase $testCases directly without TestSelfCopy

iSazonov · 2017-04-17T07:45:20Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

@@ -629,14 +801,14 @@ Describe "Extended FileSystem Path/Location Cmdlet Provider Tests" -Tags "Featur
            $result = Split-Path -Path $level1_0Full -Leaf
            $result | Should Be $level1_0
        }
-        
+


It distracts attention. We should make separate PR for formatting.

iSazonov · 2017-04-17T07:46:15Z

src/libpsl-native/src/issamefilesystemitem.h

+#pragma once
+
+#include "pal.h"
+


Extra line.

iSazonov · 2017-04-17T07:47:49Z

src/libpsl-native/src/issamefilesystemitem.cpp

@@ -0,0 +1,48 @@
+//! @file issamefilesystemitem.cpp
+//! @author Jeff Bienstadt <v-jebien@microsoft.com>
+//! @brief returns if two paths ultimately point to the same filesystem object


returns what?
Below the same.

Please correct this.

iSazonov · 2017-04-17T07:48:00Z

src/libpsl-native/src/issamefilesystemitem.cpp

+
+#include "getstat.h"
+#include "issamefilesystemitem.h"
+


Extra line.

jeffbi · 2017-04-17T19:28:28Z

@iSazonov In response to your review:

RequireAdminOnWindows: I knew about this, but decided against it in this case.
- Whether a test requires elevation on Windows is not about the test itself, but about the precondition for the test---the test can run without elevation, but it requires elevation (on Windows) to create the symbolic links the test works with.
- Using this tag would mean that we would need a separate Describe for almost every test, in some cases with the tests duplicated only because we couldn't do part of the setup without elevation. If I understand this tag correctly, this would also require creating identical tests that do no have this tag so they can be run on Unix, and possibly coming up with a mechanism for ensuring that only one of those tests is run. This seems to me to be a lot of extra code for little gain. Since all these tests are validating the same basic thing, I thought they should be contained in a single Describe.
Using It "" -TestCase $testCases: If we end up having multiple Describe blocks as discussed above, using the $testCases might not be practical since we'd probably end up with the logic contained within the It block in each Describe block.
Remove-Item after the ShouldBe: You're right about that, of course. However, we can't move the Remove-Item $destination into the AfterAll block because most of the time the source and the destination object are the same object and removing it would likely cause the next test to fail because its source object no longer exists. I can, though, do something like this:

$pathExists = Test-Path $testCase.Destaination
Remove-Item -Path $testCase.Destination -Force -ErrorAction SilentlyContinue
$pathExists | Should Be $true

Native code: For consistency I modeled the coding style of the existing isdirectory.h and isdirectory.cpp files. I'm happy to make your suggested changes if you prefer.

iSazonov · 2017-04-18T05:12:32Z

RequireAdminOnWindows

We don't test Windows behavior, we test PowerShell behavior. So the tests is the same for elevated and non-elevated sessions. In other words if you perform these tests with RequireAdminOnWindows, we will not miss any bug. So we can safely put RequireAdminOnWindows tag.

Unix tests is not skipped if RequireAdminOnWindows present!

Using It "" -TestCase $testCases

Based on my comments we can use this.

Remove-Item after the ShouldBe

My suggesion is AfterEach not AfterAll. The Remove-Item is in foreach and it is executed for every test case.

Native code

I agree with using current pattern.

iSazonov · 2017-04-18T08:21:27Z

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

+            var access = FileAccess.Read;
+            var share = FileShare.Read;
+            var creation = FileMode.Open;
+            var attributes = FileAttributes.BackupSemantics | FileAttributes.PosixSemantics;


Please clarify - do we really need PosixSemantics here?

That's for if/when we find ourselves in running in a Windows environment in which case-sensitivity has been turned on. Since the purpose of the function is to see if two files/directories are the same, it made sense to me to give it every advantage in making a correct assessment.

jeffbi · 2017-04-18T22:43:10Z

@iSazonov I've re-worked the structure of the tests.

jeffbi · 2017-04-19T05:29:39Z

The Travis CI build failure puzzles me. It appears to have succeeded on Linux but failed on OS X.

It appears that the offending line is

Remove-Item -Path $destinationPath -Force -ErrorAction SilentlyContinue

and it looks to me like it's ignoring the -ErrorAction SilentlyContinue part. Have I missed something?

daxian-dbw · 2017-04-19T06:59:33Z

@jeffbi That error seems to indicate a bug in the product code. The error string is from FileSystemProviderStrings.resx:

  <data name="ItemDoesNotExist" xml:space="preserve">
    <value>An object at the specified path {0} does not exist.</value>
  </data>

It looks to me an exception with this resource string got thrown. Searching where this resource string is used in code might lead you somewhere.

iSazonov

@jeffbi Sorry, I was pretty sure that we can already use the template:

$exc = { .. } | ShouldBeErrorId "..."

but we still blocked by #3456
😕 I asked PS team to help.

iSazonov · 2017-04-19T04:04:46Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

@@ -246,19 +246,16 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" {
    }
 }

-Describe "Copy-Item can avoid copying an item onto itself" -Tags "CI" {
+
+Describe "Copy-Item can avoid copying an item onto itself" -Tags "CI", "RequireAdminOnWindows" {
    BeforeAll {
        # In Windows, only Administrators can create symbolic links.
        # In Unix, anyone can.
        function CanMakeSymlink


This is unneeded function because RequireAdminOnWindows ensures elevated rights.

Function removed, along with use of $canSymLink

iSazonov · 2017-04-19T04:06:24Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

@@ -280,7 +277,8 @@ Describe "Copy-Item can avoid copying an item onto itself" -Tags "CI" {
        # Names of files, directories, and links we'll be using to test


This comment is obvious and useless. Please remove.

iSazonov · 2017-04-19T04:07:42Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

@@ -291,7 +289,8 @@ Describe "Copy-Item can avoid copying an item onto itself" -Tags "CI" {
        $symdToOther = "$subDir/symd-to-other"
        $junctionToOther = "$subDir/junction-to-other"

-        # Set up our directories and links
+        # Set up our files, directories, links


This comment is obvious and useless. Please remove.

iSazonov · 2017-04-19T05:43:27Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

-                SelfCopyExpected = $true
+            else
+            {
+                Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction SilentlyContinue | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"


We cannot use -ErrorAction SilentlyContinue with ShouldBeErrorId and we should use script block:

{ Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction Stop } | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"

iSazonov · 2017-04-19T05:48:15Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            {
+                Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction SilentlyContinue | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"
+                $_.Exception | Should BeOfType System.IO.IOException
+                $_.Exception.Data[$selfCopyKey] | Should Not Be $null


The template is:

$exc = { { Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction Stop } | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand" } $exc.Exception.Data[$selfCopyKey] | Should Not Be $null

I've changed the test code to use $Error[0] rather than $_. I'll gladly update to use your template when ShouldBeErrorId returns something other than Boolean.

iSazonov · 2017-04-19T05:48:51Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

-            }
+    Context "Copy-Item avoids copying an item onto itself" {
+        BeforeAll {
+            # Set up our test cases


The same about comment.

jeffbi · 2017-04-19T07:41:45Z

@daxian-dbw Thanks for the suggestion. I'll take a look tomorrow. It's odd because I've run the Pester tests on all three platforms, and run the Remove-Item with -ErrorAction SilentlyContinue alone, and they all work.

iSazonov · 2017-04-20T12:42:12Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

        # For now, we'll assume the tests are running the platform's
        # native filesystem, in its default mode
        $isCaseSensitive = $IsLinux
-        $canSymlink = CanMakeSymlink
-        $canDirSymLink = $IsWindows -And (CanMakeSymlink)
+        $canDirSymLink = $IsWindows


It is not used and can be removed.

iSazonov · 2017-04-20T12:49:41Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            # The source and destination paths must exist.
+            # If either do not, it's because they could not be created in the BeforeAll block
+            # This is not a test failure, but rather a precondition failure.
+            if ((-Not (Test-Path -Path $Source)) -or (-Not (Test-Path -Path $Destinat
4E34
ion)))


Please clarify 'ShouldBeErrorId' below don't fail if the items don't exist?
If failed please remove the "if-return".
If not failed we should replace the if-return with Should.

Removed if for source file existence.

jeffbi · 2017-04-21T04:32:59Z

@iSazonov Can you look this over please?

iSazonov · 2017-04-21T08:10:21Z

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1

+            )
+
+            # Junctions and directory symbolic links are Windows and NTFS only
+            if ($IsWindows)


Oh, we shouldn't skip tests silently 😕
Maybe define the test body (below in "It" block) as function and make two "It" blocks (second "skip on non-Windows")?

iSazonov · 2017-04-21T08:18:10Z

Please correct https://github.com/PowerShell/PowerShell/pull/3441/files#r111716269

Revise comments in C++ code.

jeffbi · 2017-04-21T09:23:16Z

C++ comments updated

iSazonov · 2017-04-21T11:23:52Z

@jeffbi I don't see last commit (with C++)

jeffbi · 2017-04-21T17:24:48Z

@iSazonov Commit 19702a0

iSazonov · 2017-04-21T17:54:36Z

Thanks! I see now.

LGTM.

jeffbi · 2017-04-21T18:01:07Z

@iSazonov Thanks for the review! 👍

iSazonov · 2017-04-21T19:54:57Z

@jeffbi Do you specifically choose a work with the most sophisticated tests? 😄

daxian-dbw · 2017-04-21T23:06:24Z

@jeffbi @iSazonov This is awesome!!! This is not a trivial issue to resolve, thank you both very much for the great fix and the thorough review discussion!

msftclas added the cla-already-signed label Mar 28, 2017

daxian-dbw requested a review from iSazonov March 28, 2017 01:03

daxian-dbw self-assigned this Mar 28, 2017

iSazonov reviewed Apr 4, 2017

View reviewed changes

jeffbi mentioned this pull request Apr 11, 2017

New-Item -ItemType SymbolicLink folder cannot be opened #2915

Closed

daxian-dbw added the Review - Waiting on Author label Apr 11, 2017

iSazonov mentioned this pull request Apr 14, 2017

Fix New-Item to create correct symlink type (#2915) #3509

Merged

iSazonov reviewed Apr 17, 2017

View reviewed changes

iSazonov reviewed Apr 18, 2017

View reviewed changes

Update tests per code review

3263893

Fix Pester test for Windows

a1e905b

iSazonov reviewed Apr 19, 2017

View reviewed changes

More test changes per code review

9fff05b

iSazonov reviewed Apr 20, 2017

View reviewed changes

More changes per code review, and fix Travis CI failure on macOS

8814054

iSazonov reviewed Apr 21, 2017

View reviewed changes

jeffbi added 2 commits April 21, 2017 02:13

Move Windows-only test into separate It block.

19702a0

Revise comments in C++ code.

Move test body from It block into function.

07b1594

daxian-dbw merged commit 8183732 into PowerShell:master Apr 21, 2017

jeffbi deleted the copy-item-1930 branch April 21, 2017 23:50

iSazonov removed the Review - Waiting on Author label Apr 22, 2017

joeyaiello mentioned this pull request May 15, 2017

File extension and path checks need to take into account case-sensitive file systems #3218

Closed

iSazonov mentioned this pull request May 12, 2018

Should we do path normalizations on Unix well as on Windows? #6833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace string-compare-based test for copying to same file with more … #3441

Replace string-compare-based test for copying to same file with more … #3441

jeffbi Apr 17, 2017

		@@ -280,7 +277,8 @@ Describe "Copy-Item can avoid copying an item onto itself" -Tags "CI" {
		# Names of files, directories, and links we'll be using to test

		#pragma once

		#include "pal.h"


		#include "getstat.h"
		#include "issamefilesystemitem.h"

Replace string-compare-based test for copying to same file with more … #3441

Replace string-compare-based test for copying to same file with more … #3441

Conversation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffbi Apr 17, 2017

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

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

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

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