Fix git-lfs smudge by quoting special characters path #1880
Conversation
jelmer
left a comment
jelmer
left a comment
There was a problem hiding this comment.
Thanks for the PR. Overalls looks good, one comment inline.
petrchmelar
force-pushed
the
quote-smudge-path
branch
2 times, most recently
from
7882bc7 to
a7139bb
Compare
|
I'm going to fix the CI |
dulwich/filters.py
Outdated
|
|
||
| # Substitute %f placeholder with file path | ||
| cmd = self.smudge_cmd.replace("%f", path_str) | ||
| cmd = self.smudge_cmd.replace("%f", shlex.quote(path_str)) |
There was a problem hiding this comment.
Unfortunatelly the shlex quoting does not produce a command compatible with subprocess.run(..., shell=True) on windows (see failed job). To do so I have two options:
- use barely documented list2cmdline for windows codepath
- change the subprocess call to be executed with shell=False - I'm really not sure if it is a right thing to do in context of the commands beeing called
There was a problem hiding this comment.
Sorry for the slow reply.
I'm inclined to special-case Windows and use list2cmdline; using subprocess.run(shell=True) seems like the obvious correct thing to do on other platforms.
There was a problem hiding this comment.
That is OK, I tried to use the list2cmdline for windows. Unfortunately I do not have win machine to test the behavior. Could you launch the CI please?
There was a problem hiding this comment.
Done - unfortunately github doesn't give me a way to just approve all your CI runs upfront, but it will automatically whitelist you once your first PR lands.
Head branch was pushed to by a user without write access
petrchmelar
force-pushed
the
quote-smudge-path
branch
2 times, most recently
from
add270c to
7e653b3
Compare
| """Implementation of Git filter drivers (clean/smudge filters).""" | ||
|
|
||
| import logging | ||
| import shlex |
There was a problem hiding this comment.
I noticed you are usually using lazy imports in methods themselves. Should I move the shlex to smudge method like I did with os?
There was a problem hiding this comment.
This is fine I think; I'm not super consistent either. Generally speaking, I try to lazily import to reduce startup times but some modules are unavoidable or minor in terms of overhead.
petrchmelar
force-pushed
the
quote-smudge-path
branch
2 times, most recently
from
0dcd524 to
ce9b0c9
Compare
petrchmelar
force-pushed
the
quote-smudge-path
branch
from
ce9b0c9 to
ff7efa0
Compare
3 participants
Fixes #1878