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

Add space to not-owned notices #196

Merged
merged 1 commit into from
Jul 23, 2016

Conversation

erikkemperman
Copy link
Member

Pedantic, I know, but it looked off :-)

@phated
Copy link
Member

phated commented Jul 23, 2016

Good catch

@phated phated merged commit b4b1049 into gulpjs:test-refactor Jul 23, 2016
@erikkemperman
Copy link
Member Author

Huh, I just chowned this dir and file (to root) and now I have failing tests. Need to look further into this, but that's not supposed to happen, right?

@phated
Copy link
Member

phated commented Jul 23, 2016

You have to make sure the files have the correct permissions too. I had to chmod and chown to get them to work locally. They work on travis so I'm not too concerned.

@phated
Copy link
Member

phated commented Jul 23, 2016

They are working on my machine also.

@erikkemperman
Copy link
Member Author

Yeah, my git sets not-owned.txt up with 644 (-rw-r--r--) after clone and the file needs to be 666 for the tests to work (with the user having followed the notice and chowned these fixtures).

Interestingly though, git doesn't pick up a change if I chmod this file to 666. I thought git accounts for modes (as opposed to uid/gid). In fact I made a similar change to a file over in the gulp repo recently, which was executable for no reason...

But here it doesn't work. I played around a little, and it seems git only cares about the x privilege to the current user, it ignores r and w for this user, and it ignores all of them for "others" and "group".

That it does work on Travis (and I presume Linux generally) but not on Mac makes me think the (default?) behaviour of Git is distinct in this regard. I have to call it day now but would suggest having the test check if the file has w privilege for "group" (if current user is part of the group) and/or "others" and append the need to chmod to the printed notice where applicable?

@erikkemperman erikkemperman deleted the test-refactor branch July 23, 2016 21:30
@erikkemperman
Copy link
Member Author

@phated I just quickly followed this up and added a notice about chmod to the not-owned test. It's more pedantry, I guess, sorry about that. I trust you'll let me know if it gets to be annoying...

erikkemperman@639215b

@erikkemperman
Copy link
Member Author

PS I may be missing something obvious, but why does the directory have to be chowned as well? The chown to root requires sudo anyway, so access to the directory should not be a problem.

@phated
Copy link
Member

phated commented Jul 25, 2016

It probably doesn't need to be. I just did it to be safe.

@erikkemperman
Copy link
Member Author

All right, I see. I didn't want to presume and make a PR of the chmod notice, but it's there if you want it.

@phated
Copy link
Member

phated commented Jul 27, 2016

Feel free to send a PR. Easier for me to merge.

phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 27, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 28, 2017
phated pushed a commit that referenced this pull request Nov 30, 2017
phated pushed a commit that referenced this pull request Nov 30, 2017
phated pushed a commit that referenced this pull request Nov 30, 2017
phated pushed a commit that referenced this pull request Nov 30, 2017
phated pushed a commit that referenced this pull request Nov 30, 2017
phated pushed a commit that referenced this pull request Dec 5, 2017
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.

2 participants