Make failed uninstalls roll back more reliably and better at avoiding naming conflicts

news/6194.bugfix

@@ -0,0 +1 @@
+Make failed uninstalls roll back more reliably and better at avoiding naming conflicts.

src/pip/_internal/req/req_uninstall.py

@@ -15,7 +15,7 @@
 from pip._internal.utils.logging import indent_log
 from pip._internal.utils.misc import (
     FakeFile, ask, dist_in_usersite, dist_is_local, egg_link_path, is_local,
-    normalize_path, renames,
+    normalize_path, renames, rmtree,
 )
 from pip._internal.utils.temp_dir import AdjacentTempDirectory
 
@@ -241,7 +241,10 @@ def _stash(self, path):
             best = AdjacentTempDirectory(os.path.dirname(path))
             best.create()
             self._save_dirs.append(best)
-        return os.path.join(best.path, os.path.relpath(path, best.original))
+        relpath = os.path.relpath(path, best.original)
+        if not relpath or relpath == os.path.curdir:
+            return best.path
+        return os.path.join(best.path, relpath)
 
     def remove(self, auto_confirm=False, verbose=False):
         """Remove paths in ``self.paths`` with confirmation (unless
@@ -265,6 +268,13 @@ def remove(self, auto_confirm=False, verbose=False):
                     new_path = self._stash(path)
                     logger.debug('Removing file or directory %s', path)
                     self._moved_paths.append((path, new_path))
+                    if os.path.isdir(path) and os.path.isdir(new_path):
+                        # If we're moving a directory, we need to
+                        # remove the destination first or else it will be
+                        # moved to inside the existing directory.
+                        # We just created new_path ourselves, so it will
+                        # be removable.
+                        os.rmdir(new_path)
                     renames(path, new_path)
                 for pth in self.pth.values():
                     pth.remove()
@@ -311,9 +321,13 @@ def rollback(self):
         logger.info('Rolling back uninstall of %s', self.dist.project_name)
         for path, tmp_path in self._moved_paths:
             logger.debug('Replacing %s', path)
+            if os.path.isdir(tmp_path) and os.path.isdir(path):
+                rmtree(path)
             renames(tmp_path, path)
         for pth in self.pth.values():
             pth.rollback()
+        for save_dir in self._save_dirs:
+            save_dir.cleanup()
 
     def commit(self):
         """Remove temporary save dir: rollback will no longer be possible."""

src/pip/_internal/utils/temp_dir.py

@@ -98,7 +98,11 @@ class AdjacentTempDirectory(TempDirectory):
 
     """
     # The characters that may be used to name the temp directory
-    LEADING_CHARS = "-~.+=%0123456789"
+    # We always prepend a ~ and then rotate through these until
+    # a usable name is found.
+    # pkg_resources raises a different error for .dist-info folder
+    # with leading '-' and invalid metadata
+    LEADING_CHARS = "-~.=%0123456789"
 
     def __init__(self, original, delete=None):
         super(AdjacentTempDirectory, self).__init__(delete=delete)
@@ -117,8 +121,8 @@ def _generate_names(cls, name):
             if name[i] in cls.LEADING_CHARS:
                 continue
             for candidate in itertools.combinations_with_replacement(
-                    cls.LEADING_CHARS, i):
-                new_name = ''.join(candidate) + name[i:]
+                    cls.LEADING_CHARS, i - 1):
+                new_name = '~' + ''.join(candidate) + name[i:]
                 if new_name != name:
                     yield new_name
 

tests/unit/test_utils.py

@@ -573,8 +573,9 @@ def names():
         assert not any(n == name for n in names())
 
         # Check the first group are correct
-        assert all(x == y for x, y in
-                   zip(some_names, [c + name[1:] for c in chars]))
+        for x, y in zip(some_names, ['~' + name[1:]] +
+                ['~' + c + name[2:] for c in chars]):
+            assert x == y
 
 
 class TestGlibc(object):