Patchwork [1,of,2] vfs: add a 'reljoin' function for joining relative path

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 15, 2014, 9:48 p.m.
Message ID <85b532e6228482d7d772.1418680136@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/7121/
State Accepted
Delegated to: Augie Fackler
Headers show

Comments

Pierre-Yves David - Dec. 15, 2014, 9:48 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1418678866 28800
#      Mon Dec 15 13:27:46 2014 -0800
# Node ID 85b532e6228482d7d7723827254fe01c7ea753cf
# Parent  495bc1b65d25872324a0220354f048b220304bd1
vfs: add a 'reljoin' function for joining relative path

the 'vfs.join' method onlyh works for absolute path. We want something that
work for relative patch too when transforming file name. We want to avoid using
os.path.join because it may misbehave in some tricky encoding situation.

In the same go, we replace the usage of 'os.patch.join' in transaction code.
Augie Fackler - Dec. 16, 2014, 3:55 p.m.
On Mon, Dec 15, 2014 at 01:48:56PM -0800, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1418678866 28800
> #      Mon Dec 15 13:27:46 2014 -0800
> # Node ID 85b532e6228482d7d7723827254fe01c7ea753cf
> # Parent  495bc1b65d25872324a0220354f048b220304bd1
> vfs: add a 'reljoin' function for joining relative path
>
> the 'vfs.join' method onlyh works for absolute path. We want something that
> work for relative patch too when transforming file name. We want to avoid using
> os.path.join because it may misbehave in some tricky encoding situation.

It took me multiple readings to understand that the base vfs will use
os.path.join, but that others may do their own logic. Perhaps it would
be more clearly worded thus:

The vfs.join method only works for absolute paths. We need something
that works for relative paths too when transforming filenames. Since
os.path.join may misbehave in tricky encoding situations, encapsulate
the new join method in our vfs abstraction. The default implementation
remains os.path.join, but this opens the door to other VFSes doing
something more intelligent based on their needs.

Is it okay if I queue with that paragraph instead of your paragraph?

>
> In the same go, we replace the usage of 'os.patch.join' in transaction code.
>
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -259,10 +259,17 @@ class abstractvfs(object):
>          return os.path.isfile(self.join(path))
>
>      def islink(self, path=None):
>          return os.path.islink(self.join(path))
>
> +    def reljoin(self, *paths):
> +        """join various elements of a path together (as os.path.join would do)
> +
> +        The vfs base is not injected so that path stay relative. This exists
> +        to allow handling of strange encoding if needed."""
> +        return os.path.join(*paths)
> +
>      def lexists(self, path=None):
>          return os.path.lexists(self.join(path))
>
>      def lstat(self, path=None):
>          return os.lstat(self.join(path))
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -198,12 +198,12 @@ class transaction(object):
>
>          if file in self.map or file in self._backupmap:
>              return
>          dirname, filename = os.path.split(file)
>          backupfilename = "%s.backup.%s" % (self.journal, filename)
> -        backupfile = os.path.join(dirname, backupfilename)
>          vfs = self._vfsmap[location]
> +        backupfile = vfs.reljoin(dirname, backupfilename)
>          if vfs.exists(file):
>              filepath = vfs.join(file)
>              backuppath = vfs.join(backupfile)
>              util.copyfiles(filepath, backuppath, hardlink=hardlink)
>          else:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Dec. 16, 2014, 5:02 p.m.
On 12/16/2014 07:55 AM, Augie Fackler wrote:
> On Mon, Dec 15, 2014 at 01:48:56PM -0800, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1418678866 28800
>> #      Mon Dec 15 13:27:46 2014 -0800
>> # Node ID 85b532e6228482d7d7723827254fe01c7ea753cf
>> # Parent  495bc1b65d25872324a0220354f048b220304bd1
>> vfs: add a 'reljoin' function for joining relative path
>>
>> the 'vfs.join' method onlyh works for absolute path. We want something that
>> work for relative patch too when transforming file name. We want to avoid using
>> os.path.join because it may misbehave in some tricky encoding situation.
>
> It took me multiple readings to understand that the base vfs will use
> os.path.join, but that others may do their own logic. Perhaps it would
> be more clearly worded thus:
>
> The vfs.join method only works for absolute paths. We need something
> that works for relative paths too when transforming filenames. Since
> os.path.join may misbehave in tricky encoding situations, encapsulate
> the new join method in our vfs abstraction. The default implementation
> remains os.path.join, but this opens the door to other VFSes doing
> something more intelligent based on their needs.
>
> Is it okay if I queue with that paragraph instead of your paragraph?

