Fix and refactor conflict resolution on deploy/delete

Refactor repos.deploy_files and repos.delete_version
to make relationships between properties of deployed
files clearer and to avoid converting back and forth
between related properties.

Location of each deployable file in its rock manifest is
pair deploy_type - the first subtree name ("bin", "lib", or "lua")
and file_path - remaining path from the subtree to the file. These
components determine where each file is physically located.

Conflicts are considered based on two other properties: type
and name of an item a file provides. Type can be "command" or "module".
For items deployed using non-versioned names pairs (type, name) should be
unique.

Conversion from (deploy_type, file_path) to (item_type, item_name)
is obvious, using path.path_to_module() for modules. Reversing this
conversion is necessary for moving files between versioned and
non-versioned locations on conflicts, and also for path.which
function used in luarocks.show. However,
rock tree manifest only allows to get file_path, which
is not enough for modules - deploy_type can be both "lua" and "lib".

Currently path.which infers deploy_type based on extension,
falling back to "lib" if it's unknown, causing luarocks.show
to display wrong paths (#424). This commit does not address that
but adds relevant funcionality.

Currently conflict resolution assumes that both files in conflict
have same deploy_type and errors on conflict between
a C module and a Lua module. This commit fixes that, inferring
deploy_type for files with unknown extension using rock manifest.

2 files changed, 205 insertions, 183 deletions

diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
index 86209d06..88a13f10 100644
--- a/src/luarocks/manif.lua
+++ b/src/luarocks/manif.lua
@@ -540,85 +540,89 @@ function manif.zip_manifests()
    end
 end
 
-local function relative_path(from_dir, to_file)
-   -- It is assumed that `from_dir` is prefix of `to_file`.
-   return (to_file:sub(#from_dir + 1):gsub("^[\\/]*", ""))
+--- Get type and name of an item (a module or a command) provided by a file.
+-- @param deploy_type string: rock manifest subtree the file comes from ("bin", "lua", or "lib").
+-- @param file_path string: path to the file relatively to deploy_type subdirectory.
+-- @return (string, string): item type ("module" or "command") and name.
+function manif.get_provided_item(deploy_type, file_path)
+   assert(type(deploy_type) == "string")
+   assert(type(file_path) == "string")
+   local item_type = deploy_type == "bin" and "command" or "module"
+   local item_name = item_type == "command" and file_path or path.path_to_module(file_path)
+   return item_type, item_name
 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)
-
-   if util.starts_with(file, deploy_lua) then
-      return "modules", path.path_to_module(relative_path(deploy_lua, file):gsub("\\", "/")), deploy_lua
-   elseif util.starts_with(file, deploy_lib) then
-      return "modules", path.path_to_module(relative_path(deploy_lib, file):gsub("\\", "/")), deploy_lib
-   elseif util.starts_with(file, deploy_bin) then
-      return "commands", relative_path(deploy_bin, file), deploy_bin
-   else
-      assert(false, "Assertion failed: '"..file.."' is not a deployed file.")
-   end
+local function get_providers(item_type, item_name, repo)
+   assert(type(item_type) == "string")
+   assert(type(item_name) == "string")
+   local rocks_dir = path.rocks_dir(repo or cfg.root_dir)
+   local manifest = manif_core.load_local_manifest(rocks_dir)
+   return manifest and manifest[item_type .. "s"][item_name]
 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"
+--- Given a name of a module or a command, figure out which rock name and version
+-- correspond to it in the rock tree manifest.
+-- @param item_type string: "module" or "command".
+-- @param item_name string: module or command name.
+-- @param root string or nil: A local root dir for a rocks tree. If not given, the default is used.
+-- @return (string, string) or nil: name and version of the provider rock or nil if there
+-- is no provider.
+function manif.get_current_provider(item_type, item_name, repo)
+   local providers = get_providers(item_type, item_name, repo)
+   if providers then
+      return providers[1]:match("([^/]*)/([^/]*)")
    end
+end
 
-   local type_key, key = file_manifest_coordinates(manifest, file, root)
-
-   local providers = manifest[type_key][key]
-   if not providers then
-      return nil, "untracked"
+function manif.get_next_provider(item_type, item_name, repo)
+   local providers = get_providers(item_type, item_name, repo)
+   if providers and providers[2] then
+      return providers[2]:match("([^/]*)/([^/]*)")
    end
-   return providers
 end
 
---- Given a path of a deployed file, figure out which rock name and version
--- correspond to it in the tree manifest.
--- @param file string: The full path of a deployed file.
+--- Given a name of a module or a command provided by a package, figure out
+-- which file provides it.
+-- @param name string: package name.
+-- @param version string: package version.
+-- @param item_type string: "module" or "command".
+-- @param item_name string: module or command name.
 -- @param root string or nil: A local root dir for a rocks tree. If not given, the default is used.
--- @return string, string: name and version of the provider rock.
-function manif.find_current_provider(file, root)
-   local providers, err = find_providers(file, root)
-   if not providers then return nil, err end
-   return providers[1]:match("([^/]*)/([^/]*)")
-end
+-- @return (string, string): rock manifest subtree the file comes from ("bin", "lua", or "lib")
+-- and path to the providing file relatively to that subtree.
+function manif.get_providing_file(name, version, item_type, item_name, repo)
+   local rocks_dir = path.rocks_dir(repo or cfg.root_dir)
+   local manifest = manif_core.load_local_manifest(rocks_dir)
 
-function manif.find_next_provider(file, root)
-   local providers, err = find_providers(file, root)
-   if not providers then return nil, err end
-   if providers[2] then
-      return providers[2]:match("([^/]*)/([^/]*)")
-   else
-      return nil
+   local entry_table = manifest.repository[name][version][1]
+   local file_path = entry_table[item_type .. "s"][item_name]
+
+   if item_type == "command" then
+      return "bin", file_path
    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
+   -- A module can be in "lua" or "lib". Decide based on extension first:
+   -- most likely Lua modules are in "lua/" and C modules are in "lib/".
+   if file_path:match("%." .. cfg.lua_extension .. "$") then
+      return "lua", file_path
+   elseif file_path:match("%." .. cfg.lib_extension .. "$") then
+      return "lib", file_path
    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])
