Copy/get/put empty directories correctly (#1199)

* Copy/get/put empty directories correctly

* Trailing slash is enough to indicate directory in cp/get/put

* Performance improvements

Author: ianthomas23

fsspec/asyn.py

@@ -337,7 +337,9 @@ async def _copy(
             on_error = "raise"
 
         paths = await self._expand_path(path1, maxdepth=maxdepth, recursive=recursive)
-        isdir = isinstance(path2, str) and await self._isdir(path2)
+        isdir = isinstance(path2, str) and (
+            path2.endswith("/") or await self._isdir(path2)
+        )
         path2 = other_paths(
             paths,
             path2,
@@ -486,7 +488,9 @@ async def _put(
             lpath = make_path_posix(lpath)
         fs = LocalFileSystem()
         lpaths = fs.expand_path(lpath, recursive=recursive)
-        isdir = isinstance(rpath, str) and await self._isdir(rpath)
+        isdir = isinstance(rpath, str) and (
+            rpath.endswith("/") or await self._isdir(rpath)
+        )
         rpaths = other_paths(
             lpaths,
             rpath,
@@ -538,7 +542,9 @@ async def _get(
         rpath = self._strip_protocol(rpath)
         lpath = make_path_posix(lpath)
         rpaths = await self._expand_path(rpath, recursive=recursive)
-        isdir = isinstance(lpath, str) and LocalFileSystem().isdir(lpath)
+        isdir = isinstance(lpath, str) and (
+            lpath.endswith("/") or LocalFileSystem().isdir(lpath)
+        )
         lpaths = other_paths(
             rpaths,
             lpath,

fsspec/implementations/tests/test_local.py

@@ -910,6 +910,55 @@ def test_cp_get_put_directory_recursive(tmpdir, funcname):
         assert fs.find(target) == [make_path_posix(os.path.join(target, "file"))]
 
 
+@pytest.mark.parametrize("funcname", ["cp", "get", "put"])
+def test_cp_get_put_empty_directory(tmpdir, funcname):
+    # https://github.com/fsspec/filesystem_spec/issues/1198
+    # cp/get/put of empty directory.
+    fs = LocalFileSystem(auto_mkdir=True)
+    empty = os.path.join(str(tmpdir), "empty")
+    fs.mkdir(empty)
+
+    target = os.path.join(str(tmpdir), "target")
+    fs.mkdir(target)
+
+    if funcname == "cp":
+        func = fs.cp
+    elif funcname == "get":
+        func = fs.get
+    elif funcname == "put":
+        func = fs.put
+
+    # cp/get/put without slash, target directory exists
+    assert fs.isdir(target)
+    func(empty, target)
+    assert fs.find(target, withdirs=True) == [
+        make_path_posix(os.path.join(target, "empty"))
+    ]
+
+    fs.rm(target + "/empty", recursive=True)
+
+    # cp/get/put with slash, target directory exists
+    assert fs.isdir(target)
+    func(empty + "/", target)
+    assert fs.find(target, withdirs=True) == []
+
+    fs.rm(target, recursive=True)
+
+    # cp/get/put without slash, target directory doesn't exist
+    assert not fs.isdir(target)
+    func(empty, target)
+    assert fs.isdir(target)
+    assert fs.find(target, withdirs=True) == []
+
+    fs.rm(target, recursive=True)
+
+    # cp/get/put with slash, target directory doesn't exist
+    assert not fs.isdir(target)
+    func(empty + "/", target)
+    assert fs.isdir(target)
+    assert fs.find(target, withdirs=True) == []
+
+
 def test_cp_two_files(tmpdir):
     fs = LocalFileSystem(auto_mkdir=True)
     src = os.path.join(str(tmpdir), "src")

fsspec/implementations/tests/test_memory.py

@@ -272,6 +272,44 @@ def test_put_directory_recursive(m, tmpdir):
         assert m.find(target) == correct
 
 
+def test_cp_empty_directory(m):
+    # https://github.com/fsspec/filesystem_spec/issues/1198
+    # cp/get/put of empty directory.
+    empty = "/src/empty"
+    m.mkdir(empty)
+
+    target = "/target"
+    m.mkdir(target)
+
+    # cp without slash, target directory exists
+    assert m.isdir(target)
+    m.cp(empty, target)
+    assert m.find(target, withdirs=True) == [target + "/empty"]
+
+    m.rm(target + "/empty", recursive=True)
+
+    # cp with slash, target directory exists
+    assert m.isdir(target)
+    m.cp(empty + "/", target)
+    assert m.find(target, withdirs=True) == []
+
+    m.rm(target, recursive=True)
+
+    # cp without slash, target directory doesn't exist
+    assert not m.isdir(target)
+    m.cp(empty, target)
+    assert m.isdir(target)
+    assert m.find(target, withdirs=True) == []
+
+    m.rm(target, recursive=True)
+
+    # cp with slash, target directory doesn't exist
+    assert not m.isdir(target)
+    m.cp(empty + "/", target)
+    assert m.isdir(target)
+    assert m.find(target, withdirs=True) == []
+
+
 def test_cp_two_files(m):
     src = "/src"
     file0 = src + "/file0"

fsspec/spec.py

@@ -884,7 +884,9 @@ def get(self, rpath, lpath, recursive=False, callback=_DEFAULT_CALLBACK, **kwarg
         if isinstance(lpath, str):
             lpath = make_path_posix(lpath)
         rpaths = self.expand_path(rpath, recursive=recursive)
-        isdir = isinstance(lpath, str) and LocalFileSystem().isdir(lpath)
+        isdir = isinstance(lpath, str) and (
+            lpath.endswith("/") or LocalFileSystem().isdir(lpath)
+        )
         lpaths = other_paths(
             rpaths,
             lpath,
@@ -937,7 +939,7 @@ def put(self, lpath, rpath, recursive=False, callback=_DEFAULT_CALLBACK, **kwarg
             lpath = make_path_posix(lpath)
         fs = LocalFileSystem()
         lpaths = fs.expand_path(lpath, recursive=recursive)
-        isdir = isinstance(rpath, str) and self.isdir(rpath)
+        isdir = isinstance(rpath, str) and (rpath.endswith("/") or self.isdir(rpath))
         rpaths = other_paths(
             lpaths,
             rpath,
@@ -978,7 +980,7 @@ def copy(self, path1, path2, recursive=False, on_error=None, **kwargs):
             on_error = "raise"
 
         paths = self.expand_path(path1, recursive=recursive)
-        isdir = isinstance(path2, str) and self.isdir(path2)
+        isdir = isinstance(path2, str) and (path2.endswith("/") or self.isdir(path2))
         path2 = other_paths(
             paths,
             path2,

fsspec/tests/test_utils.py

@@ -255,42 +255,76 @@ def test_common_prefix(paths, out):
 
 
 @pytest.mark.parametrize(
-    "paths, other, is_dir, expected",
+    "paths, other, is_dir, exists, expected",
     (
-        (["/path1"], "/path2", False, ["/path2"]),
-        (["/path1"], "/path2", True, ["/path2/path1"]),
-        (["/path1"], "/path2", None, ["/path2"]),
-        (["/path1"], "/path2/", True, ["/path2/path1"]),
-        (["/path1"], ["/path2"], True, ["/path2"]),
-        (["/path1", "/path2"], "/path2", True, ["/path2/path1", "/path2/path2"]),
+        (["/path1"], "/path2", False, False, ["/path2"]),
+        (["/path1"], "/path2", True, True, ["/path2/path1"]),
+        (["/path1"], "/path2", None, False, ["/path2"]),
+        (["/path1"], "/path2/", True, True, ["/path2/path1"]),
+        (["/path1"], ["/path2"], True, False, ["/path2"]),
+        (["/path1"], ["/path2"], True, True, ["/path2"]),
+        (["/path1", "/path2"], "/path2", True, False, ["/path2/path1", "/path2/path2"]),
+        (["/path1", "/path2"], "/path2", True, True, ["/path2/path1", "/path2/path2"]),
         (
             ["/more/path1", "/more/path2"],
             "/path2",
             True,
+            False,
             ["/path2/path1", "/path2/path2"],
         ),
         (
             ["/more/path1", "/more/path2"],
             "/path2",
+            True,
+            True,
+            ["/path2/more/path1", "/path2/more/path2"],
+        ),
+        (
+            ["/more/path1", "/more/path2"],
+            "/path2",
+            False,
             False,
             ["/path2/path1", "/path2/path2"],
         ),
+        (
+            ["/more/path1", "/more/path2"],
+            "/path2",
+            False,
+            True,
+            ["/path2/more/path1", "/path2/more/path2"],
+        ),
         (
             ["/more/path1", "/more/path2"],
             "/path2/",
             None,
+            False,
             ["/path2/path1", "/path2/path2"],
         ),
+        (
+            ["/more/path1", "/more/path2"],
+            "/path2/",
+            None,
+            True,
+            ["/path2/more/path1", "/path2/more/path2"],
+        ),
+        (
+            ["/more/path1", "/diff/path2"],
+            "/path2/",
+            None,
+            False,
+            ["/path2/more/path1", "/path2/diff/path2"],
+        ),
         (
             ["/more/path1", "/diff/path2"],
             "/path2/",
             None,
+            True,
             ["/path2/more/path1", "/path2/diff/path2"],
         ),
     ),
 )
-def test_other_paths(paths, other, is_dir, expected):
-    assert other_paths(paths, other, is_dir) == expected
+def test_other_paths(paths, other, is_dir, exists, expected):
+    assert other_paths(paths, other, is_dir, exists) == expected
 
 
 def test_log():

fsspec/utils.py

@@ -366,16 +366,10 @@ def other_paths(paths, path2, is_dir=None, exists=False):
     if isinstance(path2, str):
         is_dir = is_dir or path2.endswith("/")
         path2 = path2.rstrip("/")
-        if len(paths) > 1:
-            cp = common_prefix(paths)
-            if exists:
-                cp = cp.rsplit("/", 1)[0]
-            path2 = [p.replace(cp, path2, 1) for p in paths]
-        else:
-            if is_dir:
-                path2 = [path2.rstrip("/") + "/" + paths[0].rsplit("/")[-1]]
-            else:
-                path2 = [path2]
+        cp = common_prefix(paths)
+        if exists:
+            cp = cp.rsplit("/", 1)[0]
+        path2 = [p.replace(cp, path2, 1) for p in paths]
     else:
         assert len(paths) == len(path2)
     return path2