This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9471

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

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

Download Raw Diff

Details

Reviewers

mharbison72

Group Reviewers

hg-reviewers

Commits

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

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptDec 1 2020, 1:18 AM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

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

martinvonz added a subscriber: martinvonz.Dec 1 2020, 1:34 AM

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.

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

durin42 added a commit: rHGfdd54a876213: procutil: use rapply(tonativestr, ...) to preserve lists when they come in.Dec 1 2020, 6:25 PM

Closed by commit rHGfdd54a876213: procutil: use rapply(tonativestr, ...) to preserve lists when they come in (authored by durin42). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/utils/procutil.py (2 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	23895		Dec 1 2020, 1:18 AM	★	★
Diff 2	23905		Dec 1 2020, 11:27 AM	★	★
Diff 3	23917	rHGfdd54a87621389d7c12ee3cc69dd57c97a37ac19	Dec 1 2020, 1:18 AM	★	★

Diff 23917

mercurial/utils/procutil.py

	stdout=None,			stdout=None,
	stderr=None,			stderr=None,
	ensurestart=True,			ensurestart=True,
	record_wait=None,			record_wait=None,
	):			):
	'''Spawn a command without waiting for it to finish.'''			'''Spawn a command without waiting for it to finish.'''
	# we can't use close_fds and redirect stdin. I'm not sure that we			# we can't use close_fds and redirect stdin. I'm not sure that we
	# need to because the detached process has no console connection.			# need to because the detached process has no console connection.
	p = subprocess.Popen(			p = subprocess.Popen(
	tonativestr(script),			pycompat.rapply(tonativestr, script),
	shell=shell,			shell=shell,
	env=tonativeenv(env),			env=tonativeenv(env),
				martinvonzUnsubmitted Not Done 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? martinvonz: Reading https://docs.python.org/3/library/subprocess.html#popen-constructor, it sounds like…
				mharbison72Unsubmitted Not Done 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. mharbison72: I was wondering if the right thing to do is use `pycompat.rapply` so a list stays a list and a…
	close_fds=True,			close_fds=True,
	creationflags=_creationflags,			creationflags=_creationflags,
	stdout=stdout,			stdout=stdout,
	stderr=stderr,			stderr=stderr,
	)			)
	if record_wait is not None:			if record_wait is not None:
	record_wait(p.wait)			record_wait(p.wait)