dart:io | Add Link class, as sibling to File and Directory.

dart:io | Add Link class, as sibling to File and Directory.


BUG=

Committed: https://code.google.com/p/dart/source/detail?r=19772
Reverted: https://code.google.com/p/dart/source/detail?r=19777

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

[Bill Hesse, 2013-03-08 09:57:20]

Just sending out the proposed Link class, to get feedback on the API.  We know
FullPath is underspecified, and needs to be replaced/documented/augmented, but
that is a separate issue.

[Anders Johnsen, 2013-03-08 10:11:47]

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode50
sdk/lib/io/link.dart:50: void createSync();
String target

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode70
sdk/lib/io/link.dart:70: void updateSync();
String target

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode102
sdk/lib/io/link.dart:102: Future<FileSystemEntity> target();
Imo, this should be String

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode108
sdk/lib/io/link.dart:108: int targetSync();
FileSystemEntity or String

[Søren Gjesse, 2013-03-08 10:22:31]

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode24
sdk/lib/io/link.dart:24: * [:Future<bool>:] that completes when the answer is
known.
Maybe expand this comment to say that the existence if the link says nothing
about whether it points to something.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode29
sdk/lib/io/link.dart:29: * Synchronously check if the link exists.
Ditto.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode36
sdk/lib/io/link.dart:36: * will throw an exception on the future.
"the function will throw an exception on the future" -> "the future will
complete with an error".

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode53
sdk/lib/io/link.dart:53: * Updates a symbolic link to point to a different
target.
Maybe remove symbolic here, as in all other places it is just called link.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode56
sdk/lib/io/link.dart:56: * the function will throw an exception on the future.
As above change to "the future will complete with an error".

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode75
sdk/lib/io/link.dart:75: * affect, the body of the link.
body -> target

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode81
sdk/lib/io/link.dart:81: * affect, the body of the link.
body -> target

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode90
sdk/lib/io/link.dart:90: Future<Directory> directory();
Do we need directory() when we have target()?

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode100
sdk/lib/io/link.dart:100: * is either a File, Directory, or Link.
What happens if the link is broken?

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode124
sdk/lib/io/link.dart:124: String get path;
Isn't path already on FileSystemEntity?

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode124
sdk/lib/io/link.dart:124: String get path;
Should we perhaps have

String get targetPath

or is link.target().path sufficient?

[Anders Johnsen, 2013-03-08 10:27:08]

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode124
sdk/lib/io/link.dart:124: String get path;
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Should we perhaps have
> 
> String get targetPath
> 
> or is link.target().path sufficient?

Not if the link is broken. I'm all for having a helper function for getting the
entity, but the 'data' of a Link is a String, thus the reason for why I would
prefer 'target' to be String.

[Bill Hesse, 2013-03-08 12:04:26]

Comments addressed.  linkRelative optional parameter added for creating links
and reporting the target.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode24
sdk/lib/io/link.dart:24: * [:Future<bool>:] that completes when the answer is
known.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Maybe expand this comment to say that the existence if the link says nothing
> about whether it points to something.

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode24
sdk/lib/io/link.dart:24: * [:Future<bool>:] that completes when the answer is
known.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Maybe expand this comment to say that the existence if the link says nothing
> about whether it points to something.

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode36
sdk/lib/io/link.dart:36: * will throw an exception on the future.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> "the function will throw an exception on the future" -> "the future will
> complete with an error".

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode36
sdk/lib/io/link.dart:36: * will throw an exception on the future.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> "the function will throw an exception on the future" -> "the future will
> complete with an error".

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode50
sdk/lib/io/link.dart:50: void createSync();
On 2013/03/08 10:11:47, Anders Johnsen wrote:
> String target

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode50
sdk/lib/io/link.dart:50: void createSync();
On 2013/03/08 10:11:47, Anders Johnsen wrote:
> String target

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode53
sdk/lib/io/link.dart:53: * Updates a symbolic link to point to a different
target.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Maybe remove symbolic here, as in all other places it is just called link.

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode56
sdk/lib/io/link.dart:56: * the function will throw an exception on the future.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> As above change to "the future will complete with an error".

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode90
sdk/lib/io/link.dart:90: Future<Directory> directory();
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Do we need directory() when we have target()?

Directory is just the parent directory of the link location, not the link
target.  Since link-relative paths are relative to this, I think it is important
to have.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode100
sdk/lib/io/link.dart:100: * is either a File, Directory, or Link.
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> What happens if the link is broken?

