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

Already on GitHub? Sign in to your account

Only chmodr git remotes in Windows #5728

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

job13er commented Jul 18, 2014

Recursively setting file permissions on git remotes breaks git
hooks for anyone who uses git templates and symlinks their hooks
across repositories by removing execute permissions on the shared
hooks.

Since the original commit that added this feature:
48263bf
was a fix for a Windows only issue:
npm#3117

The proposed solution provided in this PR is to only apply the
file permissions in Windows.

Only chmodr git remotes in Windows
Recursively setting file permissions on git remotes breaks git
hooks for anyone who uses git templates and symlinks their hooks
across repositories by removing execute permissions on the shared
hooks.

Since the original commit that added this feature:
48263bf
was a fix for a Windows only issue:
#3117

The proposed solution provided in this PR is to only apply the
file permissions in Windows.
Contributor

othiym23 commented Jul 20, 2014

This is a pretty blunt solution to the problem. I'd like to at least see a test to go with it, but ideally I'd like to see a solution for this that sets the write permissions correctly while also preserving any existing execute permissions.

Contributor

job13er commented Jul 20, 2014

@othiym23 Agreed. I would love to provide a more robust solution. Unfortunately, I did not see a test associated with the original fix that introduced this issue. And from that commit and the accompanying issue I wasn't quite able to figure out what the underlying issue was.

So am I to understand that the requirement is for all the files under that directory to have write permissions added to make sure the ENOTDIR error on Windows does not regress? If so, I can do that in a way that preserves the execute perms too, as you suggested.

Was there ever a test written to confirm that fix? If not, I'd be happy to add one, I just want to make sure that I understand what the fix was supposed to deliver, then I can make sure my change doesn't break it. I didn't see a test with the commit that provided the fix, but maybe it was added in another one. I'll take another look through the tests.

Also, the travis error does not appear to be related to my change, something about ECONNREFUSED and no credentials to set when trying adducer anything I can do to fix that on my end?

Thanks for taking a look and for the feedback. Would you prefer the revised solution in a new PR or in this one?

Contributor

othiym23 commented Jul 20, 2014

Was there ever a test written to confirm that fix?

Nope. I'm trying to do a better job of adding tests going forward, especially for things like this that can introduce weird regressions on Windows (at least until we have the resources and time necessary to get a dedicated Windows CI server set up).

Also, the travis error does not appear to be related to my change

We pulled the npm-registry-couchapp tests into the npm test suite about a month or so, and there's something about that change and the attendant dependency on Couch that makes Travis sad. I think @isaacs was looking at fixing that at one point, but ultimately I think it's my responsibility to get that sorted out. In any case, it's not your problem, but if you want to take a look at it, I won't stop you. 😬

Would you prefer the revised solution in a new PR or in this one?

This one is fine. Thank you for your help! 🍉

Contributor

othiym23 commented Jul 20, 2014

So am I to understand that the requirement is for all the files under that directory to have write permissions added to make sure the ENOTDIR error on Windows does not regress? If so, I can do that in a way that preserves the execute perms too, as you suggested.

Yes. The goal is to have the permissions set on all of the files in the cache such that Windows doesn't complain when npm / rimraf tries to remove them, without messing up the executable bits on your hook scripts. I could actually see this being added to cache cleanup rather than when the git repo is added to the cache, but that would basically be a do-over from the work you've done so far, so you might as well just do the permission tweaking we discussed.

Contributor

job13er commented Jul 24, 2014

@othiym23 Closing this one since I took longer than anticipated to implement the more robust solution, and I needed to rebase anyway. The new PR is here.

@job13er job13er closed this Jul 24, 2014

othiym23 added a commit that referenced this pull request Jul 24, 2014

Don't squash execute perms in _git-remotes/ dir
A previous fix for #3117
explicitly set the permissions for all files to a hard-coded
value, regardless of what the permissions were previously.

This changes that behavior to *add* the permissions that were
previously being *set* so that existing execute permissions are
maintained.

This is a redo of the much more blunt solution attempted in
#5728
thanks to some very helpful feedback from @othiym23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment