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

RESOLVED FIXED in mozilla19

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Yoric, Assigned: eduardn)

Tracking

({dev-doc-complete})

unspecified
mozilla19
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js][patch=bug-804566-fix])

Attachments

(2 attachments, 6 obsolete attachments)

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.
Whiteboard: [mentor=Yoric][lang=js]
(Assignee)

Comment 1

5 years ago
I would like to work on this bug, could you tell me where to start. Thanks!
Flags: needinfo?(dteller)
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.
Assignee: nobody → eduardnem
Flags: needinfo?(dteller)
Summary: [OS.File] Add an option ignoreExisting to OS.File.createDirectory → [OS.File] Add an option ignoreExisting to OS.File.makeDir
(Assignee)

Comment 3

5 years ago
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.
Attachment #675286 - Flags: review?(dteller)
(Assignee)

Comment 4

5 years ago
Created attachment 675330 [details] [diff] [review]
Second patch
Attachment #675286 - Attachment is obsolete: true
Attachment #675286 - Flags: review?(dteller)
Attachment #675330 - Flags: review?(dteller)
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|.
Attachment #675330 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 675705 [details] [diff] [review]
Third Patch
Attachment #675330 - Attachment is obsolete: true
Attachment #675705 - Flags: review?(dteller)
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 :)
Attachment #675705 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 8

5 years ago
Created attachment 675825 [details] [diff] [review]
Last Patch

Sorry for so many iterations, this should be the last one.
Attachment #675705 - Attachment is obsolete: true
Attachment #675825 - Flags: review?(dteller)
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|.
Attachment #675825 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 10

5 years ago
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.
Attachment #675825 - Attachment is obsolete: true
Attachment #675889 - Flags: review?(dteller)
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.)
Attachment #675889 - Flags: review?(dteller) → review+
Testing: https://tbpl.mozilla.org/?tree=Try&rev=0dca97610f0f
(Assignee)

Comment 13

5 years ago
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.
You should become a level 1 committer: http://www.mozilla.org/hacking/committer/
I will vouch for you.
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
Attachment #675889 - Attachment is obsolete: true
Attachment #676352 - Flags: review+
Keywords: checkin-needed
(Assignee)

Comment 16

5 years ago
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?
(Assignee)

Comment 17

5 years ago
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.
Attachment #676508 - Flags: review?(dteller)
Keywords: checkin-needed
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.
Attachment #676508 - Flags: review?(dteller) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 676829 [details] [diff] [review]
bug-804566-fix
Attachment #676508 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][patch=bug-804566-fix]

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a33857ce6108
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a33857ce6108
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.