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

Gitignore overhaul #2523

Merged
merged 5 commits into from
Jan 20, 2014
Merged

Conversation

bilderbuchi
Copy link
Member

An overhaul of the gitignore structure, refactoring out some gitignore files, while keeping the structure granular and as self-contained as possible.

Closes #2057.

Refactor out gitignore in /other/serialTest.
Add some more noticable headers.
Some simplifaction and restructuring.
Clean up some files.
@kylemcdonald
Copy link
Contributor

it's cool to see all this consolidated and cleaned up. i looked through the changes and i think you've done it right -- it makes sense to keep all the .gitignore info in one place and get rid of the separate files :) 👍

@bilderbuchi
Copy link
Member Author

thanks goes to @LecColomb who initiated this in the first place. :-)

@bilderbuchi
Copy link
Member Author

Last item in #2057 done. Ready for final review and merge.

@bilderbuchi
Copy link
Member Author

I think this looks good, can I get some PR reviews?
@LeoColomb what do you think? I incorporated most of the changes you originally proposed.

@kylemcdonald
Copy link
Contributor

after seeing theo's comments in the other thread, and your last few changes, i'm going to give this another 👍

@LeoColomb
Copy link
Member

Hmm, well...
It looks good, but the .gitignore into new /tutorials directory isn't formated like the others.

Also, but it's just because I'm terribly maniac, there is one useless line at the begining of the global .gitignore, and each comment should be:

# blahblah
[#][space][text]

And, some !.gitignore still present, and we should avoid this, and if you are afraid that one .gitignore file could disappear, at least declare the # don't ignore the .gitignore file only in the global.

libneondetection.so
Application.mk
Android.mk

/*/*/test link
Copy link
Member

Choose a reason for hiding this comment

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

Why this line if this?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, those are a bit redundant. I'll take the /*/*/test link rule out, and leave android/*/test link in, I think it should only affect the android subfolder anyway, i.e. test links should only ever be created somewhere in /examples/android. Correct, @arturoc ?

@bilderbuchi
Copy link
Member Author

hum, I missed that /tutorials/.gitignore ^^. I made it match the examples/.gitignore for now. They are identical, I'd say we wait with merging them until the final structure/purpose of the tutorials folder is more clear.

Also, but it's just because I'm terribly maniac, there is one useless line at the begining of the global .gitignore, and each comment should be:

Man, you're a hard customer. :D ;-).. Fixed that...

Regarding the !.gitignore:
Again, just putting this in the root .gitignore file does not work the same!

Let's say you remove all the !.gitignore entries and only put one in the root .gitignore file, as you propose.
Then, add a .gitignore in examples/3d/3DPrimitivesExample/bin/data, just containing * (without !.gitignore). Then, git ls-files -i --exclude-standard will show you that the file .gitkeep, of.png, of_nonPow2.png are now committed already but [ws]hould actually be ignored (of course, because of the * rule).
But, and this is where stuff breaks, git status will not show you the .gitignore file you just added (because it's being ignored, too!), so you have to add an exception (i.e. !.gitignore), but you have to add the exception in the .gitignore file in examples/3d/3DPrimitivesExample/bin/data.
The reason is that gitignore parsing works from tip to root of the file tree, and by the time the parser arrives at the root and sees the !.gitignore rule there, the folder examples/3d/3DPrimitivesExample/bin/data has already been excluded completely and is not visible to the gitignore parser anymore, so it can't un-exclude the .gitignore file there.

This is why doing what you propose simply does not work correctly, sorry.

@LeoColomb
Copy link
Member

You're right, sorry, I've spoken to quickly.
See https://github.com/LeoColomb/openFrameworks/commit/f0c69524d542014c50fd648aec65b76f7d5d920c to see exactly what I said.

@bilderbuchi
Copy link
Member Author

I see, thank you.
Still, we only save one line of code (so the clarity gain is not very large), other !.gitignore entries are (necessarily!) still present, and if we leave that part like it is now, there's no negative consequences that I know of. are there things I have overlooked? why do you say "we should avoid this"?

@LeoColomb
Copy link
Member

why do you say "we should avoid this"?

OK, it's a bit strong, I admit. :-)
And I understand the result is a little ridiculous!

So, your PR looks good for me 👍

@bilderbuchi
Copy link
Member Author

Thanks. :-)
So, two 👍's have been obtained. Can a core member please take a look, and merge?

@bilderbuchi
Copy link
Member Author

push could I get another review, so we can merge this, please?

@bilderbuchi
Copy link
Member Author

@ofZach @ofTheo @arturoc could I get a review and/or merge, please? :-) <3

arturoc added a commit that referenced this pull request Jan 20, 2014
@arturoc arturoc merged commit 815b2b7 into openframeworks:master Jan 20, 2014
@bilderbuchi bilderbuchi deleted the gitignore-overhaul branch January 21, 2014 14:25
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.

New .gitignore structure overhaul
4 participants