Patchwork D1020: largefiles: 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-ry6zalaecl2gytzsosfm-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24764/
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/D1020

AFFECTED FILES
  hgext/largefiles/lfutil.py

CHANGE DETAILS




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

INLINE COMMENTS

> lfutil.py:83
>              return os.path.join(appdata, longname)
> -    elif platform.system() == 'Darwin':
> +    elif pycompat.sysplatform == 'darwin':
>          home = encoding.environ.get('HOME')

why is this not a pycompat.osname check? It doesn't make sense to abort below with pycompat.osname if that might still be posix.

To be clear, your patch isn't making this worse, but it's exposing something sketchy. Either we need the Abort below to include the sysplatform in its output or we need this check to be against pycombat.osname.

I suspect we want to change the abort below, in a separate patch.

REPOSITORY
  rHG Mercurial

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

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

INLINE COMMENTS

> ryanmce wrote in lfutil.py:83
> why is this not a pycompat.osname check? It doesn't make sense to abort below with pycompat.osname if that might still be posix.
> 
> To be clear, your patch isn't making this worse, but it's exposing something sketchy. Either we need the Abort below to include the sysplatform in its output or we need this check to be against pycombat.osname.
> 
> I suspect we want to change the abort below, in a separate patch.

`pycompat.osname` returns `posix` on OS X.

REPOSITORY
  rHG Mercurial

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

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


  I changed it in a later patch.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -80,7 +80,7 @@ 
                         encoding.environ.get('APPDATA'))
         if appdata:
             return os.path.join(appdata, longname)
-    elif platform.system() == 'Darwin':
+    elif pycompat.sysplatform == 'darwin':
         home = encoding.environ.get('HOME')
         if home:
             return os.path.join(home, 'Library', 'Caches', longname)