Add Directory.createTemp() to asynchronously create a temporary directory.

Add Directory.createTemp() to asynchronously create a temporary directory.


BUG=
TEST=

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

[Bill Hesse, 2011-11-17 16:46:17]

I think I still need some error-handling code, and must add tests, and the
Windows name generation is not great, but take a look, and offer suggestions.

[Mads Ager (google), 2011-11-18 09:50:45]

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/builtin_in.cc
File runtime/bin/builtin_in.cc (right):

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/builtin_in.cc#ne...
runtime/bin/builtin_in.cc:28: V(Directory_CreateTemp, 2)                        
                              \
Align the '\' with the other ones.

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

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory.dart#n...
runtime/bin/directory.dart:27: * 'X' characters, which will be replaced to
create a uniqe directory name.
unique?

Is there a requirement on the number of 'X' characters?

I'm not sure about the interface here. I think it might be more natural to have
the createTemp call actually modify this directory object replacing the name
with the name of the freshly created directory. Otherwise you have this extra
directory object that you don't need for anything.

var temp = new Directory("nameXXXXXX");
temp.createTempSync();
// use temp.

var temp = new Directory("nameXXXXXX");
temp.createTempHandler = () {
  // use temp.
}
temp.createTemp();

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory.dart#n...
runtime/bin/directory.dart:33: void createTemp(void createdHandler(Directory
tempDir));
Please follow the style in the rest of the file and have the createTempHandler
on the object and not in the function. We can change that at some point if we
want to, but let's keep it consistent for now.

