-
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
Resolve symlink target relative to the symlink instead of the working directory (#15235) #20943
base: master
Are you sure you want to change the base?
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
So, um, is there a chance of anyone looking at this PR? It's been almost 4 months now without any response. |
18a2e31
to
c6b9cea
Compare
@SteveL-MSFT Apologies for pinging, but since @anmenaga, who is assigned, did not review the PR during the past 4 months, do you think you could review it instead? |
c6b9cea
to
f6697ba
Compare
@MatejKafka sorry, I'm reviewing this now |
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.
Remove created items after each test. Make the tests pass in strict mode.
@SteveL-MSFT Done. |
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 #15235.
PR Context
As described in the linked issue, the current implementation of symlink creation in the filesystem provider resolves relative symlink target paths relative to the working directory, instead of the location of the symlink. This PR fixes the issue by always resolving the path relative to the symlink. Additionally, since
Path.Combine
is used now, the path should be correctly resolved even if it does not start with./
or.\
, which is currently not the case.It should be possible to add tests for this, but from a cursory look, there don't seem to by any tests of the filesystem provider touching the filesystem, so I did not add any.
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.(which runs in a different PS Host).