923540 - [OS.File] Add a function to recursively remove directories

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: removeDir

Comment 1

[Marco Castelluccio [:marco]]

(The patch doesn't handle options yet, which options should we provide?)

Comment 2

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

Comment on attachment 813638 [details] [diff] [review]
removeDir

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

Looks good.
An option |ignoreAbsent| would be useful.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +393,5 @@
>    OS.File.move(options.tmpPath, path, {noCopy: true});
>    return bytesWritten;
>  };
>  
> +AbstractFile.removeDir = function(path, options = {}) {

Documentation will be needed.

@@ +397,5 @@
> +AbstractFile.removeDir = function(path, options = {}) {
> +  let iterator = new OS.File.DirectoryIterator(path);
> +
> +  try {
> +    while(true) {

You should replace this |while| with a |for (let entry in iterator)|

@@ +404,5 @@
> +      if (entry.isDir) {
> +        OS.File.removeDir(entry.path);
> +      } else {
> +        OS.File.remove(entry.path);
> +      }

Could you file a followup bug to optimize this for Android?
We'll want to use at least |unlinkat|.

@@ +411,5 @@
> +    iterator.close();
> +    if (ex != StopIteration) {
> +      throw(ex);
> +    }
> +  }

You should replace this |catch| with a |finally|.

::: toolkit/components/osfile/tests/xpcshell/test_removeDir.js
@@ +11,5 @@
> +  run_next_test();
> +}
> +
> +add_task(function() {
> +  OS.Shared.DEBUG = true;

Please use the preference instead.

@@ +51,5 @@
> +
> +  // Remove directory that contains multiple files
> +  yield OS.File.makeDir(dir);
> +  yield OS.File.open(file1, {create:true});
> +  yield OS.File.open(file2, {create:true});

You should rather use writeAtomic here. There are less changes of intermittent failures under Windows.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 4

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> @@ +404,5 @@
> > +      if (entry.isDir) {
> > +        OS.File.removeDir(entry.path);
> > +      } else {
> > +        OS.File.remove(entry.path);
> > +      }
> 
> Could you file a followup bug to optimize this for Android?
> We'll want to use at least |unlinkat|.

Bug 770501? Or is it too general?

Comment 5

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

It's a bit too general.

Comment 6

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

Comment on attachment 814561 [details] [diff] [review]
Patch

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

Could you please number your patches?
r=me with the following changes

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +398,5 @@
> +  * Remove an existing directory and its contents.
> +  *
> +  * @param {string} path The name of the directory.
> +  * @param {*=} options Additional options.
> +  *   - {bool} ignoreAbsent If |false|, throw an error if the file does

If the *directory* doesn't exist.

@@ +401,5 @@
> +  * @param {*=} options Additional options.
> +  *   - {bool} ignoreAbsent If |false|, throw an error if the file does
> +  *     not exist. |true| by default.
> +  *
> +  * @throws {OS.File.Error} In case of I/O error.

... in particular if |path| is not a directory.

@@ +405,5 @@
> +  * @throws {OS.File.Error} In case of I/O error.
> +  */
> +AbstractFile.removeDir = function(path, options = {}) {
> +  let iterator = new OS.File.DirectoryIterator(path);
> +  if (!iterator._exists && options.ignoreAbsent) {

Please use |exists()| instead of |_exist|.

@@ +412,5 @@
> +
> +  try {
> +    for (let entry in iterator) {
> +      if (entry.isDir) {
> +        OS.File.removeDir(entry.path);

Don't forget to propagate |options|.

Comment 7

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

Oh, I forgot: could you add preliminary support for field |ignorePermissions| (see bug 921229)? I believe that this only requires documentation and passing the appropriate |options| object to both |remove| and |removeDir|.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch v2

I've also added a test that tries to remove a file with removeDir.

When bug 921229 lands, we'll need a test for ignorePermissions too.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/dc3d80fd48b0

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Marco Castelluccio [:marco]]

Updated https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_workers and https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread