Node-fs-extra: fs.copy() with clobber set to false return EEXIST error

[popod]

In version 1.0.0, fs.copy() with clobber set to false return an EEXIST error.

I think this is a bug because we explicitly set clobber to false (overwrite existing file), so this should not return an error !

[RyanZim]

@popod clobber: true will overwrite an existing file. When set to false, it will return an EEXIST error if the file exists.

clobber will be renamed to overwrite in v2.0.0: https://github.com/jprichardson/node-fs-extra/issues/172.

[DanielSchwiperich]

@popod is right on this imho.
We set it explicitly to false, so we know that there are files already existing and don't want them to be overwritten.
The copy process should not stop then.

[popod]

@DanielSchwiperich Yeah.. This is exactly my pain !

For now I use this hack, but I hope fs-extra would provide a better solution..

fs.copy(sourcePath, destPath, {clobber: false}, function (err) {
    if (err && err.code != "EEXIST") {
        // error
    } else {
        // do something
    }
})

[jprichardson]

Ah, the semantics and intent of clobber aren't clear. Hmm. So pre-1.0.0 behavior with clobber: false would not error... perhaps that is the correct behavior. Maybe we should introduce another option (I hate options)... something like errorOnExists or something. @RyanZim thoughts?

[RyanZim]

So pre-1.0.0 behavior with clobber: false would not error... perhaps that is the correct behavior

Man, didn't know that. I don't know what's correct behavior.

I can sorta see another option, but I don't like them any better than you do. :cry:

I want to do a git bisect to see where this change came from. BTW, this just shows we have some holes that need fixing in our test suite.

[RyanZim]

The commit that changed this is 3da22b5803a9634dd361b871a521e96332557307. We changed this on purpose; see https://github.com/jprichardson/node-fs-extra/issues/272 & https://github.com/jprichardson/node-fs-extra/pull/294. I just forgot all about this.

@jprichardson What _is_ the correct behavior? Either way; we need to test this.

[DanielSchwiperich]

I think adding a flag like continueOnExist would be best and also be BC

[RyanZim]

@jprichardson What should be the default behavior here for v2?

[jprichardson]

@jprichardson What should be the default behavior here for v2?

It should revert back to the behavior pre-1.0.0. We should then add a flag to error on exist / continue on exist... whatever seems to have the most clarity.

[RyanZim]

:+1: SGTM.

While we are at it, we should quit trying to make fake EEXIST errors and just throw a reasonable error message.

errorOnExist would make the most sense to me; YMMV, IDK.

[RyanZim]

Fixed by https://github.com/jprichardson/node-fs-extra/pull/330, will be released in v2.