Skip to content

Add support for same on POSIX #211

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brianquinlan
Copy link
Contributor

  • This is different than dart:io's identical method because it resolves the final path component if it is a symlink

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

/// Links are resolved before determining if the paths refer to the same
/// object. Throws `PathNotFoundException` if either path requires resolving
/// a broken link.
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bool same(String path1, String path2) {
throw UnsupportedError('same');
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just not have this method, if it's not supported on all file systems.
Only have it on PosixFileSystem.

If you have to do is PosixFileSystem before you can use it safely, better just have to do that before being able to use it at all.

If you plan to implement it on all file systems, make it abstract here, to force an implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be implemented on all file systems.

@@ -30,6 +30,18 @@ class WriteMode {

/// An abstract representation of a file system.
base class FileSystem {
Copy link
Member

Choose a reason for hiding this comment

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

Make this class abstract?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've been discussing this at #202

I'll leave it unchanged here and hopefully we can come to a conclusion in that PR ;-)

throw _getError(errno, 'stat failed', path2);
}

return (stat1.st_ino == stat2.st_ino) && (stat1.st_dev == stat2.st_dev);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it equates filled with the same inode. That means that hard links, non-symbolic links, to the same inode should be equal, even if they resolve to filled with different paths.

Should be tested too:

  • two files linked to same inode.
  • two different paths linked through hard links to the same directory.

Generally, test with targets that are directories too.

Do symlinks have inodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it equates filled with the same inode. That means that hard links, non-symbolic links, to the same inode should be equal, even if they resolve to filled with different paths.

Yep.

Should be tested too:

  • two files linked to same inode.
  • two different paths linked through hard links to the same directory.

Will do. I'll need to make some upstream changes to package:stdlibc.

Generally, test with targets that are directories too.

I already do this.

Do symlinks have inodes?

Yes, but same resolves the symlink before checking the inode.

FileSystemEntity.identical does not resolve the final symlink but same does. For example:

Link('foo').create('bar');
FileSystemEntity.identical('foo', 'bar'); // false
same('foo', 'bar'); // true

I was thinking that maybe same should be called equivalent and then I could have an identical that behaves the same was as FileSystemEntity.identical. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants