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

login
register
mail settings
Submitter phabricator
Date Oct. 12, 2017, 12:57 a.m.
Message ID <differential-rev-PHID-DREV-fjkinhyqm2ow5rz7i3t7-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24763/
State Superseded
Headers show

Comments

phabricator - Oct. 12, 2017, 12:57 a.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  See the previous patch for the reason.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1019

AFFECTED FILES
  hgext/logtoprocess.py

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 12, 2017, 2:53 p.m.
ryanmce requested changes to this revision.
ryanmce added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> logtoprocess.py:55
>  def uisetup(ui):
> -    if platform.system() == 'Windows':
> +    if pycompat.osname == 'nt':
>          # no fork on Windows, but we can create a detached process

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1019

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
phabricator - Oct. 12, 2017, 2:57 p.m.
quark added inline comments.

INLINE COMMENTS

> ryanmce wrote in logtoprocess.py:55
> 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 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':

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1019

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
phabricator - Oct. 12, 2017, 8:12 p.m.
quark requested review of this revision.
quark added a comment.


  It's changed by https://phab.mercurial-scm.org/D1034.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1019

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel

Patch

diff --git a/hgext/logtoprocess.py b/hgext/logtoprocess.py
--- a/hgext/logtoprocess.py
+++ b/hgext/logtoprocess.py
@@ -40,16 +40,19 @@ 
 import subprocess
 import sys
 
-from mercurial import encoding
+from mercurial import (
+    encoding,
+    pycompat,
+)
 
 # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
 # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
 # be specifying the version(s) of Mercurial they are tested with, or
 # leave the attribute unspecified.
 testedwith = 'ships-with-hg-core'
 
 def uisetup(ui):
-    if platform.system() == 'Windows':
+    if pycompat.osname == 'nt':
         # no fork on Windows, but we can create a detached process
         # https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx
         # No stdlib constant exists for this value