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.

if (recurse && Context.MyInvocation.BoundParameters.ContainsKey("FollowSymlink"))

This is fragile as you don't check the value. Think about dir -follow:$false. We should go with the dynamic parameter approach for -FollowSymlink

I think Skip already-visited directory {0} might be better.

Should this be "Skip"? Or perhaps "Skipping" or "Skipped"?

Fixed.

I see that you are following a pattern like IsSameFileSystemItem or IsHardLink. Ideally, code here should be refactored to consolidate Unix/Windows implementations in one place and eliminate wrapper functions like IsSameFileSystemItem and IsHardLink. Instead they should be

internal static bool IsSameFileSystemItem(string pathOne, string pathTwo)
{
#if UNIX
   // Unix implementation goes here
#else
   // Windows implementation goes here
#endif
}

I'm not asking you to do this change for this PR, just want to call out that it should be this way moving forward. I opened #4086 to track the refactoring task.

Correspondingly, maybe this name can be changed to VisitPath.

And, does this method really need to return anything? If GetInodeData failed, then we just don't skip anything.

Fixed both.

Now we return nothing. Please remove the line.

Closed.

Wrong pattern.
We can use tracker?.VisitPath(directory.FullName); or if (...) { ... }.

Closed.

We can remove one if.

if (fspDynamicParam?.FollowSymlink) 

Sorry, I again under 'tracker == null' doulble check pressure.
It seem we can remove one if remove lines 1675-1678, add its before first Dir call and here add:

                                else
                                {
                                    tracker.VisitPath(directory.FullName);
                                }

Next, we can exclude VisitPath at all if make "visit" in IsPathVisited (we should rename it maybe as TryPathVisit).

I don't agree with this. If we mark the directory has having been visited here, then if the test at line 1883 evaluates to false the Dir method will not be called and we will not visit the directory even though we have already claimed to have done so.

Do you mean "hidden" directories? If so we can safely mark "hidden" directories as "visited" - in both cases they is really skipped.

A directory might be hidden. A symbolic link (or several such) might not be.

You are right. We can move the hidden check above line 1861 - it is less expensive then double call 'GetInodeData' in VisitPath and IsPathVisited.

Now CoreFX has a new method Dictionary.TryAdd - Could we use them?

Used new method

Missing </summary>

Fixed.

Why using a Dictionary? It seems HashSet would be good.

We use new Dictionary.TryAdd() but HashSet.Add() works the same - so we can use HashSet.

HashSet is much better. Fixed.

A new parameter should usually be added to the end of the parameter list.

Fixed.

If recurse == false, then we should avoid creating the tracker.

if (recurse)
{
    GetChildDynamicParameters fspDynamicParam = DynamicParameters as GetChildDynamicParameters;
    if (fspDynamicParam != null && fspDynamicParam.FollowSymlink)
    {
        tracker = new InodeTracker(directory.FullName);
    }
}

I thought I had a check for recurse at one time. It must have gotten lost in the shuffle. Fixed.

Here we can use else if (...) so that one nested block can be avoided.

Fixed.

This method goes against the convention of Try<Verb> pattern. Take TryAdd as an instance, if added, then return true; if not added, then return false. Following that conversion, TryVisitPath should be: if visited, then return true; if not visited, then return false. So TryVisitPath makes it confusing understanding the code.

How about making it IsPathVisited(bool visitIfNotAlready = false)?

internal bool IsPathVisited(bool visitIfNotAlready = false)
{
    bool isPathVisited = false;
    if (InternalSymbolicLinkLinkCodeMethods.GetInodeData(path, out (UInt64, UInt64) inodeData))
    {
        if (visitIfNotAlready)
        {
            isPathVisited = !_visitations.Add(inodeData)
        }
        else
        {
            isPathVisited = _visitations.Contains(inodeData)
        }
    }
    return isPathVisited;
}

With HashSet it will looks just better.

Fixed.

This class seems to be a helper that will only be used in FileSystemProvider code, so can we make it a private nested class within FileSystemProvider? (just like private static class NativeMethods in FileSystemProvider)

Fixed.

Minor comment: maybe use the following pattern to avoid var inodeData = (0UL, 0UL);?

if (InternalSymbolicLinkLinkCodeMethods.GetInodeData(path, out (UInt64, UInt64) inodeData))

Looks good.

Fixed.

Should we add the parameter if we don't use them?

Normally I would say no. In this case the existence of the parameter and its name give a clue about the side effect of marking the path as visited.

I give this some more thinking, and believes that the TryVisitPath signature is more natural in this case. But we need to follow the Try<> pattern.

Again, take HashSet.Add(object) as an example:

1. If the object is not in the set, add it and return true.
2. If the object is already in the set, return false which means it was not added in this call because it's already there.

Similarly, InodeTracker.TryVisitPath(path) should be

1. If the path is not visited yet, visit it and return true.
2. If the path is already visited, return false which means it was not visited in this call because it's already visited.

The code would be like:

internal bool TryVisitPath(string path)
{
    bool returnValue = false;
    if (InternalSymbolicLinkLinkCodeMethods.GetInodeData(path, out (UInt64, UInt64) inodeData))
    {
        returnValue = _visitations.Add(inodeData);
    }
    return returnValue;
}

jeffbi Ilya (@iSazonov) what do you think?

I like the TryVisitPath more the IsVisitPath.

Works for me. I'll make the change.