Change how File/Directory/Link .delete works.

Change how File/Directory/Link .delete works.

Now:
- File.delete works iff path is File or Link.
- Directory.delete works iff path is Directory or Link.
- Link.delete works iff path Link.

This now matches .exists.

BUG=

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

[Anders Johnsen, 2013-04-05 11:21:25]

Mac and Android will follow once LGTM, as they are both copies of Linux.

It's also still missing some non-sync tests.

[Søren Gjesse, 2013-04-05 12:52:49]

This is looking good.

We need to figure out how to report errors when deleting a directory as a file.
Should we fake an OS error or not?

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/directory_win....
runtime/bin/directory_win.cc:432: }
If the name does not point to a directory this will return false. This is an
OSError, which is not present so probably an OSError indicating success will be
returned.

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

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file.cc#newcod...
runtime/bin/file.cc:500: new OSError(-1, "Not a link", OSError::kSystem));
Is this only failing when this is not a link? Could also be permission error I
guess.

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file_linux.cc
File runtime/bin/file_linux.cc (right):

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file_linux.cc#...
runtime/bin/file_linux.cc:173: }
Set errno before returning false.

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file_linux.cc#...
runtime/bin/file_linux.cc:184: return false;
Set errno before returning false.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_delete_test.dart (right):

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_delete_test.dart:87:
Expect.isFalse(link.existsSync());
Add

Expect.isFalse(new File(link.path).existsSync());

more cases below.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_delete_test.dart:93:
Expect.isTrue(link.existsSync());
Add

Expect.isTrue(new File(link.path).existsSync());

more cases below.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_delete_test.dart:193: // Not available on
Windows.
Just use 

  if (Platform.operatingSystem != 'windows') {
    ...
  }

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_exists_test.dart (right):

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_exists_test.dart:55: link.deleteSync();
Add

  Expect.isTrue(file.existsSync());
  Expect.isFalse(new File(link.path).existsSync());
  Expect.isFalse(new Directory(link.path).existsSync());

here

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_exists_test.dart:78: link.deleteSync();
Ditto.

[Anders Johnsen, 2013-04-08 06:50:49]

PTAL

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/directory_win....
runtime/bin/directory_win.cc:432: }
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> If the name does not point to a directory this will return false. This is an
> OSError, which is not present so probably an OSError indicating success will
be
> returned.

Done.

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

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file.cc#newcod...
runtime/bin/file.cc:500: new OSError(-1, "Not a link", OSError::kSystem));
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Is this only failing when this is not a link? Could also be permission error I
> guess.

You are right. Do you know how we can set this from the File::* methods?

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file_linux.cc
File runtime/bin/file_linux.cc (right):

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file_linux.cc#...
runtime/bin/file_linux.cc:173: }
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Set errno before returning false.

Done.

https://codereview.chromium.org/13654002/diff/5001/runtime/bin/file_linux.cc#...
runtime/bin/file_linux.cc:184: return false;
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Set errno before returning false.

Done.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_delete_test.dart (right):

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_delete_test.dart:87:
Expect.isFalse(link.existsSync());
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Add
> 
> Expect.isFalse(new File(link.path).existsSync());
> 
> more cases below.

These kinds of 'exists' tests are tested in the file_system_exists_test.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_delete_test.dart:93:
Expect.isTrue(link.existsSync());
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Add
> 
> Expect.isTrue(new File(link.path).existsSync());
> 
> more cases below.

Ditto

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_delete_test.dart:193: // Not available on
Windows.
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Just use 
> 
>   if (Platform.operatingSystem != 'windows') {
>     ...
>   }

Done.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_exists_test.dart (right):

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_exists_test.dart:55: link.deleteSync();
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Add
> 
>   Expect.isTrue(file.existsSync());
>   Expect.isFalse(new File(link.path).existsSync());
>   Expect.isFalse(new Directory(link.path).existsSync());
> 
> here

This is already tested in broken-link test.

https://codereview.chromium.org/13654002/diff/5001/tests/standalone/io/file_s...
tests/standalone/io/file_system_exists_test.dart:78: link.deleteSync();
On 2013/04/05 12:52:50, Søren Gjesse wrote:
> Ditto.

Same.

[Søren Gjesse, 2013-04-08 07:21:18]

LGTM!!

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/directory_win...
runtime/bin/directory_win.cc:433: errno = ENOTDIR;
I don't this this works. On Windows we use GetLastError and FormatMessageW to
handle OS errors. You should use SetLastError an appropriate error instead.

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

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:10: #include <errno.h>  // NOLINT
No need for this

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:278: errno = EINVAL;
SetLastError

[Anders Johnsen, 2013-04-08 07:44:04]

Ty, landing.

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/directory_win...
runtime/bin/directory_win.cc:433: errno = ENOTDIR;
On 2013/04/08 07:21:18, Søren Gjesse wrote:
> I don't this this works. On Windows we use GetLastError and FormatMessageW to
> handle OS errors. You should use SetLastError an appropriate error instead.

Done.

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

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:10: #include <errno.h>  // NOLINT
On 2013/04/08 07:21:18, Søren Gjesse wrote:
> No need for this

Done.

https://codereview.chromium.org/13654002/diff/16001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:278: errno = EINVAL;
On 2013/04/08 07:21:18, Søren Gjesse wrote:
> SetLastError

Done.

[Anders Johnsen, 2013-04-08 07:44:39]

Committed patchset #7 manually as r21045 (presubmit successful).

[Bill Hesse, 2013-04-08 12:16:33]

LGTM.

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

https://codereview.chromium.org/13654002/diff/23001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:279: free(const_cast<wchar_t*>(system_name));
I think there is a problem calling "free" between an implicit or explicit
SetLastError, and the GetLastError after the function returns.  See line 333 for
how we wrap a call to free elsewhere.

https://codereview.chromium.org/13654002/diff/23001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:506: CloseHandle(dir_handle);
Since we have the handle to the file, and this might be a symbolic link, rather
than a junction, should we check the attributes for file or directory (or even
the original "attributes" field), and report file or directory here?  Since we
don't support creating symbolic links, we couldn't test this.

[Anders Johnsen, 2013-04-08 12:22:33]

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

https://codereview.chromium.org/13654002/diff/23001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:279: free(const_cast<wchar_t*>(system_name));
On 2013/04/08 12:16:33, Bill Hesse wrote:
> I think there is a problem calling "free" between an implicit or explicit
> SetLastError, and the GetLastError after the function returns.  See line 333
for
> how we wrap a call to free elsewhere.

Can you link me some info on the problem? I'm not sure what the problem should
be. (it would render any stack-allocated c++ classes dangerous?)

https://codereview.chromium.org/13654002/diff/23001/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:506: CloseHandle(dir_handle);
On 2013/04/08 12:16:33, Bill Hesse wrote:
> Since we have the handle to the file, and this might be a symbolic link,
rather
> than a junction, should we check the attributes for file or directory (or even
> the original "attributes" field), and report file or directory here?  Since we
> don't support creating symbolic links, we couldn't test this.

I understand now. Yes, we should probably add that back.