+   -- Fallback to rock manifest scanning.
+   local rock_manifest = manif.load_rock_manifest(name, version)
+   local subtree = rock_manifest.lib
+
+   for path_part in file_path:gmatch("[^/]+") do
+      if type(subtree) == "table" then
+         subtree = subtree[path_part]
+      else
+         -- Assume it's in "lua/" if it's not in "lib/".
+         return "lua", file_path
+      end
+   end
+
+   return type(subtree) == "string" and "lib" or "lua", file_path
 end
 
 return manif
diff --git a/src/luarocks/repos.lua b/src/luarocks/repos.lua
index d4d9694e..5d5eac70 100644
--- a/src/luarocks/repos.lua
+++ b/src/luarocks/repos.lua
@@ -11,6 +11,23 @@ local dir = require("luarocks.dir")
 local manif = require("luarocks.manif")
 local deps = require("luarocks.deps")
 
+-- Tree of files installed by a package are stored
+-- in its rock manifest. Some of these files have to
+-- be deployed to locations where Lua can load them as
+-- modules or where they can be used as commands.
+-- These files are characterised by pair
+-- (deploy_type, file_path), where deploy_type is the first
+-- component of the file path and file_path is the rest of the
+-- path. Only files with deploy_type in {"lua", "lib", "bin"}
+-- are deployed somewhere.
+-- Each deployed file provides an "item". An item is
+-- characterised by pair (item_type, item_name).
+-- item_type is "command" for files with deploy_type
+-- "bin" and "module" for deploy_type in {"lua", "lib"}.
+-- item_name is same as file_path for commands
+-- and is produced using path.path_to_module(file_path)
+-- for modules.
+
 --- Get all installed versions of a package.
 -- @param name string: a package name.
 -- @return table or nil: An array of strings listing installed
@@ -192,44 +209,56 @@ end
 local function delete_suffixed(file, suffix)
    local suffixed_file, err = find_suffixed(file, suffix)
    if not suffixed_file then
-      return nil, "Could not remove " .. file .. ": " .. err, "not found"
+      return nil, "Could not remove " .. file .. ": " .. err
    end
 
    fs.delete(suffixed_file)
    if fs.exists(suffixed_file) then
-      return nil, "Failed deleting " .. suffixed_file .. ": file still exists", "fail"
+      return nil, "Failed deleting " .. suffixed_file .. ": file still exists"
    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
+-- Files can be deployed using versioned and non-versioned names.
+-- Several items with same type and name can exist if they are
+-- provided by different packages or versions. In any case
+-- item from the newest version of lexicographically smallest package
+-- is deployed using non-versioned name and others use versioned names.
+
+local function get_deploy_paths(name, version, deploy_type, file_path)
+   local deploy_dir = cfg["deploy_" .. deploy_type .. "_dir"]
+   local non_versioned = dir.path(deploy_dir, file_path)
+   local versioned = path.versioned_name(non_versioned, deploy_dir, name, version)
+   return non_versioned, versioned
+end
+
+local function prepare_target(name, version, deploy_type, file_path, suffix)
+   local non_versioned, versioned = get_deploy_paths(name, version, deploy_type, file_path)
+   local item_type, item_name = manif.get_provided_item(deploy_type, file_path)
+   local cur_name, cur_version = manif.get_current_provider(item_type, item_name)
+
+   if not cur_name then
+      return non_versioned
+   elseif 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 cur_deploy_type, cur_file_path = manif.get_providing_file(cur_name, cur_version, item_type, item_name)
+      local cur_non_versioned, cur_versioned = get_deploy_paths(cur_name, cur_version, cur_deploy_type, cur_file_path)
 
