Merge pull request #1457 from ferd/windows-mv

Fix `mv` on windows

src/rebar_file_utils.erl

@@ -185,32 +185,113 @@ mv(Source, Dest) ->
                     ok
             end;
         {win32, _} ->
-            Cmd = case filelib:is_dir(Source) of
-                      true ->
-                          ?FMT("robocopy /move /e \"~s\" \"~s\" 1> nul",
-                               [rebar_utils:escape_double_quotes(filename:nativename(Source)),
-                                rebar_utils:escape_double_quotes(filename:nativename(Dest))]);
-                      false ->
-                          ?FMT("robocopy /move /e \"~s\" \"~s\" \"~s\" 1> nul",
-                               [rebar_utils:escape_double_quotes(filename:nativename(filename:dirname(Source))),
-                                rebar_utils:escape_double_quotes(filename:nativename(Dest)),
-                                rebar_utils:escape_double_quotes(filename:basename(Source))])
-                  end,
-            Res = rebar_utils:sh(Cmd,
-                        [{use_stdout, false}, return_on_error]),
-            case win32_ok(Res) of
-                true -> ok;
+            case filelib:is_dir(Source) of
+                true ->
+                    SrcDir = filename:nativename(Source),
+                    DestDir = case filelib:is_dir(Dest) of
+                        true ->
+                            %% to simulate unix/posix mv, we have to replicate
+                            %% the same directory movement by moving the whole
+                            %% top-level directory, not just the insides
+                            SrcName = filename:basename(Source),
+                            filename:nativename(filename:join(Dest, SrcName));
+                        false ->
+                            filename:nativename(Dest)
+                    end,
+                    robocopy_dir(SrcDir, DestDir);
                 false ->
-                    {error, lists:flatten(
-                              io_lib:format("Failed to move ~s to ~s~n",
-                                            [Source, Dest]))}
+                    SrcDir = filename:nativename(filename:dirname(Source)),
+                    SrcName = filename:basename(Source),
+                    DestDir = filename:nativename(filename:dirname(Dest)),
+                    DestName = filename:basename(Dest),
+                    IsDestDir = filelib:is_dir(Dest),
+                    if IsDestDir ->
+                        %% if basename and target name are different because
+                        %% we move to a directory, then just move there.
+                        %% Similarly, if they are the same but we're going to
+                        %% a directory, let's just do that directly.
+                        FullDestDir = filename:nativename(Dest),
+                        robocopy_file(SrcDir, FullDestDir, SrcName)
+                    ;  SrcName =:= DestName ->
+                        %% if basename and target name are the same and both are files,
+                        %% we do a regular move with robocopy without rename.
+                        robocopy_file(SrcDir, DestDir, DestName)
+                    ;  SrcName =/= DestName->
+                        robocopy_mv_and_rename(Source, Dest, SrcDir, SrcName, DestDir, DestName)
+                    end
+
             end
     end.
 
