( )⚙ D1019 logtoprocess: do not use platform.system()

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1019

logtoprocess: do not use platform.system()
ClosedPublic

Authored by quark on Oct 11 2017, 8:57 PM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG68ed3b4f86ef: logtoprocess: do not use platform.system()
Summary

See the previous patch for the reason.

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

quark created this revision.Oct 11 2017, 8:57 PM
ryanmce requested changes to this revision.Oct 12 2017, 10:53 AM
ryanmce added a subscriber: ryanmce.

I'm +1 on this direction but think we shouldn't have == 'nt' as the "is this windows?" check here.

hgext/logtoprocess.py
54

seems like this should be pycompat.iswindows (which probably doesn't exist yet, but should) because comparisons to things that should be constants scattered throughout the code scare me.

This revision now requires changes to proceed.Oct 12 2017, 10:53 AM
quark added inline comments.Oct 12 2017, 10:57 AM
hgext/logtoprocess.py
54

This check is common in the code base:

mercurial/scmutil.py
44:if pycompat.osname == 'nt':
294:    abort = pycompat.osname == 'nt' or lval == 'abort'

mercurial/i18n.py
32:if (pycompat.osname == 'nt'

mercurial/rcutil.py
18:if pycompat.osname == 'nt':

mercurial/subrepo.py
1350:            elif pycompat.osname == 'nt':

mercurial/color.py
213:    if pycompat.osname == 'nt':
382:if pycompat.osname == 'nt':

mercurial/vfs.py
546:        defaultenabled = pycompat.osname == 'nt'

mercurial/util.py
95:if pycompat.osname == 'nt':
1351:if pycompat.osname == 'nt':

mercurial/ui.py
1044:        if pycompat.osname == 'nt' and not shell:

mercurial/sslutil.py
480:                pycompat.osname == 'nt'):
720:    if pycompat.osname == 'nt':

hgext/win32mbcs.py
193:        if pycompat.osname == 'nt':

hgext/logtoprocess.py
55:    if pycompat.osname == 'nt':

hgext/schemes.py
119:        if (pycompat.osname == 'nt' and len(scheme) == 1 and scheme.isalpha()

mercurial/hgweb/server.py
269:    if pycompat.osname == 'nt':

hgext/convert/subversion.py
106:        if pycompat.osname == 'nt':
257:            if (pycompat.osname == 'nt' and path[:1] == '/'

hgext/largefiles/lfutil.py
78:    if pycompat.osname == 'nt':
quark requested review of this revision.Oct 12 2017, 4:12 PM

It's changed by D1034.

quark mentioned this in rHGc0a6c19690ff: pycompat: define operating system constants.Oct 13 2017, 10:05 AM
Closed by commit rHG68ed3b4f86ef: logtoprocess: do not use platform.system() (authored by quark). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/logtoprocess.py (8 lines)

Diff 2678

hgext/logtoprocess.py

not ensure that they exit cleanly. not ensure that they exit cleanly.
""" """
from __future__ import absolute_import from __future__ import absolute_import
import itertools import itertools
import os import os
import platform
import subprocess import subprocess
import sys import sys
from mercurial import encoding from mercurial import (
encoding,
pycompat,
)
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
# be specifying the version(s) of Mercurial they are tested with, or # be specifying the version(s) of Mercurial they are tested with, or
# leave the attribute unspecified. # leave the attribute unspecified.
testedwith = 'ships-with-hg-core' testedwith = 'ships-with-hg-core'
def uisetup(ui): def uisetup(ui):
if platform.system() == 'Windows': if pycompat.osname == 'nt':
ryanmceUnsubmitted
Not Done

seems like this should be pycompat.iswindows (which probably doesn't exist yet, but should) because comparisons to things that should be constants scattered throughout the code scare me.

ryanmce: seems like this should be `pycompat.iswindows` (which probably doesn't exist yet, but should)…
quarkAuthorUnsubmitted
Not Done

This check is common in the code base:

mercurial/scmutil.py
44:if pycompat.osname == 'nt':
294:    abort = pycompat.osname == 'nt' or lval == 'abort'

mercurial/i18n.py
32:if (pycompat.osname == 'nt'

mercurial/rcutil.py
18:if pycompat.osname == 'nt':

mercurial/subrepo.py
1350:            elif pycompat.osname == 'nt':

mercurial/color.py
213:    if pycompat.osname == 'nt':
382:if pycompat.osname == 'nt':

mercurial/vfs.py
546:        defaultenabled = pycompat.osname == 'nt'

mercurial/util.py
95:if pycompat.osname == 'nt':
1351:if pycompat.osname == 'nt':

mercurial/ui.py
1044:        if pycompat.osname == 'nt' and not shell:

mercurial/sslutil.py
480:                pycompat.osname == 'nt'):
720:    if pycompat.osname == 'nt':

hgext/win32mbcs.py
193:        if pycompat.osname == 'nt':

hgext/logtoprocess.py
55:    if pycompat.osname == 'nt':

hgext/schemes.py
119:        if (pycompat.osname == 'nt' and len(scheme) == 1 and scheme.isalpha()

mercurial/hgweb/server.py
269:    if pycompat.osname == 'nt':

hgext/convert/subversion.py
106:        if pycompat.osname == 'nt':
257:            if (pycompat.osname == 'nt' and path[:1] == '/'

hgext/largefiles/lfutil.py
78:    if pycompat.osname == 'nt':
quark: This check is common in the code base: ``` mercurial/scmutil.py 44:if pycompat.osname == 'nt'…
# no fork on Windows, but we can create a detached process # no fork on Windows, but we can create a detached process
# https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx # https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx
# No stdlib constant exists for this value # No stdlib constant exists for this value
DETACHED_PROCESS = 0x00000008 DETACHED_PROCESS = 0x00000008
_creationflags = DETACHED_PROCESS | subprocess.CREATE_NEW_PROCESS_GROUP _creationflags = DETACHED_PROCESS | subprocess.CREATE_NEW_PROCESS_GROUP
def runshellcommand(script, env): def runshellcommand(script, env):
# 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