-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Fix Unix FileStatus flag retrieval without perf regression #47348
Changes from 29 commits
a50c0fc
b8f69b7
84433dd
5070cde
0686368
5e5932b
a866e73
48d0d5a
96dd50c
f6b35c3
f865030
334b561
e8e5d9b
82c382f
4dbb740
b3cb947
a69901b
9dc2681
6ba7c79
4b3aec0
75552fe
62b77b8
ac7062b
c4aa27f
371d1a4
f9e1619
6c489ad
136e2c3
43476e7
a869d2c
76dd793
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,8 +36,10 @@ internal static FileAttributes Initialize( | |
|
||
// IMPORTANT: Attribute logic must match the logic in FileStatus | ||
|
||
entry._status = default; | ||
entry._status.Invalidate(); | ||
|
||
bool isDirectory = false; | ||
bool isSymlink = false; | ||
if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_DIR) | ||
{ | ||
// We know it's a directory. | ||
|
@@ -46,38 +48,45 @@ internal static FileAttributes Initialize( | |
// Some operating systems don't have the inode type in the dirent structure, | ||
// so we use DT_UNKNOWN as a sentinel value. As such, check if the dirent is a | ||
// directory. | ||
else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK | ||
|| directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN) | ||
&& Interop.Sys.Stat(entry.FullPath, out Interop.Sys.FileStatus targetStatus) >= 0) | ||
else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK || directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN) && | ||
entry._status.TryRefreshSecondaryCache(entry.FullPath)) // Only call stat if inode is link or unknown | ||
{ | ||
// Symlink or unknown: Stat to it to see if we can resolve it to a directory. | ||
isDirectory = (targetStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR; | ||
// Symlink or unknown: Stat should tell us if we can resolve it to a directory. | ||
isDirectory = entry._status.HasSecondaryDirectoryFlag; | ||
} | ||
// Same idea as the directory check, just repeated for (and tweaked due to the | ||
// nature of) symlinks. | ||
|
||
// Same idea as the directory check, just repeated for (and tweaked due to the nature of) symlinks. | ||
bool isSymlink = false; | ||
if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK) | ||
{ | ||
isSymlink = true; | ||
} | ||
else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN) | ||
&& (Interop.Sys.LStat(entry.FullPath, out Interop.Sys.FileStatus linkTargetStatus) >= 0)) | ||
else if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN && | ||
entry._status.TryRefreshMainCache(entry.FullPath)) // Only call lstat if inode is unknown | ||
{ | ||
isSymlink = (linkTargetStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK; | ||
isSymlink = entry._status.HasSymbolicLinkFlag; | ||
} | ||
|
||
entry._status = default; | ||
FileStatus.Initialize(ref entry._status, isDirectory); | ||
entry._status.InitiallyDirectory = isDirectory; | ||
|
||
FileAttributes attributes = default; | ||
if (isSymlink) | ||
{ | ||
attributes |= FileAttributes.ReparsePoint; | ||
} | ||
if (isDirectory) | ||
{ | ||
attributes |= FileAttributes.Directory; | ||
if (directoryEntry.Name[0] == '.') | ||
} | ||
if (entry.IsHidden) | ||
{ | ||
attributes |= FileAttributes.Hidden; | ||
} | ||
|
||
if (attributes == default) | ||
{ | ||
attributes = FileAttributes.Normal; | ||
} | ||
|
||
entry._initialAttributes = attributes; | ||
return attributes; | ||
|
@@ -143,12 +152,22 @@ public FileAttributes Attributes | |
public DateTimeOffset LastAccessTimeUtc => _status.GetLastAccessTime(FullPath, continueOnError: true); | ||
public DateTimeOffset LastWriteTimeUtc => _status.GetLastWriteTime(FullPath, continueOnError: true); | ||
public bool IsDirectory => _status.InitiallyDirectory; | ||
public bool IsHidden => _directoryEntry.Name[0] == '.'; | ||
public bool IsHidden => | ||
(_directoryEntry.NameLength > 0 && _directoryEntry.Name[0] == '.') || _status.IsHidden(FullPath, continueOnError: true); | ||
|
||
public FileSystemInfo ToFileSystemInfo() | ||
{ | ||
Debug.Assert(!PathInternal.IsPartiallyQualified(FullPath), "FullPath should be fully qualified when constructed from directory enumeration"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debug.Assert in public API? If it is important check it should be a real time check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to the other comment below, I decided to revert this change since it was unrelated to the purpose of this PR. |
||
|
||
string fullPath = ToFullPath(); | ||
return FileSystemInfo.Create(fullPath, new string(FileName), ref _status); | ||
string fileName = new(FileName); | ||
|
||
FileSystemInfo info = IsDirectory | ||
? new DirectoryInfo(fullPath, fileName: fileName, isNormalized: true) | ||
: new FileInfo(fullPath, fileName: fileName, isNormalized: true); | ||
|
||
info.Init(ref _status); | ||
Comment on lines
+171
to
+175
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not create internal ctor-s with the parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I can revert this change. Originally it was not a constructor but a factory method. I thought we could save one method call if I put all that logic in the only place where it was called. But it's unrelated to this change anyway. |
||
return info; | ||
} | ||
|
||
/// <summary> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to encapsulate the logic that is modifying the
FileStatus
below and move it intoFileStatus
itself? So theFileStatus
could keep all the caching logic as it's private detail and return only a validFileStatus
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.