Page MenuHomePhabricator

procutil: use rapply(tonativestr, ...) to preserve lists when they come in
ClosedPublic

Authored by durin42 on Dec 1 2020, 1:18 AM.

Details

Summary

This was broken when script was a list instead of a string. I caught this
with an internal extension at Google, and I'm not really sure why it wasn't
caught in any kind of CI.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Dec 1 2020, 1:18 AM

@mharbison72 this seems like you might have opinions, since it's windows-themed

martinvonz added inline comments.
mercurial/utils/procutil.py
642–645

Reading https://docs.python.org/3/library/subprocess.html#popen-constructor, it sounds like it's valid to pass a sequence also when shell=True, so maybe the condition should be if isinstance(script, bytes) or similar instead?

procutil: correctly convert to bytes when shell=False

nit: we're converting away from bytes (at least on Windows), aren't we?

I'll take a look at this tomorrow. I assume you triggered a crash- is there an easy way to hit this new code? I wonder if the other process-spawning functions have the same issue.

mharbison72 added inline comments.Dec 1 2020, 1:43 AM
mercurial/utils/procutil.py
642–645

I was wondering if the right thing to do is use pycompat.rapply so a list stays a list and a byte string is converted directly.

durin42 retitled this revision from procutil: correctly convert to bytes when shell=False to procutil: use rapply(tonativestr, ...) to preserve lists when they come in.Dec 1 2020, 11:27 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 23905.

@mharbison72 was right on the money with rapply here. Fixes my issue much more elegantly.

mharbison72 accepted this revision.Dec 1 2020, 6:20 PM

Queued for stable, thanks. It looks like none of the other Popen() calls in this module use rapply. IDK why they wouldn't trigger a failure

This revision is now accepted and ready to land.Dec 1 2020, 6:20 PM