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

Restructure .gitignore files #4707

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Restructure .gitignore files #4707

merged 1 commit into from
Jun 10, 2020

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Jun 4, 2020

  • Add .gitignore files in each subdirectory of src/, so as to reduce the size of the .gitignore file in the project root, and also make it easier to maintain (i.e., if a directory in src/ is removed, there will not be outdated entries in the root .gitignore file.

  • Also add missing .gitignore entries and remove outdated entries and duplicates.

@jleveque jleveque requested review from lguohan, qiluo-msft and yxieca June 4, 2020 20:15
@jleveque jleveque self-assigned this Jun 4, 2020
@@ -0,0 +1,3 @@
*
!.gitignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is repeated in many places? Can you remove?

Copy link
Contributor Author

@jleveque jleveque Jun 4, 2020

Choose a reason for hiding this comment

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

It's technically not necessary since the .gitignore file is itself added. I added it in all the files because I assume in the future, if someone adds a new directory they will use one of the existing .gitignore files as a template. However, If the file doesn't un-ignore itself, it will not show up in their list of modified files, so it will not get added to their commit and may get overlooked. They would need to make sure to explicitly add the new .gitignore file.

I can simply remove them all if you prefer.

@@ -0,0 +1,3 @@
*
!.gitignore
!Makefile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you need unignore? Who ignore it at first place?

Copy link
Contributor Author

@jleveque jleveque Jun 4, 2020

Choose a reason for hiding this comment

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

The first line, * ignores everything, then we un-ignore the Makefile here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why ignore everything? That will make things complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we only want to commit the Makefile. We don't care about the generated files, and if the generated files change, we will need to update this. This way, we never need to update it unless we need to add any files.

!debian/changelog
!debian/compat
!debian/control
!debian/rules
Copy link
Collaborator

Choose a reason for hiding this comment

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

Merge them into one line rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As with the above, these are inverse ignore rules. We ignore all, then un-ignore the files we want to keep. Otherwise, there would be more lines :)

Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 4, 2020

Choose a reason for hiding this comment

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

debian/* make things complex. Could you just ignore what you want to ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree. There are more files we want to ignore than the number we want to commit.

*
!.gitignore
!Makefile
!patch/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Who ignored it?

Copy link
Contributor Author

@jleveque jleveque Jun 4, 2020

Choose a reason for hiding this comment

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

The first line, * ignores everything, then we un-ignore the Makefile and patch directory here.

@jleveque
Copy link
Contributor Author

jleveque commented Jun 8, 2020

Retest mellanox please

@jleveque
Copy link
Contributor Author

jleveque commented Jun 8, 2020

Retest vsimage please

2 similar comments
@jleveque
Copy link
Contributor Author

jleveque commented Jun 9, 2020

Retest vsimage please

@jleveque
Copy link
Contributor Author

jleveque commented Jun 9, 2020

Retest vsimage please

Copy link
Contributor

@yxieca yxieca left a comment

Choose a reason for hiding this comment

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

Qi/Joe/Myself had an offline discussion. Qi is ok with moving ahead but he has different opinion on this PR.

@jleveque jleveque merged commit c6365e7 into sonic-net:master Jun 10, 2020
@jleveque jleveque deleted the update_gitignore branch June 10, 2020 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants