Last Comment Bug 804566 - [OS.File] Add an option ignoreExisting to OS.File.makeDir
: [OS.File] Add an option ignoreExisting to OS.File.makeDir
Status: RESOLVED FIXED
[mentor=Yoric][lang=js][patch=bug-804...
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Eduard Neculaesi (:eduardn)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-23 06:06 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-11-08 13:36 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First patch (5.21 KB, patch)
2012-10-25 14:06 PDT, Eduard Neculaesi (:eduardn)
no flags Details | Diff | Splinter Review
Second patch (6.36 KB, patch)
2012-10-25 15:24 PDT, Eduard Neculaesi (:eduardn)
dteller: feedback+
Details | Diff | Splinter Review
Third Patch (6.28 KB, patch)
2012-10-26 14:52 PDT, Eduard Neculaesi (:eduardn)
dteller: feedback+
Details | Diff | Splinter Review
Last Patch (6.32 KB, patch)
2012-10-27 04:27 PDT, Eduard Neculaesi (:eduardn)
dteller: feedback+
Details | Diff | Splinter Review
Last Patch (v2) (6.35 KB, patch)
2012-10-27 13:28 PDT, Eduard Neculaesi (:eduardn)
dteller: review+
Details | Diff | Splinter Review
Ready to upload (6.46 KB, patch)
2012-10-29 15:16 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Little problem (6.35 KB, patch)
2012-10-30 01:32 PDT, Eduard Neculaesi (:eduardn)
dteller: review+
Details | Diff | Splinter Review
bug-804566-fix (6.44 KB, patch)
2012-10-30 16:21 PDT, Eduard Neculaesi (:eduardn)
no flags Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-10-23 06:06:48 PDT
One of the main uses of directory creation is ensuring that a directory exists, regardless of whether it already existed.

We should add an option |ignoreExisting| to |OS.File.createDirectory|. If directory creation fails because a file or directory already exists at this path, it should simply ignore the error.
Comment 1 Eduard Neculaesi (:eduardn) 2012-10-24 08:53:44 PDT
I would like to work on this bug, could you tell me where to start. Thanks!
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-10-25 12:31:12 PDT
Hi Eduard, and welcome aboard!

This bug is about method |OS.File.makeDir|, which is defined in:
- http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/osfile_unix_front.jsm.html#l322 (Unix version);
- http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/osfile_win_front.jsm.html#l380 (Windows version).

Both implementations proceed as follows:
- decode options;
- call the operating system's low-level function;
- use throw_on_negative/throw_on_zero to convert any low-level error into an exception.

In both versions, you will need to:
- determine if |options.ignoreExisting| is true;
- under Unix, if |options.ignoreExisting| is indeed true, and if the low-level function returns -1, and if |ctypes.errno| is |OS.Constants.libc.EEXIST|, return normally, otherwise throw an error;
- under Windows, if |options.ignoreExisting| is indeed true, and if the low-level function returns 0, and if |ctypes.winLastError| is |OS.Constants.Win.ERROR_ALREADY_EXISTS|, return normally, otherwise throw an error.

A second step will be to add a test for the new feature, but let's see that a little later.
Comment 3 Eduard Neculaesi (:eduardn) 2012-10-25 14:06:10 PDT
Created attachment 675286 [details] [diff] [review]
First patch

I took the liberty of creating a test (hope it's fine) but I can't seem to make it run.
Comment 4 Eduard Neculaesi (:eduardn) 2012-10-25 15:24:43 PDT
Created attachment 675330 [details] [diff] [review]
Second patch
Comment 5 David Teller [:Yoric] (please use "needinfo") 2012-10-25 23:52:19 PDT
Comment on attachment 675330 [details] [diff] [review]
Second patch

Review of attachment 675330 [details] [diff] [review]:
-----------------------------------------------------------------

Good start, Eduard.
Could you please make the following changes?

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +331,5 @@
> +          return;
> +        }
> +        throw_on_negative("makeDir",
> +          UnixFile.mkDir(path, options));
> +       }

This would always fail, since you attempt to create the directory twice.

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +389,5 @@
> +          return;
> +        }
> +        throw_on_zero("makeDir", 
> +          WinFile.CreateDirectory(path, options))
> +       }

Same as the Unix version, this would always fail.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +472,5 @@
>      test.ok(stat.isDir, "I have effectively created a directory");
>  
> +    // Creating a directory with ignoreExisting (should succeed)
> +    yield OS.File.makeDir(DIRNAME, {ignoreExisting: true});
> +    test.ok(true, "Creating a directory with ignoreExisting succeeds");

No, that won't work: if |makeDir| fails, this will just skip the test. Just do the same thing you did in worker_test_osfile_front.js, with a |yield|.
Comment 6 Eduard Neculaesi (:eduardn) 2012-10-26 14:52:34 PDT
Created attachment 675705 [details] [diff] [review]
Third Patch
Comment 7 David Teller [:Yoric] (please use "needinfo") 2012-10-26 14:58:19 PDT
Comment on attachment 675705 [details] [diff] [review]
Third Patch

Review of attachment 675705 [details] [diff] [review]:
-----------------------------------------------------------------

Good progress. I suspect that the next iteration will be the right one.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +334,1 @@
>       };

