Skip to content

build: add .DS_store to .gitgnore#23554

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
Closed
frony wants to merge 1 commit into from
Closed

build: add .DS_store to .gitgnore #23554

frony wants to merge 1 commit into from

Conversation

@frony
Copy link
Contributor

@fronyfrony 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-botnodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 12, 2018
@jasnelljasnell 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

@TrottTrott 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

@TrottTrott 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

joyeecheung commented Oct 12, 2018

@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

joyeecheung commented Oct 12, 2018

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

@TrottTrott dismissed their stale reviewOctober 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
deps/v8/gypfiles/Release/
deps/v8/third_party/eu-strip/

.DS_Store
Copy link
Contributor

@refackrefackOct 12, 2018

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

BridgeAR commented Oct 15, 2018

Landed in d9a1776 🎉

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]>
@codebyterecodebytere 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]>
@codebyterecodebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheungjoyeecheungjoyeecheung approved these changes

@BridgeARBridgeARBridgeAR approved these changes

@TrottTrottTrott left review comments

+2 more reviewers

@refackrefackrefack approved these changes

@digitalinfinitydigitalinfinitydigitalinfinity approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.metaIssues 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