Returning a String instead.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode102
sdk/lib/io/link.dart:102: Future<FileSystemEntity> target();
On 2013/03/08 10:11:47, Anders Johnsen wrote:
> Imo, this should be String

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode108
sdk/lib/io/link.dart:108: int targetSync();
On 2013/03/08 10:11:47, Anders Johnsen wrote:
> FileSystemEntity or String

Done.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode124
sdk/lib/io/link.dart:124: String get path;
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Isn't path already on FileSystemEntity?

yES. Removed.

https://codereview.chromium.org/12691002/diff/1/sdk/lib/io/link.dart#newcode124
sdk/lib/io/link.dart:124: String get path;
On 2013/03/08 10:22:31, Søren Gjesse wrote:
> Should we perhaps have
> 
> String get targetPath
> 
> or is link.target().path sufficient?

.target() now returns a string.

[nweiz, 2013-03-08 20:26:41]

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:42: * directory.
Explain what [target] is relative to if [linkRelative] is false (presumably the
CWD). Also mention whether the symlink will be relative or absolute in this case
-- that is, will "new Symlink('foo').create('bar')" create a symlink that points
to "bar", or to "`pwd`/bar"?

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:45: * target directory must exist. The link will be created
as a Junction.
Is it possible to create actual symlinks on Windows, rather than junction
points? I believe it supports them in all our supported versions. We're only
using junctions in pub because we couldn't figure out how to get the permissions
right to create real symlinks.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:124: * Otherwise, the future completes with an error.
Again, explain what happens if the target is relative and [linkRelative] is
false.

It seems unhelpful for this to complete with an error if the link happens to be
absolute when [linkRelative] is true. It would be better to return either null
or the absolute path; throwing an error just obligates everyone who uses this
method with [linkRelative] to add cumbersome error-handling.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:175: "Link.create with relative path must be
linkRelative");
This is not user-friendly behavior. This puts a lot of burden onto the user of
this code to make sure that the input path is either absolute or relative to the
link, and to make sure that [linkRelative] is set correctly. It also means that
the [linkRelative] flag doesn't do anything other than cause additional errors
if it's unset.

My preferred behavior as a user of the code would be to have "new
Symlink(linkPath).create(relativePath)" create a symlink with target
"path.relative(relativePath, from: path.dirname(linkPath))".

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:184: delete().then((link) {
return

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:186: });
It worries me that this isn't atomic. It could leave the filesystem in an
unexpected state if delete succeeds and create fails, to say nothing of
inspecting the filesystem during the operation.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:201: }
It would be nice to have a [follow] method that returned a
Future<FileSystemEntity> that is the target of the link.

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
File tests/standalone/io/file_system_links_test.dart (right):

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:9: createLink(String dst, String
link, bool symbolic, void callback()) {
The "symbolic" parameter no longer means anything.

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:19: Expect.isFalse(new
File(y).existsSync());
It's a little weird to me that all these File APIs follow links by default.
Please document this behavior in File.

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:158: for (var entry in
temp.listSync(recursive:true)) {
Shouldn't list start returning Links now?

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:239: }
I assume you're planning on adding a bunch more tests to fully exercise the Link
API?

[Bill Hesse, 2013-03-11 09:25:59]

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:42: * directory.
On 2013/03/08 20:26:41, nweiz wrote:
> Explain what [target] is relative to if [linkRelative] is false (presumably
the
> CWD). Also mention whether the symlink will be relative or absolute in this
case
> -- that is, will "new Symlink('foo').create('bar')" create a symlink that
points
> to "bar", or to "`pwd`/bar"?

Done.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:45: * target directory must exist. The link will be created
as a Junction.
Eventually, we want to provide access to all types of links, including hard
links.  The permissions problem for symbolic links on Windows is pretty severe,
so the default link type should be Junction, I think.  There is different
treatment for admin and non-admin users, even if they both have permission to
create symbolic links, and admin users are actually harder to handle than
non-admin users.


On 2013/03/08 20:26:41, nweiz wrote:
> Is it possible to create actual symlinks on Windows, rather than junction
> points? I believe it supports them in all our supported versions. We're only
> using junctions in pub because we couldn't figure out how to get the
permissions
> right to create real symlinks.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:124: * Otherwise, the future completes with an error.
On 2013/03/08 20:26:41, nweiz wrote:
> Again, explain what happens if the target is relative and [linkRelative] is
> false.
> 
> It seems unhelpful for this to complete with an error if the link happens to
be
> absolute when [linkRelative] is true. It would be better to return either null
> or the absolute path; throwing an error just obligates everyone who uses this
> method with [linkRelative] to add cumbersome error-handling.

This was intended as an initial implementation, with unimplemented cases being
filled out later.  But I have specified that the returned path will be absolute
if linkRelative is false.  This will throw an UnimplementedError currently.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:175: "Link.create with relative path must be
linkRelative");
On 2013/03/08 20:26:41, nweiz wrote:
> This is not user-friendly behavior. This puts a lot of burden onto the user of
> this code to make sure that the input path is either absolute or relative to
the
> link, and to make sure that [linkRelative] is set correctly. It also means
that
> the [linkRelative] flag doesn't do anything other than cause additional errors
> if it's unset.
> 
> My preferred behavior as a user of the code would be to have "new
> Symlink(linkPath).create(relativePath)" create a symlink with target
> "path.relative(relativePath, from: path.dirname(linkPath))".

This CL was intended as a partial, initial implementation, which is why this
case throws UnimplementedError, rather than the
LinkIOException.

We think that this is not always possible, and that creating a relative path
when it is possible and an absolute path when it isn't makes the behavior harder
to predict for the user.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:186: });
On 2013/03/08 20:26:41, nweiz wrote:
> It worries me that this isn't atomic. It could leave the filesystem in an
> unexpected state if delete succeeds and create fails, to say nothing of
> inspecting the filesystem during the operation.

