Repository: jprichardson/node-fs-extra (js)

Issue:
fs.copy() with clobber set to false return EEXIST error
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 !

Pull request:
BREAKING: Do not error when copy destination exists & clobber: false
Add errorOnExist option for users that want an error

Resolves #321 

@jprichardson What do you think of the error message?

Patch:
diff --git a/README.md b/README.md
index f9cc5d4..87f7e4d 100644
--- a/README.md
+++ b/README.md
@@ -127,7 +127,8 @@ Methods
 Copy a file or directory. The directory can have contents. Like `cp -r`.
 
 Options:
-- clobber (boolean): overwrite existing file or directory, default is `true`.
+- clobber (boolean): overwrite existing file or directory, default is `true`. _Note that the copy operation will silently fail if you set this to `false` and the destination exists._ Use the `errorOnExist` option to change this behavior.
+- errorOnExist (boolean): when `clobber` is `false` and the destination exists, throw an error. Default is `false`.
 - dereference (boolean): dereference symlinks, default is `false`.
 - preserveTimestamps (boolean): will set last modification and access times to the ones of the original source files, default is `false`.
 - filter: Function to filter copied files. Return `true` to include, `false` to exclude. This can also be a RegExp, however this is deprecated (See [issue #239](https://github.com/jprichardson/node-fs-extra/issues/239) for background). _Warning: `copySync` currently applies the filter only to files (see [#180](https://github.com/jprichardson/node-fs-extra/issues/180)). This will be fixed in a future release._
diff --git a/lib/copy-sync/copy-file-sync.js b/lib/copy-sync/copy-file-sync.js
index 9cac892..3023505 100644
--- a/lib/copy-sync/copy-file-sync.js
+++ b/lib/copy-sync/copy-file-sync.js
@@ -5,18 +5,15 @@ var _buff = new Buffer(BUF_LENGTH)
 
 function copyFileSync (srcFile, destFile, options) {
   var clobber = options.clobber
+  var errorOnExist = options.errorOnExist
   var preserveTimestamps = options.preserveTimestamps
 
   if (fs.existsSync(destFile)) {
     if (clobber) {
       fs.unlinkSync(destFile)
-    } else {
-      var err = new Error('EEXIST: ' + destFile + ' already exists.')
-      err.code = 'EEXIST'
-      err.errno = -17
-      err.path = destFile
-      throw err
-    }
+    } else if (errorOnExist) {
+      throw new Error(destFile + ' already exists')
+    } else return
   }
 
   var fdr = fs.openSync(srcFile, 'r')
diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js
index e827028..532508b 100644
--- a/lib/copy-sync/copy-sync.js
+++ b/lib/copy-sync/copy-sync.js
@@ -36,7 +36,11 @@ function copySync (src, dest, options) {
 
   if (stats.isFile() && performCopy) {
     if (!destFolderExists) mkdir.mkdirsSync(destFolder)
-    copyFileSync(src, dest, {clobber: options.clobber, preserveTimestamps: options.preserveTimestamps})
+    copyFileSync(src, dest, {
+      clobber: options.clobber,
+      errorOnExist: options.errorOnExist,
+      preserveTimestamps: options.preserveTimestamps
+    })
   } else if (stats.isDirectory() && performCopy) {
     if (!fs.existsSync(dest)) mkdir.mkdirsSync(dest)
     var contents = fs.readdirSync(src)
diff --git a/lib/copy/ncp.js b/lib/copy/ncp.js
index a951004..c448678 100644
--- a/lib/copy/ncp.js
+++ b/lib/copy/ncp.js
@@ -17,6 +17,7 @@ function ncp (source, dest, options, callback) {
   var filter = options.filter
   var transform = options.transform
   var clobber = options.clobber !== false // default true
+  var errorOnExist = options.errorOnExist
   var dereference = options.dereference
   var preserveTimestamps = options.preserveTimestamps === true
 
@@ -81,12 +82,10 @@ function ncp (source, dest, options, callback) {
           rmFile(target, function () {
             copyFile(file, target)
           })
+        } else if (errorOnExist) {
+          onError(new Error(target + ' already exists'))
         } else {
-          var err = new Error('EEXIST: ' + target + ' already exists.')
-          err.code = 'EEXIST'
-          err.errno = -17
-          err.path = target
-          onError(err)
+          doneOne()
         }
       }
     })


Test patch:
diff --git a/lib/copy-sync/__tests__/copy-sync-file.test.js b/lib/copy-sync/__tests__/copy-sync-file.test.js
index 7d0dd4c..4365814 100644
--- a/lib/copy-sync/__tests__/copy-sync-file.test.js
+++ b/lib/copy-sync/__tests__/copy-sync-file.test.js
@@ -167,9 +167,16 @@ describe('+ copySync()', function () {
         })
 
         describe('> when clobber is false', function () {
-          it('should copy the file and THROW an error', function () {
+          it('should not throw an error', function () {
+            fs.copySync(src, dest, {clobber: false})
+
+            // copy never happened
+            var destDataNew = fs.readFileSync(dest, 'utf8')
+            assert.strictEqual(destData, destDataNew)
+          })
+          it('should throw an error when errorOnExist is true', function () {
             assert.throws(function () {
-              fs.copySync(src, dest, {clobber: false})
+              fs.copySync(src, dest, {clobber: false, errorOnExist: true})
             })
 
             // copy never happened
diff --git a/lib/copy/__tests__/ncp/ncp.test.js b/lib/copy/__tests__/ncp/ncp.test.js
index e091306..62dcfd5 100644
--- a/lib/copy/__tests__/ncp/ncp.test.js
+++ b/lib/copy/__tests__/ncp/ncp.test.js
@@ -97,9 +97,20 @@ describe('ncp', function () {
           cb()
         })
       })
-      it('errors if files exist', function (cb) {
+      it('should not error if files exist', function (cb) {
         ncp(src, out, function () {
           ncp(src, out, {clobber: false}, function (err) {
+            assert.ifError(err)
+            cb()
+          })
+        })
+      })
+      it('should error if errorOnExist and file exists', function (cb) {
+        ncp(src, out, function () {
+          ncp(src, out, {
+            clobber: false,
+            errorOnExist: true
+          }, function (err) {
             assert(err)
             cb()
           })