dart:io | Add FileSystemEntity.typeSync to test for symbolic links.

dart:io | Add FileSystemEntity.typeSync to test for symbolic links.

BUG=dartbug.com/8935

Committed: https://code.google.com/p/dart/source/detail?r=19608

[Bill Hesse, 2013-03-06 16:36:07]

The Windows implementation should be pretty easy, and follow within a day, and
similarly the async methods should be easy.

[Søren Gjesse, 2013-03-06 17:47:32]

lgtm

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:38: static FileSystemEntityType
typeSync(String path, {bool followLinks: true})
Use { } body for multiline methods.

[Bob Nystrom, 2013-03-06 19:02:52]

Just skimmed the API but this looks like a big step in the right direction.

Once we can detect links, will dart:io be able to work with them? If I do new
File("path/to/broken/symlink").deleteSync(), will that work?

https://codereview.chromium.org/12533008/diff/2001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12533008/diff/2001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:284: //  if (S_ISLNK(entry_info.st_mode)) return
File::kIsLink;
What kind of symlinks does this detect? For what it's worth, pub uses NTFS
junction points for "symlinks" on Windows.

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:11: static const NOT_FOUND = const
FileSystemEntityType._internal(3);
This feels wrong to me. "NOT FOUND" isn't a kind of file. I would prefer
throwing an exception for this case.

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:14: String toString() => ['FILE',
'DIRECTORY', 'LINK', 'NOT_FOUND'][_type];
Why not just make the type the string instead of an int?

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:27: abstract class FileSystemEntity {
Tangential, but how about "Entry" for this type name? I believe that's the most
common word used to mean "file or directory".

https://codereview.chromium.org/12533008/diff/2001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_links_test.dart (right):

https://codereview.chromium.org/12533008/diff/2001/tests/standalone/io/file_s...
tests/standalone/io/file_system_links_test.dart:9: createLink(String dst, String
link, bool symbolic, void callback()) {
Would be nice if this worked on Windows too. :)

[Bill Hesse, 2013-03-07 09:53:12]

https://codereview.chromium.org/12533008/diff/2001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12533008/diff/2001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:284: //  if (S_ISLNK(entry_info.st_mode)) return
File::kIsLink;
The Windows implementation will report all types of rewrite points as links,
including junction points and symbolic links.  We may need to add extra code if
we want to avoid reporting mount points of shares as links.

On Linux and Mac, this detects symbolic links, but not hard links (since hard
links are identical to normal files and directories).

On 2013/03/06 19:02:52, Bob Nystrom wrote:
> What kind of symlinks does this detect? For what it's worth, pub uses NTFS
> junction points for "symlinks" on Windows.

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:11: static const NOT_FOUND = const
FileSystemEntityType._internal(3);
On 2013/03/06 19:02:52, Bob Nystrom wrote:
> This feels wrong to me. "NOT FOUND" isn't a kind of file. I would prefer
> throwing an exception for this case.

I think the exception can be harder to handle than dealing with the cases, since
we are probably handling cases already if we call this.  I'm not convinced by
the argument about "type" - this could also be called "status", or the name
could be changed from "NOT_FOUND" to "NONEXISTENT".

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:14: String toString() => ['FILE',
'DIRECTORY', 'LINK', 'NOT_FOUND'][_type];
On 2013/03/06 19:02:52, Bob Nystrom wrote:
> Why not just make the type the string instead of an int?

I'd like other's comments on this.  This is an implementation issue, that does
not affect the API.  Moving the "lookup" constructor to a factory in this class.

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:27: abstract class FileSystemEntity {
On 2013/03/06 19:02:52, Bob Nystrom wrote:
> Tangential, but how about "Entry" for this type name? I believe that's the
most
> common word used to mean "file or directory".

I think that is a good idea too.  What do others think?

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:38: static FileSystemEntityType
typeSync(String path, {bool followLinks: true})
Moved to a static method, and done.

On 2013/03/06 17:47:33, Søren Gjesse wrote:
> Use { } body for multiline methods.

https://codereview.chromium.org/12533008/diff/2001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_links_test.dart (right):

https://codereview.chromium.org/12533008/diff/2001/tests/standalone/io/file_s...
tests/standalone/io/file_system_links_test.dart:9: createLink(String dst, String
link, bool symbolic, void callback()) {
On 2013/03/06 19:02:52, Bob Nystrom wrote:
> Would be nice if this worked on Windows too. :)

That will be added with the Link class.

https://codereview.chromium.org/12533008/diff/2001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/12533008/diff/2001/tests/standalone/standalon...
tests/standalone/standalone.status:19: io/file_system_links_test: Fail # Issue
8935
The test is already skipped on Windows.  Removed

[Bill Hesse, 2013-03-07 11:18:55]

Committed patchset #3 manually as r19608 (presubmit successful).

[Søren Gjesse, 2013-03-07 11:33:30]

Still lgtm

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:11: static const NOT_FOUND = const
FileSystemEntityType._internal(3);
On 2013/03/07 09:53:12, Bill Hesse wrote:
> On 2013/03/06 19:02:52, Bob Nystrom wrote:
> > This feels wrong to me. "NOT FOUND" isn't a kind of file. I would prefer
> > throwing an exception for this case.
> 
> I think the exception can be harder to handle than dealing with the cases,
since
> we are probably handling cases already if we call this.  I'm not convinced by
> the argument about "type" - this could also be called "status", or the name
> could be changed from "NOT_FOUND" to "NONEXISTENT".

I agree with Bill here. Lets call it "NONEXISTENT".

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:14: String toString() => ['FILE',
'DIRECTORY', 'LINK', 'NOT_FOUND'][_type];
On 2013/03/07 09:53:12, Bill Hesse wrote:
> On 2013/03/06 19:02:52, Bob Nystrom wrote:
> > Why not just make the type the string instead of an int?
> 
> I'd like other's comments on this.  This is an implementation issue, that does
> not affect the API.  Moving the "lookup" constructor to a factory in this
class.

This is an implementation issue so I don't care whether it is an int or a String
- it is not exposed.

https://codereview.chromium.org/12533008/diff/2001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:27: abstract class FileSystemEntity {
On 2013/03/07 09:53:12, Bill Hesse wrote:
> On 2013/03/06 19:02:52, Bob Nystrom wrote:
> > Tangential, but how about "Entry" for this type name? I believe that's the
> most
> > common word used to mean "file or directory".
> 
> I think that is a good idea too.  What do others think?
> 

I think Entry is a better name. I kind of says that to might not be anything
behind the path whether Entity indicate that there is an entity pointed to. It
is a long name, so we might as well just change it to FSEntry unless we find
something better soon.

[Mads Ager (google), 2013-03-07 11:41:11]

On 2013/03/06 19:02:52, Bob Nystrom wrote:
> Just skimmed the API but this looks like a big step in the right direction.
> 
> Once we can detect links, will dart:io be able to work with them? If I do new
> File("path/to/broken/symlink").deleteSync(), will that work?

I don't see a direct answer to this part: We are going to add a Link class that
should be used to work with links. So the code to delete a broken link would be

new Link('path/to/broken/symlink').deleteSync();

[Bob Nystrom, 2013-03-12 21:05:11]

On 2013/03/07 11:41:11, Mads Ager wrote:
> On 2013/03/06 19:02:52, Bob Nystrom wrote:
> > Just skimmed the API but this looks like a big step in the right direction.
> > 
> > Once we can detect links, will dart:io be able to work with them? If I do
new
> > File("path/to/broken/symlink").deleteSync(), will that work?
> 
> I don't see a direct answer to this part: We are going to add a Link class
that
> should be used to work with links. So the code to delete a broken link would
be
> 
> new Link('path/to/broken/symlink').deleteSync();

SGTM!