Experimental Feature: Provide Unix stat information in filesystem output by JamesWTruher · Pull Request #11042 · PowerShell/PowerShell

JamesWTruher · 2019-11-11T23:33:16Z

PR Summary

This PR provides new behavior, as experimental feature PSUnixFileStat, to include data from the Unix stat API in the output of the file system provider to facilitate a more Unix-like file listing. It adds a new note property in the filesystem provider named UnixStat which includes a common expression of the stat(2) API from the underlying Unix type system. The native calls in the powershell-native assembly have created

The output from Get-ChildItem should look something like this:

PS> dir | select -first 4 -skip 5


    Directory: /Users/jimtru/src/github/forks/JamesWTruher/PowerShell-1

UnixMode   User             Group                 LastWriteTime           Size Name
--------   ----             -----                 -------------           ---- ----
drwxr-xr-x jimtru           staff              10/23/2019 13:16            416 test
drwxr-xr-x jimtru           staff               11/8/2019 10:37            896 tools
-rw-r--r-- jimtru           staff               11/8/2019 10:37         112858 build.psm1
-rw-r--r-- jimtru           staff               11/8/2019 10:37         201297 CHANGELOG.md

The username and groupname are retrieved at the time of formatting to reduce the number of PInvokes required.

PR Context

I've been somewhat frustrated with viewing the output of Get-ChildItem since it's different from /bin/ls -l output. This is an attempt to improve the experience with regard to the file system provider when running on a non-Windows platforms.

The old format is still available by selecting the children view with format-table:

PS> dir | select -skip 5 -first 4 | ft -view children

    Directory: /Users/jimtru/src/github/forks/JamesWTruher/PowerShell-1

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----          10/23/2019  1:16 PM                test
d----           11/8/2019 10:37 AM                tools
-----           11/8/2019 10:37 AM         112858 build.psm1
-----           11/8/2019 10:37 AM         201297 CHANGELOG.md

PR Checklist

…t for UnixStat

It includes the new common Unix stat methods.

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

adityapatwardhan · 2019-11-12T19:24:36Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                /// <returns>The mode converted into a Unix style string similar to the output of ls.</returns>
+                public string GetModeString()
+                {
+                    StatMask[] permissions = new StatMask[]


Can this be defined statically? would reduce allocations i think.

adityapatwardhan · 2019-11-12T19:27:24Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                    };
+
+                    // start the mode string with the ItemType
+                    System.Text.StringBuilder sb = new System.Text.StringBuilder();


The length of the output string is fixed to 10 i guess, so we should initialize the string builder with 10 as the capacity.

i rewrote this method using a char array.

adityapatwardhan · 2019-11-12T19:38:22Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                    internal int tm_isdst;
                }

+                // We need a way to convert a DateTime to a unix date.j


Suggested change

// We need a way to convert a DateTime to a unix date.j

// We need a way to convert a DateTime to a unix date.

src/System.Management.Automation/engine/TypeTable_Types_Ps1Xml.cs

adityapatwardhan · 2019-11-12T19:48:52Z

test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1

+        $experimentalFeatureName = "PSUnixFileStat"
+        $skipTest = $true
+        if ( Get-ExperimentalFeature -Name $experimentalFeatureName ) {
+            $skipTest = $false
+        }


This can be:

$skipTest = -not $EnabledExperimentalFeatures.Contains('PSUnixFileStat')

…ntal feature

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

…array of chars A number of smaller fixes suggested by review.

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

…dows

rjmholt · 2019-11-14T19:44:23Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+        /// <summary>Unix specific implementations of required functionality.</summary>
        internal static class Unix
        {
+            // Please note that `Win32Exception(Marshal.GetLastWin32Error())`


It might be worth either moving this comment or putting a newline between it and the dictionary fields to imply its relevance throughout the class.

I know the linter doesn't like the second suggestion, but I feel strongly that the linter is wrong about that. This is a really helpful comment, but applies generally within the class, while still being an implementation detail (so shouldn't be in something like <remarks/>)

yah - I moved this comment all around to get it through code factor. I can try a different location :)

Maybe we can add it in a <remarks> field in an XML comment for the class itself?

rjmholt · 2019-11-14T19:56:53Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                            modeCharacters[offset] = '-';
+                        }
+
+                        offset++;


If there's an offset++ here, would it make sense to use a double indexed for () loop instead? That way rather than nesting ifs and elseifs you could just continue.

