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

Raise issue if trying to upload .git/ folder + ignore .git/ folder in upload_folder #1408

Merged
merged 10 commits into from
Mar 24, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Mar 24, 2023

Fix #1401 and #1402 (thanks @lewtun for reporting it).

This PR:

  • adds a check in CommitOperationAdd and CommitOperationDelete => it is not possible to upload/delete a file in the .git/ folder. Server would not accept it anyway but let's raise an issue early.
  • ignores the .git/ folder when uploading with upload_folder
  • adds tests and docs

One thing that would be cool before having a huggingface-cli push command would be to comply with the .gitignore specification. However this is not straightforward (cannot be converted to a list of allow_patterns/ignore_patterns that easily). I have found a gitignore_parser that does the trick but it would add another dependency for which I don't know much about the quality of it. Let's keep this discussion for later.


Note: I also refactored a few tests in hf_api to be more consistent with creating/deleting repos. Not exhaustive but it's still cleaner.

@Wauplin Wauplin requested review from lewtun and LysandreJik March 24, 2023 09:52
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 24, 2023

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

Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

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

I'm no expert on the inner workings on hfh, but the parts I understand in this PR look great to me. I just left a turbo nit :)

docs/source/guides/upload.mdx Outdated Show resolved Hide resolved
Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>
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, LGTM! Thanks for the PR @Wauplin

@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 24, 2023

Thanks for the reviews @lewtun @LysandreJik! It'll avoid mistakes in the future :)

@Wauplin Wauplin merged commit ce2789c into main Mar 24, 2023
@Wauplin Wauplin deleted the 1402-do-not-upload-git-files branch March 24, 2023 13:53
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.

Internal Server Error when pushing local folder to repo branch
4 participants