+robocopy_mv_and_rename(Source, Dest, SrcDir, SrcName, DestDir, DestName) ->
+    %% If we're moving a file and the origin and
+    %% destination names are different:
+    %%  - mktmp
+    %%  - robocopy source_dir tmp_dir srcname
+    %%  - rename srcname destname (to avoid clobbering)
+    %%  - robocopy tmp_dir dest_dir destname
+    %%  - remove tmp_dir
+    case ec_file:insecure_mkdtemp() of
+        {error, _Reason} ->
+            {error, lists:flatten(
+                     io_lib:format("Failed to move ~s to ~s (tmpdir failed)~n",
+                                   [Source, Dest]))};
+        TmpPath ->
+            case robocopy_file(SrcDir, TmpPath, SrcName) of
+                {error, Reason} ->
+                    {error, Reason};
+                ok ->
+                    TmpSrc = filename:join(TmpPath, SrcName),
+                    TmpDst = filename:join(TmpPath, DestName),
+                    case file:rename(TmpSrc, TmpDst) of
+                        {error, _} ->
+                            {error, lists:flatten(
+                                      io_lib:format("Failed to move ~s to ~s (via rename)~n",
+                                                    [Source, Dest]))};
+                        ok ->
+                            case robocopy_file(TmpPath, DestDir, DestName) of
+                                Err = {error, _} -> Err;
+                                OK -> rm_rf(TmpPath), OK
+                            end
+                    end
+            end
+    end.
+
+robocopy_file(SrcPath, DestPath, FileName) ->
+    Cmd = ?FMT("robocopy /move /e \"~s\" \"~s\" \"~s\"",
+               [rebar_utils:escape_double_quotes(SrcPath),
+                rebar_utils:escape_double_quotes(DestPath),
+                rebar_utils:escape_double_quotes(FileName)]),
+    Res = rebar_utils:sh(Cmd, [{use_stdout, false}, return_on_error]),
+    case win32_ok(Res) of
+        false ->
+            {error, lists:flatten(
+                        io_lib:format("Failed to move ~s to ~s~n",
+                                      [filename:join(SrcPath, FileName),
+                                       filename:join(DestPath, FileName)]))};
+        true ->
+            ok
+    end.
+
+robocopy_dir(Source, Dest) ->
+    Cmd = ?FMT("robocopy /move /e \"~s\" \"~s\"",
+               [rebar_utils:escape_double_quotes(Source),
+                rebar_utils:escape_double_quotes(Dest)]),
+    Res = rebar_utils:sh(Cmd,
+                         [{use_stdout, false}, return_on_error]),
+    case win32_ok(Res) of
+        true -> ok;
+        false ->
+            {error, lists:flatten(
+                      io_lib:format("Failed to move ~s to ~s~n",
+                                    [Source, Dest]))}
+    end.
+
 win32_ok({ok, _}) -> true;
 win32_ok({error, {Rc, _}}) when Rc<9; Rc=:=16 -> true;
 win32_ok(_) -> false.
 
+
 delete_each([]) ->
     ok;
 delete_each([File | Rest]) ->

test/rebar_file_utils_SUITE.erl

@@ -4,6 +4,8 @@
          groups/0,
          init_per_group/2,
          end_per_group/2,
+         init_per_testcase/2,
+         end_per_testcase/2,
          raw_tmpdir/1,
          empty_tmpdir/1,
          simple_tmpdir/1,
@@ -15,7 +17,13 @@
          canonical_path/1,
          resolve_link/1,
          split_dirname/1,
-         mv_warning_is_ignored/1]).
+         mv_warning_is_ignored/1,
+         mv_dir/1,
+         mv_file_same/1,
+         mv_file_diff/1,
+         mv_file_dir_same/1,
+         mv_file_dir_diff/1,
+         mv_no_clobber/1]).
 
 -include_lib("common_test/include/ct.hrl").
 -include_lib("eunit/include/eunit.hrl").
@@ -25,6 +33,7 @@
 all() ->
     [{group, tmpdir},
      {group, reset_dir},
+     {group, mv},
      path_from_ancestor,
      canonical_path,
      resolve_link,
@@ -33,7 +42,9 @@ all() ->
 
 groups() ->
     [{tmpdir, [], [raw_tmpdir, empty_tmpdir, simple_tmpdir, multi_tmpdir]},
-     {reset_dir, [], [reset_nonexistent_dir, reset_empty_dir, reset_dir]}].
+     {reset_dir, [], [reset_nonexistent_dir, reset_empty_dir, reset_dir]},
+     {mv, [], [mv_dir, mv_file_same, mv_file_diff,
+               mv_file_dir_same, mv_file_dir_diff, mv_no_clobber]}].
 
 init_per_group(reset_dir, Config) ->
     TmpDir = rebar_file_utils:system_tmpdir(["rebar_file_utils_SUITE", "resetable"]),
@@ -41,6 +52,20 @@ init_per_group(reset_dir, Config) ->
 init_per_group(_, Config) -> Config.
 end_per_group(_, Config) -> Config.
 
+init_per_testcase(Test, Config) ->
+    case os:type() of
+        {win32, _} ->
+            case lists:member(Test, [resolve_link, mv_warning_is_ignored]) of
+                true -> {skip, "broken in windows"};
+                false -> Config
+            end;
+        _ ->
+            Config
+    end.
+
+end_per_testcase(_Test, Config) ->
+    Config.
+
 raw_tmpdir(_Config) ->
     case rebar_file_utils:system_tmpdir() of
         "/tmp"  -> ok;
