Ensure File.cp_r/3 respects trailing / in sources

Author: José Valim

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 * Bug fixes
   * [Atom] Inspect `:...` and `:foo@bar` without quoting
+  * [File] Respect source directories terminating with "/" in `File.cp_r/3` with the same semantics as Unix
   * [Kernel] Guarantee nullary funs/macros are allowed in guards
   * [Process] Ensure monitoring functions are inlined by the compiler
 

lib/elixir/lib/file.ex

@@ -349,7 +349,7 @@ defmodule File do
     output =
       if dir?(destination) do
         mkdir(destination)
-        FN.join(destination, FN.basename(source))
+        do_cp_join(destination, source)
       else
         destination
       end
@@ -376,9 +376,10 @@ defmodule File do
 
   @doc %S"""
   Copies the contents in source to destination.
+
   Similar to the command `cp -r` in Unix systems,
   this function behaves differently depending
-  if `source` and `destination` are a file or a directory.
+  if `source` and `destination` are files or directories.
 
   If both are files, it simply copies `source` to
   `destination`. However, if `destination` is a directory,
@@ -412,10 +413,10 @@ defmodule File do
       File.cp_r "samples", "tmp"
 
       # Copies all files in "samples" to "tmp"
-      File.cp_r "samples/.", "tmp"
+      File.cp_r "samples/", "tmp"
 
       # Same as before, but asks the user how to proceed in case of conflicts
-      File.cp_r "samples/.", "tmp", fn(source, destination) ->
+      File.cp_r "samples/", "tmp", fn(source, destination) ->
         IO.gets("Overwriting #{destination} by #{source}. Type y to confirm.") == "y"
       end
 
@@ -424,7 +425,10 @@ defmodule File do
     output =
       if dir?(destination) || dir?(source) do
         mkdir(destination)
-        FN.join(destination, FN.basename(source))
+        case do_cp_last(source) do
+          ?/ -> destination
+          _  -> do_cp_join(destination, source)
+        end
       else
         destination
       end
@@ -450,6 +454,23 @@ defmodule File do
     end
   end
 
+  defp do_cp_last(source) when is_atom(source),    do: :lists.last(atom_to_list(source))
+  defp do_cp_last(source) when is_list(source),    do: do_cp_last(:lists.last(source))
+  defp do_cp_last(source) when is_binary(source),  do: :binary.last(source)
+  defp do_cp_last(source) when is_integer(source), do: source
+
+  defp do_cp_join(destination, source) when source in [".", '.'] do
+    destination
+  end
+
+  defp do_cp_join(destination, source) do
+    case FN.basename(source) do
+      dot when dot in ["..", '..'] -> do_cp_join(destination, FN.dirname(FN.dirname(source)))
+      dot when dot in [".", '.'] -> do_cp_join(destination, FN.dirname(source))
+      base -> FN.join(destination, base)
+    end
+  end
+
   # src may be a file or a directory, dest is definitely
   # a directory. Returns nil unless an error is found.
   defp do_cp_r(src, dest, callback, acc) when is_list(acc) do

lib/elixir/lib/path.ex

@@ -408,8 +408,8 @@ defmodule Path do
     do_join(rest, relativename, [?/|result], os_type)
   defp do_join(<<?/, rest :: binary>>, relativename, [?/|result], os_type), do:
     do_join(rest, relativename, [?/|result], os_type)
-  defp do_join(<<>>, <<>>, result, os_type), do:
-    iolist_to_binary(maybe_remove_dirsep(result, os_type))
+  defp do_join(<<>>, <<>>, result, _os_type), do:
+    iolist_to_binary(:lists.reverse(result))
   defp do_join(<<>>, relativename, [?:|rest], :win32), do:
     do_join(relativename, <<>>, [?:|rest], :win32)
   defp do_join(<<>>, relativename, [?/|result], os_type), do:
@@ -419,15 +419,6 @@ defmodule Path do
   defp do_join(<<char, rest :: binary>>, relativename, result, os_type) when is_integer(char), do:
     do_join(rest, relativename, [char|result], os_type)
 
-  defp maybe_remove_dirsep([?/, ?:, letter], :win32), do:
-    [letter, ?:, ?/]
-  defp maybe_remove_dirsep([?/], _), do:
-    [?/]
-  defp maybe_remove_dirsep([?/|name], _), do:
-    :lists.reverse(name)
-  defp maybe_remove_dirsep(name, _), do:
-    :lists.reverse(name)
-
   @doc """
   Returns a list with the path split by the path separator.
   If an empty string is given, returns the root path.

lib/elixir/test/elixir/file_test.exs

@@ -210,6 +210,28 @@ defmodule FileTest do
 
       File.mkdir(dest)
 
