Inconsistency in fs.cp operation

[jasnell]

In the fs.cp... API, we have options such as errorOnExist and force that are intended to control what happens when the destination already exists. There is, however, a discrepancy in how the options are implemented depending on whether the src/dest are files or directories.

For instance, let's suppose we have the following directory layout:

~/tmp
 | -- ~/tmp/a (file)
 | -- ~/tmp/b (file)
 | -- ~/tmp/c (directory)
 | | -- /tmp/c/1 (file)
 | -- ~/tmp/d (directory)
 | -- /tmp/d/2 (file)

Then, in the fs.cp operation, let's first try copying a to b, with options errorOnExist: true and force: false we get the appropriate error indicating that the destination exists and we're not going to overwrite it:

> fs.cp('a', 'b', { errorOnExist: true, force: false }, console.log)
undefined
> SystemError [ERR_FS_CP_EEXIST]: Target already exists: cp returned EEXIST (b already exists) b
 at mayCopyFile (node:internal/fs/cp/cp:250:11)
 at onFile (node:internal/fs/cp/cp:242:10)
 at getStatsForCopy (node:internal/fs/cp/cp:211:12) {
 code: 'ERR_FS_CP_EEXIST',
 info: {
 message: 'b already exists',
 path: 'b',
 syscall: 'cp',
 errno: 17,
 code: 'EEXIST'
 },
 errno: [Getter/Setter: 17],
 syscall: [Getter/Setter: 'cp'],
 path: [Getter/Setter: 'b']
}

However, if we try copying c to d, with options errorOnExist: true and force: false, we happily go ahead and copy contents of 'c' over to 'd', erroring only if one of the individual files in d overlaps with a file in c

> fs.cp('c', 'd', { recursive: true, errorOnExist: true, force: false }, console.log)
undefined
> null undefined

This just feels wrong. d already exists and we're being told not to force. I would expect the behavior to be the same as copying the files. If d exists, this operation should fail.

I don't think we can change this behavior now without it being semver-major and potentially breaking a lot of things but we really ought to have an option of enforcing that no copy is performed if d exists.

@nodejs/fs @anonrig @dario-piotrowicz

[jasnell]

Similar inconsistency: When attempting to copy a symlink (not-dereferenced) over an existing file with force: false and errorOnExist: false, the fs.cp(...) call still reports an error, while the fs.cpSync(...) also throws an error but for entirely the wrong reason.

> fs.cp('b', 'e', { dereference: true, force: false, errorOnExist: false }, console.log)
undefined
> null undefined
> fs.cp('b', 'e', { dereference: false, force: false, errorOnExist: false }, console.log)
undefined
> Error: EEXIST: file already exists, symlink '/home/jsnell/tmp/a' -> 'e'
 at async symlink (node:internal/fs/promises:1007:10) {
 errno: -17,
 code: 'EEXIST',
 syscall: 'symlink',
 path: '/home/jsnell/tmp/a',
 dest: 'e'
}
> fs.cp('b', 'e', { dereference: false, force: true, errorOnExist: false }, console.log)
undefined
> Error: EEXIST: file already exists, symlink '/home/jsnell/tmp/a' -> 'e'
 at async symlink (node:internal/fs/promises:1007:10) {
 errno: -17,
 code: 'EEXIST',
 syscall: 'symlink',
 path: '/home/jsnell/tmp/a',
 dest: 'e'
}
> fs.cpSync('b', 'e', { dereference: false, force: false, errorOnExist: false })
Uncaught Error: EEXIST: file already exists, symlink '/home/jsnell/tmp/a' -> 'e'
 at symlinkSync (node:fs:1873:11)
 at onLink (node:internal/fs/cp/cp-sync:205:14)
 at getStats (node:internal/fs/cp/cp-sync:74:12)
 at cpSyncFn (node:internal/fs/cp/cp-sync:58:10)
 at Object.cpSync (node:fs:3111:3) {
 errno: -17,
 code: 'EEXIST',
 syscall: 'symlink',
 path: '/home/jsnell/tmp/a',
 dest: 'e'
}
> fs.cpSync('b', 'e', { dereference: false, force: true, errorOnExist: false })
Uncaught Error: EEXIST: file already exists, symlink '/home/jsnell/tmp/a' -> 'e'
 at symlinkSync (node:fs:1873:11)
 at onLink (node:internal/fs/cp/cp-sync:205:14)
 at getStats (node:internal/fs/cp/cp-sync:74:12)
 at cpSyncFn (node:internal/fs/cp/cp-sync:58:10)
 at Object.cpSync (node:fs:3111:3) {
 errno: -17,
 code: 'EEXIST',
 syscall: 'symlink',
 path: '/home/jsnell/tmp/a',
 dest: 'e'
}
> fs.cpSync('b', 'e', { dereference: false, force: true, errorOnExist: true })
Uncaught Error: EEXIST: file already exists, symlink '/home/jsnell/tmp/a' -> 'e'
 at symlinkSync (node:fs:1873:11)
 at onLink (node:internal/fs/cp/cp-sync:205:14)
 at getStats (node:internal/fs/cp/cp-sync:74:12)
 at cpSyncFn (node:internal/fs/cp/cp-sync:58:10)
 at Object.cpSync (node:fs:3111:3) {
 errno: -17,
 code: 'EEXIST',
 syscall: 'symlink',
 path: '/home/jsnell/tmp/a',
 dest: 'e'
}
> fs.cpSync('b', 'e', { dereference: true, force: true, errorOnExist: true })
Uncaught Error: Cannot overwrite non-directory b with directory e
 at cpSyncFn (node:internal/fs/cp/cp-sync:56:13)
 at Object.cpSync (node:fs:3111:3) {
 code: 'ERR_FS_CP_DIR_TO_NON_DIR'
}
> fs.cpSync('b', 'e', { dereference: true, force: true, errorOnExist: false })
Uncaught Error: Cannot overwrite non-directory b with directory e
 at cpSyncFn (node:internal/fs/cp/cp-sync:56:13)
 at Object.cpSync (node:fs:3111:3) {
 code: 'ERR_FS_CP_DIR_TO_NON_DIR'
}

[abhay5624]

Hi 👋,

I’ve reviewed this issue and identified that the checkPaths function is missing a check for existing directories when errorOnExist is true and force options is false. I’d like to work on this and submit a fix. Could you please assign this issue to me?

Thank you!