Ignore symlinks. You can't move them, and we don't modify them.

Summary:
I don't know that this is strictly necessary: in practice what seems
to happen when you modify a file that's actually a symlink is that we
modify the underlying file (that is, `open(f, 'w')` opens the file
that the symlink points to, and leaves the actual link untouched).
And when we try to modify the file the second time, under its new
name, then all the codemods have already been applied via the symlink
so it's a noop, and all is good.

But I don't know if that's posix standard, and anyway it seems
confusing.  Now we notice symlinks and just skip over them.

Test Plan: make test

Reviewers: benkraft

Subscribers: davidflanagan, carter

Differential Revision: https://phabricator.khanacademy.org/D41041

Author: csilvers

inputs.py

@@ -30,15 +30,17 @@
 
 
 def _expand_and_normalize_one(project_root, old_fullname, new_fullname,
-                              path_filter=khodemod.default_path_filter()):
+                              path_filter=None):
     """See expand_and_normalize.__doc__."""
     def filename_for(mod):
         return os.path.join(project_root, util.filename_for_module_name(mod))
 
-    def _assert_exists(module, error_prefix):
-        if not os.path.exists(filename_for(module)):
-            raise ValueError("%s: %s not found"
-                             % (error_prefix, filename_for(module)))
+    def _assert_exists_as_file(module, error_prefix):
+        fname = filename_for(module)
+        if not os.path.exists(fname):
+            raise ValueError("%s: %s not found" % (error_prefix, fname))
+        if os.path.islink(fname):
+            raise ValueError("%s: %s is a symlink" % (error_prefix, fname))
 
     def _normalize_fullname_and_get_type(fullname):
         # Check the cases that fullname is a file or a directory.
@@ -70,7 +72,11 @@ def _normalize_fullname_and_get_type(fullname):
     def _modules_under(package_name):
         """Yield module-names relative to package_name-root."""
         package_dir = os.path.dirname(filename_for(package_name + '.__init__'))
-        for path in khodemod.resolve_paths(path_filter, root=package_dir):
+        if path_filter is None:
+            this_path_filter = khodemod.default_path_filter(package_dir)
+        else:
+            this_path_filter = path_filter
+        for path in khodemod.resolve_paths(this_path_filter, root=package_dir):
             yield util.module_name_for_filename(path)
 
     (old_fullname, old_type) = _normalize_fullname_and_get_type(old_fullname)
@@ -85,7 +91,7 @@ def _modules_under(package_name):
 
     if old_type == "symbol":
         (module, symbol) = old_fullname.rsplit('.', 1)
-        _assert_exists(module, "Cannot move %s" % old_fullname)
+        _assert_exists_as_file(module, "Cannot move %s" % old_fullname)
 
         # TODO(csilvers): check that the 2nd element of the return-value
         # doesn't refer to a symbol that already exists.
@@ -107,7 +113,7 @@ def _modules_under(package_name):
                 yield (old_fullname, '%s.%s' % (new_fullname, symbol), True)
 
     elif old_type == "module":
-        _assert_exists(old_fullname, "Cannot move %s" % old_fullname)
+        _assert_exists_as_file(old_fullname, "Cannot move %s" % old_fullname)
         if new_type == "symbol":
             raise ValueError("Cannot move a module '%s' to a symbol (%s)"
                              % (old_fullname, new_fullname))
@@ -129,8 +135,8 @@ def _modules_under(package_name):
             yield (old_fullname, new_fullname, False)
 
     elif old_type == "package":
-        _assert_exists(old_fullname + '.__init__',
-                       "Cannot move %s" % old_fullname)
+        _assert_exists_as_file(old_fullname + '.__init__',
+                               "Cannot move %s" % old_fullname)
         if new_type in ("symbol", "module"):
             raise ValueError("Cannot move a package '%s' into a %s (%s)"
                              % (old_fullname, new_type, new_fullname))
@@ -162,7 +168,7 @@ def _modules_under(package_name):
 
 
 def expand_and_normalize(project_root, old_fullnames, new_fullname,
-                         path_filter=khodemod.default_path_filter()):
+                         path_filter=None):
     """Return a list of old-new-info triples that effect the requested rename.
 
     In the simple case old_fullname is a module and new_fullname is a

khodemod.py

@@ -158,6 +158,10 @@ def dotfiles_path_filter():
                                 for part in os.path.split(path))
 
 
+def symlink_path_filter(root):
+    return lambda path: not os.path.islink(os.path.join(root, path))
+
+
 def exclude_paths_filter(exclude_paths):
     return lambda path: not any(part in exclude_paths
                                 for part in path.split(os.path.sep))
@@ -167,12 +171,14 @@ def and_filters(filters):
     return lambda item: all(f(item) for f in filters)
 
 
-def default_path_filter(extensions=DEFAULT_EXTENSIONS,
+def default_path_filter(root,
+                        extensions=DEFAULT_EXTENSIONS,
                         include_extensionless=False,
                         exclude_paths=DEFAULT_EXCLUDE_PATHS):
     return and_filters([
         extensions_path_filter(extensions, include_extensionless),
         dotfiles_path_filter(),
+        symlink_path_filter(root),
         exclude_paths_filter(exclude_paths),
     ])
 
@@ -377,9 +383,10 @@ def run_suggestor_on_files(self, suggestor, filenames, root='.'):
         for filename in self.progress_bar(filenames):
             self._run_suggestor_on_file(suggestor, filename, root)
 
-    def run_suggestor(self, suggestor,
-                      path_filter=default_path_filter(), root='.'):
+    def run_suggestor(self, suggestor, path_filter=None, root='.'):
         """Run the suggestor on all files matching the path_filter."""
+        if path_filter is None:
+            path_filter = default_path_filter(root)
         self.run_suggestor_on_files(
             suggestor, resolve_paths(path_filter, root), root)
 

test_inputs.py

@@ -1,5 +1,6 @@
 from __future__ import absolute_import
 
+import os
 import unittest
 
 import inputs
@@ -147,6 +148,26 @@ def test_symbol_to_existing_symbol(self):
                  "'bar' already defines a symbol named 'myfunc'.")
         self.assert_fails('foo.myfunc', 'bar', error)
 
+    def test_symlink_for_file(self):
+        self.write_file('bar.py', 'def myfunc(): return 4\n')
+        os.symlink('bar.py', self.join('sym.py'))
+        error = 'Cannot move sym: %s is a symlink' % self.join('sym.py')
+        self.assert_fails('sym', 'newdir', error)
+
+    def test_symlink_for_symbol(self):
+        self.write_file('bar.py', 'def myfunc(): return 4\n')
+        os.symlink('bar.py', self.join('sym.py'))
+        error = 'Cannot move sym.myfunc: %s is a symlink' % self.join('sym.py')
+        self.assert_fails('sym.myfunc', 'newdir', error)
+
+    def test_symlink_for_package(self):
+        self.write_file('dir_with_symlink/bar.py', 'def myfunc(): return 4\n')
+        self.write_file('dir_with_symlink/__init__.py', '')
+        os.symlink('bar.py', self.join('dir_with_symlink/sym.py'))
+        self._assert('dir_with_symlink', 'newdir',
+                     [('dir_with_symlink.bar', 'newdir.bar', False),
+                      ('dir_with_symlink.__init__', 'newdir.__init__', False)])
+
 
 class ManyInputsTest(test_slicker.TestBase):
     def setUp(self):

test_khodemod.py

@@ -18,20 +18,22 @@ def test_resolve_paths(self):
 
         self.assertItemsEqual(
             khodemod.resolve_paths(
-                khodemod.default_path_filter(),
+                khodemod.default_path_filter(self.tmpdir),
                 root=self.tmpdir),
             ['foo.py', 'bar/baz.py', 'build/qux.py'])
 
         self.assertItemsEqual(
             khodemod.resolve_paths(
                 khodemod.default_path_filter(
+                    self.tmpdir,
                     exclude_paths=('genfiles', 'build')),
                 root=self.tmpdir),
             ['foo.py', 'bar/baz.py'])
 
         self.assertItemsEqual(
             khodemod.resolve_paths(
                 khodemod.default_path_filter(
+                    self.tmpdir,
                     extensions=('js', 'css'), include_extensionless=True),
                 root=self.tmpdir),
             ['foo_extensionless_py', 'foo.js', 'foo.css'])

test_slicker.py

@@ -1121,6 +1121,16 @@ def test_repeated_name(self):
             'repeated_name',
             'foo.foo', 'bar.foo.foo')
 
+    def test_ignore_uses_in_symlink(self):
+        self.create_module('foo')
+        os.symlink('simple_in.py', self.join('sym.py'))
+        self.run_test(
+            'simple',
+            'foo.some_function', 'bar.new_name')
+        # We shouldn't have rewritten the symlink, so it should still be
+        # a symlink.
+        self.assertTrue(os.path.islink(self.join('sym.py')))
+
 
 class AliasTest(TestBase):
     def assert_(self, old_module, new_module, alias,