FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: add .DS_store to .gitgnore by frony · Pull Request #23554 · nodejs/node · GitHub

/ node Public

build: add .DS_store to .gitgnore - #23554

Closed
frony wants to merge 1 commit into
nodejs:masterfrom
frony:gitignore-DS_store
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

frony commented Oct 12, 2018

Copy link
Copy Markdown
Contributor

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

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 added the meta Issues and PRs related to the general management of the project. label Oct 12, 2018
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 reviewed Oct 12, 2018
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 12, 2018

Copy link
Copy Markdown
Member

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

Trott commented Oct 12, 2018

Copy link
Copy Markdown
Member

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 previously requested changes Oct 12, 2018
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

joyeecheung commented Oct 12, 2018

Copy link
Copy Markdown
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 commented Oct 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Oct 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Oct 12, 2018

Copy link
Copy Markdown
Member

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

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 commented Oct 12, 2018

Copy link
Copy Markdown
Member

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

refack commented Oct 12, 2018

Copy link
Copy Markdown
Contributor

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 reviewed Oct 12, 2018
View reviewed changes
Comment thread .gitignore
deps/v8/gypfiles/Release/
deps/v8/third_party/eu-strip/

.DS_Store

refack Oct 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe add a comment why it's needed?

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
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
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 commented Oct 15, 2018

Copy link
Copy Markdown
Member

Landed in d9a1776 🎉

BridgeAR closed this Oct 15, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
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
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
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 mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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
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 mentioned this pull request Nov 29, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung approved these changes
BridgeAR BridgeAR approved these changes
Trott Trott left review comments
+2 more reviewers
refack refack approved these changes
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

Back | FazBrowse Home | New Git URL