Fix for Move-Item with wildcard source and a non-existent destination directory implodes first directory #26603
Conversation
|
@daxian-dbw Could you please review this PR as potential fix for the issue: #24837? Thank you. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where Move-Item with wildcard source paths to a non-existent destination directory would flatten the directory structure. The issue occurred because the first directory in the wildcard expansion was renamed to the destination path instead of being moved into it as a subdirectory, causing subsequent items to be moved into that first directory incorrectly.
Key changes:
- Adds logic to pre-create the destination directory when it doesn't exist but its parent does
- Adds a comprehensive test case to verify directory structure is preserved during wildcard moves
- Makes single directory move behavior consistent with wildcard move behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/System.Management.Automation/namespaces/FileSystemProvider.cs |
Modified MoveItem method to create the destination directory before moving if it doesn't exist but the parent directory does, ensuring consistent move-into behavior |
test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 |
Added test case that verifies directory structure preservation when moving wildcard contents to a non-existent destination directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Copy-Item -Path $zeroLengthFile -Destination "$TestDrive\zeroLengthFile2.txt" -Force | ||
| "$TestDrive\zeroLengthFile2.txt" | Should -Exist | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider adding a test comment explaining what specific bug this test is guarding against. While the test name is descriptive, a brief comment mentioning issue #24837 would help future maintainers understand the historical context and the specific flattening bug that was fixed.
| # Regression test for issue #24837: Ensure Move-Item with wildcards preserves directory structure and does not flatten contents. |
There was a problem hiding this comment.
We never add references to issues.
| Directory.CreateDirectory(destination); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace at the end of this line to maintain code cleanliness and consistency with project standards.
| if (!ItemExists(destination)) | ||
| { | ||
| string parentDir = Path.GetDirectoryName(destination); | ||
| if (!string.IsNullOrEmpty(parentDir) && Directory.Exists(parentDir)) | ||
| { | ||
| Directory.CreateDirectory(destination); | ||
| } | ||
| } |
There was a problem hiding this comment.
This fix introduces a breaking change in behavior for single directory moves to non-existent destinations. Previously, Move-Item source/dir -Destination dest/newname (where dest exists but newname doesn't) would rename dir to newname. With this fix, it creates newname and then moves dir into it, resulting in dest/newname/dir. This breaks the existing test "Verify Move-Item across devices for directory" at line 155 of FileSystem.Tests.ps1, which expects the directory contents to be at $destination/file.txt, not $destination/dir/file.txt. While this change makes wildcard and non-wildcard moves consistent, it's a breaking change that should be explicitly acknowledged and documented. The PR description incorrectly marks "User-facing changes: Not Applicable".
| if (!ItemExists(destination)) | |
| { | |
| string parentDir = Path.GetDirectoryName(destination); | |
| if (!string.IsNullOrEmpty(parentDir) && Directory.Exists(parentDir)) | |
| { | |
| Directory.CreateDirectory(destination); | |
| } | |
| } | |
| // Restore previous behavior: if destination does not exist, and its parent exists, | |
| // treat as a rename (do not create the destination directory). | |
| // Only create the destination directory if moving multiple items or using wildcards. | |
| // For single directory move, do not create destination directory. | |
| // (No action needed here; skip creating destination directory.) | |
| // If needed, logic for wildcard moves should be handled elsewhere. | |
| // (Intentionally left blank to restore previous behavior.) |
|
@alexprabhat99 Please look how Copilot fixed the issue in #26604. Copilot started with fix like yours and did some iterations to avoid side effects. If you agree with that fix you can grab it. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
PR Summary
Fixes: #24837
PR Context
This PR fixes the follwing scenario:
When you call Move-Item with a wildcard into a directory that doesn't exist yet, it seems like the first move of a directory acts as a directory creation, which ends up moving the files into the new directory, instead of creating a new sub-directory, and I don't understand why / find this a rather odd behavior.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header