Skip to content

Removing Hardlink from Mode property in default file system format #8789

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.

Sign up for GitHub

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

Jump to bottom
Merged
adityapatwardhan merged 33 commits into from
Mar 12, 2019
Merged

Removing Hardlink from Mode property in default file system format #8789

adityapatwardhan merged 33 commits into from
Mar 12, 2019

Conversation

@powercode
Copy link
Collaborator

@powercode powercode commented Jan 30, 2019 •
edited
Loading

Adding a new table view, childrenWithHardlink, that has the same behavior as children view had before this PR.

The children view now uses the modified ModeWithoutHardlink property that doesn't check for hardlinks when setting the l letter of Mode column.

The Mode property keeps it's existing behavior, so this change should not be breaking.

PR Summary

#8051

Significant speedup for default filesystem output

$files = gci -Recurse c:\windows -ea:0
$files | ft | out-null
$files | ft -view childrenWithHardlink  | out-null
view time speedup
childrenWithHardlink 3m22s 1x
children 7.6s 26x

Formatting result

Output before

PS> gci

    Directory: D:\repos\PowerShell\symlinks


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       2019-02-03     15:37                dir
d----l       2019-02-03     15:38                symdir
d----l       2019-02-03     15:39                symsubdir
-a----       2019-02-03     15:38              3 file
-a---l       2019-02-03     15:37              3 hardlink
-a---l       2019-02-03     15:38              0 symfile1

Output after

PS> gci

    Directory: D:\repos\PowerShell\symlinks

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d----         2019-02-03    15:37                dir
l----         2019-02-03    15:38                symdir -> D:\repos\PowerShell\symlinks\dir
l----         2019-02-03    15:39                symsubdir -> D:\repos\PowerShell\symlinks\dir\subdir\
-a---         2019-02-03    15:38              3 file
-a---         2019-02-03    15:37              3 hardlink
la---         2019-02-03    15:38              0 symfile1 -> D:\repos\PowerShell\file

PS> gci | ft -view childrenWithHardlink

    Directory: D:\repos\PowerShell\symlinks

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d----         2019-02-03    15:37                dir
l----         2019-02-03    15:38                symdir -> D:\repos\PowerShell\symlinks\dir
l----         2019-02-03    15:39                symsubdir -> D:\repos\PowerShell\symlinks\dir\subdir\
-a---         2019-02-03    15:38              3 file
la---         2019-02-03    15:37              3 hardlink
la---         2019-02-03    15:38              0 symfile1 -> D:\repos\PowerShell\file

produces the old output.

PR Context

PR Checklist

SteveL-MSFT
SteveL-MSFT requested changes Jan 30, 2019
View reviewed changes
@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jan 30, 2019
adityapatwardhan
adityapatwardhan requested changes Jan 30, 2019
View reviewed changes
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 30, 2019

@powercode The change looks good in general. Could you also put a before and after of how the output of Get-ChildItem looks like for future reference in the PR description.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 30, 2019

@powercode Could also have a look at the CodeFactor issue, some on them are about documentation and formating. Would be nice to get those fixed.

@JamesWTruher
Copy link
Collaborator

JamesWTruher commented Jan 30, 2019

symlinks will still appear in the default view, right?
j

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 30, 2019
@powercode
Copy link
Collaborator Author

powercode commented Jan 30, 2019 •
edited
Loading

@JamesWTruher Yes, symlinks appear as an l.

@powercode
Copy link
Collaborator Author

powercode commented Jan 31, 2019

The build errors seem unrelated to my change. Is there a way to trigger a build without a new commit?

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 31, 2019

@powercode Restarted the CI.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 31, 2019

@powercode Please respond to the committee recommendation about re-using the d field for links instead of adding one at the end.

@powercode
Copy link
Collaborator Author

powercode commented Jan 31, 2019 •
edited
Loading

@adityapatwardhan I'm on it :)

Is this what we want, or should I skip the Link resolution?

    Directory: D:\repos\PowerShell\symlinktest

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d----         2019-02-01    00:22                dir
l----         2019-02-01    00:23                symdir -> D:\repos\PowerShell\symlinktest\dir\subdir\
-a---         2019-02-01    00:20              3 file

    Directory: D:\repos\PowerShell\symlinktest\dir

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d----         2019-02-01    00:22                subdir
-a---         2019-02-01    00:22              3 subfile
la---         2019-02-01    00:21              0 symfile1 -> D:\repos\PowerShell\symlinktest\file

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 31, 2019 •
edited
Loading

@powercode yes, that's perfect!

@powercode
Copy link
Collaborator Author

powercode commented Jan 31, 2019

@SteveL-MSFT I'm playing now with moving AddScriptBlockColumn into hidden code properties. Seems to be a bit faster to.

Any reason not to do that? The Name string needed it to get to the internal methods for Link management.

@powercode
Copy link
Collaborator Author

powercode commented Feb 1, 2019

A bit of feature creep here :) Feel free to pick the commits you want!

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Feb 1, 2019

