Merge lp:~jelmer/brz/win32-fstypes into lp:brz/3.2

[Jelmer Vernooij]

Attempt to support finding filesystem type on Windows.

[Aleksandr Smyshliaev (a1s)]

I found these problems with this patch:

1. Line 425 in win32utils.py is useless.
2. kernel32 is not defined in win32utils.py line 423.
3. The call in win32utils.py line 426 should use the API call computed in line 423. (It is possible to just put the whole path here and drop line 423 altogether.)
4. The argument to GetVolumeInformation must be a Unicode string and it must include the trailing backslash.
5. The path must be made absolute before passing it to splitdrive().
7. The type string returned by GetVolumeInformation() is upper case.

And one more note: for FAT volumes the type is "FAT32"; perhaps it should be added to the type checks.

[Jelmer Vernooij (jelmer)]

Please take another look.

[Aleksandr Smyshliaev (a1s)]

Two errors:

  File "D:\py3\brz\win32-fstypes\breezy\osutils.py", line 2596, in get_fs_type
    return _FILESYSTEM_FINDER.find(path)
  File "D:\py3\brz\win32-fstypes\breezy\osutils.py", line 2568, in find
    fs_type = win32utils.get_fs_type(drive + "\\")
TypeError: can't concat str to bytes

and (after undoing path.encode() in osutils.py line 2594)

  File "D:\py3\brz\win32-fstypes\breezy\tests\test_osutils.py", line 2262, in test_returns_none
    osutils.FilesystemFinder([]))
TypeError: FilesystemFinder() takes no arguments

[Jelmer Vernooij (jelmer)]

Fixed now I hope. PTAL.

[Aleksandr Smyshliaev (a1s)]

:-) This works, only GetFsTypeTests became mtab-specific.

Let it be this way, having Breezy functional is MUCH more important than split hairs on test coverage.

Thank you very much for working on this, Jelmer!

review: Approve

[Aleksandr Smyshliaev (a1s)]

I would add the requirements for the argument to the docstring on win32utils.get_fs_type: it requires the argument to be a Unicode string, and to include the trailing slash. Those requirements are very unobvious, and MS API docs only tell about one of them, that about the slash.

[Jelmer Vernooij (jelmer)]

Thanks, I've clarified the docstring.

I agree that the test coverage could be better here; I'll leave that open for the moment in the interest of incremental improvement.

[Jelmer Vernooij (jelmer)]

review: Approve

=== modified file 'breezy/osutils.py'
--- breezy/osutils.py	2022-01-12 23:43:40 +0000
+++ breezy/osutils.py	2022-01-16 17:50:49 +0000
@@ -1160,6 +1160,8 @@
         trace.mutter('Unable to get fs type for %r: %s', path, e)
         return True
     else:
+        if fs_type is None:
+            return sys.platform != 'win32'
         if fs_type in ('vfat', 'ntfs'):
             # filesystems known to not support hardlinks
             return False
@@ -1595,6 +1597,8 @@
     except errors.DependencyNotPresent as e:
         trace.mutter('Unable to get fs type for %r: %s', path, e)
     else:
+        if fs_type is None:
+            return sys.platform != 'win32'
         if fs_type in ('vfat', 'ntfs'):
             # filesystems known to not support executable bit
             return False
@@ -1612,6 +1616,8 @@
     except errors.DependencyNotPresent as e:
         trace.mutter('Unable to get fs type for %r: %s', path, e)
     else:
+        if fs_type is None:
+            return sys.platform != 'win32'
         if fs_type in ('vfat', 'ntfs'):
             # filesystems known to not support symlinks
             return False
@@ -2510,11 +2516,18 @@
             yield cols[1], cols[2].decode('ascii', 'replace')
 
 
-MTAB_PATH = '/etc/mtab'
-
 class FilesystemFinder(object):
     """Find the filesystem for a particular path."""
 
+    def find(self, path):
+        raise NotImplementedError
+
+
+class MtabFilesystemFinder(FilesystemFinder):
+    """Find the filesystem for a particular path."""
+
+    MTAB_PATH = '/etc/mtab'
+
     def __init__(self, mountpoints):
         def key(x):
             return len(x[0])
@@ -2530,7 +2543,7 @@
         # TODO(jelmer): Use inotify to be notified when /etc/mtab changes and
         # we need to re-read it.
         try:
-            return cls(read_mtab(MTAB_PATH))
+            return cls(read_mtab(cls.MTAB_PATH))
         except EnvironmentError as e:
             trace.mutter('Unable to read mtab: %s', e)
             return cls([])
@@ -2542,12 +2555,29 @@
         :return: Filesystem name (as text type) or None, if the filesystem is
             unknown.
         """
+        if not isinstance(path, bytes):
+            path = path.encode(_fs_enc)
         for mountpoint, filesystem in self._mountpoints:
             if is_inside(mountpoint, path):
                 return filesystem
         return None
 
 
+class Win32FilesystemFinder(FilesystemFinder):
+
+    def find(self, path):
+        drive = os.path.splitdrive(os.path.abspath(path))[0]
+        if isinstance(drive, bytes):
+            drive = drive.decode(_fs_enc)
+        fs_type = win32utils.get_fs_type(drive + "\\")
+        if fs_type is None:
+            return None
+        return {
+            'FAT32': 'vfat',
+            'NTFS': 'ntfs',
+            }.get(fs_type, fs_type)
+
+
 _FILESYSTEM_FINDER = None
 
 
@@ -2559,10 +2589,10 @@
     """
     global _FILESYSTEM_FINDER
     if _FILESYSTEM_FINDER is None:
