Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FileSystemEntry.Unix: ensure properties are available when file is deleted. #60214

Merged
merged 12 commits into from
Nov 18, 2021
Merged

FileSystemEntry.Unix: ensure properties are available when file is deleted. #60214

merged 12 commits into from
Nov 18, 2021

Conversation

tmds
Copy link
Member

@tmds tmds commented Oct 9, 2021

When the file no longer exists, we create attributes based on what we know.

The test for this was passing because it cached the attributes before the
item was deleted due to enumerating with skipping FileAttributes.Hidden.

@carlossanlop ptal.

@tmds
FileSystemEntry.Unix: ensure attributes are available when file is de…
47183e9 
…leted.

When the file no longer exists, we create attributes based on what we know.

The test for this was passing because it cached the attributes before the
item was deleted due to enumerating with skipping FileAttributes.Hidden.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 9, 2021
@ghost
Copy link

ghost commented Oct 9, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

When the file no longer exists, we create attributes based on what we know.

The test for this was passing because it cached the attributes before the
item was deleted due to enumerating with skipping FileAttributes.Hidden.

@carlossanlop ptal.

Author: tmds
Assignees: -
Labels:

area-System.IO, community-contribution
Milestone: -

@tmds
Copy link
Member Author

tmds commented Oct 9, 2021

I'll extend the tests so it covers more of the changes.

@tmds tmds changed the title FileSystemEntry.Unix: ensure attributes are available when file is deleted. FileSystemEntry.Unix: ensure properties are available when file is deleted. Oct 11, 2021
@tmds
Copy link
Member Author

tmds commented Oct 12, 2021

This is up for review.

@tmds tmds mentioned this pull request Oct 13, 2021
Merged
@tmds
Copy link
Member Author

tmds commented Oct 26, 2021

@carlossanlop ptal. This fixes a bug in how FileSystemEntry behaves when the underlying entry was deleted. There is a test for this, but it inadvertently captured the properties up-front due to the default EnumerationOptions checking to skip FileAttributes.Hidden. I've extended the test.

@tmds
Copy link
Member Author

tmds commented Nov 9, 2021

@dotnet/area-system-io @stephentoub ptal.

@tmds
Copy link
Member Author

tmds commented Nov 15, 2021

@jeffhandley @carlossanlop @jozkee @adamsitnik can you make some time to review the PRs I've created for System.IO: https://github.com/dotnet/runtime/pulls?q=is%3Aopen+is%3Apr+author%3Atmds+label%3Aarea-System.IO.

adamsitnik
adamsitnik reviewed Nov 15, 2021
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but it would be great if @carlossanlop or @jozkee could review the FileSystemEntry changes.

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs Outdated Show resolved Hide resolved
src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs Show resolved Hide resolved
@tmds
Copy link
Member Author

tmds commented Nov 17, 2021

I'll look at addressing the feedback tomorrow.

carlossanlop
carlossanlop reviewed Nov 17, 2021
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far, only 3 things left to address:

  • Remove the AssertExtensions.EqualsTo code.
  • Please open an issue to describe the two bugs being fixed by this PR.
  • In particular, the second bug, which you mentioned in a comment in line 333 of FileStatus.Unix.cs, I'm not sure how you caught that, or if it needs a unit test to verify the change worked. Would you mind elaborating? Normally we expect to have an issue open first, where the bug is described and discussed.

Thanks for sending the PR, and apologies for the delay.

src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs Show resolved Hide resolved
@tmds
Copy link
Member Author

tmds commented Nov 18, 2021

@carlossanlop I've created #61764 to described the issues that are fixed by this PR.

adamsitnik
adamsitnik approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @tmds!

@adamsitnik adamsitnik added this to the 7.0.0 milestone Nov 18, 2021
@adamsitnik adamsitnik merged commit 5181d12 into dotnet:main Nov 18, 2021
@adamsitnik adamsitnik mentioned this pull request Nov 18, 2021
Closed
@tmds
Copy link
Member Author

tmds commented Nov 18, 2021

Thanks for the review @adamsitnik and @carlossanlop!

This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlossanlop carlossanlop carlossanlop left review comments

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees

@carlossanlop carlossanlop

Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tmds @carlossanlop @adamsitnik