-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
Backslash-ed curly bracket in settings #1708
Comments
#1709 includes what should be an acceptable implementation of |
#1710 provides an almost complete implementation of IMO rejecting If we dont reject |
One more corner case:
Run:
Expected:
Got:
|
Has been fixed with v4. |
config.rst states
A minor note, the
= {posargs}
should be moved so that it is clearer that it is output, and not part of thecommands
.The docs don't indicate where this is possible, which led to #1656 and #1690 to get it working in
setenv
and in the subprocess runtime environment variables.And there is one very obvious omission from the docs. Any time an escaping character is introduced, there needs to be a way to escape the escape character. The typical way to escape
\
is\\
, and this is de-facto implemented because of use ofshlex
. #1691 added the first test for\\{
escaping the\
to become literal\{
.\\
should be mentioned in the docs. However,\\
is not implemented consistently; all of the non-shlex parsing ignores it.e.g.
commands = echo "\\{posargs}"
should be\
followed by substitution of{posargs}
. Instead it results in literal\{posargs}
.An additional complication is that
\
is valid in many contexts, e.g.\
is valid in paths on Linux, so the above works fine on Linux. The above would probably have strange results on Windows as\
is a path separator. I think and hope it would be reasonable to prohibit\
in envname and any paths on all platforms.#1698 goes some of the way to handle paths a bit more intelligently, but there are some very low level problems, and there is at least one case where better docs can help. Because the implementation uses
shlex
, the behaviour of\
depends on whether it is within quotes or not.e.g. the following will put literal
\{
into the setting, and almost every other setting is based on that setting, so that\{
will appear in many other paths.toxworkdir = {toxinidir}/.tox\{dir
It also causes subprocess env to contain
however the
\
is unnecessary there. This has the same effect:toxworkdir = {toxinidir}/.tox{dir
and this also works sometimes
toxworkdir = {toxinidir}/.tox}dir
but ^ that fails if the user is in directory
~/tmp/tox{foo
The more common/obvious failure is something like
toxworkdir = {toxinidir}/.tox{dir}
That causes
So we can improve the docs to state that
\
is only needed to escape balanced curly brackets. i.e. when both{
and}
are literals in the path.Most escaping problems wrt paths can be fixed if
getpath
does unescaping of path elements before joining path elements together. I have a fix for this.And to fix
path settings need to be escaped before they are pushed into the substitution engine. This is the bit I havent started yet.
Then there is still the issue of literal curly brackets not in paths, especially inside and outside of quotes.
There is another way forward, which is to not promote backslash escaping. The benefit would be for the top level ini syntax to not be reliant on the low level shlex parsing.
This can be done at two levels:
Instead of encouraging
\\
to escape\
, we could introduce{\}
. On its face, this is easy to implement.instead of relying on
\{
and\}
, we can use a balanced parsing mechanism like{{}
and{}}
, which should be safe because{}
is currently unspecified (it actually returns literal:
). I havent tried this yet, so it might have some serious stumbling block.I havent fully thought through those, and am not advocating them, but I have frequently done parsers, and been finding and fixing escaping bugs in tox, and sticking with
\\
,\{
and\}
has problems worth looking for alternatives, especially for\\
which isnt documented yet.The text was updated successfully, but these errors were encountered: