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

Respect .gitignore file in commits #1868

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Respect .gitignore file in commits #1868

merged 5 commits into from
Nov 28, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Nov 27, 2023

Solves #1826 and follow-up after server-side change (https://github.com/huggingface/moon-landing/pull/8007 - private link).

I have now added a respect_gitignore: bool = True parameter to create_commit and upload_folder to respect the gitignore file, if it exists.

By default we check if a .gitignore file is present in the commit itself. Otherwise the one hosted on the Hub is used. Only the gitignore file at root is respected (not in subdirectories).

I have adapted the documentation and added a few tests for it.

Note: as mentioned by @julien-c in #1826 (comment), let's consider this as a "bug-fix" and not a "breaking change" as we can expect that no-one was intentionally uploading ignored files on purpose.


# Test to upload my `huggingface_hub/` folder to the Hub
>>> huggingface-cli upload test_tmp_upload ../huggingface_hub
Skipped upload for 53 LFS file(s) (ignored by gitignore file).
Skipped 597 file(s) in commit (ignored by gitignore file).
https://huggingface.co/Wauplin/test_tmp_upload/tree/main/.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 27, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Yes! And I agree that this is a bugfix.

The implementation looks good to me.

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 28, 2023

Thanks for the review @LysandreJik! Failing tests are unrelated so I'll merge this one and fix the CI in a separate PR.

@Wauplin Wauplin merged commit f59692b into main Nov 28, 2023
12 of 16 checks passed
@Wauplin Wauplin deleted the 1826-respect-gitignore branch November 28, 2023 10:56
@julien-c
Copy link
Member

sorry i'm late to this, but are you sure we even needed a respect_gitignore param? Feels unlikely that anyone will ever want to set it to False no?

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 30, 2023

My feeling was that in some cases users still want to force upload a file. But yeah, maybe unlikely so I'd be fine with removing it (it has not been released yet so should be ok).

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 30, 2023

Opened a PR to remove respect_gitignore before it's too late 😄 #1876

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.

4 participants