Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error "Source and destination must not be the same" in move-sync on unicode normalization difference #859

Open
Offirmo opened this issue Jan 15, 2021 · 8 comments

Comments

@Offirmo
Copy link

Offirmo commented Jan 15, 2021

  • Operating System: macOs 10.15.7 (latest Catalina)
  • Node.js version: v14.15.3
  • fs-extra version: 9.0.1

First I'd like to thank you for fs-extra which is really great!

I'm fighting with an error when trying to move files: "Source and destination must not be the same."

I filtered the src and destination with === and they are NOT the same -> so why is fs-extra rejecting me?

Digging deeper, they look the same. When comparing their normalized unicode representation, they are indeed the same. So I'm suspecting (but don't really know) that fs-extra is doing a unicode normalization before validating src vs dest. (maybe Object.checkPathsSync (xxx/node_modules/fs-extra/lib/util/stat.js:51:11) [EDIT] the file system is doing unicode normalization.

If true, I believe this is incorrect. There are discussions for ex. in https://news.ycombinator.com/item?id=13953800 that states that for ex. APFS is treating pathes are "bags of bytes" and intentionally not normalizing (cf. linked https://mjtsai.com/blog/2017/03/24/apfss-bag-of-bytes-filenames/)

Hence I believe that fs-extra should not normalize. (not an expert, happy to be explained wrong)

somehow related to #759

@Offirmo
Copy link
Author

Offirmo commented Jan 15, 2021

Update: my FS is APFS, encrypted, NOT case-sensitive

@Offirmo
Copy link
Author

Offirmo commented Jan 15, 2021

Digging into the code, it seems the check made by fs-extra is an inode check, which would imply that APFS is doing unicode normalization… Hence my analysis is false.

Keeping the thread open in case some wisdom/advice is to be shared by the owners, but feel free to close this issue.

@Offirmo Offirmo closed this as completed Jan 17, 2021
@RyanZim
Copy link
Collaborator

RyanZim commented Jan 18, 2021

My apologies for the radio silence here; I've been super-busy, and put open-source on hold for a few days. Glad you managed to figure it out; filesystems in general tend to do weird things, which is why we abandoned comparing actual paths for inode checks.

@Offirmo
Copy link
Author

Offirmo commented Jan 24, 2021

@RyanZim no worries, your lib is awesome 👍

@Offirmo
Copy link
Author

Offirmo commented May 6, 2021

Reopening this issue after reading #759

Could we fix this unicode normalization issue in the same way than case sensitivity ? Renaming a file into a different unicode normalization should work as well!

@Offirmo Offirmo reopened this May 6, 2021
@RyanZim
Copy link
Collaborator

RyanZim commented May 6, 2021

Attn @manidlou

@manidlou
Copy link
Collaborator

manidlou commented May 7, 2021

@Offirmo can you please give us a reproducible test case?

@Offirmo
Copy link
Author

Offirmo commented May 7, 2021

@manidlou sure. On a file system doing automatic unicode normalization:

const path = `voyage à Paris.jpg`.normalize('NFD') // user input = force normalization to an exotic one for demo purpose
const target_path = path.normalize('NFC') // systematic normalization of user input
assert(a !== b) // same visual appearance but not same binary reprensentation
fs_extra.moveSync(path, target_path) // error "same inode"

This is not a cosmetic fix, I read in a blog post recently someone having trouble storing then retrieving their file on S3 due to this kind of normalization being done by safari on file upload, then the database storing a different representation.

interesting reads:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants