classification
Title: regression in subprocess.call() command quoting
Type: Stage: resolved
Components: Library (Lib) Versions: Python 2.7
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: tim.golden Nosy List: benjamin.peterson, djc, eric.araujo, georg.brandl, kiilerix, r.david.murray, tim.golden
Priority: normal Keywords:

Created on 2010-12-03 20:21 by djc, last changed 2010-12-15 03:32 by r.david.murray. This issue is now closed.

Messages (7)
msg123291 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-12-03 20:21
http://mercurial.selenic.com/bts/issue2534 is a regression in 2.7.1 relative to 2.7, around the way commands are called (it seems to concern subprocess.call()). The error seems to be raised from mercurial's util.system() call, which uses subprocess.call() or subprocess.Popen() depending on the circumstances.
msg123292 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-03 20:30
The culprit seems to be r83957, which was done to fix #2304.  Assigning to Tim, who committed that revision.
msg123293 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-03 20:39
In util.system(), Mercurial adds its own pair of quotes:

    if os.name == 'nt':
        cmd = '"%s"' % cmd

That will result in one level of quoting too much.

Now it seems unfortunate that this change was done in a minor version.
It is definitely a bug fix, but one that many users have already worked around, probably in the same way as Mercurial.

Possible ways to resolve:

* make addition of quotes Python-version-specific in Mercurial
* revert to old behavior in Python 2.7.2 (ugly)
* add a check for quotes around the string in Python 2.7.2, and refrain from adding another set of quotes

(Adding 2.7 release manager to nosy.)
msg123294 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-12-03 20:55
Ask, Jesse, sorry, I got confused about multiprocessing and subprocess. Taking you out of the nosy now.
msg123313 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2010-12-04 02:21
2010/12/3 Georg Brandl <report@bugs.python.org>:
>
> Georg Brandl <georg@python.org> added the comment:
>
> In util.system(), Mercurial adds its own pair of quotes:
>
>    if os.name == 'nt':
>        cmd = '"%s"' % cmd
>
> That will result in one level of quoting too much.
>
> Now it seems unfortunate that this change was done in a minor version.
> It is definitely a bug fix, but one that many users have already worked around, probably in the same way as Mercurial.
>
> Possible ways to resolve:
>
> * make addition of quotes Python-version-specific in Mercurial
> * revert to old behavior in Python 2.7.2 (ugly)
> * add a check for quotes around the string in Python 2.7.2, and refrain from adding another set of quotes

I think we should just leave it alone. If it change the behavior again
in 2.7.2, it'll be even more consistent than before.
msg123329 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-12-04 10:33
I'm not quite sure how anyone's supposed to determine
which bugs are likely to have been worked around and
which haven't :) I'm also unsure why a clear bugfix
shouldn't make it into a minor version release. Surely
this isn't the only one to do so...

I'm happy to repatch/test to strip quotes before adding,
but I see that Benjamin prefers to leave it alone.
msg124005 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-15 03:32
Tim: we just do our best to guess, and try to err on the conservative side.  But things will always happen.  Given benjamin's reply I'm closing the issue.  Mercurial would have to add conditional code now anyway no matter what we do.
History
Date User Action Args
2010-12-15 03:32:06r.david.murraysetstatus: open -> closed

nosy: + r.david.murray
messages: + msg124005

resolution: wont fix
stage: resolved
2010-12-13 21:53:15kiilerixsetnosy: + kiilerix
2010-12-04 19:51:59eric.araujosetnosy: + eric.araujo
2010-12-04 10:33:03tim.goldensetmessages: + msg123329
2010-12-04 02:21:36benjamin.petersonsetmessages: + msg123313
2010-12-03 20:55:06djcsetnosy: - jnoller, asksol
messages: + msg123294
2010-12-03 20:39:10georg.brandlsetnosy: + benjamin.peterson
messages: + msg123293
2010-12-03 20:30:47georg.brandlsetassignee: tim.golden

messages: + msg123292
nosy: + tim.golden, georg.brandl
2010-12-03 20:21:44djccreate