804566 - [OS.File] Add an option ignoreExisting to OS.File.makeDir

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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)]

I would like to work on this bug, could you tell me where to start. Thanks!

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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)]

Attachment: 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)]

Attachment: Second patch

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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)]

Attachment: Third Patch

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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)]

Attachment: Last Patch

Sorry for so many iterations, this should be the last one.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

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)]

Attachment: 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] - still alive but not very active]

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] - still alive but not very active]

Testing: https://tbpl.mozilla.org/?tree=Try&rev=0dca97610f0f

Comment 13

[Eduard Neculaesi (:eduardn)]

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] - still alive but not very active]

You should become a level 1 committer: http://www.mozilla.org/hacking/committer/
I will vouch for you.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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)]

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)]

Attachment: 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] - still alive but not very active]

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)]

Attachment: bug-804566-fix

Comment 20

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a33857ce6108

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a33857ce6108