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

Result inconsistency for __call__, is_ignored_file and is_ignored_dir #4333

Closed
karajan1001 opened this issue Aug 4, 2020 · 0 comments · Fixed by #4378
Closed

Result inconsistency for __call__, is_ignored_file and is_ignored_dir #4333

karajan1001 opened this issue Aug 4, 2020 · 0 comments · Fixed by #4378
Labels
bug Did we break something? help wanted p2-medium Medium priority, should be done, but less important

Comments

@karajan1001
Copy link
Contributor

karajan1001 commented Aug 4, 2020

Bug Report

Please provide information about your setup

Output of dvc version:

$ dvc version

master

Additional Information (if any):

If applicable, please also provide a --verbose output of the command, eg: dvc add --verbose.

Now there are several interfaces for the dvcignore.
In is_ignored_dir we ignored all paths outside the repo.

dvc/dvc/ignore.py

Lines 270 to 272 in 35dd1fd

def _is_ignored(self, path, is_dir=False):
if self._outside_repo(path):
return True

In a direct call __call__ all outside paths are included.

dvc/dvc/ignore.py

Lines 241 to 244 in 35dd1fd

if ignore_pattern:
return ignore_pattern(root, dirs, files)
else:
return dirs, files

And in is_ignored_files there is a bug that makes us skipped "outside the repo" check and return False forever.

dvc/dvc/ignore.py

Lines 275 to 278 in 35dd1fd

if ignore_pattern:
return ignore_pattern.matches(dirname, basename, is_dir)
else:
return False

They should be unified. So in what condition do we need to mask files outside the repo? And should we put this logic in dvcignore?

@triage-new-issues triage-new-issues bot added the triage Needs to be triaged label Aug 4, 2020
@efiop efiop added bug Did we break something? p2-medium Medium priority, should be done, but less important labels Aug 6, 2020
@triage-new-issues triage-new-issues bot removed triage Needs to be triaged labels Aug 6, 2020
karajan1001 added a commit to karajan1001/dvc that referenced this issue Aug 12, 2020
Fix iterative#4333, as `is_ignored_file` and `is_ignored_dir` and `__call__`
share the same call `DvcIgnorePatterns.matches`, we only need to focus
on the out of repo situation.
1. Add tests for the `is_ignored_file` and `is_ignored_dir` for the out
of repo condition.
2. Now `is_ignored_file` and `is_ignored_dir` returns false for those
files outside the repo.
efiop pushed a commit that referenced this issue Aug 12, 2020
Fix #4333, as `is_ignored_file` and `is_ignored_dir` and `__call__`
share the same call `DvcIgnorePatterns.matches`, we only need to focus
on the out of repo situation.
1. Add tests for the `is_ignored_file` and `is_ignored_dir` for the out
of repo condition.
2. Now `is_ignored_file` and `is_ignored_dir` returns false for those
files outside the repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? help wanted p2-medium Medium priority, should be done, but less important
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants