Patchwork [6,of,6,v2] paths: allow util.finddirs touse os.path.sep

login
register
mail settings
Submitter Matt Harbison
Date Sept. 30, 2017, 4:32 a.m.
Message ID <op.y7c2vzf39lwrgf@envy>
Download mbox | patch
Permalink /patch/24223/
State Not Applicable
Headers show

Comments

Matt Harbison - Sept. 30, 2017, 4:32 a.m.
On Sat, 23 Sep 2017 00:20:35 -0400, Matt Harbison <mharbison72@gmail.com>  
wrote:

> Kostia- I'll try to look deeper at this series this weekend.  But does  
> the suggestion in the 2012 thread above seem reasonable, about just  
> tweaking the open function instead of (I assume) trying to pass full  
> \\?\ paths around all over?  That seems like it would be easier than  
> trying to get the paths converted everywhere, and I think would be  
> transparent to the test runner and the end user.

More info on this.  I applied this patch:


I see a lot of files under .hg being accessed like this when issuing diff  
and status, but none in wvfs:

     f is \\?\c:\Users\Matt\projects\reallyrealylongnames\.hg\requires

I did see a file in wvfs accessed like that when committing.

I created a repo on Linux with a 330 character long directory + file name,  
committed it, and pushed it to a Windows served repo.  That worked fine.   
Doing an update on the Windows side successfully created the file, but  
then aborted in dirstate, because it isn't using vfs:

   File "mercurial\merge.py", line 1721, in update
     recordupdates(repo, actions, branchmerge)
   File "mercurial\merge.py", line 1433, in recordupdates
     repo.dirstate.normal(f)
   File "mercurial\dirstate.py", line 569, in normal
     s = os.lstat(self._join(f))
   WindowsError: [Error 3] The system cannot find the path specified:  
(really long path)

`hg status` thinks the state of the file is '?'.  `hg status --rev '.^'`  
thinks that the file was removed.  And `hg update null -C` doesn't remove  
the file.  I'm thinking a lot of the status/diff flakiness is because  
dirstate doesn't use vfs.
Kostia Balytskyi - Oct. 1, 2017, 10:33 a.m.
You guys are correct, these patches should not land. Yesterday I did another pass of experiments on Windows 10 (build 10.0.14393), and if looks like HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled now works better than advertised (it did not two months ago...) in that if it is enabled, both CreateFileA and CreateFileW accept long paths without \\?\ prefixes and work well with them, including paths that contain forward slashes as separators. So if I understand the situation correctly, MS has fixed stuff for us and there's literally nothing we should do.  In any case, this series is obsolete now.

> -----Original Message-----
> From: Matt Harbison [mailto:mharbison72@gmail.com]
> Sent: Saturday, 30 September, 2017 05:32
> To: Augie Fackler <raf@durin42.com>; Adrian Buehlmann
> <adrian@cadifra.com>; Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 6 of 6 v2] paths: allow util.finddirs touse os.path.sep
> 
> On Sat, 23 Sep 2017 00:20:35 -0400, Matt Harbison
> <mharbison72@gmail.com>
> wrote:
> 
> > Kostia- I'll try to look deeper at this series this weekend.  But does
> > the suggestion in the 2012 thread above seem reasonable, about just
> > tweaking the open function instead of (I assume) trying to pass full
> > \\?\ paths around all over?  That seems like it would be easier than
> > trying to get the paths converted everywhere, and I think would be
> > transparent to the test runner and the end user.
> 
> More info on this.  I applied this patch:
> 
> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
> --- a/mercurial/vfs.py
> +++ b/mercurial/vfs.py
> @@ -372,6 +372,8 @@ class vfs(abstractvfs):
>           if not text and "b" not in mode:
>               mode += "b" # for that other OS
> 
> +        f = '\\\\?\\' + f.replace('/', '\\')
> +        print('f is %s' % f)
>           nlink = -1
>           if mode not in ('r', 'rb'):
>               dirname, basename = util.split(f)
> 
> I see a lot of files under .hg being accessed like this when issuing diff and
> status, but none in wvfs:
> 
>      f is \\?\c:\Users\Matt\projects\reallyrealylongnames\.hg\requires
> 
> I did see a file in wvfs accessed like that when committing.
> 
> I created a repo on Linux with a 330 character long directory + file name,
> committed it, and pushed it to a Windows served repo.  That worked fine.
> Doing an update on the Windows side successfully created the file, but then
> aborted in dirstate, because it isn't using vfs:
> 
>    File "mercurial\merge.py", line 1721, in update
>      recordupdates(repo, actions, branchmerge)
>    File "mercurial\merge.py", line 1433, in recordupdates
>      repo.dirstate.normal(f)
>    File "mercurial\dirstate.py", line 569, in normal
>      s = os.lstat(self._join(f))
>    WindowsError: [Error 3] The system cannot find the path specified:
> (really long path)
> 
> `hg status` thinks the state of the file is '?'.  `hg status --rev '.^'` thinks that the
> file was removed.  And `hg update null -C` doesn't remove the file.  I'm
> thinking a lot of the status/diff flakiness is because dirstate doesn't use vfs.
Matt Harbison - Oct. 2, 2017, 3:04 a.m.
> On Oct 1, 2017, at 6:33 AM, Kostia Balytskyi <ikostia@fb.com> wrote:
> 
> You guys are correct, these patches should not land. Yesterday I did another pass of experiments on Windows 10 (build 10.0.14393), and if looks like HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled now works better than advertised (it did not two months ago...) in that if it is enabled, both CreateFileA and CreateFileW accept long paths without \\?\ prefixes and work well with them, including paths that contain forward slashes as separators. So if I understand the situation correctly, MS has fixed stuff for us and there's literally nothing we should do.  In any case, this series is obsolete now.

I don’t have easy access to Windows 10, but given this, maybe we should try running the test suite with a TEMP path > 260 characters and Greg’s manifest patch, to see if we can make this work out of the box?  I think the xxxA methods are usually a thin wrapper around the xxxW methods, so I’d be surprised if a registry entry makes them work, but the manifest entry doesn’t.  Adrian’s concern about whether or not python will handle long paths seems reasonable, but your testing seems to show that it might just work.  (I’m assuming you were running hg commands, and not doing toy C apps.)

Patch

diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -372,6 +372,8 @@  class vfs(abstractvfs):
          if not text and "b" not in mode:
              mode += "b" # for that other OS

+        f = '\\\\?\\' + f.replace('/', '\\')
+        print('f is %s' % f)
          nlink = -1
          if mode not in ('r', 'rb'):
              dirname, basename = util.split(f)