-      local ok, err = fs.make_dir(dir.dir_name(versioned))
-      if not ok then
-         return nil, err
-      end
+      local dir_ok, dir_err = fs.make_dir(dir.dir_name(cur_versioned))
+      if not dir_ok then return nil, dir_err end
 
-      ok, err = move_suffixed(cur_target, versioned, suffix)
-      if not ok then
-         return nil, err
-      end
+      local move_ok, move_err = move_suffixed(cur_non_versioned, cur_versioned, suffix)
+      if not move_ok then return nil, move_err end
 
-      return target
+      return non_versioned
    else
       -- Current version has priority, deploy new version using versioned name.
-      return path.versioned_name(target, deploy_dir, name, version)
+      return versioned
    end
 end
 
 --- Deploy a package from the rocks subdirectory.
--- 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 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.
@@ -241,49 +270,44 @@ 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, 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 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
+   local rock_manifest = manif.load_rock_manifest(name, version)
 
-            local ok, err = fs.make_dir(dir.dir_name(target))
-            if not ok then return nil, err end
+   local function deploy_file_tree(deploy_type, source_dir, move_fn, suffix)
+      if not rock_manifest[deploy_type] then
+         return true
+      end
 
-            local suffixed_target, mover = move_fn(source, target, name, version)
-            if fs.exists(suffixed_target) then
-               local backup = suffixed_target
-               repeat
-                  backup = backup.."~"
-               until not fs.exists(backup) -- Slight race condition here, but shouldn't be a problem.
-
-               util.printerr("Warning: "..suffixed_target.." is not tracked by this installation of LuaRocks. Moving it to "..backup)
-               local ok, err = fs.move(suffixed_target, backup)
-               if not ok then
-                  return nil, err
-               end
-            end
+      return recurse_rock_manifest_tree(rock_manifest[deploy_type], function(parent_path, parent_module, file)
+         local file_path = parent_path .. file
+         local source = dir.path(source_dir, file_path)
 
-            ok, err = mover()
-            fs.remove_dir_tree_if_empty(dir.dir_name(source))
-            return ok, err
+         local target, prepare_err = prepare_target(name, version, deploy_type, file_path, suffix)
+         if not target then return nil, prepare_err end
+
+         local dir_ok, dir_err = fs.make_dir(dir.dir_name(target))
+         if not dir_ok then return nil, dir_err end
+
+         local suffixed_target, mover = move_fn(source, target)
+         if fs.exists(suffixed_target) then
+            local backup = suffixed_target
+            repeat
+               backup = backup.."~"
+            until not fs.exists(backup) -- Slight race condition here, but shouldn't be a problem.
+
+            util.printerr("Warning: "..suffixed_target.." is not tracked by this installation of LuaRocks. Moving it to "..backup)
+            local move_ok, move_err = fs.move(suffixed_target, backup)
+            if not move_ok then return nil, move_err end
          end
-      )
-   end
 
-   local rock_manifest = manif.load_rock_manifest(name, version)
+         local move_ok, move_err = mover()
+         if not move_ok then return nil, move_err end
+
+         fs.remove_dir_tree_if_empty(dir.dir_name(source))
+         return true
+      end)
+   end
 
-   local function install_binary(source, target, name, version)
+   local function install_binary(source, target)
       if wrap_bin_scripts and fs.is_lua(source) then
          return target .. (cfg.wrapper_suffix or ""), function() return fs.wrap_script(source, target, name, version) end
       else
@@ -297,28 +321,19 @@ function repos.deploy_files(name, version, wrap_bin_scripts, deps_mode)
       end
    end
 
-   local ok, err = true
-   if rock_manifest.bin then
-      ok, err = deploy_file_tree(rock_manifest.bin, path.bin_dir, cfg.deploy_bin_dir, install_binary, cfg.wrapper_suffix)
-   end
-   if ok and rock_manifest.lua then
-      ok, err = deploy_file_tree(rock_manifest.lua, path.lua_dir, cfg.deploy_lua_dir, make_mover(cfg.perm_read))
-   end
-   if ok and rock_manifest.lib then
-      ok, err = deploy_file_tree(rock_manifest.lib, path.lib_dir, cfg.deploy_lib_dir, make_mover(cfg.perm_exec))
-   end
+   local ok, err = deploy_file_tree("bin", path.bin_dir(name, version), install_binary, cfg.wrapper_suffix)
+   if not ok then return nil, err end
 
