Skip to content

build: add .DS_store to .gitgnore#23554

Closed
frony wants to merge 1 commit intonodejs:masterfrom
frony:gitignore-DS_store
Closed

build: add .DS_store to .gitgnore#23554
frony wants to merge 1 commit intonodejs:masterfrom
frony:gitignore-DS_store

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

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

@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
deps/v8/gypfiles/Release/
deps/v8/third_party/eu-strip/

.DS_Store
Copy link
Contributor

@refack refack Oct 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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Oct 15, 2018

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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
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 <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@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

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@Trott Trott Trott left review comments

+2 more reviewers

@refack refack refack approved these changes

@digitalinfinity digitalinfinity digitalinfinity approved these changes

Reviewers whose approvals may not affect merge requirements

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