Should this be

return !DirectoryExists(drive.Root)

Seems original code is easier to understand.

This part was in windows-only section. You moved it above at the beginning of the method, but that's not made "compile-only-windows"

I assumed that we will always receive the path with the right case. (Seems we get the path in code not from user). So the first block will always return a value on all platforms. The code below will only search the file on Unix with ignoring case.

Yes, it makes sense to to always have that check on all plats. #closed

Can you please explain how do you decide which Utils.DirectoryExists or Utils.FileExists can be replaced with Directory.Exists and File.Exists (swallow all exceptions), and which ones need to continue throw exceptions? I wonder why we want to get exceptions in this method.

I tried to keep the logic of the previous code. And I tried to make as few changes as possible. I'm afraid that this PR will grow many times if we try to correct the previous logic in all places where Utils.DirectoryExists or Utils.FileExists exist.

Here I'm afraid that we'll get issues in LoadStringFromPath() .

Why do you think this method GetFilePath(string path) should throw the exception from checking the file existence?

I tried to keep the logic of the previous code

The previous code just used Utils.FileExists and Utils.DirectoryExists like everywhere else. Here, you didn't replace them to File.Exists and Directory.Exists but check the existence in a way that may throw exception. I want to know what your decision is based on.

Utils.FileExists and Utils.DirectoryExists can throw but File.Exists and Directory.Exists don't throw.
If calling code catch exceptions and ignore them we can safely replace Utils.FileExists and Utils.DirectoryExists with File.Exists and Directory.Exists.
Here calling code don't catch and ignore exceptions so I used item.Attributes.

If calling code catch exceptions and ignore them we can safely replace Utils.FileExists and Utils.DirectoryExists with File.Exists and Directory.Exists.
Here calling code don't catch and ignore exceptions so I used item.Attributes.

Does this mean for all the places you changed to File.Exists or Directory.Exists, you have checked that the calling code actually caches exceptions and ignore them?

Yes, I did this commit by commit.

[daxian-dbw] #Closed

Do you need to cast fsinfo to DirectoryInfo to access FullName?

I see FullName is in different code paths in the classes:
https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System/IO/FileInfo.cs#L29
https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs#L38

The same for Name in comment below.

Here is a quick check of the properties, and it looks the casting is not needed.

void Main()
{
	FileSystemInfo info1 = GetFileSystemInfo(@"C:\arena\temp.txt");
	FileSystemInfo info2 = GetFileSystemInfo(@"C:\arena");
	
	Console.WriteLine("info1.Name: {0}", info1.Name);
	Console.WriteLine("info1.FullName: {0}", info1.FullName);
	
	Console.WriteLine("info2.Name: {0}", info2.Name);
	Console.WriteLine("info2.FullName: {0}", info2.FullName);
}

static FileSystemInfo GetFileSystemInfo(string path)
{
    if (File.Exists(path))
	{
            return new FileInfo(path);
	}
	else if (Directory.Exists(path))
	{
	    return new DirectoryInfo(path);
	}

	return null;
}

Console Output:

info1.Name: temp.txt
info1.FullName: C:\arena\temp.txt
info2.Name: arena
info2.FullName: C:\arena

Ah, we don't need casting because the value is evaluated in object creation time and here we only retrive the value.

Fixed.
[daxian-dbw] #Closed

same here, do you need to cast?

Ah, we don't need casting because the value is evaluated in object creation time and here we only retrive the value.

Fixed.
[daxian-dbw] #Closed

No need to cast here.

Fixed.
[daxian-dbw] #Closed

same question, why we ignore all exceptions here?

This behavior corresponds to the xml description of the IsItemContainer method. We use result in MakePath or to get DirectoryInfo/FileInfo - so we always get specific error later. Seems we could cleanup using IsItemContainer() - it turns out we make a lot of extra kernel calls.

I see. I guess it makes sense to swallow all exceptions here. #closed

no need to cast here

Fixed.
[daxian-dbw] #Closed

ditto.
Actually, this if/else block can be removed.

Fixed.
[daxian-dbw] #Closed

fileSystemInfoShell still could be null, right?

Why? We call PSObject.AsPSObject(fsinfo) where fsinfo != null.

Ah, you moved the code within if (fsinfo != null). I missed that. #Closed

can you please explain why having this change? And this might not be the right place for the new code, because a new collection is just allocated above, but you might return without using the collection at all.

In this PR I went through so many code ways that I do not remember how I got here.
As far as I see the idea was simple - do not perform an analysis if the path already exists.

Fixed - I return without using the collection.

From a quick browsing of the code after this, it seems the new code you added treats the userPath as a literal path without consulting context.SuppressWildcardExpansion. Say userPath is /blah/a[bc], and it happens to have a folder named a[bc].

I'm not that familiar with the file system code, so there could be other concerns on this early return. I suggest to focus on the topic of this PR, and leave optimization to a separate issue/PR.

The code was removed.
[daxian-dbw] #Closed

This variable now should be moved in if (fsinfo != null), since it's only used in the if block.

Fixed.
[daxian-dbw] #Closed

From a quick browsing of the code after this, it seems the new code you added treats the userPath as a literal path without consulting context.SuppressWildcardExpansion. Say userPath is /blah/a[bc], and it happens to have a folder named a[bc].

I'm not that familiar with the file system code, so there could be other concerns on this early return. I suggest to focus on the topic of this PR, and leave optimization to a separate issue/PR.

The original code doesn't check for existence for the files returned from Direcotry.GetFiles. Why do you add the check now? Can't we trust direcotryInfo.GetFiles?

Please also respond to this comment: #7129 (comment)

@iSazonov Can you please respond to the first comment in this thread (#7129 (comment))?

That's the only open comment left now :)

Sorry, GitHub sometimes tricky hides comments and I can not find where to type response :-)

You are right - the check is dead code - I removed it.

Great, thanks! #Closed