This was just a temporary implementation, but you are right, it is not atomic. 
Replacing with Unimplemented until it is correct.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:201: }
On 2013/03/08 20:26:41, nweiz wrote:
> It would be nice to have a [follow] method that returned a
> Future<FileSystemEntity> that is the target of the link.

Do you mean "transitively follow links, until a non-link is reached"?  That
would be a useful method.

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
File tests/standalone/io/file_system_links_test.dart (right):

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:9: createLink(String dst, String
link, bool symbolic, void callback()) {
On 2013/03/08 20:26:41, nweiz wrote:
> The "symbolic" parameter no longer means anything.

Removed.  This removes a test for hard linking, but it will be restored when
hard links are provided by the Link class.

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:19: Expect.isFalse(new
File(y).existsSync());
On 2013/03/08 20:26:41, nweiz wrote:
> It's a little weird to me that all these File APIs follow links by default.
> Please document this behavior in File.

Done.

https://codereview.chromium.org/12691002/diff/10001/tests/standalone/io/file_...
tests/standalone/io/file_system_links_test.dart:158: for (var entry in
temp.listSync(recursive:true)) {
On 2013/03/08 20:26:41, nweiz wrote:
> Shouldn't list start returning Links now?

This will occur in a later change.  It should be made optional, so users can
either see (unfollowed) Links, or their targets, depending on an optional
parameter.

[Bill Hesse, 2013-03-11 10:35:04]

Committed patchset #4 manually as r19772 (presubmit successful).

[Mads Ager (google), 2013-03-11 11:23:45]

On 2013/03/11 10:35:04, Bill Hesse wrote:
> Committed patchset #4 manually as r19772 (presubmit successful).

Bill, was this an accidental commit. I don't see a LGTM from any reviewer on
this change.

[Bill Hesse, 2013-03-11 11:34:48]

Committed accidentally without any LGTM.  Please make additional comments, or
suggest reverting, and I will make the changes immediately.

[Søren Gjesse, 2013-03-11 12:04:17]

I think we need to agree on the exact semantics of Link.create and the optional
linkRelative argument. Also we should:

  1. Trim the Link interface to what is actually implemented
  2. Implement the explose methods on all platform.

[Søren Gjesse, 2013-03-11 12:04:40]

On 2013/03/11 12:04:17, Søren Gjesse wrote:
> I think we need to agree on the exact semantics of Link.create and the
optional
> linkRelative argument. Also we should:
> 
>   1. Trim the Link interface to what is actually implemented
>   2. Implement the explose methods on all platform.

explose -> exposed

[Bill Hesse, 2013-03-11 12:32:10]

CL reverted, reopened and reuploaded, with fixes.

I will implement Link.Create on Windows, and other methods, and remove
unimplemented methods.  Also, change dart2js to import dart:io with a prefix, to
avoid collision with class named Link in dart2js.

The semantics of relative links should be agreed upon:
1.  We need capabilities to create link-relative and absolute symbolic links.
2.  (Opinion) This cannot be done adequately by making relative target arguments
into relative links and absolute target arguments into absolute links (mainly
because of differences between CWD-relative and link-relative paths).
3.  So if we have two createLink methods, or an optional linkRelative flag on
createLink, there are at least 4 cases to handle:

a) Absolute path, no linkRelative  -> clearly, a link to an absolute target.
b) Absolute path, linkRelative -> try and make a relative path from link
location to target?  If we fail, or backtrack all the way to the root of the
file system in our relativeTo path, do we make an absolute link?
c) Relative path, linkRelative -> Interpret target as a path relative to the
link location, rather than relative to CWD?  Store relative path to target as-is
in the link?
d) Relative path, no linkRelative -> Create an absolute link to the target,
after changing it from a CWD-relative path to an absolute path.