-        _FILESYSTEM_FINDER = FilesystemFinder.from_mtab()
-
-    if not isinstance(path, bytes):
-        path = path.encode(_fs_enc)
+        if sys.platform == 'win32':
+            _FILESYSTEM_FINDER = Win32FilesystemFinder()
+        else:
+            _FILESYSTEM_FINDER = MtabFilesystemFinder.from_mtab()
 
     return _FILESYSTEM_FINDER.find(path)
 

=== modified file 'breezy/tests/test_osutils.py'
--- breezy/tests/test_osutils.py	2022-01-12 23:43:40 +0000
+++ breezy/tests/test_osutils.py	2022-01-16 17:50:49 +0000
@@ -2217,9 +2217,37 @@
 
 class SupportsSymlinksTests(tests.TestCaseInTempDir):
 
+    def setUp(self):
+        super(SupportsSymlinksTests, self).setUp()
+        self.overrideAttr(
+            osutils, '_FILESYSTEM_FINDER',
+            osutils.MtabFilesystemFinder([
+                (b'/usr', 'ext4'),
+                (b'/home', 'vfat'),
+                (b'/home/jelmer/smb', 'ntfs'),
+                (b'/home/jelmer', 'ext2'),
+            ]))
+
     def test_returns_bool(self):
         self.assertIsInstance(osutils.supports_symlinks(self.test_dir), bool)
 
+    def test_known(self):
+        self.assertTrue(osutils.supports_symlinks("/usr"))
+        self.assertFalse(osutils.supports_symlinks("/home/bogus"))
+        self.assertTrue(osutils.supports_symlinks("/home/jelmer/osx"))
+        self.assertFalse(osutils.supports_symlinks("/home/jelmer/smb"))
+
+    def test_unknown(self):
+        have_symlinks = sys.platform != "win32"
+        self.assertIs(osutils.supports_symlinks("/var"), have_symlinks)
+
+    def test_error(self):
+        have_symlinks = sys.platform != "win32"
+        def raise_error(path):
+            raise errors.DependencyNotPresent('FS', 'TEST')
+        self.overrideAttr(osutils, 'get_fs_type', raise_error)
+        self.assertIs(osutils.supports_symlinks("/var"), have_symlinks)
+
 
 class MtabReader(tests.TestCaseInTempDir):
 
@@ -2246,7 +2274,7 @@
     def test_returns_most_specific(self):
         self.overrideAttr(
             osutils, '_FILESYSTEM_FINDER',
-            osutils.FilesystemFinder(
+            osutils.MtabFilesystemFinder(
                 [(b'/', 'ext4'), (b'/home', 'vfat'),
                  (b'/home/jelmer', 'ext2')]))
         self.assertEqual(osutils.get_fs_type(b'/home/jelmer/blah'), 'ext2')
@@ -2259,7 +2287,7 @@
     def test_returns_none(self):
         self.overrideAttr(
             osutils, '_FILESYSTEM_FINDER',
-            osutils.FilesystemFinder([]))
+            osutils.MtabFilesystemFinder([]))
         self.assertIs(osutils.get_fs_type('/home/jelmer/blah'), None)
         self.assertIs(osutils.get_fs_type(b'/home/jelmer/blah'), None)
         self.assertIs(osutils.get_fs_type('/home/jelmer'), None)

=== modified file 'breezy/win32utils.py'
--- breezy/win32utils.py	2022-01-09 13:12:27 +0000
+++ breezy/win32utils.py	2022-01-16 17:50:49 +0000
@@ -16,16 +16,17 @@
 
 """Win32-specific helper functions"""
 
-import ctypes
 import glob
 import os
 import struct
-import sys
-
-from breezy import (
-    cmdline,
-    )
+
+from .lazy_import import lazy_import
+lazy_import(globals(), """
+import ctypes
+
+from breezy import cmdline
 from breezy.i18n import gettext
+""")
 
 
 # Special Win32 API constants
@@ -344,7 +345,6 @@
 def set_file_attr_hidden(path):
     """Set file attributes to hidden if possible"""
     from ctypes.wintypes import BOOL, DWORD, LPWSTR
-    import ctypes
     # <https://docs.microsoft.com/windows/desktop/api/fileapi/nf-fileapi-setfileattributesw>
     SetFileAttributes = ctypes.windll.kernel32.SetFileAttributesW
     SetFileAttributes.argtypes = LPWSTR, DWORD
@@ -399,8 +399,8 @@
 
 
 def _ctypes_is_local_pid_dead(pid):
-    kernel32 = ctypes.windll.kernel32
     """True if pid doesn't correspond to live process on this machine"""
+    kernel32 = ctypes.windll.kernel32
     handle = kernel32.OpenProcess(1, False, pid)
     if not handle:
         errorcode = ctypes.GetLastError()
@@ -414,3 +414,29 @@
     return False
 
 is_local_pid_dead = _ctypes_is_local_pid_dead
+
+
+def get_fs_type(drive):
+    """Return file system type for a drive on the system.
+
+    Args:
+      drive: Unicode string with drive including trailing backslash (e.g.
+         "C:\\")
+    Returns:
+      Windows filesystem type name (e.g. "FAT32", "NTFS") or None
+      if the drive can not be found
+    """
+    MAX_FS_TYPE_LENGTH = 16
+    kernel32 = ctypes.windll.kernel32
+    GetVolumeInformation = kernel32.GetVolumeInformationW
+    fs_type = ctypes.create_unicode_buffer(MAX_FS_TYPE_LENGTH + 1)
+    if GetVolumeInformation(
+        drive,
+        None, 0, # lpVolumeName
+        None, # lpVolumeSerialNumber
+        None, # lpMaximumComponentLength
+        None, # lpFileSystemFlags
+        fs_type, MAX_FS_TYPE_LENGTH,
+    ):
+        return fs_type.value
+    return None