import/get: escape non-pattern LFS path parts #10416
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10416 +/- ##
=======================================
Coverage 90.50% 90.50%
=======================================
Files 502 503 +1
Lines 38982 39004 +22
Branches 5622 5626 +4
=======================================
+ Hits 35279 35300 +21
- Misses 3006 3007 +1
Partials 697 697 ☔ View full report in Codecov by Sentry. |
sisp
force-pushed
the
lfs/glob-escape
branch
2 times, most recently
from
86323f7
to
8f7cf59
Compare
|
@shcheklein Done. WDYT? |
sisp
force-pushed
the
lfs/glob-escape
branch
2 times, most recently
from
72587f6
to
250f798
Compare
tests/func/test_scm.py
Outdated
| @@ -44,3 +46,76 @@ def test_lfs_prefetch(tmp_dir, dvc, scm, mocker): | |||
| with dvc.switch(rev): | |||
| lfs_prefetch(dvc.dvcfs, ["foo"]) | |||
| mock_fetch.assert_called_once() | |||
|
|
|||
|
|
|||
| def test_download_lfs_prefetch_directory(tmp_dir, dvc, scm, mocker): | |||
There was a problem hiding this comment.
should these test belong to something like test_download.py?
There was a problem hiding this comment.
Sounds good. I wasn't sure what the test structure under tests/func/ is. I've moved the tests as you suggested.
tests/func/test_scm.py
Outdated
| ) | ||
|
|
||
|
|
||
| @pytest.mark.skipif(os.name == "nt", reason="invalid directory name syntax on Windows") |
There was a problem hiding this comment.
are any of special (to be escaped) symbols allowed on NT? (make a simpler test that runs there as well?)
There was a problem hiding this comment.
Good point. I've checked Microsoft docs and the relevant characters are * and ?. I've refactored the tests to test each subpattern case independently and skip only those on Windows that are incompatible. WDYT?
There was a problem hiding this comment.
Looks good, thanks @sisp . Just a few questions re the test. Thanks a lot.
|
Thanks for your feedback, @shcheklein! Would you mind another review? 🙏 |
|
thanks @sisp ! |
❗ 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've fixed a bug related to passing literal/non-pattern path (parts) to the LFS prefetching function from
scmrepowhich would interpret them as Unix filename patterns and result in incorrect path filtering results. Now, the literal/non-path path (parts) are escaped.See iterative/scmrepo#355 (comment) for the preceding discussion.
/cc @shcheklein