Does this sound right to people?  Any alternatives, especially on b and C?

[Anders Johnsen, 2013-03-11 13:44:24]

My opinion is quite clear on this:

If you are working with the Link class, you know what you are doing. We only
return Link objects if you explicit ask for it (don't-follow-links directly
listing, etc). Thus, the .target property should be exactly what the system
provides - a String, either relative to the containing directory, or absolute
(if I'm wrong in this interpretation, please correct me).

Not providing exactly what the system is doing, will limit the user from working
with our API, as it'll behave differently then what's actually happening - may
it be more convenient in 90% of the cases. If we can agree on a set of 90%
helpful convenience methods, I'm all for this, as long as we make it clear that
it's an abstraction on top of the system symbolic/junction link.

Then again, this is strictly my opinion, and any API providing access to
symbolic links is a huge plus! :)

Yours,

- Anders

[Søren Gjesse, 2013-03-11 13:50:40]

On 2013/03/11 12:32:10, Bill Hesse wrote:
> CL reverted, reopened and reuploaded, with fixes.
> 
> I will implement Link.Create on Windows, and other methods, and remove
> unimplemented methods.  Also, change dart2js to import dart:io with a prefix,
to
> avoid collision with class named Link in dart2js.
> 
> The semantics of relative links should be agreed upon:
> 1.  We need capabilities to create link-relative and absolute symbolic links.
> 2.  (Opinion) This cannot be done adequately by making relative target
arguments
> into relative links and absolute target arguments into absolute links (mainly
> because of differences between CWD-relative and link-relative paths).
> 3.  So if we have two createLink methods, or an optional linkRelative flag on
> createLink, there are at least 4 cases to handle:
> 
> a) Absolute path, no linkRelative  -> clearly, a link to an absolute target.
> b) Absolute path, linkRelative -> try and make a relative path from link
> location to target?  If we fail, or backtrack all the way to the root of the
> file system in our relativeTo path, do we make an absolute link?
> c) Relative path, linkRelative -> Interpret target as a path relative to the
> link location, rather than relative to CWD?  Store relative path to target
as-is
> in the link?
> d) Relative path, no linkRelative -> Create an absolute link to the target,
> after changing it from a CWD-relative path to an absolute path.
> 
> Does this sound right to people?  Any alternatives, especially on b and C?

I am not sure we should try to be so clever when creating a link. How about just
setting the link target to the literal text passed? Then explain in the comment
that this is what it does and the user will have to construct the correct path.

When using the ln command there is also an inconsistency. If you have the
following directory structure:

a
 b
  c

and cwd is b.

Then

ln ../a b/c

will create a working hardlink whereas

ln -s ../a b/c

will create a broken symlink.

Instead of trying to be clever with create we could add a method the method
linkTo which takes a FileSystemEntity which could then do some well defined path
gymnastics.

[Søren Gjesse, 2013-03-11 13:51:54]

On 2013/03/11 13:50:40, Søren Gjesse wrote:
> On 2013/03/11 12:32:10, Bill Hesse wrote:
> > CL reverted, reopened and reuploaded, with fixes.
> > 
> > I will implement Link.Create on Windows, and other methods, and remove
> > unimplemented methods.  Also, change dart2js to import dart:io with a
prefix,
> to
> > avoid collision with class named Link in dart2js.
> > 
> > The semantics of relative links should be agreed upon:
> > 1.  We need capabilities to create link-relative and absolute symbolic
links.
> > 2.  (Opinion) This cannot be done adequately by making relative target
> arguments
> > into relative links and absolute target arguments into absolute links
(mainly
> > because of differences between CWD-relative and link-relative paths).
> > 3.  So if we have two createLink methods, or an optional linkRelative flag
on
> > createLink, there are at least 4 cases to handle:
> > 
> > a) Absolute path, no linkRelative  -> clearly, a link to an absolute target.
> > b) Absolute path, linkRelative -> try and make a relative path from link
> > location to target?  If we fail, or backtrack all the way to the root of the
> > file system in our relativeTo path, do we make an absolute link?
> > c) Relative path, linkRelative -> Interpret target as a path relative to the
> > link location, rather than relative to CWD?  Store relative path to target
> as-is
> > in the link?
> > d) Relative path, no linkRelative -> Create an absolute link to the target,
> > after changing it from a CWD-relative path to an absolute path.
> > 
> > Does this sound right to people?  Any alternatives, especially on b and C?
> 
> I am not sure we should try to be so clever when creating a link. How about
just
> setting the link target to the literal text passed? Then explain in the
comment
> that this is what it does and the user will have to construct the correct
path.
> 
> When using the ln command there is also an inconsistency. If you have the
> following directory structure:
> 
> a
>  b
>   c
> 
> and cwd is b.
> 
> Then
> 
> ln ../a b/c
> 
> will create a working hardlink whereas
> 
> ln -s ../a b/c
> 
> will create a broken symlink.
> 
> Instead of trying to be clever with create we could add a method the method
> linkTo which takes a FileSystemEntity which could then do some well defined
path
> gymnastics.

