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

Loose package weight #546

Merged
merged 1 commit into from
Nov 7, 2018
Merged

Loose package weight #546

merged 1 commit into from
Nov 7, 2018

Conversation

ankurk91
Copy link
Contributor

@ankurk91 ankurk91 commented Nov 5, 2018

Hi,

Let's not publish all those file.
.gitattributes can be used to exclude files from final archive that is downloaded by composer.

Read more

@brandur-stripe
Copy link
Contributor

@ankurk91 Thanks! Do you know if it'd be possible to make this a whitelist instead of a blacklist like you can do with .gitignore?

I generally find this strategy to be more effective because you can just whitelist the most important stuff like lib/, and then don't have to remember to add new things to .gitattributes every time someone adds a new top level file.

@ankurk91
Copy link
Contributor Author

ankurk91 commented Nov 5, 2018

.gitignore file prevent files and folders from being published in git repository.

While .gitattributes file tells git to ignore files and folders during export.
So .gitattribues will not prevent listed files from being pushed in git repo. It will only affect downloads by composer.

https://git-scm.com/docs/git-archive#ATTRIBUTES

Most of the packages already following same pattern see
https://github.com/guzzle/guzzle/blob/master/.gitattributes

@brandur-stripe
Copy link
Contributor

brandur-stripe commented Nov 5, 2018

@ankurk91 Ah, yes I understand the difference between .gitignore and .gitattributes. What I was asking is whether .gitattributes could support a whitelist like you can do with .gitignore. Check that link in my comment above for details, but the whitelist-style .gitignore looks a little like this (for example):

# Ignore everything
*
# But descend into directories
!*/
# Recursively allow files under subtree
!/subtree/**
# You can be specific with these rules
!/some/other/deep/path/**
!.gitignore

You basically start with a coarse rule that says "ignore all", then you pull individual files or directories back in for consideration.

@ankurk91
Copy link
Contributor Author

ankurk91 commented Nov 5, 2018

I got your point. Unfortunately I couldn't find option to whitelist files in .gitattributes.

We can test it locally by creating the .gitattributes files and running
git archive --format zip -o export.zip master command.

Now check the contents of the zip file

@ankurk91
Copy link
Contributor Author

ankurk91 commented Nov 6, 2018

@brandur-stripe
I just tested the whitelisting pattern in .gitattributes file and it didn't worked.

git throws an error saying

warning: Negative patterns are ignored in git attributes
Use '\!' for literal leading exclamation.

.gitattributes Outdated
/.vscode export-ignore
/tests export-ignore
/examples export-ignore
.coveralls.yml export-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple minor asks:

  1. Since you're using the prefixing / for the first set of these, would you mind just using it for all of them?
  2. Can you alphabetize the list?
  3. Can you add a short comment to the top of the file indicating that the purpose of this list is to make sure that these files are excluded from the final Composer package.
  4. Can you squash down your commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandur-stripe
Copy link
Contributor

I just tested the whitelisting pattern in .gitattributes file and it didn't worked.

Cool. Thanks for taking a look at that anyway!

Added a few asks above, but otherwise this looks fine. Thanks!

@brandur-stripe
Copy link
Contributor

Looks good. Thanks!

@brandur-stripe brandur-stripe merged commit 559b223 into stripe:master Nov 7, 2018
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.

2 participants