Remove Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods by iSazonov · Pull Request #7100 · PowerShell/PowerShell

iSazonov · 2018-06-18T15:03:29Z

PR Summary

Continue #6929

Cleanup code to replace Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods with .Net Core methods.

PR Checklist

adityapatwardhan · 2018-06-18T18:41:25Z

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

return was within the catch block.

@adityapatwardhan Could you please clarify why we should move the return in catch block to continue recursion removal?
Previous code was trying the recursion removal if only we can not remove reparse point silently without any error and exception - is it real scenario?

@iSazonov misread the diff. Signed off.

adityapatwardhan · 2018-06-20T00:25:50Z

@iSazonov Please have a look at the test failures.

iSazonov · 2018-06-20T05:57:27Z

Rebased to get latest fixes.

Exclude GetFileSystemInfo() and Utils.FileExists()

File.Exists and Directory.Exists return false for device names so we can not call IsReservedDeviceName(path)

…oItem()

…ath()

iSazonov · 2018-06-20T15:33:40Z

I update some commits:

Fix typo if ((int)item.Attributes != 1) -> if ((int)item.Attributes != -1)
Replace wrong pattern isDirectory = attrs.HasFlag(FileAttributes.Directory); with isDirectory = exists && attrs.HasFlag(FileAttributes.Directory);
Fix CodeFactor issues.

Seems I fixed all Appveyor CI issues. I can not build Unix version locally so I haven't a progress with Travis CI issues.

daxian-dbw · 2018-06-20T17:52:33Z

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

+            FileSystemInfo fsinfo = new FileInfo(path);
+            var attr = fsinfo.Attributes;
+            var exists = (int)attr != -1;
+            isContainer = exists && attr.HasFlag(FileAttributes.Directory);


I see code like this for checking if a path exists and whether it's directory at 8 places. I think keeping the redundant code in Utils.ItemExists is better. Thoughts?

I agree. I was hoping to remove "unnecessary" checks of existence at all but it's too complicated the PR. I suppose the correct approach is that an operation is performed and an exception is handled instead of up to 6 times to check for each file's existence.
I think I'll close the PR and open new one where add commit by commit to discover and fix Unix issues and replace ItemExists with CheckItemExistsAndThrowOnError .

Thanks @iSazonov

iSazonov · 2018-06-21T15:45:45Z

Continue in #7129

iSazonov assigned daxian-dbw Jun 18, 2018

iSazonov requested review from SteveL-MSFT and daxian-dbw June 18, 2018 15:03

iSazonov requested review from BrucePay, Francisco-Gamino, adityapatwardhan and anmenaga as code owners June 18, 2018 15:03

adityapatwardhan requested changes Jun 18, 2018

View reviewed changes

adityapatwardhan approved these changes Jun 20, 2018

View reviewed changes

iSazonov added 4 commits June 20, 2018 10:56

Use File.Exists in GetFileSecurityZone

51803c3

Use File.Exists in CommandSearcher.cs

c692d1c

Use File.Exists in AnalysisCache.cs

9f8375f

Use File.Exists in ModuleUtils.cs

5950a94

iSazonov force-pushed the cleanup-file-exists branch from 4b63525 to a0acbd0 Compare June 20, 2018 05:56

iSazonov force-pushed the cleanup-file-exists branch from a0acbd0 to 68d9f25 Compare June 20, 2018 08:05

iSazonov added 2 commits June 20, 2018 15:10

Use File.Exists in UpdatableHelpSystem.cs

4f5db0e

Use File.Exists in ModuleCmdletBase.cs

358c14b

iSazonov force-pushed the cleanup-file-exists branch from 68d9f25 to 2d28133 Compare June 20, 2018 10:10

Refactor FileSystemProvider.NormalizeThePath() method

1c1386b

Exclude GetFileSystemInfo() and Utils.FileExists()

iSazonov force-pushed the cleanup-file-exists branch 3 times, most recently from e629b77 to 132ef5e Compare June 20, 2018 14:58

iSazonov added 6 commits June 20, 2018 20:15

Use File.Exists and Directory.Exists in GetFileSystemInfo

68448bb

File.Exists and Directory.Exists return false for device names so we can not call IsReservedDeviceName(path)

Use File.Exists in CopyItemFromRemoteSession

3e0d086

Remove Utils.FileExists() method

42b5629

Use Directory.Exists in ModuleCmdletBase.cs

848d59b

Use Directory.Exists in SessionStateDriveAPIs.cs

7de56f1

Use Directory.Exists in LocationGlobber.cs

e30b381

iSazonov added 14 commits June 20, 2018 20:15

Use Directory.Exists in FileSystemProvider.cs

10f481e

Remove Utils.DirectoryExists()

4e5bd0d

Exclude Utils.ItemExists() from FileSystemProvider.ItemExists()

bb22d3c

Exclude Utils.ItemExists() from FileSystemProvider.NewItem()

a99bd63

Exclude Utils.ItemExists() from FileSystemProvider.RemoveDirectoryInf…

abff189

…oItem()

Exclude Utils.ItemExists() from FileSystemProvider.GetProperty()

666de17

Exclude Utils.ItemExists() from FileSystemProvider.NewItem() - 2

f9f08c7

Exclude Utils.ItemExists() from FileSystemProvider.NewItem() - 3

85a9865

Exclude Utils.ItemExists() from FileSystemProvider.NewItem() - 4

b81b3c5

Exclude Utils.ItemExists() from FileSystemProvider.GetPathItems()

f678fe9

Exclude Utils.ItemExists() from FileSystemProvider.SetProperty()

f57ec8a

Exclude Utils.ItemExists() from FileSystemProvider.NormalizeRelativeP…

08b2e99

…ath()

Remove Utils.ItemExists()

4dc4838

[Feature] Exclude Utils.ItemExists() from security\Utils.cs

0172040

iSazonov force-pushed the cleanup-file-exists branch from 132ef5e to 0172040 Compare June 20, 2018 15:28

daxian-dbw reviewed Jun 20, 2018

View reviewed changes

iSazonov mentioned this pull request Jun 21, 2018

Cleanup Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods #7129

Merged

11 tasks

iSazonov closed this Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods#7100

Remove Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods#7100
iSazonov wants to merge 27 commits intoPowerShell:masterfrom
iSazonov:cleanup-file-exists

iSazonov commented Jun 18, 2018

Uh oh!

adityapatwardhan Jun 18, 2018

Uh oh!

iSazonov Jun 19, 2018

Uh oh!

adityapatwardhan Jun 20, 2018

Uh oh!

adityapatwardhan commented Jun 20, 2018

Uh oh!

iSazonov commented Jun 20, 2018

Uh oh!

iSazonov commented Jun 20, 2018

Uh oh!

daxian-dbw Jun 20, 2018

Uh oh!

iSazonov Jun 21, 2018 •

edited

Loading

Uh oh!

daxian-dbw Jun 21, 2018

Uh oh!

iSazonov commented Jun 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

iSazonov commented Jun 18, 2018

PR Summary

PR Checklist

Uh oh!

adityapatwardhan Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Jun 20, 2018

Uh oh!

iSazonov commented Jun 20, 2018

Uh oh!

iSazonov commented Jun 20, 2018

Uh oh!

daxian-dbw Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jun 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iSazonov Jun 21, 2018 •

edited

Loading