The directory structure above does of cause not contain c before executing the
ln command.

[Bill Hesse, 2013-03-11 14:49:01]

OK, so it sounds like we can use a single createLink function which, in the case
of symbolic links, will set the link target to whatever string is specified for
the target.  This will therefore be a link-relative target if it is relative,
and an absolute target if it is absolute.

If we implement hard links with the other semantics (cwd-relative paths), then
we will just document that difference.

Therefore, we will remove the linkRelative optional argument, and only support
linkRelative targets, that we do not manipulate at all.

[nweiz, 2013-03-11 20:26:32]

All the changes I'm looking at LGTM.

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/10001/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:201: }
On 2013/03/11 09:25:59, Bill Hesse wrote:
> On 2013/03/08 20:26:41, nweiz wrote:
> > It would be nice to have a [follow] method that returned a
> > Future<FileSystemEntity> that is the target of the link.
> 
> Do you mean "transitively follow links, until a non-link is reached"?  That
> would be a useful method.

No, I meant just once, although a transitive version (or flag) might also be
useful.

[Søren Gjesse, 2013-03-12 08:41:49]

https://codereview.chromium.org/12691002/diff/22002/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/12691002/diff/22002/runtime/bin/file.cc#newco...
runtime/bin/file.cc:443: Dart_Handle err = DartUtils::NewDartOSError(&os_error);
I think we should use DartUtils::ArgumentError here.

https://codereview.chromium.org/12691002/diff/22002/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/22002/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:47: Future<Link> create(String target, {bool linkRelative:
false });
I think we agreed on getting rid of linkRelative and just have create use the
literal string.

https://codereview.chromium.org/12691002/diff/22002/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:78: Future<Link> update(String target, {bool linkRelative:
false });
Please remove all the methods not implemented, and add them as they get
implemented.

[Bill Hesse, 2013-03-13 16:17:37]

Windows implementation of Link.create added.  link_test.dart and
windows_file_system_links_test.dart added.  More tests of Link will be added in
future CLs.  Comments addressed.

Please take a look.  The remaining files are unchanged.

https://codereview.chromium.org/12691002/diff/22002/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/12691002/diff/22002/runtime/bin/file.cc#newco...
runtime/bin/file.cc:443: Dart_Handle err = DartUtils::NewDartOSError(&os_error);
On 2013/03/12 08:41:49, Søren Gjesse wrote:
> I think we should use DartUtils::ArgumentError here.

Done.

https://codereview.chromium.org/12691002/diff/22002/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12691002/diff/22002/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:47: Future<Link> create(String target, {bool linkRelative:
false });
On 2013/03/12 08:41:49, Søren Gjesse wrote:
> I think we agreed on getting rid of linkRelative and just have create use the
> literal string.

Done.

https://codereview.chromium.org/12691002/diff/22002/sdk/lib/io/link.dart#newc...
sdk/lib/io/link.dart:78: Future<Link> update(String target, {bool linkRelative:
false });
On 2013/03/12 08:41:49, Søren Gjesse wrote:
> Please remove all the methods not implemented, and add them as they get
> implemented.

Done.

[Søren Gjesse, 2013-03-13 16:44:01]

lgtm

https://codereview.chromium.org/12691002/diff/10002/tests/standalone/io/link_...
File tests/standalone/io/link_test.dart (right):

https://codereview.chromium.org/12691002/diff/10002/tests/standalone/io/link_...
tests/standalone/io/link_test.dart:12: bool windows = false;
This is not used yet.

[Bill Hesse, 2013-03-14 09:37:59]

Committed patchset #10 manually as r19995 (presubmit successful).