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

Conditionally stop doing the chmod stuff #22

Closed
sindresorhus opened this issue Oct 4, 2017 · 8 comments · Fixed by #39
Closed

Conditionally stop doing the chmod stuff #22

sindresorhus opened this issue Oct 4, 2017 · 8 comments · Fixed by #39
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 4, 2017

Issuehunt badges

Note libuv/libuv@eaf25ae 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


IssueHunt Summary

stroncium stroncium has been rewarded.

Backers (Total: $40.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@kevva
Copy link
Contributor

kevva commented Oct 6, 2017

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

kevva added a commit that referenced this issue Oct 17, 2017
@sindresorhus
Copy link
Owner Author

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.

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jun 11, 2019
@IssueHuntBot
Copy link

@issuehunt has funded $40.00 to this issue.


@stroncium
Copy link

stroncium commented Jun 14, 2019

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.

@sindresorhus
Copy link
Owner Author

@stroncium It was added in 22ef37b to preserve ownership of the file. I'm not that well-versed in chown, so I trust your judgement more. Let's remove it.

@stroncium
Copy link

@sindresorhus If we put it the way it was described in #16(which was solved by 22ef37b) 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.

@sindresorhus
Copy link
Owner Author

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

stroncium added a commit to stroncium/cp-file that referenced this issue Feb 27, 2020
@issuehunt-oss
Copy link

issuehunt-oss bot commented Mar 5, 2020

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

  • 💰 Total deposit: $40.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $4.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
4 participants