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.

Sign up for GitHub

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

Jump to bottom

build: add .DS_store to .gitgnore #23554

Closed
wants to merge 1 commit into from
Closed

build: add .DS_store to .gitgnore #23554

wants to merge 1 commit into from

Conversation

frony
Copy link
Contributor

@frony frony commented Oct 12, 2018

The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

@frony
build: add .DS_store to .gitgnore
7db4bcf 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 12, 2018
@jasnell jasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
Trott
Trott reviewed Oct 12, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @frony! Welcome and thanks for the pull request!

I believe in the past, this has been blocked on the grounds that users should put such things in their global .gitignore files and that we shouldn't stack OS-specific and IDE-specific files up in our .gitignore. I'll look and see if I can find that...

@Trott
Copy link
Member

Trott commented Oct 12, 2018

Also: We ignore all .* files except those we list explicitly as exceptions so I'm surprised this even came up....

@Trott
Copy link
Member

Trott commented Oct 12, 2018

Previous PRs:

#14721
#14779
#10052
#3357
#159

The number of prior times this has come up could be used as an argument that we should just do this...

Trott
Trott previously requested changes Oct 12, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file name is already ignored by line 2 of the .gitignore file. I don't think this addition is needed.

@joyeecheung
Copy link
Member

@Trott I was there when the PR was created, it seems somehow the .gitignore file in the OP was unable to ignore these two files even with .* in the existing .gitignore, it probably has something to do with them being in deps/npm/node_modules

@Trott
Copy link
Member

Trott commented Oct 12, 2018 •
edited
Loading

I created a .DS_store in my project directory and its ignored. But indeed, when I create deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store, it is not ignored. That seems to be because of the allow list in !deps/npm/node_modules in line 102.

That exception (line 102) was added in 5af1ac6 by @refack. I think it was a mistake. Looking at the diff, I think it was supposed to be an exception for deps/npm/node_modules/.bin rather than deps/npm/node_modules. I think the fix might be to update that line instead.

EDIT: No, I'm mistaken about it being in error.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 12, 2018 •
edited
Loading

From #14721 it seems the conclusion at that time was contributors should add .DS_store to their global .gitignore. I am not necessarily opposed to that, but we should document that in CONTRIBUTING.md, although most new contributors probably don't read that document from top to bottom so I still prefer to add this file in .gitignore - I don't see any harm in putting it in .gitignore anyway.

@joyeecheung
Copy link
Member

cc @Fishrock123 because you didn't think #14721 should land

@Trott Trott dismissed their stale review October 12, 2018 23:17

this is fine by me, but I suspect others may block...let's see

@Trott
Copy link
Member

Trott commented Oct 12, 2018

I think @bnoordhuis also had opinions on this sort of thing way back when...

@refack
Copy link
Contributor

refack commented Oct 12, 2018

Let's just do this (till the next time someone reshuffles .gitignore)

That exception (line 102) was added in 5af1ac6 by @refack.

Well I did only test this on Windows & Ubuntu (.DS_Store is such bad citizenship)

refack
refack reviewed Oct 12, 2018
View reviewed changes
.gitignore
@@ -133,3 +133,5 @@ deps/uv/docs/src/guide/
deps/v8/gypfiles/Debug/
deps/v8/gypfiles/Release/
deps/v8/third_party/eu-strip/

.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment why it's needed?

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
@frony @BridgeAR
build: add .DS_store to .gitgnore
8096979 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: nodejs#23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
@frony @BridgeAR
build: add .DS_store to .gitgnore
d9a1776 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: nodejs#23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BridgeAR
Copy link
Member

Landed in d9a1776 🎉

@BridgeAR BridgeAR closed this Oct 15, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@frony @jasnell
build: add .DS_store to .gitgnore
c9fd435 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: #23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@frony @addaleax
build: add .DS_store to .gitgnore
43ccfc7 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: #23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@frony @MylesBorins
build: add .DS_store to .gitgnore
715768e 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: #23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@frony @rvagg
build: add .DS_store to .gitgnore
6c7c74e 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: #23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@frony @MylesBorins
build: add .DS_store to .gitgnore
d886095 
The following files were not being ignored:

deps/npm/node_modules/node-gyp/gyp/tools/.DS_Store
deps/npm/node_modules/node-gyp/gyp/tools/Xcode/.DS_Store

PR-URL: #23554
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@digitalinfinity digitalinfinity digitalinfinity approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@frony @Trott @joyeecheung @refack @BridgeAR @digitalinfinity @jasnell @nodejs-github-bot