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

Add more tests according to gitignore #4166

Merged
merged 9 commits into from
Jul 7, 2020
Merged

Conversation

karajan1001
Copy link
Contributor

fix #4103

  • ❗ I have followed the Contributing to DVC checklist.

  • πŸ“– If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@karajan1001
Copy link
Contributor Author

karajan1001 commented Jul 4, 2020

Two issues found.

  1. Other consecutive asterisks are considered regular asterisks and will match according to the previous rules.

This is an issue of PathSpec.

  1. It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

Because paths didn't distinguish with files in dvcignore. So dir/ can't match pathname dir.

@efiop efiop requested a review from pared July 4, 2020 19:21
@karajan1001 karajan1001 changed the title Add more tests according to .gitignore Add more tests according to gitignore Jul 5, 2020
@efiop
Copy link
Contributor

efiop commented Jul 6, 2020

@karajan1001 Sorry for the delay. This is outstanding work! Thank you so much for looking into this!

Other consecutive asterisks are considered regular asterisks and will match according to the previous rules.

Should we report it to pathspec and wait for the solution on their side? Or how do you think we should tackle it?

It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

Because paths didn't distinguish with files in dvcignore. So dir/ can't match pathname dir.

Nice catch! So that one is solved and we are doing it the same way gitignore does it, right?

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

One minor thingy. Great idea with citing the .gitignore documentation. Makes it much more understandable.

tests/func/test_ignore.py Outdated Show resolved Hide resolved
@pared
Copy link
Contributor

pared commented Jul 6, 2020

Seems like builds are failing.

@karajan1001
Copy link
Contributor Author

Should we report it to pathspec and wait for the solution on their side? Or how do you think we should tackle it?

@efiop
I'd like to report it to PathSpec and fix it myself, but only after work could I do it.

Nice catch! So that one is solved and we are doing it the same way gitignore does it, right?
Yes, I had tested with git If the pattern format description is complex or not clear.
image
image
image

Seems like builds are failing.
@pared A bug from pathspec is waiting for fixing.

@efiop
Copy link
Contributor

efiop commented Jul 7, 2020

I'd like to report it to PathSpec and fix it myself, but only after work could I do it.

@karajan1001 Sounds great! πŸ™ So maybe let's remove or comment-out that particular test case for now, so we could merge?

@karajan1001
Copy link
Contributor Author

karajan1001 commented Jul 7, 2020

I'd like to report it to PathSpec and fix it myself, but only after work could I do it.

@karajan1001 Sounds great! πŸ™ So maybe let's remove or comment-out that particular test case for now, so we could merge?

@efiop , @pared

There is still something wrong with triple asterisks, According to the document.

Other consecutive asterisks are considered regular asterisks and will match according to the previous rules.

What the result from my computer shows that triple asterisks act like double ones.

image
image
image

Seems that this because the document is changing all the time, and in a previous version

Other consecutive asterisks are considered invalid.

Seems that Git, the documents of gitignore, and Pathspec go different ways. Maybe we should remove the triple asterisks tests, and wait for a stable standard.

And I found another problem. data/* can match data/1/2 just like data/**. This makes conflict with both the documents and the Pathspec

@pared
Copy link
Contributor

pared commented Jul 7, 2020

Checked latest git master (2.26.0-rc1) and @karajan1001 is right, behavior of x/* is different that explicitly stated in gitignore docs. @karajan1001 that seems like a bug and potential contribution opportunity.

As to current build, I think we should comment out failing ones and wait till it gets done on PathSpec side. As to triple asterisk I would just leave it. If some will ever come to us with valid use case of triple asterisk other than 'match all', then we can consider it. There is no point in blindly following git docs.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Outstanding! Thank you so much @karajan1001 ! πŸ™

@efiop efiop merged commit 253a0ea into iterative:master Jul 7, 2020
@karajan1001 karajan1001 deleted the fix_4103 branch April 7, 2021 07:41
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.

.dvcignore is broken on negation when blacklisting all
3 participants