dart:io | Change File.fullPath to FileSystemEntity.resolveSymbolicLinks.

dart:io | Change File.fullPath to FileSystemEntity.resolveSymbolicLinks.

BUG=dartbug.com/1016
R=sgjesse@google.com

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

[Bill Hesse, 2013-09-09 11:55:39]

https://codereview.chromium.org/23444037/diff/5001/tests/standalone/io/resolv...
File tests/standalone/io/resolve_symbolic_links_test.dart (right):

https://codereview.chromium.org/23444037/diff/5001/tests/standalone/io/resolv...
tests/standalone/io/resolve_symbolic_links_test.dart:18:
'resolve_symbolic_links_test.dart')));
Violates style guide about putting arguments on one line or all on
separatelines, but I think this is better.  But I can also change it.

[Anders Johnsen, 2013-09-09 12:24:48]

Overall comments:

- What's the long term plan here. I thought that we would remove `fullPath` and
add it as `absolutePath`?
- What's the difference between fullPath/absolutePath and resolveSymbolicLinks -
thinking long-term?
- As this is simply path operations, I think we should just have one
implementation in FileSystemEntity, calling into file.cc.

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/directory.cc#n...
runtime/bin/directory.cc:295: static CObject*
DirectoryResolveSymbolicLinksRequest(
Why don't we use the file.cc version for all paths?

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

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:500: int result_size =
GetFinalPathNameByHandle(file_handle,
From the spec:

"The string that is returned by this function uses the \\?\ syntax."

I read that as we will always get the four character header.

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:505: // Remove leading \\?\ if possible, unless input
used it.
I was under the impression that we:

- Add '\\?\' our-self if needed.
- Remove it if we get it back from the system

Meaning, the user should never see it. Is this assumption wrong?

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:507: if (result_size < MAX_PATH - 1 + 4 &&
Why test for < MAX_PATH?

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/directory.dart
File sdk/lib/io/directory.dart (right):

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/directory.dart#...
sdk/lib/io/directory.dart:91: Future<String> resolveSymbolicLinks();
Move to FileSystemEntity?

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:462: Future<String> fullPath() =>
resolveSymbolicLinks();
This looks odd. Deprecate fullPath?

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

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:245: Future<String> resolveSymbolicLinks();
Impl could be here, as it should do the same for FileSystemEntities.

https://codereview.chromium.org/23444037/diff/6001/tests/standalone/io/resolv...
File tests/standalone/io/resolve_symbolic_links_test.dart (right):

https://codereview.chromium.org/23444037/diff/6001/tests/standalone/io/resolv...
tests/standalone/io/resolve_symbolic_links_test.dart:31: .then((_) =>
Future.wait(
This is very difficult to understand. Would it be possible to split it up,
case-by-case, with a comment of what it's testing. I'm not sure what coverage is
here.

[Søren Gjesse, 2013-09-11 07:21:00]

lgtm

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

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:505: // Remove leading \\?\ if possible, unless input
used it.
On 2013/09/09 12:24:48, Anders Johnsen wrote:
> I was under the impression that we:
> 
> - Add '\\?\' our-self if needed.
> - Remove it if we get it back from the system
> 
> Meaning, the user should never see it. Is this assumption wrong?

In general we should not show '\\?\' to the user however in some cases it's
needed. In addition to the MAX_PATH case there are also the special names 'con',
'prn', etc. See
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
which should have the exhaustive list. I think we should remove \\?\ whenever
it's not needed.

In Uri.toFilePath we need the same checks.

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:462: Future<String> fullPath() =>
resolveSymbolicLinks();
We should set a deadline for when fullPath is removed.

https://codereview.chromium.org/23444037/diff/6001/tests/standalone/io/resolv...
File tests/standalone/io/resolve_symbolic_links_test.dart (right):

https://codereview.chromium.org/23444037/diff/6001/tests/standalone/io/resolv...
tests/standalone/io/resolve_symbolic_links_test.dart:57: Future testFile(String
name) {
Wouldn't it be better to have the expected output of resolveSymbolicLinks
instead of using identical? Of cause in the case of . and .. segments the test
implies that the two strings are different. However it still is somewhat subtle.

[Bill Hesse, 2013-09-13 06:34:13]

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/directory.cc#n...
runtime/bin/directory.cc:295: static CObject*
DirectoryResolveSymbolicLinksRequest(
On 2013/09/09 12:24:48, Anders Johnsen wrote:
> Why don't we use the file.cc version for all paths?

Done.

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

https://codereview.chromium.org/23444037/diff/6001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:500: int result_size =
GetFinalPathNameByHandle(file_handle,
On 2013/09/09 12:24:48, Anders Johnsen wrote:
> From the spec:
> 
> "The string that is returned by this function uses the \\?\ syntax."
> 
> I read that as we will always get the four character header.

Our current thinking is that we should not add or remove \\?\ from
the user's input, except in the case that \\?\ becomes necessary because the
path is too long.

So this code returns a path beginning with \\?\ only if the input begins with it
or if the path is longer than MAX_PATH.

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/directory.dart
File sdk/lib/io/directory.dart (right):

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/directory.dart#...
sdk/lib/io/directory.dart:91: Future<String> resolveSymbolicLinks();
On 2013/09/09 12:24:48, Anders Johnsen wrote:
> Move to FileSystemEntity?

Done.

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:462: Future<String> fullPath() =>
resolveSymbolicLinks();
On 2013/09/09 12:24:48, Anders Johnsen wrote:
> This looks odd. Deprecate fullPath?

Deprecated, with deadline added.

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

https://codereview.chromium.org/23444037/diff/6001/sdk/lib/io/file_system_ent...
sdk/lib/io/file_system_entity.dart:245: Future<String> resolveSymbolicLinks();
On 2013/09/09 12:24:48, Anders Johnsen wrote:
> Impl could be here, as it should do the same for FileSystemEntities.

Done.

https://codereview.chromium.org/23444037/diff/6001/tests/standalone/io/resolv...
File tests/standalone/io/resolve_symbolic_links_test.dart (right):

https://codereview.chromium.org/23444037/diff/6001/tests/standalone/io/resolv...
tests/standalone/io/resolve_symbolic_links_test.dart:57: Future testFile(String
name) {
On 2013/09/11 07:21:01, Søren Gjesse wrote:
> Wouldn't it be better to have the expected output of resolveSymbolicLinks
> instead of using identical? Of cause in the case of . and .. segments the test
> implies that the two strings are different. However it still is somewhat
subtle.

We cannot predict the output on all platforms, because the temp directory can be
resolved in platform- and machine-specific ways.

[Søren Gjesse, 2013-09-13 06:52:20]

lgtm

https://codereview.chromium.org/23444037/diff/17001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/23444037/diff/17001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:8: // #include "bin/file.h"
Remove?

[Bill Hesse, 2013-09-13 11:15:36]

Committed patchset #6 manually as r27475 (presubmit successful).

[Bob Nystrom, 2013-09-13 15:29:10]

https://codereview.chromium.org/23444037/diff/5001/tests/standalone/io/resolv...
File tests/standalone/io/resolve_symbolic_links_test.dart (right):

https://codereview.chromium.org/23444037/diff/5001/tests/standalone/io/resolv...
tests/standalone/io/resolve_symbolic_links_test.dart:18:
'resolve_symbolic_links_test.dart')));
On 2013/09/09 11:55:39, Bill Hesse wrote:
> Violates style guide about putting arguments on one line or all on
> separatelines, but I think this is better.

What style guide is this?