I agree with the idea, but I have the impression that the |if (result == -1) {| is never closed.

Also, the guidelines suggest doing the opposite:

if (result != -1 || options.ignoreExisting ...) {
...
}

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +392,1 @@
>       };

Same remark as Unix version.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +472,5 @@
>      test.ok(stat.isDir, "I have effectively created a directory");
>  
> +    // Creating a directory with ignoreExisting (should succeed)
> +    let result = yield OS.File.makeDir(DIRNAME, {ignoreExisting: true});
> +    test.ok(!result, "Creating a directory with ignoreExisting succeeds");

No, |result| here is meaningless.

try {
  yield OS.File.makeDir(...)
  test.ok(true, ...);
} catch(err) {
  test.ok(false, ...);
}

Task.js is that cool :)
Comment 8 Eduard Neculaesi (:eduardn) 2012-10-27 04:27:43 PDT
Created attachment 675825 [details] [diff] [review]
Last Patch

Sorry for so many iterations, this should be the last one.
Comment 9 David Teller [:Yoric] (please use "needinfo") 2012-10-27 09:55:05 PDT
Comment on attachment 675825 [details] [diff] [review]
Last Patch

Review of attachment 675825 [details] [diff] [review]:
-----------------------------------------------------------------

The tests and the Unix version look good, but the Windows version needs a small fix.
Sorry, that's not the final version after all :)

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +325,5 @@
>         options = options || noOptions;
>         let omode = options.unixMode || DEFAULT_UNIX_MODE_DIR;
> +       let result = UnixFile.mkdir(path, omode);
> +
> +       if (result != -1 || 

Just a nitpick: we generally avoid whitespaces at the end of a line. Since there are a few other things to fix, could you take the opportunity to remove the whitespaces here and in the Windows version?

@@ +326,5 @@
>         let omode = options.unixMode || DEFAULT_UNIX_MODE_DIR;
> +       let result = UnixFile.mkdir(path, omode);
> +
> +       if (result != -1 || 
> +           options.ignoreExisting && ctypes.errno == OS.Constants.libc.EEXIST) {

I believe that operator precedence is correct, I hope that the test suite confirms is :)

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +384,5 @@
>         let security = options.winSecurity || null;
> +       let result = WinFile.CreateDirectory(path, security);
> +
> +       if (!result || 
> +           options.ignoreExisting && ctypes.errno == OS.Constants.libc.EEXIST) {

No, under Windows, |ctypes.errno| and OS.Constants.libc.* are mostly useless. You should rather use |ctypes.winLastError| and |OS.Constants.Win.*| – here |OS.Constants.Win.ERROR_ALREADY_EXISTS|.
Comment 10 Eduard Neculaesi (:eduardn) 2012-10-27 13:28:57 PDT
Created attachment 675889 [details] [diff] [review]
Last Patch (v2)

I made the modifications you requested + successful build + ran the tests (no failures). The tests fail if I remove |ignoreExisting| which I believe is the expected behavior.

Again, I hope you were not bothered by my many patches.
Comment 11 David Teller [:Yoric] (please use "needinfo") 2012-10-29 00:06:44 PDT
Comment on attachment 675889 [details] [diff] [review]
Last Patch (v2)

Review of attachment 675889 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you very much.
I will handle the rest shortly (runnint this through the TryServer, adding a name to the patch, etc.)
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-10-29 13:35:43 PDT
Testing: https://tbpl.mozilla.org/?tree=Try&rev=0dca97610f0f
Comment 13 Eduard Neculaesi (:eduardn) 2012-10-29 14:46:26 PDT
Great, Thanks! :)

I have a question: How can I get access to the try server? It would be easier if I tested the patch myself before uploading it and save the reviewer time.
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-10-29 15:15:28 PDT
You should become a level 1 committer: http://www.mozilla.org/hacking/committer/
I will vouch for you.
Comment 15 David Teller [:Yoric] (please use "needinfo") 2012-10-29 15:16:39 PDT
Created attachment 676352 [details] [diff] [review]
Ready to upload

By the way, for your next bug, you should take a look at http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 16 Eduard Neculaesi (:eduardn) 2012-10-29 15:39:04 PDT
Thanks for all your help and resources :)

I'll apply for level 1 and let you know when I do so. Is it ok if I contact you on IRC when I create the access level bug?
Comment 17 Eduard Neculaesi (:eduardn) 2012-10-30 01:32:06 PDT
Created attachment 676508 [details] [diff] [review]
Little problem

I saw that the tests on windows failed. I investigated the problem and it turns out that the problem is with this:

if (!result || ..)

which should be

if (result || ..)

because if the |result| is 0 in the first |if| the expression will evaluate to true and return but it should remain false and evaluate the right hand side.
Comment 18 David Teller [:Yoric] (please use "needinfo") 2012-10-30 01:40:45 PDT
Good catch, thanks.

In this case, I will let you apply the instructions to which I linked above, to produce a landable patch (i.e. you need to add a name to the patch).

Also, yes, do not hesitate to ping me on IRC.
Comment 19 Eduard Neculaesi (:eduardn) 2012-10-30 16:21:55 PDT
Created attachment 676829 [details] [diff] [review]
bug-804566-fix
Comment 21 Ed Morley [:emorley] 2012-10-31 07:14:49 PDT
https://hg.mozilla.org/mozilla-central/rev/a33857ce6108

Note You need to log in before you can comment on or make changes to this bug.