Adds a .ResolvedTarget Property to File-System Items to Reflect a Symlink's Target as FileSystemInfo #16490

hammy275 · 2021-11-19T01:28:11Z

PR Summary

This PR addresses issue #13366 , adding the ability for users to access a ResolvedTarget property on file and folder items, which returns a string representation of a path, depending on the type of file/folder:

If the file/folder is not a symbolic link, ResolvedTarget will return a string referring to itself.
If the file/folder is a symbolic link that points to a file/folder that currently exists, ResolvedTarget will return a string referring to the file/folder that the symbolic link points to.
If the file/folder is a symbolic link that points to a file that does not exist, ResolvedTarget returns a string referring to the broken destination.

PR Context

This PR mainly addresses the problems described in #13366 , mainly that there is no easy way to resolve the path of a file if said file has a chance of being a symbolic link (and as a result, points to a different file in the end). This PR addresses this with the ResolvedTarget property, which points to "the actual file" whether the file being referenced is a symbolic link or not.

PR Checklist

ghost · 2021-11-19T01:28:23Z

All CLA requirements met.

ghost · 2021-11-26T02:00:42Z

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

iSazonov · 2021-11-26T06:03:31Z

@hammy3502 Please look test fails.

@mklement0 Could you please review? Is this addressed a behavior you requested?

hammy275 · 2021-12-02T14:45:47Z

My apologies for the unit tests! The PR should be good to go now. The one failing unit test here looks to be from upstream from when I last rebased, rather than this PR.

EDIT: Looks like the unit tests have all passed; the PR should be good to go!

iSazonov · 2021-12-02T16:16:55Z

/azp restart PowerShell-CI-macos

azure-pipelines · 2021-12-02T16:18:34Z

Azure Pipelines successfully started running 1 pipeline(s).

anamnavi

LGTM, just had one recommendation to remove extra newline

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

hammy275 · 2021-12-11T17:04:34Z

It looks like the failed tests are from outside the scope of this PR, so we should still be good to merge. For clarification, all of the tests passed before 9c1b43f, which just removed an empty newline.

EDIT: Looks like the failing unit tests were re-ran, and are good to go now!

daxian-dbw · 2021-12-14T23:51:25Z

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

FileSystemInfo.ResolveLinkTarget may throw exception in various cases, such as when the current file system doesn't support reparsing point. The Target and FileSystemInfo.LinkedTarget don't throw, so we shouldn't throw either. Please handle the exception and return null in that case.

What is a negative effects from the throw? Should we really mask the exceptions?

When calling the method on a file on a USB drive or micro sd drive, it will fail because the underlying filesystem doesn't support reparsing point. However, the behavior in that case should be returning the full path of the file.

Actually, this scenario should be handled, meaning when exception happens, it should decide when to return the FullName of the current FileSystemInfo, and when to return null.
[edited] For example, when the file is on a USB drive, ResolvedTarget should return the FullName of the file, even though exception will be thrown from fileSysInfo.ResolveLinkTarget(true).

This would weird behavior in formatting output - if user see Target and LinkType properties are "empty" but ResolvedTarget is FullName.
And if I remember correctly .Net ResolveLinkTarget can return partially resolved path. https://github.com/dotnet/runtime/blob/12a8819eee9865eb38bca6c05fdece1053102854/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs#L621-L627

The behavior I describe is what was asked from the issue:

If the item at hand isn't a symlink / reparse point, simply report its own full path - this allows unified treatment of symlinks and non-symlinks, as a simple way to get an item's full path.

I'm about to push a commit that wraps a try-catch around the ResolveLinkTarget call and returning null, however I'm not sure in what situation ResolveLinkTarget would throw an exception and want to return FullName. It cleanly returns null when encountering something that isn't a symlink, and flash drives (by flashdrives, I'm assuming you mean the usage of FAT32/exFAT here) don't seem to support symlinks in any capacity. Am I missing something here? Thanks!

I think we want to return FullName only if the FileSystemInfo is just a regular file/directory instead of a link.

Looking more into it, I found on portable drive with exFAT file system, ResolveLinkTarget(false) throws while ResolveLinkTarget(true) doesn't throw (see code here). So, maybe we don't really need the try / catch (IOException) here.

So, maybe we can do the following:

if ResolveLinkTarget(true) returns non-null value, just return that value. (I see you check on symbTarget.Exists here, but could it even be possible that the method returns a non-null value, but it doesn't exist?)

if ResolveLinkTarget(true) returns null value, assuming that means it's not a link, and thus return the FullName or the current FileSystemInfo depending on what we want for return type.

if ResolveLinkTarget(true) throws, then we assume that means it could be link, but something is wrong when resolving it. We just let the exception propagate in this case.

Based on the docstring for ResolveLinkTarget, it looks like we do need to check if it exists:

Returns: A System.IO.FileSystemInfo instance if the link exists, independently if the target exists or not

Besides that, the commit I mentioned earlier and the ones I just pushed should match the requested behavior!

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

daxian-dbw · 2021-12-15T18:40:41Z

@PowerShell/powershell-committee We need the committee to review the issue and PR on 2 behaviors of the proposed ResolvedTarget property:

when the file or directory is not a symlink / reparse point, whether it should report its own full path, or return null. See Adds a .ResolvedTarget Property to File-System Items to Reflect a Symlink's Target as FileSystemInfo #16490 (comment) for the discussion.
whether the parameter type of the proposed property to be FileSystemInfo or string. see Adds a .ResolvedTarget Property to File-System Items to Reflect a Symlink's Target as FileSystemInfo #16490 (comment) for the discussion.

ghost · 2021-12-30T20:00:40Z

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.

hammy275 · 2021-12-31T17:04:13Z

Apologies for the slow response! I'll do my best to have the changes back within a couple days!

daxian-dbw · 2022-01-04T00:24:54Z

@hammy3502 Thanks for your reply. I responded to your comments, and let's wait on the committee to decide the return type.

hammy275 · 2022-01-04T03:49:21Z

@daxian-dbw Thank you so much! I handled the two threads you responded to, and I'll be eagerly awaiting a response from the committee! :)

SteveL-MSFT · 2022-01-05T23:30:38Z

@PowerShell/powershell-committee reviewed this:

We agreed that .ResolvedTarget should always have a value that is the full path regardless if the item is a link. Users should rely on .Attributes to determine if an item is a link or not. By having this property always have a value, users won't need to check if it's $null and can also make it easier to format without conditionals.
We also agreed that for consistency and also it's easy to convert the string path to a FileSystemInfo object and most users may not want to incur the additional memory to allocate that object, it makes sense for .ResolvedTarget to be a string.

Co-authored-by: Ilya <darpa@yandex.ru>

iSazonov · 2022-01-19T06:31:47Z

@hammy3502 Please update the PR description to reflect new logic is implemented and open new issue in Documentation repository https://github.com/MicrosoftDocs/PowerShell-Docs.

GitHub
GitHub - MicrosoftDocs/PowerShell-Docs: The official PowerShell documentation sources
The official PowerShell documentation sources. Contribute to MicrosoftDocs/PowerShell-Docs development by creating an account on GitHub.

hammy275 · 2022-01-19T14:49:59Z

@iSazonov I've updated the PR description, however I can't seem to find a page to propose any needed documentation changes (there doesn't appear to be any pages for FileInfo/DirectoryInfo that I could find). Are there any pages you'd recommend be updated so I can open an issue? Thank you so much!

iSazonov · 2022-01-19T17:35:43Z

I've updated the PR description

I don't see all cases covered in #16490 (comment)

I can't seem to find a page to propose any needed documentation changes

Since it is new addition I think it should be documented. If PowerShell documentation hasn't appropriate pages you can new doc issue as "idea" in doc repo.

iSazonov · 2022-01-19T17:36:20Z

@daxian-dbw How could we re-run Update cgmanifest CI?

daxian-dbw · 2022-01-19T17:45:55Z

~~I have no idea. I guess it's stuck for some reason, and we may have to close the PR and then re-open.~~
I just tried re-running the job in the Actions tab. Will see if that helps.
https://github.com/PowerShell/PowerShell/actions/runs/1714599480

iSazonov · 2022-01-19T17:54:24Z

@daxian-dbw Thanks!

This cgmanifest is reminiscent of the Files.wxs story. At first we had to update it manually every time. After that exhausted everyone, the update became automatic. 😄 I hope to see auto update soon!

iSazonov · 2022-01-19T18:11:28Z

@daxian-dbw The action was finished but not reported again. 😟

pull-request-quantifier-deprecated · 2022-01-19T18:49:05Z

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

Quantification details

Label      : Small
Size       : +61 -1
Percentile : 24.8%

Total files changed: 4

Change summary by file extension:
.cs : +25 -0
.ps1 : +36 -1

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? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

iSazonov · 2022-01-20T03:55:25Z

Adding the ability for users to access a ResolvedTarget property on file and folder items, which returns a string representation of a path, depending on the type of file/folder:

If the file/folder is not a symbolic link, ResolvedTarget will return a string referring to itself.
If the file/folder is a symbolic link that points to a file/folder that currently exists, ResolvedTarget will return a string referring to the file/folder that the symbolic link points to.
If the file/folder is a symbolic link that points to a file that does not exist, ResolvedTarget returns a string referring to the broken destination.

iSazonov · 2022-01-20T03:56:08Z

@hammy3502 Thanks for your contribution! Please open doc issue.

hammy275 · 2022-01-20T15:38:16Z

@iSazonov The docs issue has been opened!

iSazonov · 2022-01-21T04:23:27Z

@hammy3502 Thanks for your contribution!

ghost · 2022-02-24T23:33:49Z

🎉v7.3.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

hammy275 requested a review from anmenaga as a code owner November 19, 2021 01:28

ghost assigned iSazonov Nov 19, 2021

pull-request-quantifier-deprecated bot added the Small label Nov 19, 2021

ghost added the Review - Needed The PR is being reviewed label Nov 26, 2021

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 26, 2021

ghost removed the Review - Needed The PR is being reviewed label Nov 26, 2021

hammy275 force-pushed the resolved-target branch from c57a5f3 to d435fa6 Compare December 2, 2021 00:52

This comment has been minimized.

Sign in to view

anamnavi approved these changes Dec 10, 2021

View reviewed changes

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

iSazonov approved these changes Dec 11, 2021

View reviewed changes

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

daxian-dbw requested changes Dec 15, 2021

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 15, 2021

daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 15, 2021

ghost added the Stale label Dec 30, 2021

ghost removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept Stale labels Dec 31, 2021

hammy275 and others added 9 commits January 19, 2022 06:22

ResolvedTarget catch IOException

94f1c13

ResolvedTarget dont catch IOException

2f9d25d

ResolvedTarget slight simplification

d8dae75

ResolvedTarget always return non-null

9a8821e

Co-authored-by: Ilya <darpa@yandex.ru>

Fix Tests and Return Type for ResolvedTarget

ba6f85b

ResolvedTarget simplification

b5f6b3c

ResolvedTarget Remove try-catch

0e3aab7

ResolvedTarget fix tests

49ca348

Use -BeTrue instead of -Be $true

9a68c0f

github-actions bot force-pushed the resolved-target branch from 4571f09 to 9a68c0f Compare January 19, 2022 06:22

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Jan 19, 2022

daxian-dbw closed this Jan 19, 2022

daxian-dbw reopened this Jan 19, 2022

hammy275 mentioned this pull request Jan 20, 2022

Page for FileInfo/DirectoryInfo MicrosoftDocs/PowerShell-Docs#8505

Closed

2 tasks

iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label Jan 21, 2022

iSazonov merged commit 3eca7c0 into PowerShell:master Jan 21, 2022

sdwheeler mentioned this pull request Jan 25, 2022

Update documentation for FileInfo/DirectoryInfo #16799

Closed

Adds a .ResolvedTarget Property to File-System Items to Reflect a Symlink's Target as FileSystemInfo #16490

Adds a .ResolvedTarget Property to File-System Items to Reflect a Symlink's Target as FileSystemInfo #16490

Uh oh!

Conversation

hammy275 commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

ghost commented Nov 19, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 26, 2021

Uh oh!

iSazonov commented Nov 26, 2021

Uh oh!

hammy275 commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Dec 2, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

azure-pipelines bot commented Dec 2, 2021

Uh oh!

anamnavi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hammy275 commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

hammy275 Jan 2, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

hammy275 Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 30, 2021

Uh oh!

hammy275 commented Dec 31, 2021

Uh oh!

daxian-dbw commented Jan 4, 2022

Uh oh!

hammy275 commented Jan 4, 2022

Uh oh!

SteveL-MSFT commented Jan 5, 2022

Uh oh!

iSazonov commented Jan 19, 2022 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hammy275 commented Jan 19, 2022

Uh oh!

hammy275 commented Nov 19, 2021 •

edited

Loading

ghost commented Nov 19, 2021 •

edited by ghost

Loading

hammy275 commented Dec 2, 2021 •

edited

Loading

hammy275 commented Dec 11, 2021 •

edited

Loading

daxian-dbw Dec 14, 2021 •

edited

Loading

daxian-dbw Dec 15, 2021 •

edited

Loading

daxian-dbw commented Dec 15, 2021 •

edited

Loading

iSazonov commented Jan 19, 2022 •

edited by unfurl-links bot

Loading

daxian-dbw commented Jan 19, 2022 •

edited

Loading