@@ -143,3 +168,164 @@ mv_warning_is_ignored(_Config) ->
     meck:expect(rebar_utils, sh, fun("mv ding dong", _) -> {ok, "Warning"}  end),
     ok = rebar_file_utils:mv("ding", "dong"),
     meck:unload(rebar_utils).
+
+%%% Ensure Windows & Unix operations to move files
+
+mv_dir(Config) ->
+    %% Move a directory to another one location
+    PrivDir = ?config(priv_dir, Config),
+    BaseDir = mk_base_dir(PrivDir, mv_dir),
+    SrcDir = filename:join(BaseDir, "src/"),
+    ec_file:mkdir_p(SrcDir),
+    ?assert(filelib:is_dir(SrcDir)),
+    %% empty dir movement
+    DstDir1 = filename:join(BaseDir, "dst1/"),
+    ?assertNot(filelib:is_dir(DstDir1)),
+    ?assertEqual(ok, rebar_file_utils:mv(SrcDir, DstDir1)),
+    ?assert(filelib:is_dir(DstDir1)),
+
+    %% move files from dir to empty dir
+    F1 = filename:join(SrcDir, "file1"),
+    F2 = filename:join(SrcDir, "subdir/file2"),
+    filelib:ensure_dir(F2),
+    file:write_file(F1, "hello"),
+    file:write_file(F2, "world"),
+    DstDir2 = filename:join(BaseDir, "dst2/"),
+    D2F1 = filename:join(DstDir2, "file1"),
+    D2F2 = filename:join(DstDir2, "subdir/file2"),
+    ?assertNot(filelib:is_dir(DstDir2)),
+    ?assertEqual(ok, rebar_file_utils:mv(SrcDir, DstDir2)),
+    ?assert(filelib:is_file(D2F1)),
+    ?assert(filelib:is_file(D2F2)),
+
+    %% move files from dir to existing dir moves it to
+    %% a subdir
+    filelib:ensure_dir(F2),
+    file:write_file(F1, "hello"),
+    file:write_file(F2, "world"),
+    DstDir3 = filename:join(BaseDir, "dst3/"),
+    D3F1 = filename:join(DstDir3, "src/file1"),
+    D3F2 = filename:join(DstDir3, "src/subdir/file2"),
+    ec_file:mkdir_p(DstDir3),
+    ?assert(filelib:is_dir(DstDir3)),
+    ?assertEqual(ok, rebar_file_utils:mv(SrcDir, DstDir3)),
+    ?assertNot(filelib:is_file(F1)),
+    ?assertNot(filelib:is_file(F2)),
+    ?assert(filelib:is_file(D3F1)),
+    ?assert(filelib:is_file(D3F2)),
+    ?assertNot(filelib:is_dir(SrcDir)),
+    ok.
+
+mv_file_same(Config) ->
+    %% Move a file from a directory to the other without renaming
+    PrivDir = ?config(priv_dir, Config),
+    BaseDir = mk_base_dir(PrivDir, mv_file_same),
+    SrcDir = filename:join(BaseDir, "src/"),
+    ec_file:mkdir_p(SrcDir),
+    ?assert(filelib:is_dir(SrcDir)),
+    F = filename:join(SrcDir, "file"),
+    file:write_file(F, "hello"),
+    DstDir = filename:join(BaseDir, "dst/"),
+    ec_file:mkdir_p(DstDir),
+    Dst = filename:join(DstDir, "file"),
+    ?assertEqual(ok, rebar_file_utils:mv(F, Dst)),
+    ?assert(filelib:is_file(Dst)),
+    ?assertNot(filelib:is_file(F)),
+    ok.
+
+mv_file_diff(Config) ->
+    %% Move a file from a directory to another one while renaming
+    %% into a pre-existing file
+    PrivDir = ?config(priv_dir, Config),
+    BaseDir = mk_base_dir(PrivDir, mv_file_diff),
+    SrcDir = filename:join(BaseDir, "src/"),
+    ec_file:mkdir_p(SrcDir),
+    ?assert(filelib:is_dir(SrcDir)),
+    F = filename:join(SrcDir, "file"),
+    file:write_file(F, "hello"),
+    DstDir = filename:join(BaseDir, "dst/"),
+    ec_file:mkdir_p(DstDir),
+    Dst = filename:join(DstDir, "file-rename"),
+    file:write_file(Dst, "not-the-right-content"),
+    ?assert(filelib:is_file(Dst)),
+    ?assertEqual(ok, rebar_file_utils:mv(F, Dst)),
+    ?assert(filelib:is_file(Dst)),
+    ?assertEqual({ok, <<"hello">>}, file:read_file(Dst)),
+    ?assertNot(filelib:is_file(F)),
+    ok.
+
+mv_file_dir_same(Config) ->
+    %% Move a file to a directory without renaming
+    PrivDir = ?config(priv_dir, Config),
+    BaseDir = mk_base_dir(PrivDir, mv_file_dir_same),
+    SrcDir = filename:join(BaseDir, "src/"),
+    ec_file:mkdir_p(SrcDir),
+    ?assert(filelib:is_dir(SrcDir)),
+    F = filename:join(SrcDir, "file"),
+    file:write_file(F, "hello"),
+    DstDir = filename:join(BaseDir, "dst/"),
+    ec_file:mkdir_p(DstDir),
+    Dst = filename:join(DstDir, "file"),
+    ?assert(filelib:is_dir(DstDir)),
+    ?assertEqual(ok, rebar_file_utils:mv(F, DstDir)),
+    ?assert(filelib:is_file(Dst)),
+    ?assertNot(filelib:is_file(F)),
+    ok.
+
+mv_file_dir_diff(Config) ->
+    %% Move a file to a directory while renaming
+    PrivDir = ?config(priv_dir, Config),
+    BaseDir = mk_base_dir(PrivDir, mv_file_dir_diff),
+    SrcDir = filename:join(BaseDir, "src/"),
+    ec_file:mkdir_p(SrcDir),
+    ?assert(filelib:is_dir(SrcDir)),
+    F = filename:join(SrcDir, "file"),
+    file:write_file(F, "hello"),
+    DstDir = filename:join(BaseDir, "dst/"),
+    ec_file:mkdir_p(DstDir),
+    Dst = filename:join(DstDir, "file-rename"),
+    ?assert(filelib:is_dir(DstDir)),
+    ?assertNot(filelib:is_file(Dst)),
+    ?assertEqual(ok, rebar_file_utils:mv(F, Dst)),
+    ?assert(filelib:is_file(Dst)),
+    ?assertNot(filelib:is_file(F)),
+    ok.
+
+mv_no_clobber(Config) ->
+    %% Moving a file while renaming does not clobber other files
+    PrivDir = ?config(priv_dir, Config),
+    BaseDir = mk_base_dir(PrivDir, mv_no_clobber),
+    SrcDir = filename:join(BaseDir, "src/"),
+    ec_file:mkdir_p(SrcDir),
+    ?assert(filelib:is_dir(SrcDir)),
+    F = filename:join(SrcDir, "file"),
+    file:write_file(F, "hello"),
+    FBad = filename:join(SrcDir, "file-alt"),
+    file:write_file(FBad, "wrong-data"),
+    DstDir = filename:join(BaseDir, "dst/"),
+    ec_file:mkdir_p(DstDir),
+    Dst = filename:join(DstDir, "file-alt"),
+    DstBad = filename:join(DstDir, "file"),
+    file:write_file(DstBad, "wrong-data"),
+    ?assert(filelib:is_file(F)),
+    ?assert(filelib:is_file(FBad)),
+    ?assert(filelib:is_dir(DstDir)),
+    ?assertNot(filelib:is_file(Dst)),
+    ?assert(filelib:is_file(DstBad)),
+    ?assertEqual(ok, rebar_file_utils:mv(F, Dst)),
+    ?assert(filelib:is_file(Dst)),
+    ?assertNot(filelib:is_file(F)),
+    ?assert(filelib:is_file(DstBad)),
+    ?assert(filelib:is_file(FBad)),
+    ?assertEqual({ok, <<"hello">>}, file:read_file(Dst)),
+    ?assertEqual({ok, <<"wrong-data">>}, file:read_file(FBad)),
+    ?assertEqual({ok, <<"wrong-data">>}, file:read_file(DstBad)),
+    ok.
+
+
+mk_base_dir(BasePath, Name) ->
+    {_,_,Micro} = os:timestamp(),
+    Index = integer_to_list(Micro),
+    Path = filename:join(BasePath, atom_to_list(Name) ++ Index),
+    ec_file:mkdir_p(Path),
+    Path.