fix: glob characters in local paths

[wtanksleyjr]

Addresses only the local part of #481 with minimal changes and no new dependencies. I haven't worked with the remote parts of plumbum, sorry.

[henryiii]

Great to see work on this! Most importantly, can you add a test for the fix? I can probably do the remote part if there's a test. I can probably add a test instead, but it's unlikely to be before SciPy is over (next week) at the earliest.

[wtanksleyjr]

Sorry to be dumb... but I'm really not sure what to do here. This is the first time I've submitted a pull request.

I don't know what I need to do to see these test results. The results given in these failure reports are absurdly truncated to the point of being useless. (Why on earth would they do that truncation on a failure?) Is there a way to access a more complete transcript?

Definitely I should run the tests on my own machine ... but how? I installed pytest (obviously) using python3.8 -m pip install pytest, but I don't see where to take it from there; simply running pytest obviously isn't enough:

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov-config=setup.cfg
  inifile: /home/william/plumbum/setup.cfg
  rootdir: /home/william/plumbum

[henryiii]

Sorry, didn't mean it to be hard. I'll add a noxfile at some point so you'll be able to run it with nox without any other setup. The remote tests will likely never run easily on a local machine due to requiring being able to SSH into localhost. Probably should make the coverage non-mandatory. 🤦

You should be able to do (showing my favorite formula, using bash):

python -m venv .venv
source .venv/bin/activate
python -m install .[dev]
pytest

We should have a [test] extra at some point, currently we only have [dev], but that's basically what I'd normally call test.

Mostly I wanted the contents of a test (like setting up a glob example that would normally fail but now passes), I can help with the plumbing (no pun intended... well, maybe).

[henryiii]

You can see the test requirements under the dev extra in setup.cfg, if you are curious, BTW.

[henryiii]

And -v adds more verbosity to the tests, and can be stacked, like -vv (hope I remember correctly and it's not -V).

[wtanksleyjr]

I'm sorry, I've been distracted; I've got some more time, but I still can't figure out how to make the tests run. You gave me a sequence of commands, and they stop working for me at

python -m install .[dev]

That doesn't look like a valid Bash command -- the non-quoted .[dev] string doesn't match anything and won't run. When I try to quote it, it tells me I don't have an "install" module.

[henryiii]

Sorry, python -m pip install ".[dev]" , and the quotes are optional on bash and most other shells except Windows, IIRC. I can try to be more help later.

[wtanksleyjr]

Oh, now that makes sense. But it doesn't quite work -- ``` ERROR: Directory '.[dev]' is not installable. Neither 'setup.py' nor 'pyproject.toml' found. ``` I'm thinking I might be in the wrong folder ... I'm in `plumbum/tests`. But I can't find a folder named `.[dev]` anywhere else either. Maybe I'm not using the right commands, it's really hard to deal with folders with wildcards in their names (ironically the whole purpose of this fix).

…

On Sun, Jul 11, 2021 at 8:31 PM Henry Schreiner ***@***.***> wrote: Sorry, didn't mean it to be hard. I'll add a noxfile at some point so you'll be able to run it with nox without any other setup. The remote tests will likely never run easily on a local machine due to requiring being able to SSH into localhost. Probably should make the coverage non-mandatory. 🤦 You should be able to do (showing my favorite formula, using bash): python -m venv .venv source .venv/bin/activate python -m install .[dev] pytest We should have a [test] extra at some point, currently we only have [dev], but that's basically what I'd normally call test. Mostly I wanted the contents of a test (like setting up a glob example that would normally fail but now passes), I can help with the plumbing (no pun intended... well, maybe). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#552 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7H6KOLCDJEOREDW3MX3TTXJOZVANCNFSM5AETQDMQ> .

[henryiii]

"." refers to the current directory, so it needs to be in the root folder of the project.

I'll probably try to add a noxfile at some point to make this easier to run without as much setup.

[AndydeCleyre]

If you check the logs from CI, we're getting this on Python 2.7:

AttributeError: 'module' object has no attribute 'escape'

. . . where module is glob. Looking at the docs, that was added in 3.4.

[henryiii]

Python 2 will get dropped at some point before or on Jan 1, 2022, so if you just want to wait. :)

[wtanksleyjr]

Wow, that's a very good point (the 3.4+ specific function), I should have checked that! Some thoughts: 0. This is not a problem for Windows, which doesn't allow wildcard characters to be included in path names. That plausibly makes the task a bit easier. 1. Of course we COULD simply decree no more 2.x support. I ... dunno if you want to do that. I don't think I would if I were in your place. 2. Implementing 2.x support for this exact code might be unduly hard; escapes aren't always as simple as they seem, edge cases can kill you and we also have to consider multiplatform support. Remember that this is essentially a data vulnerability, not anything that can be solved by careful coding -- that is, it depends on all of the filenames in the path, including path components that the programmer might not be able to standardize. 3. The "best" way to solve this (and the reason I called my code a "prototype") would be to change the implementation so that wildcard expansion is only done on the actual wildcard, not on the concatenated path -- so in a sense, the wildcard is relative to the base path, rather than being an absolute wildcard. Since this is concordant both with normal expectation and with the documentation's claim that the wildcard matches "under" the path it's applied to, it's a net win. Unfortunately doing this is a little more complicated than the method I chose. 4. I can't run the tests at all, sorry. No idea why.

…

On Tue, Sep 28, 2021 at 9:51 AM Andy Kluger ***@***.***> wrote: If you check the logs from CI, we're getting this on Python 2.7: AttributeError: 'module' object has no attribute 'escape' . . . where module is glob. Looking at the docs <https://docs.python.org/3/library/glob.html#glob.escape>, that was added <https://github.com/python/cpython/blob/fe7746c61a73f58f0cf905ad1378be884d479b5b/Lib/glob.py#L168> in 3.4. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#552 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7H6J4QHZTVKX4G5LB3UTUEHXCRANCNFSM5AETQDMQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[henryiii]

4: you can now run nox -s tests-3.9 (or whatever version you want). No environment setup except to install nox. (And, if you are a pipx user, then pipx run nox -s tests-3.9 and no nox install needed either). The remote/sudo tests are off by default.

2: 2.x support will be removed at the end of the year. So one option is to wait. Another option would be to leave the old behavior on 2.x and just fix it in 3.x.

[henryiii]

How about root_dir? Sounds perfect!

Changed in version 3.10: Added the root_dir and dir_fd parameters.

Noooooo 😭

[henryiii]

The contents of glob.escape look pretty easy, actually:

https://github.com/python/cpython/blob/024209401ebc8a011f242af00efdd8ecece6953d/Lib/glob.py#L205-L234

(ignore the functions in the middle, not used)

It could be backported in plumbum.six if you wanted to get this in before Jan 1.

[henryiii]

magic_check = re.compile(u'([*?[])')
magic_check_bytes = re.compile(b'([*?[])')

def escape(pathname):
    drive, pathname = os.path.splitdrive(pathname)
    if isinstance(pathname, bytes):
        pathname = magic_check_bytes.sub(br'[\1]', pathname)
    else:
        pathname = magic_check.sub(ur'[\1]', pathname)
    return drive + pathname

Something like this. If I add this, could you add a test?

[henryiii]

I came up with a working test, let's see.

[coveralls]

Coverage remained the same at 82.268% when pulling d289941 on wtanksleyjr:master into e388030 on tomerfiliba:master.

[henryiii]

Thanks!