-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Enable New-Item creates symlinks to non-existent targets #13082
Conversation
2853031
to
cc6f867
Compare
@mklement0 Could you please review and try this? I hope this address your requests. |
if (WildcardPattern.ContainsWildcardCharacters(targetPath)) | ||
// If the original target was a relative path, we want to leave it as relative if it did not require | ||
// globbing to resolve. | ||
if (WildcardPattern.ContainsWildcardCharacters(targetPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't help, because the relative content
path is ultimately still normalized in
normalizedTargetPath = Path.Join(SessionState.Internal.CurrentLocation.ProviderPath, strTargetPath.AsSpan().Slice(2)); |
$relativePath -replace '[\\/]', [IO.Path]::DirectorySeparatorChar
to normalize the separators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, on a general note: I don't think it makes sense to ever interpret the target path as a wildcard pattern - see #13136
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you say about existent targets? In the case it is another issue. I don't change the behavior in the PR. I only address non-existent targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is about existing targets (too), as described in #13064, which your PR description notes as targeting (in addition to #9067); also, the need to normalize the path separators in relative paths applies to both existing and non-existing targets.
I'd say it makes sense to fix all these things together, but if you don't want to do that, please at least remove #13064 from the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that we can normalize non-existing targets safely. We can get unexpected results and can not check the targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused: I also don't see why the .Target
property shouldn't just use the target path as defined.
What reason is there to try to interpret it in any way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we say about different things.
Currently if we create a symlink with relative path the Target property will return absolute path but we expect a relative path used at creation time. Because of this one test fails in the PR on Unix-s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled your PR, built it on macOS 10.15 and Ubuntu 18.04 and can confirm that it correctly creates symlinks with relative paths and performs separator normalization (and that .Target
currently always reports the full path, assuming the target exists).
I'm still a bit confused about the .Target
property: on Windows, it seemingly reports the target path as stored in the link. Why did we ever resolve it to a full path on Unix?
So your plan is to later change that and make it behave like it does on Windows?
But then you wouldn't need a call to realpath()
at all, right?
Separately, knowing the full, resolved path is also useful, so we could consider adding a .ResolvedTarget
property that always reports the full path, if it exists. For non-symlink items, it could report that item's own full path, which would enable you to target items uniformly by their ultimate target path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mklement0 Thanks!
So your plan is to later change that and make it behave like it does on Windows?
But then you wouldn't need a call to realpath() at all, right?
Yes, we should open new issue to track this.
Separately, knowing the full, resolved path is also useful
Please open new issue to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
cc6f867
to
1155056
Compare
if (-not $IsWindows) { | ||
Set-ItResilt -Pending -Because "On Unix the Target property is always resolved to an absolute path." | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to mark this It
as pending, we may want to separate the assertions into two separate tests.
Otherwise, the result will be confusing from this test (it might fail sometimes, but at most it will only be Pending in Mac/Linux).
Also, the cmdlet name is misspelled here. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vexx32 Thanks! I split tests.
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
// Don't support 'New-Item -Type Directory' on the Function provider | ||
// if the runspace has ever been in constrained language mode, as the mkdir | ||
// function can be abused | ||
if (context.ExecutionContext.HasRunspaceEverUsedConstrainedLanguageMode && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a test case to cover this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not new code. It is moved code.
|
||
if (type != null) | ||
{ | ||
WildcardPattern typeEvaluator = WildcardPattern.Get(type + "*", WildcardOptions.IgnoreCase | WildcardOptions.Compiled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the use of the wildcardpattern here, type
is a string for filesystem that should be symboliclink
, junction
, hardlink
, etc... right? why isn't this a switch statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not new code. It is moved code.
PR Summary
Fix #9067.
Fix #13064.
Fix #13136.
PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.