Also, there should be a Sync version as well. However, you can leave that for a
separate change (we need async version of create, exists and delete as well).
Have a look at File to see how we have provided both there.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.dart
File runtime/bin/directory_impl.dart (right):

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.d...
runtime/bin/directory_impl.dart:38: if (path == '') path =
'/tmp/darttmpdirXXXXXX';
You should add a comment about this default behavior in the interface.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.d...
runtime/bin/directory_impl.dart:42: var message = {'success': (result != ''),
'directoryName': result};
Do this in the caller? Just do replyTo.send(_createTemp(path))?

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.d...
runtime/bin/directory_impl.dart:76: tempDir.dirHandler = _dirHandler;
This is subtle, why do you transfer the handlers? I think this is another
argument that what we really want is to modify the current Directory object.

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

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_posix....
runtime/bin/directory_posix.cc:258: // dir_template.  Creates this directory,
with mode ???.
Update comment with the actual mode.

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

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:242: char* Directory::CreateTemp(const char*
const_template) {
You are not using the template here. So we should change the interface to
something that will make sense on all platforms. The description of the
interface does not match the Windows implementation.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:249: path[0] = '\0';
Funky indentation. Are there tabs here or something?

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:253: strncpy_s(path + path_length, MAX_PATH,
"tempdir", 8);
Use snprintf instead. That is what we have done in the rest of the code.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:256: int numeric_part = time(NULL) % 1000000;
We could use UuidCreate or something to make it more likely that this will
succeed.

What you are doing here is roughly equivalent to GetTempFileName? I guess you
are doing this to limit it to 100 tries?

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:258: sprintf(path + path_length + 7, "%.6d",
numeric_part);
snprintf

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:264: // path[0] = '\0';
Commented out code and very strange indentation.

[Bill Hesse, 2011-11-21 15:48:41]

I think this is a lot closer.  I need to test on windows, and make the
DirectoryTest.dash work on Windows, but the interface is much better, I think.

I have some interesting ideas about writing asynch. code, as seen in the new
tests.  I think a class encapsulating state, with functions written in execution
order, with names showing that order, might be clearest.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/builtin_in.cc
File runtime/bin/builtin_in.cc (right):

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/builtin_in.cc#ne...
runtime/bin/builtin_in.cc:28: V(Directory_CreateTemp, 2)                        
                              \
On 2011/11/18 09:50:45, Mads Ager wrote:
> Align the '\' with the other ones.

Done.

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

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory.dart#n...
runtime/bin/directory.dart:27: * 'X' characters, which will be replaced to
create a uniqe directory name.
Changed.  The template is the path, and the unique characters are added at the
end.

On 2011/11/18 09:50:45, Mads Ager wrote:
> unique?
> 
> Is there a requirement on the number of 'X' characters?
> 
> I'm not sure about the interface here. I think it might be more natural to
have
> the createTemp call actually modify this directory object replacing the name
> with the name of the freshly created directory. Otherwise you have this extra
> directory object that you don't need for anything.
> 
> var temp = new Directory("nameXXXXXX");
> temp.createTempSync();
> // use temp.
> 
> var temp = new Directory("nameXXXXXX");
> temp.createTempHandler = () {
>   // use temp.
> }
> temp.createTemp();

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.dart
File runtime/bin/directory_impl.dart (right):

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.d...
runtime/bin/directory_impl.dart:38: if (path == '') path =
'/tmp/darttmpdirXXXXXX';
On 2011/11/18 09:50:45, Mads Ager wrote:
> You should add a comment about this default behavior in the interface.

Moved to .cc file.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.d...
runtime/bin/directory_impl.dart:42: var message = {'success': (result != ''),
'directoryName': result};
On 2011/11/18 09:50:45, Mads Ager wrote:
> Do this in the caller? Just do replyTo.send(_createTemp(path))?

Done.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_impl.d...
runtime/bin/directory_impl.dart:76: tempDir.dirHandler = _dirHandler;
All this is removed, and we do not create a new Directory object.

On 2011/11/18 09:50:45, Mads Ager wrote:
> This is subtle, why do you transfer the handlers? I think this is another
> argument that what we really want is to modify the current Directory object.

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

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_posix....
runtime/bin/directory_posix.cc:258: // dir_template.  Creates this directory,
with mode ???.
On 2011/11/18 09:50:45, Mads Ager wrote:
> Update comment with the actual mode.

Done.

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

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:242: char* Directory::CreateTemp(const char*
const_template) {
We are using a non-empty template on both platforms, and using the system
directory when passed the empty string.

On 2011/11/18 09:50:45, Mads Ager wrote:
> You are not using the template here. So we should change the interface to
> something that will make sense on all platforms. The description of the
> interface does not match the Windows implementation.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:249: path[0] = '\0';
On 2011/11/18 09:50:45, Mads Ager wrote:
> Funky indentation. Are there tabs here or something?

Done.

http://codereview.chromium.org/8588029/diff/2001/runtime/bin/directory_win.cc...
runtime/bin/directory_win.cc:256: int numeric_part = time(NULL) % 1000000;
Using Math.Random from dart.

On 2011/11/18 09:50:45, Mads Ager wrote:
> We could use UuidCreate or something to make it more likely that this will
> succeed.
> 
> What you are doing here is roughly equivalent to GetTempFileName? I guess you
> are doing this to limit it to 100 tries?

[Mads Ager (google), 2011-11-21 16:29:37]

Much better! LGTM with a couple of comments.

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

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory.dart#...
runtime/bin/directory.dart:75: * Set the handler that is called when a temporary
directory is created.
Maybe shorten this to just state that it is called when a temporary directory is
*successfully* created and leave out the part about the error handler?

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

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory_posix...
runtime/bin/directory_posix.cc:272: snprintf(path,  PATH_MAX,
"/tmp/temp_dir1_XXXXXX");
Remove extra space between ',' and 'PATH_MAX'.

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

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory_win.c...
runtime/bin/directory_win.cc:8: #include <time.h>
I don't think you need time.h now?

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory_win.c...
runtime/bin/directory_win.cc:244: // dir_template.  Creates this directory, with
mode ???.
Please fill in the mode part of the comments.

http://codereview.chromium.org/8588029/diff/11009/tests/standalone/src/Direct...
File tests/standalone/src/DirectoryTest.dart (right):

http://codereview.chromium.org/8588029/diff/11009/tests/standalone/src/Direct...
tests/standalone/src/DirectoryTest.dart:207: NestedTempDirectoryTest() {
I would put the constructor before the instance methods.

http://codereview.chromium.org/8588029/diff/11009/tests/standalone/src/Direct...
tests/standalone/src/DirectoryTest.dart:217: DirectoryTest.testMain();
Why not create a testMain method on NestedTempDirectoryTest instead of calling
it from DirectoryTest.testMain. Also, I would more the code out of the
constructor.

[Bill Hesse, 2011-11-22 12:49:38]

Added fix to skip testing the /tmp directory by name, if it does not exist.

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

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory.dart#...
runtime/bin/directory.dart:75: * Set the handler that is called when a temporary
directory is created.
On 2011/11/21 16:29:37, Mads Ager wrote:
> Maybe shorten this to just state that it is called when a temporary directory
is
> *successfully* created and leave out the part about the error handler?

Done.

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

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory_posix...
runtime/bin/directory_posix.cc:272: snprintf(path,  PATH_MAX,
"/tmp/temp_dir1_XXXXXX");
On 2011/11/21 16:29:37, Mads Ager wrote:
> Remove extra space between ',' and 'PATH_MAX'.

Done.

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

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory_win.c...
runtime/bin/directory_win.cc:8: #include <time.h>
On 2011/11/21 16:29:37, Mads Ager wrote:
> I don't think you need time.h now?

Done.

http://codereview.chromium.org/8588029/diff/11009/runtime/bin/directory_win.c...
runtime/bin/directory_win.cc:244: // dir_template.  Creates this directory, with
mode ???.
On 2011/11/21 16:29:37, Mads Ager wrote:
> Please fill in the mode part of the comments.

Done.

http://codereview.chromium.org/8588029/diff/11009/tests/standalone/src/Direct...
File tests/standalone/src/DirectoryTest.dart (right):

http://codereview.chromium.org/8588029/diff/11009/tests/standalone/src/Direct...
tests/standalone/src/DirectoryTest.dart:207: NestedTempDirectoryTest() {
On 2011/11/21 16:29:37, Mads Ager wrote:
> I would put the constructor before the instance methods.

Done.

http://codereview.chromium.org/8588029/diff/11009/tests/standalone/src/Direct...
tests/standalone/src/DirectoryTest.dart:217: DirectoryTest.testMain();
On 2011/11/21 16:29:37, Mads Ager wrote:
> Why not create a testMain method on NestedTempDirectoryTest instead of calling
> it from DirectoryTest.testMain. Also, I would more the code out of the
> constructor.

Done.