-   if not ok then
-      return nil, err
-   end
+   ok, err = deploy_file_tree("lua", path.lua_dir(name, version), make_mover(cfg.perm_read))
+   if not ok then return nil, err end
+
+   ok, err = deploy_file_tree("lib", path.lib_dir(name, version), make_mover(cfg.perm_exec))
+   if not ok then return nil, err end
 
    return manif.add_to_manifest(name, version, nil, deps_mode)
 end
 
 --- Delete a package from the local repository.
--- 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 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:
@@ -333,67 +348,70 @@ function repos.delete_version(name, version, deps_mode, quick)
    assert(type(version) == "string")
    assert(type(deps_mode) == "string")
 
-   local function delete_deployed_file_tree(file_tree, deploy_dir, suffix)
-      return recurse_rock_manifest_tree(file_tree, 
-         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, 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
+   local rock_manifest = manif.load_rock_manifest(name, version)
+   if not rock_manifest then
+      return nil, "rock_manifest file not found for "..name.." "..version.." - not a LuaRocks 2 tree?"
+   end
 
-            ok, err = delete_suffixed(target, suffix)
-            if not ok then
-               return nil, err
-            end
+   local function delete_deployed_file_tree(deploy_type, suffix)
+      if not rock_manifest[deploy_type] then
+         return true
+      end
+
+      return recurse_rock_manifest_tree(rock_manifest[deploy_type], function(parent_path, parent_module, file)
+         local file_path = parent_path .. file
+         local non_versioned, versioned = get_deploy_paths(name, version, deploy_type, file_path)
+
+         -- Figure out if the file is deployed using versioned or non-versioned name.
+         local target
+         local item_type, item_name = manif.get_provided_item(deploy_type, file_path)
+         local cur_name, cur_version = manif.get_current_provider(item_type, item_name)
 
-            if not quick then
-               local next_name, next_version = manif.find_next_provider(target)
-               if next_name then
-                  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)
+         if cur_name == name and cur_version == version then
+            -- This package has highest priority, should be in non-versioned location.
+            target = non_versioned
+         else
+            target = versioned
+         end
+
+         local ok, err = delete_suffixed(target, suffix)
+         if not ok then return nil, err end
+
+         if not quick and target == non_versioned then
+            -- If another package provides this file, move its version
+            -- into non-versioned location instead.
+            local next_name, next_version = manif.get_next_provider(item_type, item_name)
 
-                  ok, err = move_suffixed(next_versioned, next_target, suffix)
-                  if not ok then
-                     return nil, err
-                  end
+            if next_name then
+               local next_deploy_type, next_file_path = manif.get_providing_file(next_name, next_version, item_type, item_name)
+               local next_non_versioned, next_versioned = get_deploy_paths(next_name, next_version, next_deploy_type, next_file_path)
 
-                  fs.remove_dir_tree_if_empty(dir.dir_name(versioned))
-               end
+               local move_ok, move_err = move_suffixed(next_versioned, next_non_versioned, suffix)
+               if not move_ok then return nil, move_err end
+
+               fs.remove_dir_tree_if_empty(dir.dir_name(next_versioned))
             end
-            fs.remove_dir_tree_if_empty(dir.dir_name(target))
-            return true
          end
-      )
-   end
 
-   local rock_manifest = manif.load_rock_manifest(name, version)
-   if not rock_manifest then
-      return nil, "rock_manifest file not found for "..name.." "..version.." - not a LuaRocks 2 tree?"
-   end
-   
-   local ok, err = true
-   if rock_manifest.bin then
-      ok, err = delete_deployed_file_tree(rock_manifest.bin, cfg.deploy_bin_dir, cfg.wrapper_suffix)
-   end
-   if ok and rock_manifest.lua then
-      ok, err = delete_deployed_file_tree(rock_manifest.lua, cfg.deploy_lua_dir)
-   end
-   if ok and rock_manifest.lib then
-      ok, err = delete_deployed_file_tree(rock_manifest.lib, cfg.deploy_lib_dir)
+         fs.remove_dir_tree_if_empty(dir.dir_name(target))
+         return true
+      end)
    end
+
+   local ok, err = delete_deployed_file_tree("bin", cfg.wrapper_suffix)
+   if not ok then return nil, err end
+
+   ok, err = delete_deployed_file_tree("lua")
+   if not ok then return nil, err end
+
+   ok, err = delete_deployed_file_tree("lib")
    if not ok then return nil, err end
 
    fs.delete(path.install_dir(name, version))
    if not get_installed_versions(name) then
       fs.delete(dir.path(cfg.rocks_dir, name))
    end
-   
+
    if quick then
       return true
    end