Fix conflict resolution on deploy/delete

When deploying or deleting files, resolve conflicts purely
based on module names and command names, not file names.
Also, don't assume that in case of a conflict both packages have the
same file providing the module or command; it can be false due to binary
wrappers and `path_to_module("mod/init.lua")` == `path_to_module("mod.lua").

2 files changed, 146 insertions, 78 deletions

diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
index a8bbf279..1b75452b 100644
--- a/src/luarocks/manif.lua
+++ b/src/luarocks/manif.lua
@@ -469,33 +469,34 @@ local function relative_path(from_dir, to_file)
    return (to_file:sub(#from_dir + 1):gsub("^[\\/]*", ""))
 end
 
-local function find_providers(file, root)
-   assert(type(file) == "string")
-   root = root or cfg.root_dir
-
-   local manifest, err = manif_core.load_local_manifest(path.rocks_dir(root))
-   if not manifest then
-      return nil, "untracked"
-   end
+local function file_manifest_coordinates(manifest, file, root)
    local deploy_bin = path.deploy_bin_dir(root)
    local deploy_lua = path.deploy_lua_dir(root)
    local deploy_lib = path.deploy_lib_dir(root)
-   local key, manifest_tbl
 
    if util.starts_with(file, deploy_lua) then
-      manifest_tbl = manifest.modules
-      key = path.path_to_module(relative_path(deploy_lua, file):gsub("\\", "/"))
+      return "modules", path.path_to_module(relative_path(deploy_lua, file):gsub("\\", "/")), deploy_lua
    elseif util.starts_with(file, deploy_lib) then
-      manifest_tbl = manifest.modules
-      key = path.path_to_module(relative_path(deploy_lib, file):gsub("\\", "/"))
+      return "modules", path.path_to_module(relative_path(deploy_lib, file):gsub("\\", "/")), deploy_lib
    elseif util.starts_with(file, deploy_bin) then
-      manifest_tbl = manifest.commands
-      key = relative_path(deploy_bin, file)
+      return "commands", relative_path(deploy_bin, file), deploy_bin
    else
       assert(false, "Assertion failed: '"..file.."' is not a deployed file.")
    end
+end
+
+local function find_providers(file, root)
+   assert(type(file) == "string")
+   root = root or cfg.root_dir
+
+   local manifest, err = manif_core.load_local_manifest(path.rocks_dir(root))
+   if not manifest then
+      return nil, "untracked"
+   end
+
+   local type_key, key = file_manifest_coordinates(manifest, file, root)
 
-   local providers = manifest_tbl[key]
+   local providers = manifest[type_key][key]
    if not providers then
       return nil, "untracked"
    end
@@ -523,4 +524,25 @@ function manif.find_next_provider(file, root)
    end
 end
 
+--- Given a file conflicting with a module or command
+-- provided by a version of a package, return which file
+-- in that version corresponds to the conflicting item.
+-- @param name string: name of the package with conflicting module or command.
+-- @param version string: version of the package with conflicting module or command.
+-- @param file string: full, unversioned path to a deployed file.
+-- @return string: full, unversioned path to a deployed file in
+-- given package that conflicts with given file.
+function manif.find_conflicting_file(name, version, file, root)
+   root = root or cfg.root_dir
+
+   local manifest = manif_core.load_local_manifest(path.rocks_dir(root))
+   if not manifest then
+      return
+   end
+
+   local entry_table = manifest.repository[name][version][1]
+   local type_key, key, deploy_dir = file_manifest_coordinates(manifest, file, root)
+   return dir.path(deploy_dir, entry_table[type_key][key])
+end
+
 return manif
diff --git a/src/luarocks/repos.lua b/src/luarocks/repos.lua
index a1cf2bb4..762192a3 100644
--- a/src/luarocks/repos.lua
+++ b/src/luarocks/repos.lua
@@ -162,22 +162,6 @@ local function install_binary(source, target, name, version)
    end
 end
 
-local function resolve_conflict(target, deploy_dir, name, version)
-   local cname, cversion = manif.find_current_provider(target)
-   if not cname then
-      return nil, cversion
-   end
-   if name ~= cname or deps.compare_versions(version, cversion) then
-      local versioned = path.versioned_name(target, deploy_dir, cname, cversion)
-      local ok, err = fs.make_dir(dir.dir_name(versioned))
-      if not ok then return nil, err end
-      fs.move(target, versioned)
-      return target
-   else
-      return path.versioned_name(target, deploy_dir, name, version)
-   end
-end
-
 function repos.should_wrap_bin_scripts(rockspec)
    assert(type(rockspec) == "table")
 
@@ -190,11 +174,71 @@ function repos.should_wrap_bin_scripts(rockspec)
    return true
 end
 
+local function find_suffixed(file, suffix)
+   local filenames = {file}
+   if suffix and suffix ~= "" then
+      table.insert(filenames, 1, file .. suffix)
+   end
+
+   for _, filename in ipairs(filenames) do
+      if fs.exists(filename) then
+         return filename
+      end
+   end
+end
+
+local function move_suffixed(from_file, to_file, suffix)
+   local suffixed_from_file = find_suffixed(from_file, suffix)
+   if not suffixed_from_file then
+      return nil, "File not found"
+   end
+
+   suffix = suffixed_from_file:sub(#from_file + 1)
+   local suffixed_to_file = to_file .. suffix
+   return fs.move(suffixed_from_file, suffixed_to_file)
+end
+
+local function delete_suffixed(file, suffix)
+   local suffixed_file = find_suffixed(file, suffix)
+   if not suffixed_file then
+      return nil, "File not found", "not found"
+   end
+
+   fs.delete(suffixed_file)
+   if fs.exists(suffixed_file) then
+      return nil, "Failed deleting " .. suffixed_file, "fail"
+   end
+
+   return true
+end
+
+local function resolve_conflict(target, deploy_dir, name, version, cur_name, cur_version, suffix)
+   if name < cur_name or (name == cur_name and deps.compare_versions(version, cur_version)) then
+      -- New version has priority. Move currently provided version back using versioned name.
+      local cur_target = manif.find_conflicting_file(cur_name, cur_version, target)
+      local versioned = path.versioned_name(cur_target, deploy_dir, cur_name, cur_version)
+
+      local ok, err = fs.make_dir(dir.dir_name(versioned))
+      if not ok then
+         return nil, err
+      end
+
+      ok, err = move_suffixed(cur_target, versioned, suffix)
+      if not ok then
+         return nil, err
+      end
+
+      return target
+   else
+      -- Current version has priority, deploy new version using versioned name.
+      return path.versioned_name(target, deploy_dir, name, version)
+   end
+end
+
 --- Deploy a package from the rocks subdirectory.
--- It is maintained that for each file the one that is provided
+-- It is maintained that for each module and command the one that is provided
 -- by the newest version of the lexicographically smallest package
--- is installed using unversioned name, and other versions of the file
--- use versioned names.
+-- is installed using unversioned name, and other versions use versioned names.
 -- @param name string: name of package
 -- @param version string: exact package version in string format
 -- @param wrap_bin_scripts bool: whether commands written in Lua should be wrapped.
@@ -206,34 +250,40 @@ function repos.deploy_files(name, version, wrap_bin_scripts, deps_mode)
    assert(type(version) == "string")
    assert(type(wrap_bin_scripts) == "boolean")
 
-   local function deploy_file_tree(file_tree, path_fn, deploy_dir, move_fn)
+   local function deploy_file_tree(file_tree, path_fn, deploy_dir, move_fn, suffix)
       local source_dir = path_fn(name, version)
       return recurse_rock_manifest_tree(file_tree, 
          function(parent_path, parent_module, file)
             local source = dir.path(source_dir, parent_path, file)
             local target = dir.path(deploy_dir, parent_path, file)
-            local ok, err
+
+            local cur_name, cur_version = manif.find_current_provider(target)
+            if cur_name then
+               local resolve_err
+               target, resolve_err = resolve_conflict(target, deploy_dir, name, version, cur_name, cur_version, suffix)
+               if not target then
+                  return nil, resolve_err
+               end
+            end
+
             if fs.exists(target) then
-               local new_target, err = resolve_conflict(target, deploy_dir, name, version)
-               if err == "untracked" then
-                  local backup = target
-                  repeat
-                     backup = backup.."~"
-                  until not fs.exists(backup) -- slight race condition here, but shouldn't be a problem.
-                  util.printerr("Warning: "..target.." is not tracked by this installation of LuaRocks. Moving it to "..backup)
-                  fs.move(target, backup)
-               elseif err then
-                  return nil, err.." Cannot install new version."
-               else
-                  target = new_target
+               local backup = target
+               repeat
+                  backup = backup.."~"
+               until not fs.exists(backup) -- slight race condition here, but shouldn't be a problem.
+
+               util.printerr("Warning: "..target.." is not tracked by this installation of LuaRocks. Moving it to "..backup)
+               local ok, err = fs.move(target, backup)
+               if not ok then
+                  return nil, err
                end
             end
-            ok, err = fs.make_dir(dir.dir_name(target))
+
+            local ok, err = fs.make_dir(dir.dir_name(target))
             if not ok then return nil, err end
             ok, err = move_fn(source, target, name, version)
             fs.remove_dir_tree_if_empty(dir.dir_name(source))
-            if not ok then return nil, err end
-            return true
+            return ok, err
          end
       )
    end
@@ -243,7 +293,7 @@ function repos.deploy_files(name, version, wrap_bin_scripts, deps_mode)
    local ok, err = true
    if rock_manifest.bin then
       local move_bin_fn = wrap_bin_scripts and install_binary or fs.copy_binary
-      ok, err = deploy_file_tree(rock_manifest.bin, path.bin_dir, cfg.deploy_bin_dir, move_bin_fn)
+      ok, err = deploy_file_tree(rock_manifest.bin, path.bin_dir, cfg.deploy_bin_dir, move_bin_fn, cfg.wrapper_suffix)
    end
    local function make_mover(perms)
       return function (src, dest) 
@@ -264,26 +314,10 @@ function repos.deploy_files(name, version, wrap_bin_scripts, deps_mode)
    return manif.update_manifest(name, version, nil, deps_mode)
 end
 
-local function delete_suffixed(filename, suffix)
-   local filenames = { filename }
-   if suffix and suffix ~= "" then filenames = { filename..suffix, filename } end
-   for _, name in ipairs(filenames) do
-      if fs.exists(name) then
-         fs.delete(name)
-         if fs.exists(name) then
-            return nil, "Failed deleting "..name, "fail"
-         end
-         return true, name
-      end
-   end
-   return false, "File not found", "not found"
-end
-
 --- Delete a package from the local repository.
--- It is maintained that for each file the one that is provided
+-- It is maintained that for each module and command the one that is provided
 -- by the newest version of the lexicographically smallest package
--- is installed using unversioned name, and other versions of the file
--- use versioned names.
+-- is installed using unversioned name, and other versions use versioned names.
 -- @param name string: name of package
 -- @param version string: exact package version in string format
 -- @param deps_mode: string: Which trees to check dependencies for:
@@ -303,19 +337,31 @@ function repos.delete_version(name, version, deps_mode, quick)
          function(parent_path, parent_module, file)
             local target = dir.path(deploy_dir, parent_path, file)
             local versioned = path.versioned_name(target, deploy_dir, name, version)
-            local ok, name, err = delete_suffixed(versioned, suffix)
+
+            local ok, err, err_type = delete_suffixed(versioned, suffix)
             if ok then
                fs.remove_dir_tree_if_empty(dir.dir_name(versioned))
                return true
+            elseif err_type == "fail" then
+               return nil, err
+            end
+
+            ok, err = delete_suffixed(target, suffix)
+            if not ok then
+               return nil, err
             end
-            if err == "fail" then return nil, name end
-            ok, name, err = delete_suffixed(target, suffix)
-            if err == "fail" then return nil, name end
+
             if not quick then
                local next_name, next_version = manif.find_next_provider(target)
                if next_name then
-                  local versioned = path.versioned_name(name, deploy_dir, next_name, next_version)
-                  fs.move(versioned, name)
+                  local next_target = manif.find_conflicting_file(next_name, next_version, target)
+                  local next_versioned = path.versioned_name(next_target, deploy_dir, next_name, next_version)
+
+                  ok, err = move_suffixed(next_versioned, next_target, suffix)
+                  if not ok then
+                     return nil, err
+                  end
+
                   fs.remove_dir_tree_if_empty(dir.dir_name(versioned))
                end
             end
@@ -340,7 +386,7 @@ function repos.delete_version(name, version, deps_mode, quick)
    if ok and rock_manifest.lib then
       ok, err = delete_deployed_file_tree(rock_manifest.lib, cfg.deploy_lib_dir)
    end
-   if err then return nil, err end
+   if not ok then return nil, err end
 
    fs.delete(path.install_dir(name, version))
    if not get_installed_versions(name) then