rjmholt · 2019-11-14T19:57:36Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                    foreach (StatMask permission in permissions)
+                    {
+                        // determine whether we are setuid, sticky, or the usual rwx.
+                        if ((Mode & (int)permission) == (int)permission)


Actually, is there a way to make this a switch? That seems like it would be the most natural approach/how C does it.

all the examples I could find (and I only looked for a couple) they do something similar to what I have using if/else (freebsd, gnu)

rjmholt · 2019-11-14T19:59:11Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                public string GetUserName()
+                {
+                    string username;
+                    if (usernameCache.TryGetValue(UserId, out username))


Suggested change

if (usernameCache.TryGetValue(UserId, out username))

if (usernameCache.TryGetValue(UserId, out string username))

rjmholt · 2019-11-14T19:59:20Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                /// <returns>The user name.</returns>
+                public string GetUserName()
+                {
+                    string username;


Suggested change

string username;

rjmholt · 2019-11-14T20:00:47Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+                    cs.GroupId = css.GroupId;
+                    cs.HardlinkCount = css.HardlinkCount;
+                    cs.Size = css.Size;
+                    cs.AccessTime = new DateTime(1970, 1, 1).AddSeconds(css.AccessTime).ToLocalTime();


Suggested change

cs.AccessTime = new DateTime(1970, 1, 1).AddSeconds(css.AccessTime).ToLocalTime();

cs.AccessTime = DateTime.UnixEpoch.AddSeconds(css.AccessTime).ToLocalTime();

rjmholt · 2019-11-14T20:34:22Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

-                    public int tm_yday;   /* Day in the year (0-365, 1 Jan = 0) */
-                    public int tm_isdst;  /* Daylight saving time */
+                    /// <summary>Seconds (0-60).</summary>
+                    internal int tm_sec;


Any reason for changing these to internal? I know it's done elsewhere, but usually my reasoning goes like "If this type were public, should this field be visible?"

rjmholt · 2019-11-14T20:38:47Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

                                                        out UInt64 device, out UInt64 inode);
+
+                /// <summary>
+                /// This is a struct from getcommonstat.h in the native library. It's a synthetic construct of the Unix stat


Could you reword synthetic construct? It initially confused me until I realised we're saying this struct doesn't occur anywhere like this, but this type represents the maximal union of all structs in the wild.

rjmholt · 2019-11-14T20:42:01Z

src/System.Management.Automation/engine/TypeTable_Types_Ps1Xml.cs

+            #region UnixStat
+
+
+            if (ExperimentalFeature.IsEnabled("PSUnixFileStat"))


We could cache this, especially since it's an engine experimental feature, so it can't be toggled once PS has started

Caching not required for perf reasons. I believe changes to all engine experimental features need a restart of PS.

rjmholt · 2019-11-14T20:42:35Z

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

+                if (ExperimentalFeature.IsEnabled("PSUnixFileStat"))
+                {
+                    // Add a commonstat structure to file system objects
+                    if (ProviderInfo.ImplementingType == typeof(Microsoft.PowerShell.Commands.FileSystemProvider))


Could squash this into the previous condition with an &&

Move comment around to a hopefully acceptable location Change language which describes the new common stat structure.

adityapatwardhan · 2019-11-15T21:36:23Z

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

+                    }
+                    catch
+                    {
+                        result.AddOrSetProperty("UnixStat", null);


use named parameter in place of null

adityapatwardhan · 2019-11-15T21:40:42Z

test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1

+    # if PSUnixFileStat is converted from an experimental feature, these tests will need to be changed
+    BeforeAll {
+        $experimentalFeatureName = "PSUnixFileStat"
+        $skipTest = -not $EnabledExperimentalFeatures.Contains($experimentalFeatureName)


Consider setting PSDefaultParameterValue for Skip

adityapatwardhan · 2019-11-15T21:42:29Z

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

+                        var commonStat = Platform.Unix.GetLStat(path);
+                        result.AddOrSetProperty("UnixStat", commonStat);
+                    }
+                    catch


Do you want to catch a specific exception here?

no, any problem means that we can't add the property, so don't. I'll add a comment

added a comment

adityapatwardhan · 2019-11-15T21:42:43Z

test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1

+            $group = (/bin/ls -ld $testFile).split(" ",[System.StringSplitOptions]"RemoveEmptyEntries")[3]
+            $i.Group | Should -Be $Group
+        }
+


…t when adding UnixStat. Fix a couple of small things in the tests, change to use PSDefaultParameterValue rather than using individual skip parameters

ghost · 2019-11-16T00:57:05Z

Hello @adityapatwardhan!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2019-11-21T22:36:00Z

🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

JamesWTruher added 6 commits November 11, 2019 10:58

Checkpoint for UnixStat code

220f00c

revert classes to internal

79a1f27

Add tests for UnixStat

07e86e0

UnixStat is only available for Unix platforms

0efed4e

update count of available types (now that I added an additional forma…

bcfcb48

…t for UnixStat

Update to use preview.4 of the native library

5c0457b

It includes the new common Unix stat methods.

JamesWTruher requested review from TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners November 11, 2019 23:33

JamesWTruher requested a review from rjmholt November 11, 2019 23:33

PoshChan reviewed Nov 11, 2019

View reviewed changes