+      try do
+        refute File.exists?(tmp_path("tmp/cp_r/a/1.txt"))
+        refute File.exists?(tmp_path("tmp/cp_r/a/a/2.txt"))
+        refute File.exists?(tmp_path("tmp/cp_r/b/3.txt"))
+
+        { :ok, files } = File.cp_r(src, dest)
+        assert length(files) == 7
+
+        assert File.exists?(tmp_path("tmp/cp_r/a/1.txt"))
+        assert File.exists?(tmp_path("tmp/cp_r/a/a/2.txt"))
+        assert File.exists?(tmp_path("tmp/cp_r/b/3.txt"))
+      after
+        File.rm_rf dest
+      end
+    end
+
+    test :cp_r_with_src_dir_slash_and_dest_dir do
+      src  = fixture_path("cp_r") <> "/"
+      dest = tmp_path("tmp")
+
+      File.mkdir(dest)
+
       try do
         refute File.exists?(tmp_path("tmp/a/1.txt"))
         refute File.exists?(tmp_path("tmp/a/a/2.txt"))
@@ -265,7 +287,7 @@ defmodule FileTest do
     end
 
     test :cp_r_with_dir_and_file_conflict do
-      src  = fixture_path("cp_r/.")
+      src  = fixture_path("cp_r") <> "/"
       dest = tmp_path("tmp")
 
       try do
@@ -278,7 +300,7 @@ defmodule FileTest do
     end
 
     test :cp_r_with_src_dir_and_dest_dir_using_lists do
-      src  = fixture_path("cp_r/.") |> to_char_list
+      src  = (fixture_path("cp_r") <> "/") |> to_char_list
       dest = tmp_path("tmp") |> to_char_list
 
       File.mkdir(dest)
@@ -301,7 +323,7 @@ defmodule FileTest do
     end
 
     test :cp_r_with_src_with_file_conflict do
-      src  = fixture_path("cp_r/.")
+      src  = fixture_path("cp_r") <> "/"
       dest = tmp_path("tmp")
 
       File.mkdir_p tmp_path("tmp/a")
@@ -317,7 +339,7 @@ defmodule FileTest do
     end
 
     test :cp_r_with_src_with_file_conflict_callback do
-      src  = fixture_path("cp_r/.")
+      src  = fixture_path("cp_r") <> "/"
       dest = tmp_path("tmp")
 
       File.mkdir_p tmp_path("tmp/a")
@@ -337,7 +359,7 @@ defmodule FileTest do
     end
 
     test :cp_r! do
-      src  = fixture_path("cp_r/.")
+      src  = fixture_path("cp_r") <> "/"
       dest = tmp_path("tmp")
 
       File.mkdir(dest)
@@ -770,7 +792,7 @@ defmodule FileTest do
     test :rm_rf do
       fixture = tmp_path("tmp")
       File.mkdir(fixture)
-      File.cp_r!(fixture_path("cp_r/."), fixture)
+      File.cp_r!(fixture_path("cp_r") <> "/", fixture)
 
       assert File.exists?(tmp_path("tmp/a/1.txt"))
       assert File.exists?(tmp_path("tmp/a/a/2.txt"))
@@ -811,7 +833,7 @@ defmodule FileTest do
     test :rm_rf_with_char_list do
       fixture = tmp_path("tmp") |> to_char_list
       File.mkdir(fixture)
-      File.cp_r!(fixture_path("cp_r/."), fixture)
+      File.cp_r!(fixture_path("cp_r") <> "/", fixture)
 
       assert File.exists?(tmp_path("tmp/a/1.txt"))
       assert File.exists?(tmp_path("tmp/a/a/2.txt"))
@@ -847,7 +869,7 @@ defmodule FileTest do
     test :rm_rf! do
       fixture = tmp_path("tmp")
       File.mkdir(fixture)
-      File.cp_r!(fixture_path("cp_r/."), fixture)
+      File.cp_r!(fixture_path("cp_r") <> "/", fixture)
 
       assert File.exists?(tmp_path("tmp/a/1.txt"))
       assert File.exists?(tmp_path("tmp/a/a/2.txt"))

lib/elixir/test/elixir/path_test.exs

@@ -223,13 +223,15 @@ defmodule PathTest do
     assert Path.join(["foo"]) == "foo"
     assert Path.join(["/", "foo", "bar"]) == "/foo/bar"
     assert Path.join(["~", "foo", "bar"]) == "~/foo/bar"
+    assert Path.join(["/foo/", "/bar/"]) == "/foo/bar/"
   end
 
   test :join_with_list do
     assert Path.join(['']) == ''
     assert Path.join(['foo']) == 'foo'
     assert Path.join(['/', 'foo', 'bar']) == '/foo/bar'
     assert Path.join(['~', 'foo', 'bar']) == '~/foo/bar'
+    assert Path.join(['/foo/', '/bar/']) == '/foo/bar/'
   end
 
   test :join_two_with_binary do

lib/mix/lib/mix/utils.ex

@@ -343,7 +343,7 @@ defmodule Mix.Utils do
     symlink_source = make_relative_path(source, target)
     case :file.make_symlink(symlink_source, target) do
       :ok -> :ok
-      { :error, _ } -> File.cp_r!(source, Path.dirname(target))
+      { :error, _ } -> File.cp_r!(source <> "/", target)
     end
   end
 

lib/mix/test/test_helper.exs

@@ -75,7 +75,7 @@ defmodule MixTest.Case do
   end
 
   def in_fixture(which, tmp, function) do
-    src  = Path.join fixture_path(which), "."
+    src  = fixture_path(which) <> "/"
     dest = tmp_path(tmp)
     flag = tmp_path |> String.to_char_list!