Patchwork [V2] py3: convert os.readlink() path to native strings

login
register
mail settings
Submitter Matt Harbison
Date Sept. 28, 2018, 2:51 a.m.
Message ID <216114ff8d2bc57d9aa8.1538103087@Envy>
Download mbox | patch
Permalink /patch/35156/
State New
Headers show

Comments

Matt Harbison - Sept. 28, 2018, 2:51 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1537924572 14400
#      Tue Sep 25 21:16:12 2018 -0400
# Node ID 216114ff8d2bc57d9aa8913cf75f14267a8332f6
# Parent  df02cb5b9b3496aa95cbe754a92d714f4c68262b
py3: convert os.readlink() path to native strings

Windows insisted that it needs to be str.  I skipped the stuff that's obviously
posix only, and left `tests/f` and `run-tests.py` alone for now.
Yuya Nishihara - Sept. 28, 2018, 11:32 a.m.
On Thu, 27 Sep 2018 22:51:27 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1537924572 14400
> #      Tue Sep 25 21:16:12 2018 -0400
> # Node ID 216114ff8d2bc57d9aa8913cf75f14267a8332f6
> # Parent  df02cb5b9b3496aa95cbe754a92d714f4c68262b
> py3: convert os.readlink() path to native strings
> 
> Windows insisted that it needs to be str.  I skipped the stuff that's obviously
> posix only, and left `tests/f` and `run-tests.py` alone for now.
> 
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1841,7 +1841,7 @@ def makelock(info, pathname):
>  
>  def readlock(pathname):
>      try:
> -        return os.readlink(pathname)
> +        return pycompat.fsencode(os.readlink(pycompat.fsdecode(pathname)))

Well, this is still bad as it goes non-native route on Unix. If we really
need to support symlinks on Windows, we'll have to move it to platform module.
Matt Harbison - Sept. 28, 2018, 11:59 a.m.
> On Sep 28, 2018, at 7:32 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Thu, 27 Sep 2018 22:51:27 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1537924572 14400
>> #      Tue Sep 25 21:16:12 2018 -0400
>> # Node ID 216114ff8d2bc57d9aa8913cf75f14267a8332f6
>> # Parent  df02cb5b9b3496aa95cbe754a92d714f4c68262b
>> py3: convert os.readlink() path to native strings
>> 
>> Windows insisted that it needs to be str.  I skipped the stuff that's obviously
>> posix only, and left `tests/f` and `run-tests.py` alone for now.
>> 
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -1841,7 +1841,7 @@ def makelock(info, pathname):
>> 
>> def readlock(pathname):
>>     try:
>> -        return os.readlink(pathname)
>> +        return pycompat.fsencode(os.readlink(pycompat.fsdecode(pathname)))
> 
> Well, this is still bad as it goes non-native route on Unix. If we really
> need to support symlinks on Windows, we'll have to move it to platform module.

Even though I turned off symlinks in tests for now because you need admin rights to create them, I read recently that some new-ish builds of Windows 10 allow them to be created by a normal user with developer mode enabled.  So we probably shouldn’t cut too many corners here.

I originally coded it using procutil.tonativestr(), but figured an os specific module with a filesystem function would be the right thing.  The module seemed out of place for this usage too.  I didn’t look to see how much would need to be moved to move that to encoding.
Yuya Nishihara - Sept. 28, 2018, 12:14 p.m.
On Fri, 28 Sep 2018 07:59:17 -0400, Matt Harbison wrote:
> 
> > On Sep 28, 2018, at 7:32 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >> On Thu, 27 Sep 2018 22:51:27 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1537924572 14400
> >> #      Tue Sep 25 21:16:12 2018 -0400
> >> # Node ID 216114ff8d2bc57d9aa8913cf75f14267a8332f6
> >> # Parent  df02cb5b9b3496aa95cbe754a92d714f4c68262b
> >> py3: convert os.readlink() path to native strings
> >> 
> >> Windows insisted that it needs to be str.  I skipped the stuff that's obviously
> >> posix only, and left `tests/f` and `run-tests.py` alone for now.
> >> 
> >> diff --git a/mercurial/util.py b/mercurial/util.py
> >> --- a/mercurial/util.py
> >> +++ b/mercurial/util.py
> >> @@ -1841,7 +1841,7 @@ def makelock(info, pathname):
> >> 
> >> def readlock(pathname):
> >>     try:
> >> -        return os.readlink(pathname)
> >> +        return pycompat.fsencode(os.readlink(pycompat.fsdecode(pathname)))
> > 
> > Well, this is still bad as it goes non-native route on Unix. If we really
> > need to support symlinks on Windows, we'll have to move it to platform module.
> 
> Even though I turned off symlinks in tests for now because you need admin rights to create them, I read recently that some new-ish builds of Windows 10 allow them to be created by a normal user with developer mode enabled.  So we probably shouldn’t cut too many corners here.
> 
> I originally coded it using procutil.tonativestr(), but figured an os specific module with a filesystem function would be the right thing.

Perhaps. I think that's what the vfs object is meant to be.

> The module seemed out of place for this usage too.  I didn’t look to see how much would need to be moved to move that to encoding.

I think this can be just posix/windows.readlink() for now. At some point,
we might want to make windows vfs directly call windows.readlink"u"() to
support unicode filenames.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1841,7 +1841,7 @@  def makelock(info, pathname):
 
 def readlock(pathname):
     try:
-        return os.readlink(pathname)
+        return pycompat.fsencode(os.readlink(pycompat.fsdecode(pathname)))
     except OSError as why:
         if why.errno not in (errno.EINVAL, errno.ENOSYS):
             raise
diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -206,7 +206,8 @@  class abstractvfs(object):
         return util.rename(srcpath, dstpath)
 
     def readlink(self, path):
-        return os.readlink(self.join(path))
+        abspath = self.join(path)
+        return pycompat.fsencode(os.readlink(pycompat.fsdecode(abspath)))
 
     def removedirs(self, path=None):
         """Remove a leaf directory and all empty intermediate ones