Yes, you new paragraph is perfect.
Augie Fackler - Dec. 16, 2014, 5:18 p.m.
On Mon, Dec 15, 2014 at 01:48:56PM -0800, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1418678866 28800
> #      Mon Dec 15 13:27:46 2014 -0800
> # Node ID 85b532e6228482d7d7723827254fe01c7ea753cf
> # Parent  495bc1b65d25872324a0220354f048b220304bd1
> vfs: add a 'reljoin' function for joining relative path
>

these are now queued

>
> the 'vfs.join' method onlyh works for absolute path. We want something that
> work for relative patch too when transforming file name. We want to avoid using
> os.path.join because it may misbehave in some tricky encoding situation.
>
> In the same go, we replace the usage of 'os.patch.join' in transaction code.
>
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -259,10 +259,17 @@ class abstractvfs(object):
>          return os.path.isfile(self.join(path))
>
>      def islink(self, path=None):
>          return os.path.islink(self.join(path))
>
> +    def reljoin(self, *paths):
> +        """join various elements of a path together (as os.path.join would do)
> +
> +        The vfs base is not injected so that path stay relative. This exists
> +        to allow handling of strange encoding if needed."""
> +        return os.path.join(*paths)
> +
>      def lexists(self, path=None):
>          return os.path.lexists(self.join(path))
>
>      def lstat(self, path=None):
>          return os.lstat(self.join(path))
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -198,12 +198,12 @@ class transaction(object):
>
>          if file in self.map or file in self._backupmap:
>              return
>          dirname, filename = os.path.split(file)
>          backupfilename = "%s.backup.%s" % (self.journal, filename)
> -        backupfile = os.path.join(dirname, backupfilename)
>          vfs = self._vfsmap[location]
> +        backupfile = vfs.reljoin(dirname, backupfilename)
>          if vfs.exists(file):
>              filepath = vfs.join(file)
>              backuppath = vfs.join(backupfile)
>              util.copyfiles(filepath, backuppath, hardlink=hardlink)
>          else:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -259,10 +259,17 @@  class abstractvfs(object):
         return os.path.isfile(self.join(path))
 
     def islink(self, path=None):
         return os.path.islink(self.join(path))
 
+    def reljoin(self, *paths):
+        """join various elements of a path together (as os.path.join would do)
+
+        The vfs base is not injected so that path stay relative. This exists
+        to allow handling of strange encoding if needed."""
+        return os.path.join(*paths)
+
     def lexists(self, path=None):
         return os.path.lexists(self.join(path))
 
     def lstat(self, path=None):
         return os.lstat(self.join(path))
diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -198,12 +198,12 @@  class transaction(object):
 
         if file in self.map or file in self._backupmap:
             return
         dirname, filename = os.path.split(file)
         backupfilename = "%s.backup.%s" % (self.journal, filename)
-        backupfile = os.path.join(dirname, backupfilename)
         vfs = self._vfsmap[location]
+        backupfile = vfs.reljoin(dirname, backupfilename)
         if vfs.exists(file):
             filepath = vfs.join(file)
             backuppath = vfs.join(backupfile)
             util.copyfiles(filepath, backuppath, hardlink=hardlink)
         else: