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

copy file modes (permission...) to copied files #97

Closed
wants to merge 3 commits into from

Conversation

BenjaminDobler
Copy link
Contributor

@ctalkington
Copy link
Member

this should be optional, as an option, due the extra FS lookups just to get permissions that not everyone/use case cares about.

@BenjaminDobler
Copy link
Contributor Author

Makes sense to me. I added the option "copyMode".

@shama
Copy link
Member

shama commented Aug 26, 2013

Thanks for the PR. Could you make this an option instead of using filePair.orig.copyMode? Also just name it mode as I think copyMode is too redundant?

Also since we're adding this option, would it make sense to just also let the mode be set through the option? For example:

var options = this.options({
  mode: false,
});

// ...

if (options.mode !== false) {
  fs.chmodSync(dest, (options.mode === true) ? fs.lstatSync(src).mode : options.mode);
}

- mode is now an option and optionally mode settings can be passed rather than copied from the original file
@BenjaminDobler
Copy link
Contributor Author

@shama like this approach as well cause you don`t have to pass the option for every file. Change committed!

@shama shama closed this in 7f6f258 Aug 27, 2013
@shama
Copy link
Member

shama commented Aug 27, 2013

Thanks!

@unindented
Copy link

Last update was 7 months ago. Is there anything stopping project admins from pushing a new version with this option to npm?

@michaellopez
Copy link

@shama Are you waiting for #104 to release this?

@vladikoff
Copy link
Member

Hey @unindented and @michaellopez backwards compatibility (current and future) is really important to us, so we really need to be careful how we publish this on npm. Grunt core supposed to get this fix but didn't. The fix is postponed to 0.4.3.
You can use the temporary fix that @shama merged 2 months ago by doing:

npm install gruntjs/grunt-contrib-copy --save-dev

If you have time, you can really help us by joining the discussion here: gruntjs/grunt#732 and helping us test this fix. (Also gruntjs/grunt#615, but I think #732 is the one that has a link to the fixed branch)
Thanks for understanding and your help!

@michaellopez
Copy link

@vladikoff Thanks for your quick reply. I understand what you're saying. I was unaware of the issues in Grunt core, hence my question. Now that I know I might be able to help there.

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

Successfully merging this pull request may close these issues.

6 participants