We should use autoproperty if we can.

Modified the -FollowSymlink parameter to set -Recurse, and the -Depth parameter does. Thus we cannot use auto-property here.

Closed.

Can we use a Tuple<dev, inode> as key? If so it exclude "dictionary in dictionary".
Sample and c# 7.0 pattern https://stackoverflow.com/questions/2877660/composite-key-dictionary

Part of that discussion (https://stackoverflow.com/questions/2877660/composite-key-dictionary#comment14110652_2877820) suggests that the nested dictionary approach may be faster.

As for the C#7 pattern, I thought I read somewhere in another code review that C#7 code should be avoided. Is that not the case?

C#7 pattern

We have no strict documentation. Different maintainers may have different requirements. My understanding: New patterns are welcome for the new code. Existing patterns (not relevant to the PR) should not be changed without maintainer's requiring.

the nested dictionary approach may be faster.

There, the main answer has recently been updated and there is a reference to gist with the tests. The conclusion is now Tuple keys is fast.
We both think that performance is important here. So maybe it makes sense to spend time testing cases.
Another thought is, in most cases, the user will have one volume. We could use one dictionary until we find a reference to another volume. Although it will complicate the code and it's not clear how much we win. Ooops, for other volume we can make separate recursive Dir() call - in the case we have to switch Dictionary (tracker) only on "top" level and only on Symlink when we can meet new volume. In other words, we will have two dictionaries but they will not be nested and the number of comparisons will be less - we'll be faster.

And maybe rename Dir() in something like DirectoryVisitor() ?

It may be that on Windows the user will have only one volume, but that is much less the case on Unix. For example, on my Linux box the root (/) directory has device ID 2049, the /dev directory has ID 6, and the /proc directory has ID 4. Symlinks can cross device/volume boundaries.

I've changed the code to use the C#7 ValueTuple mechanism.

We have this on top level - see lines 1610-1616. I believe we should remove the two lines.

This function is called recursively, so I think we need to leave these lines here.

tracker = new InodeTracker(); - It's not expensive. We can always create this on top level and not check many times during recursion.

We can tracker.Visit(directory.FullName); before call the Dir.

In the Dir function, the tracker object also acts as a flag that the user gave the -FollowSymlink parameter.

I tried to refactor code and had no benefits.

Closed.

Should we check "recurse" here? Have we Recurse and FollowSymlink in only the same ParameterSet?

There are two parameter sets for Get-ChildItem: Items and LiteralItems. The -Recurse and -FollowSymlink parameters are in both parameter sets.

The Depth parameter, which is also in both, implies -Recurse by setting the Recurse property. Should -FollowSymlink follow that pattern? That would remove the check of recurse here.

We can not define explicitly mandatory sub-parameters 😕 That would be a useful feature.
So it is more safe to leave "recurse" here.

If -Depth implies -Recurse I believe we can do the same with -FollowSymlink .

Modified the -FollowSymlink parameter to set -Recurse, and the -Depth parameter does.

Context.MyInvocation.BoundParameters.ContainsKey("FollowSymlink"))

Can we use FollowSymlink property?

FollowSymlink is a property of GetChildItemCommand, not of the FileSystemProvider.

Thanks for clarify!

Closed.

We can move "tracker != null" and exclude one "if":

if (tracker != null && tracker.Visited(recursiveDirectory.FullName)) 

It seems we can exclude enterDir:

                            if (tracker != null)
                            {
                                if (tracker.Visited(recursiveDirectory.FullName))
                                {
                                    WriteWarning(StringUtil.Format(FileSystemProviderStrings.AlreadyListedDirectory,
                                                                    recursiveDirectory.FullName));
                                    continue;
                                }
                            }
                            else
                            {
                                if (InternalSymbolicLinkLinkCodeMethods.IsReparsePoint(recursiveDirectory))
                                {
                                    continue;
                                }
                            }

                            bool hidden = false;
                            if (!Force)
                            {
                                hidden = (recursiveDirectory.Attributes & FileAttributes.Hidden) != 0;
                            }

                            // if "Hidden" is explicitly specified anywhere in the attribute filter, then override
                            // default hidden attribute filter.
                            if (Force || !hidden || isFilterHiddenSpecified || isSwitchFilterHiddenSpecified)
                            {
                                Dir(recursiveDirectory, recurse, depth - 1, tracker, nameOnly, returnContainers);
                            }