@powercode Please have a look at the test failure

SteveL-MSFT
SteveL-MSFT requested changes Feb 1, 2019
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have some tests validating the formating changes

@powercode
Copy link
Collaborator Author

powercode commented Feb 1, 2019

I consider the remaining CodeFactor issues to be noise, and the CI static analysis seem to complain about spelling in MD files.

SteveL-MSFT
SteveL-MSFT requested changes Feb 2, 2019
View reviewed changes
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Feb 2, 2019
iSazonov
iSazonov reviewed Feb 2, 2019
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SteveL-MSFT Has PowerShell Committee approved new public APIs?

@powercode
Copy link
Collaborator Author

powercode commented Feb 3, 2019

This is what the default view and the childrenWithHardlink view looks like:

PS> gci
    Directory: D:\repos\PowerShell\symlinks

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d----         2019-02-03    15:37                dir
l----         2019-02-03    15:38                symdir -> D:\repos\PowerShell\symlinks\dir
l----         2019-02-03    15:39                symsubdir -> D:\repos\PowerShell\symlinks\dir\subdir\
-a---         2019-02-03    15:38              3 file
-a---         2019-02-03    15:37              3 hardlink
la---         2019-02-03    15:38              0 symfile1 -> D:\repos\PowerShell\file

PS> gci | ft -view childrenWithHardlink

    Directory: D:\repos\PowerShell\symlinks

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d----         2019-02-03    15:37                dir
l----         2019-02-03    15:38                symdir -> D:\repos\PowerShell\symlinks\dir
l----         2019-02-03    15:39                symsubdir -> D:\repos\PowerShell\symlinks\dir\subdir\
-a---         2019-02-03    15:38              3 file
la---         2019-02-03    15:37              3 hardlink
la---         2019-02-03    15:38              0 symfile1 -> D:\repos\PowerShell\file

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Mar 12, 2019

@powercode I have pushed changes to convert the new xUnit tests to Pester. I also rebased to master to resolve some merge conflicts. Please have a look if I have missed something. And thank you for your patience.

daxian-dbw
daxian-dbw requested changes Mar 12, 2019
View reviewed changes
iSazonov
iSazonov reviewed Mar 12, 2019
View reviewed changes
src/System.Management.Automation/namespaces/FileSystemProvider.cs
}
return Platform.IsWindows
? fileInfo.Attributes.HasFlag(System.IO.FileAttributes.ReparsePoint)
: Platform.NonWindowsIsSymLink(fileInfo);
Copy link
Collaborator

@iSazonov iSazonov Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we use Platform.IsWindows not #if UNIX?

@powercode
isHardLink -> isLink
ad839f9
@powercode
Changing signature of GetTarget from IEnumerable<string> to string.
6542c7f 
The only code path that used multiple return values was !CORECLR.

I removed the obsolete #ifdef-ed code.
@powercode
Copy link
Collaborator Author

powercode commented Mar 12, 2019

I adressed @daxian-dbw's feedback.
The other changes LGTM.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Mar 12, 2019

@daxian-dbw Can you update your review?

daxian-dbw
daxian-dbw approved these changes Mar 12, 2019
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@powercode
Copy link
Collaborator Author

powercode commented Mar 12, 2019

@daxian-dbw You won the race!
With several seconds :)

@adityapatwardhan adityapatwardhan merged commit 9983297 into PowerShell:master Mar 12, 2019
@adityapatwardhan adityapatwardhan added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed CL-Engine Indicates that a PR should be marked as an engine change in the Change Log labels Mar 12, 2019
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Mar 12, 2019

@powercode Thanks for your contribution and patience!

@TravisEz13 TravisEz13 modified the milestones: 6.2.0-GA-Consider, 6.2.0 Mar 21, 2019
@SteveL-MSFT SteveL-MSFT mentioned this pull request Sep 30, 2019
Closed
@SteveL-MSFT SteveL-MSFT mentioned this pull request Oct 30, 2019
Open
@iSazonov iSazonov mentioned this pull request Jan 12, 2020
Closed
@lukeb1961
Copy link

lukeb1961 commented Jan 12, 2020

How will a normal person using Powershell, know that these FT views exist?

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 13, 2020

@lukeb1961 Please open a feature request (for tab completion) and/or doc isuue.

@rjmholt rjmholt mentioned this pull request Jan 18, 2020
Closed
@lukeb1961 lukeb1961 mentioned this pull request Jan 20, 2020
Closed
@iSazonov iSazonov mentioned this pull request Oct 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@iSazonov iSazonov iSazonov left review comments

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@anmenaga anmenaga Awaiting requested review from anmenaga

@BrucePay BrucePay Awaiting requested review from BrucePay

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision Documentation Needed in this repo Documentation is needed in this repo

Projects

None yet

Milestone

6.2.0

Development

Successfully merging this pull request may close these issues.

8 participants

@powercode @adityapatwardhan @JamesWTruher @SteveL-MSFT @iSazonov @TravisEz13 @lukeb1961 @daxian-dbw