Implement recursive directory deletion.

Implement recursive directory deletion.

Additionally, make sure that the handler can be registered after
the operation in file and directory implementation:

  dir.delete();
  dir.deleteHandler = () => print('hest');

BUG=
TEST=

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

[Bill Hesse, 2012-02-02 17:24:39]

Note that PATH_MAX is just a limit on the behavior of certain APIs and function
calls, not a limit on the OS or file system, so if we choose to fail on absolute
paths longer than PATH_MAX, we should do so cleanly.

We can make this a todo, as long as we don't fail unsafely.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory.dart
File runtime/bin/directory.dart (right):

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory.dart#new...
runtime/bin/directory.dart:70: * and files in the directory is deleted. If
[recursive] is
... are deleted.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc
File runtime/bin/directory_posix.cc (right):

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:224: static bool DeleteFile(char* file_name,
Put a comment that strlen(path) may not be path_length, but that path_length is
the valid part of path?

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:245: ASSERT(written == strlen(dir_name));
In release mode, could a snprintf turn "long_path/..funny_directory" into
"long_path/..", and get it to DeleteRecursively?  It seems unlikely, but I'm not
seeing anything that makes it impossible.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:266: ComputeFullPath(dir_name, path,
&path_length);
ASSERT on path_length here?

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:268: // Iterated the directory and delete all
files and directories.
Iterate the directory ...

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:320: return !error;
Every use of error really wants to use !error instead.  Is there a good name for
!error, so the sense could be reversed?

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_win.cc#n...
runtime/bin/directory_win.cc:221: ASSERT(written == strlen(file_name));
Especially on Windows, I think this assert really could fail - files can exist
that have full paths that are too long (>260 chars).  So I think we should
cleanly handle this problem, by returning failure from DeleteRecursively.  We
don't have to succeed, we just should fail correctly.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_win.cc#n...
runtime/bin/directory_win.cc:235: ASSERT(written == strlen(dir_name));
These could all be made checks, rather than asserts, I think.

http://codereview.chromium.org/9316066/diff/10/tests/standalone/src/Directory...
File tests/standalone/src/DirectoryTest.dart (right):

http://codereview.chromium.org/9316066/diff/10/tests/standalone/src/Directory...
tests/standalone/src/DirectoryTest.dart:38: directory.delete(recursive: true);
I think we need some major tests for the new recursive delete.

[Mads Ager (google), 2012-02-03 11:48:54]

Thanks! Please take another look.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory.dart
File runtime/bin/directory.dart (right):

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory.dart#new...
runtime/bin/directory.dart:70: * and files in the directory is deleted. If
[recursive] is
On 2012/02/02 17:24:39, Bill Hesse wrote:
> ... are deleted.

Done.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc
File runtime/bin/directory_posix.cc (right):

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:224: static bool DeleteFile(char* file_name,
On 2012/02/02 17:24:39, Bill Hesse wrote:
> Put a comment that strlen(path) may not be path_length, but that path_length
is
> the valid part of path?

This applies to all the methods in this file. I have added a comment to that
effect in the two *Recursively methods.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:245: ASSERT(written == strlen(dir_name));
On 2012/02/02 17:24:39, Bill Hesse wrote:
> In release mode, could a snprintf turn "long_path/..funny_directory" into
> "long_path/..", and get it to DeleteRecursively?  It seems unlikely, but I'm
not
> seeing anything that makes it impossible.

Yes, that is not good. I have changed all the ASSERTS that manipulate the path
for listing and recursive deletion to explicit checks instead.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:266: ComputeFullPath(dir_name, path,
&path_length);
On 2012/02/02 17:24:39, Bill Hesse wrote:
> ASSERT on path_length here?

Reworked so ComputeFullPath returns a failure status if it could not compute the
full path reliably.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:268: // Iterated the directory and delete all
files and directories.
On 2012/02/02 17:24:39, Bill Hesse wrote:
> Iterate the directory ...

Done.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_posix.cc...
runtime/bin/directory_posix.cc:320: return !error;
On 2012/02/02 17:24:39, Bill Hesse wrote:
> Every use of error really wants to use !error instead.  Is there a good name
for
> !error, so the sense could be reversed?

Good point. Renamed success->read, error->success and reversed the sense.

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_win.cc#n...
runtime/bin/directory_win.cc:221: ASSERT(written == strlen(file_name));
On 2012/02/02 17:24:39, Bill Hesse wrote:
> Especially on Windows, I think this assert really could fail - files can exist
> that have full paths that are too long (>260 chars).  So I think we should
> cleanly handle this problem, by returning failure from DeleteRecursively.  We
> don't have to succeed, we just should fail correctly.

Absolutely! Thanks!

http://codereview.chromium.org/9316066/diff/10/runtime/bin/directory_win.cc#n...
runtime/bin/directory_win.cc:235: ASSERT(written == strlen(dir_name));
On 2012/02/02 17:24:39, Bill Hesse wrote:
> These could all be made checks, rather than asserts, I think.

Done.

http://codereview.chromium.org/9316066/diff/10/tests/standalone/src/Directory...
File tests/standalone/src/DirectoryTest.dart (right):

http://codereview.chromium.org/9316066/diff/10/tests/standalone/src/Directory...
tests/standalone/src/DirectoryTest.dart:38: directory.delete(recursive: true);
On 2012/02/02 17:24:39, Bill Hesse wrote:
> I think we need some major tests for the new recursive delete.

I have added some more tests. It is hard to do more than test a couple of error
conditions such as non-existent directory and long path names.

[Bill Hesse, 2012-02-03 12:17:51]

LGTM.

http://codereview.chromium.org/9316066/diff/4002/runtime/bin/directory_posix.cc
File runtime/bin/directory_posix.cc (right):

http://codereview.chromium.org/9316066/diff/4002/runtime/bin/directory_posix....
runtime/bin/directory_posix.cc:51: size_t written = snprintf(path +
*path_length,
I would consider moving the addition of PathSeparator from here to the sprintf
calls in HandleDir and HandleFile and the DT_UNKNOWN case, and just using '/'. 
The result is not used for anything except later concatenation at these places.

http://codereview.chromium.org/9316066/diff/4002/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

http://codereview.chromium.org/9316066/diff/4002/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:123: // least MAX_PATH.
On Windows, ComputeFullSearchPath does do something important (add the /* to the
end), so it is good as is.

[Mads Ager (google), 2012-02-03 12:37:11]

http://codereview.chromium.org/9316066/diff/4002/runtime/bin/directory_posix.cc
File runtime/bin/directory_posix.cc (right):

http://codereview.chromium.org/9316066/diff/4002/runtime/bin/directory_posix....
runtime/bin/directory_posix.cc:51: size_t written = snprintf(path +
*path_length,
On 2012/02/03 12:17:51, Bill Hesse wrote:
> I would consider moving the addition of PathSeparator from here to the sprintf
> calls in HandleDir and HandleFile and the DT_UNKNOWN case, and just using '/'.

> The result is not used for anything except later concatenation at these
places.

I would like to keep this as is. That way the code on all platforms is as close
to each other as it can be. If we can avoid doing things differently on the
platforms I'm in favor of that to make it easier to maintain.