Refactored code to eliminate enterDir.

Closed.

Please re-format the comment. Maybe put a) and b) on new lines. And "the directory the symlink" confuse me.

Modified the comment.

Closed.

"{0}" can bee very long so maybe we re-format the message?

Re-formatted the message to put the path at the end.

Closed.

Duplicated "@brief" ?

This one describes the function. The other describes the file.

I used the existing getlinkcount.cpp as the pattern for this.

Closed.

Can we use the pattern?

enterDir = tracker != null;

Or we should use explicit "false"?

Closed.

Is it supported on Windows 7?

It's supported as early as XP.

Closed.

Closed.

Context.MyInvocation.BoundParameters.ContainsKey("FollowSymlink"))

Can we use FollowSymlink property?

Now we don't use dev and inode and we could out Tuple.

We do still use dev and inode. That's how we uniquely identify a file system object.

I see the GetInodeData is only called in Visit and Visited method.

Yes. Did you want to move the implementation up into the InodeTracker class?

I meant simple change like internal static bool GetInodeData(string path, out (dev, inode) var)

Closed.

Can we exclude repetition "listing ... listed"?

Changed "already-listed" to "already-visited"

Closed.

In line 170

this.Recurse = true; // Bug 2391925 - Get-ChildItem -Depth should auto-set -Recurse

Should we use the same pattern?

It looks like that was in response to a specific bug. In our case it's just part of a new feature.

I'm afraid we could catch the same bug here. Maybe maintainers clarify.

The question for maintainers: should we follow the file patterns and move the private variable to a line below 342?

Please use the same pattern in Visited and Visit methods - insert a new line here or remove a line after "var" in Visit.

Thanks for catching that. New line inserted.

Closed.

Do it makes sense to break it down into two lines?

Made the calculation more concise.

Closed.

Please don't use public modifier for members of an internal class. Use internal/private as appropriate.

Fixed.

This method name should be IsPathVisited. It's good for readability.

Fixed.

I think -FollowSymlink should be a dynamic parameter provided by file system provider so that when using Get-ChildItem with another provider, this parameter won't show up.
-FollowSymlink is very much like the -CodeSigningCert parameter of Get-ChildItem -- -CodeSigningCert only applicable to the certificate provider and -FollowSymlink only applicable to FileSystemProvider. It's unlike the -Depth parameter which could be commonly meaningful to many underlying providers.

PS:17> Get-ChildItem Cert:\ -CodeSigningCert  -Recurse

   PSParentPath: Microsoft.PowerShell.Security\Certificate::CurrentUser\REQUEST

Thumbprint                                Subject
----------                                -------
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX  CN=BLAHBLAH
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX  CN=LAHBLAHB

It's doable to make -FollowSymlink depends on the appearance of -Recurse in the dynamic parameter, but I suggest for this PR let's make things simpler by not having this design. If it's a highly demanded feature, we can enable it in a separate PR.

Please take a look at the following places to see how dynamic parameters are retrieved from the underlying provider:
https://github.com/PowerShell/PowerShell/blob/master/src/Microsoft.PowerShell.Commands.Management/commands/management/GetChildrenCommand.cs#L372-L387

So then -FollowSymlink should not imply -Recurse? It has meaning only if -Recurse is also explicitly given?

powercode initiated #4102 for disscusion of parameter sets selection. So it is design case whether or not imply "mandatory" parameter by subparameter.

And what is if a path in -Path is Symlink?

Event without -FollowSymlink, the cmdlet will follow symlinks specified in -Path

Should we follow w/o FollowSymlink in the case? If no we can use FollowSymlink w/o Recurce. If yes we should rename FollowSymlink in FollowSymlinkInRecursion.

Maybe it will turn out that making -FollowSymlink implicitly imply -Recurse is better, but for now here are my 2 cents:

1. It's always better to force the user to be explicit about their intent, so dir -Recurse -FollowSymlink is more readable than dir -FollowSymlink in my opinion.
2. For simplicity reason, this PR can just add the -FollowSymlink parameter without worrying about its dependency to -Recurse. In my understanding (correct me if I'm wrong) the related code won't execute when it's not recursively iterating file system items. Then we can get feedback from users and base on that to decide if we should make -FollowSymlink implicitly imply -Recurse.

Changed to dynamic parameter. -FollowSymlink no longer implies -Recurse.