sindresorhus/cp-file

Conditionally stop doing the chmod stuff #22

sindresorhus posted onGitHub

Note https://github.com/libuv/libuv/commit/eaf25ae3eb8fe567b8be15614996b112dbf5c11b and https://github.com/nodejs/node/pull/15745/files#diff-194c5460fc6f1d425ebb0ae0270ead06R11

I don't think we can feature test this, so we can just do a Node.js version test.

(It's not out in a Node.js version yet, so no rush)

// @kevva


Cool! Will keep an eye on it and add support for when available.

posted by kevva over 7 years ago

We can now do this without a Node.js version check. We should add a tests to ensure chmod is still being done though.

We should also look into whether we can remove the fs.chownSync call, and if not, open an issue on Node.js whether fs.copyFile() could have an option to do this natively.

posted by sindresorhus almost 6 years ago

@issuehunt has funded $40.00 to this issue.


posted by IssueHuntBot almost 6 years ago

Should there even be chown in the first place? It's non-conventional and kinda counter-intuitive as it doesn't match behavior of any traditional utilities(as far as I know). (For that reason I don't think there is any point expecting it to be implemented as part of copyFile.)

But what's more:

  • in linux it only works when you are root
  • in macos, I'm not exactly sure, but should be same as in linux
  • in windows, as far as I understand, chown is a no-op

If it is indeed needed, I can add a check if we're root for linux and macos(if my assumption is correct), and just get rid of it under windows.

Note: it actually throws error(unless root), but it's captured by graceful-fs.

posted by stroncium almost 6 years ago

@stroncium It was added in https://github.com/sindresorhus/cp-file/commit/22ef37b5d5a8aa04fc83fb5b7785c9ff1bd1a094 to preserve ownership of the file. I'm not that well-versed in chown, so I trust your judgement more. Let's remove it.

posted by sindresorhus over 5 years ago

@sindresorhus If we put it the way it was described in #16(which was solved by https://github.com/sindresorhus/cp-file/commit/22ef37b5d5a8aa04fc83fb5b7785c9ff1bd1a094) as mirroring cp -a mode, it does work correctly at the moment.

I'm just pointing out that normally the ownership part doesn't even work(because you have to be root for that, mentioned cp -a just silently ignores that part if you aren't), and most people wouldn't even know you could do that with cp, and definitely won't expect it to be default.

It's more or less political choice at the moment, as it is undocumented quirk, and my vote is for removing ownership preservation part(leaving mode preservation as it is), but it would be really good to have at least one more opinion on that.

posted by stroncium over 5 years ago

Let's just remove it and see if anyone complains. We can explicitly document that only mode is preserved, not ownership.

posted by sindresorhus over 5 years ago

@sindresorhus has rewarded $36.00 to @stroncium. See it on IssueHunt

  • :moneybag: Total deposit: $40.00
  • :tada: Repository reward(0%): $0.00
  • :wrench: Service fee(10%): $4.00
posted by issuehunt-app[bot] about 5 years ago

Fund this Issue

$40.00
Rewarded

Rewarded pull request

Recent activities

stroncium was rewarded by sindresorhus for sindresorhus/cp-file# 22
about 5 years ago
stroncium submitted an output to  sindresorhus/ cp-file# 22
about 5 years ago