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

[hammy275]

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

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed: Page for FileInfo/DirectoryInfo MicrosoftDocs/PowerShell-Docs#8505
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[ghost]

All CLA requirements met.

[ghost]

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]

@hammy3502 Please look test fails.

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

[hammy275]

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]

/azp restart PowerShell-CI-macos

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[anamnavi]

LGTM, just had one recommendation to remove extra newline

[hammy275]

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]

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.

[iSazonov]

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

[daxian-dbw]

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).

[iSazonov]

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

[daxian-dbw]

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.

[hammy275]

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!

[daxian-dbw]

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:

1. 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?)
2. 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.
3. 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.

[hammy275]

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!

[daxian-dbw]

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

1. 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.
2. 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]

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]

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

[daxian-dbw]

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

[hammy275]

@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]

@PowerShell/powershell-committee reviewed this:

1. 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.

2. 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.

[iSazonov]

@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]

@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]

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]

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

[daxian-dbw]

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]

@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]

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

[pull-request-quantifier-deprecated]

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]

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]

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

[hammy275]

@iSazonov The docs issue has been opened!

[iSazonov]

@hammy3502 Thanks for your contribution!

[ghost]

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

Handy links:

• Release Notes