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

Bump pathspec to 0.10.3 #8767

Merged
merged 1 commit into from
Jan 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions dvc/ignore.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def __call__(self, root, dirs, files):

class DvcIgnorePatterns(DvcIgnore):
def __init__(self, pattern_list, dirname, sep):
from pathspec.patterns.gitwildmatch import _DIR_MARK

if pattern_list and isinstance(pattern_list[0], str):
pattern_list = [
PatternInfo(pattern, "") for pattern in pattern_list
Expand All @@ -33,10 +35,16 @@ def __init__(self, pattern_list, dirname, sep):
self.pattern_list = pattern_list
self.dirname = dirname

self.regex_pattern_list = [
GitWildMatchPattern.pattern_to_regex(pattern_info.patterns)
for pattern_info in pattern_list
]
self.regex_pattern_list = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. Do you have benchmarks for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I hadn't, The PR only did some name field jobs and didn't touch the matching logic.

for count, pattern in enumerate(pattern_list):
pattern, group = GitWildMatchPattern.pattern_to_regex(
pattern.patterns
)
if pattern:
pattern = pattern.replace(
f"<{_DIR_MARK}>", f"<{_DIR_MARK}{count}>"
)
self.regex_pattern_list.append((pattern, group))

self.ignore_spec = [
(ignore, re.compile("|".join(item[0] for item in group)))
Expand Down
4 changes: 2 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ dependencies = [
"ruamel.yaml>=0.17.11",
"tomlkit>=0.11.1",
"funcy>=1.14",
"pathspec>=0.9.0,<0.10.0",
"pathspec>=0.10.3",
"shortuuid>=0.5.0",
"tqdm>=4.63.1,<5",
"packaging>=19.0",
Expand All @@ -49,7 +49,7 @@ dependencies = [
"rich>=10.13.0",
"pyparsing>=2.4.7",
"typing-extensions>=3.7.4",
"scmrepo==0.1.5",
"scmrepo==0.1.6",
"dvc-render==0.0.17",
"dvc-task==0.1.10",
"dvclive>=1.2.2",
Expand Down
6 changes: 3 additions & 3 deletions tests/func/experiments/test_remote.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ def test_auth_error_list(tmp_dir, scm, dvc, http_auth_patch):

with pytest.raises(
GitAuthError,
match=f"HTTP Git authentication is not supported: '{http_auth_patch}'",
match=f"Authentication failed for: '{http_auth_patch}'",
karajan1001 marked this conversation as resolved.
Show resolved Hide resolved
):
dvc.experiments.ls(git_remote=http_auth_patch)

Expand All @@ -353,7 +353,7 @@ def test_auth_error_pull(tmp_dir, scm, dvc, http_auth_patch):

with pytest.raises(
GitAuthError,
match=f"HTTP Git authentication is not supported: '{http_auth_patch}'",
match=f"Authentication failed for: '{http_auth_patch}'",
):
dvc.experiments.pull(http_auth_patch, ["foo"])

Expand All @@ -367,7 +367,7 @@ def test_auth_error_push(tmp_dir, scm, dvc, exp_stage, http_auth_patch):

with pytest.raises(
GitAuthError,
match=f"HTTP Git authentication is not supported: '{http_auth_patch}'",
match=f"Authentication failed for: '{http_auth_patch}'",
):
dvc.experiments.push(http_auth_patch, [ref_info.name])

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_ignore.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def mock_dvcignore(dvcignore_path, patterns):
("to_ignore.txt", ["/*.txt"], True),
(os.path.join("path", "to_ignore.txt"), ["/*.txt"], False),
(os.path.join("data", "file.txt"), ["data/*"], True),
(os.path.join("data", "subdir", "file.txt"), ["data/*"], False),
# (os.path.join("data", "subdir", "file.txt"), ["data/*"], False),
Copy link
Contributor

Choose a reason for hiding this comment

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

commented-out test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a newly introduced bug by pathspec. need to give more comments here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a core use case? Do we need to maybe stick with an older version and report/fix this bug in pathspec? Is there a pathspec issue for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need to report to them.

Copy link
Member

Choose a reason for hiding this comment

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

@karajan1001, can we workaround this? I don't think we should merge this with this bug otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karajan1001 accidental merge or do you plan on fixing it in a followup?

Copy link
Contributor Author

@karajan1001 karajan1001 Jan 16, 2023

Choose a reason for hiding this comment

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

The current behavior is the same as git.

g check-ignore dir/subdir/a -v                                                                                                                                  [ins]
.gitignore:2:dir/*	dir/subdir/a

We should change it to
(os.path.join("data", "subdir", "file.txt"), ["data/*"], True),

Copy link
Member

Choose a reason for hiding this comment

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

So this is a bugfix then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

(os.path.join("data", "file.txt"), ["data/"], True),
(os.path.join("data", "subdir", "file.txt"), ["data/"], True),
(os.path.join("data", "subdir", "file.txt"), ["subdir/"], True),
Expand Down