Switch yapfignore to fnmatch

Switches the yapfignore implementation in git_cl to use fnmatch instead
of glob. This ends up bringing the .yapfignore file parsing in line with
yapf's actual implementation, namely allowing uses such as '*pb2.py'
instead of having to list out each directory manually.

Bug: 1027953
Change-Id: Ibb1cb4252c546de6f1b1af720c4c29ffd0f7be0c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1938026
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>

git_cl.py

@@ -14,7 +14,7 @@ from multiprocessing.pool import ThreadPool
 import base64
 import collections
 import datetime
-import glob
+import fnmatch
 import httplib2
 import itertools
 import json
@@ -764,8 +764,8 @@ def _FindYapfConfigFile(fpath, yapf_config_cache, top_dir=None):
   return ret
 
 
-def _GetYapfIgnoreFilepaths(top_dir):
-  """Returns all filepaths that match the ignored files in the .yapfignore file.
+def _GetYapfIgnorePatterns(top_dir):
+  """Returns all patterns in the .yapfignore file.
 
   yapf is supposed to handle the ignoring of files listed in .yapfignore itself,
   but this functionality appears to break when explicitly passing files to
@@ -778,28 +778,37 @@ def _GetYapfIgnoreFilepaths(top_dir):
     top_dir: The top level directory for the repository being formatted.
 
   Returns:
-    A set of all filepaths that should be ignored by yapf.
+    A set of all fnmatch patterns to be ignored.
   """
   yapfignore_file = os.path.join(top_dir, '.yapfignore')
-  ignore_filepaths = set()
+  ignore_patterns = set()
   if not os.path.exists(yapfignore_file):
-    return ignore_filepaths
+    return ignore_patterns
 
-  # glob works relative to the current working directory, so we need to ensure
-  # that we're at the top level directory.
-  old_cwd = os.getcwd()
-  try:
-    os.chdir(top_dir)
-    with open(yapfignore_file) as f:
-      for line in f.readlines():
-        stripped_line = line.strip()
-        # Comments and blank lines should be ignored.
-        if stripped_line.startswith('#') or stripped_line == '':
-          continue
-        ignore_filepaths |= set(glob.glob(stripped_line))
-    return ignore_filepaths
-  finally:
-    os.chdir(old_cwd)
+  with open(yapfignore_file) as f:
+    for line in f.readlines():
+      stripped_line = line.strip()
+      # Comments and blank lines should be ignored.
+      if stripped_line.startswith('#') or stripped_line == '':
+        continue
+      ignore_patterns.add(stripped_line)
+  return ignore_patterns
+
+
+def _FilterYapfIgnoredFiles(filepaths, patterns):
+  """Filters out any filepaths that match any of the given patterns.
+
+  Args:
+    filepaths: An iterable of strings containing filepaths to filter.
+    patterns: An iterable of strings containing fnmatch patterns to filter on.
+
+  Returns:
+    A list of strings containing all the elements of |filepaths| that did not
+    match any of the patterns in |patterns|.
+  """
+  # Not inlined so that tests can use the same implementation.
+  return [f for f in filepaths
+          if not any(fnmatch.fnmatch(f, p) for p in patterns)]
 
 
 def print_stats(args):
@@ -5240,11 +5249,11 @@ def CMDformat(parser, args):
     if not opts.full and filtered_py_files:
       py_line_diffs = _ComputeDiffLineRanges(filtered_py_files, upstream_commit)
 
-    ignored_yapf_files = _GetYapfIgnoreFilepaths(top_dir)
+    yapfignore_patterns = _GetYapfIgnorePatterns(top_dir)
+    filtered_py_files = _FilterYapfIgnoredFiles(filtered_py_files,
+                                                yapfignore_patterns)
 
     for f in filtered_py_files:
-      if f in ignored_yapf_files:
-        continue
       yapf_config = _FindYapfConfigFile(f, yapf_configs, top_dir)
       if yapf_config is None:
         yapf_config = chromium_default_yapf_style

tests/git_cl_test.py

@@ -3400,76 +3400,108 @@ class CMDFormatTestCase(TestCase):
     with open(os.path.join(self._top_dir, '.yapfignore'), 'w') as yapfignore:
       yapfignore.write('\n'.join(contents))
 
-  def _make_files(self, file_dict):
-    for directory, files in file_dict.items():
-      subdir = os.path.join(self._top_dir, directory)
-      if not os.path.exists(subdir):
-        os.makedirs(subdir)
-      for f in files:
-        with open(os.path.join(subdir, f), 'w'):
-          pass
-
-  def testYapfignoreBasic(self):
-    self._make_yapfignore(['test.py', '*/bar.py'])
-    self._make_files({
-        '.': ['test.py', 'bar.py'],
-        'foo': ['bar.py'],
-    })
-    self.assertEqual(
-        set(['test.py', 'foo/bar.py']),
-        git_cl._GetYapfIgnoreFilepaths(self._top_dir))
-
-  def testYapfignoreMissingYapfignore(self):
-    self.assertEqual(set(), git_cl._GetYapfIgnoreFilepaths(self._top_dir))
-
-  def testYapfignoreMissingFile(self):
-    self._make_yapfignore(['test.py', 'test2.py', 'test3.py'])
-    self._make_files({
-        '.': ['test.py', 'test3.py'],
-    })
-    self.assertEqual(
-        set(['test.py', 'test3.py']),
-        git_cl._GetYapfIgnoreFilepaths(self._top_dir))
+  def _check_yapf_filtering(self, files, expected):
+    self.assertEqual(expected, git_cl._FilterYapfIgnoredFiles(
+        files, git_cl._GetYapfIgnorePatterns(self._top_dir)))
+
+  def testYapfignoreExplicit(self):
+    self._make_yapfignore(['foo/bar.py', 'foo/bar/baz.py'])
+    files = [
+      'bar.py',
+      'foo/bar.py',
+      'foo/baz.py',
+      'foo/bar/baz.py',
+      'foo/bar/foobar.py',
+    ]
+    expected = [
+      'bar.py',
+      'foo/baz.py',
+      'foo/bar/foobar.py',
+    ]
+    self._check_yapf_filtering(files, expected)
+
+  def testYapfignoreSingleWildcards(self):
+    self._make_yapfignore(['*bar.py', 'foo*', 'baz*.py'])
+    files = [
+      'bar.py',       # Matched by *bar.py.
+      'bar.txt',
+      'foobar.py',    # Matched by *bar.py, foo*.
+      'foobar.txt',   # Matched by foo*.
+      'bazbar.py',    # Matched by *bar.py, baz*.py.
+      'bazbar.txt',
+      'foo/baz.txt',  # Matched by foo*.
+      'bar/bar.py',   # Matched by *bar.py.
+      'baz/foo.py',   # Matched by baz*.py, foo*.
+      'baz/foo.txt',
+    ]
+    expected = [
+      'bar.txt',
+      'bazbar.txt',
+      'baz/foo.txt',
+    ]
+    self._check_yapf_filtering(files, expected)
+
+  def testYapfignoreMultiplewildcards(self):
+    self._make_yapfignore(['*bar*', '*foo*baz.txt'])
+    files = [
+      'bar.py',       # Matched by *bar*.
+      'bar.txt',      # Matched by *bar*.
+      'abar.py',      # Matched by *bar*.
+      'foobaz.txt',   # Matched by *foo*baz.txt.
+      'foobaz.py',
+      'afoobaz.txt',  # Matched by *foo*baz.txt.
+    ]
+    expected = [
+      'foobaz.py',
+    ]
+    self._check_yapf_filtering(files, expected)
 
   def testYapfignoreComments(self):
     self._make_yapfignore(['test.py', '#test2.py'])
-    self._make_files({
-        '.': ['test.py', 'test2.py'],
-    })
-    self.assertEqual(
-        set(['test.py']), git_cl._GetYapfIgnoreFilepaths(self._top_dir))
+    files = [
+      'test.py',
+      'test2.py',
+    ]
+    expected = [
+      'test2.py',
+    ]
+    self._check_yapf_filtering(files, expected)
 
   def testYapfignoreBlankLines(self):
     self._make_yapfignore(['test.py', '', '', 'test2.py'])
-    self._make_files({'.': ['test.py', 'test2.py']})
-    self.assertEqual(
-        set(['test.py', 'test2.py']),
-        git_cl._GetYapfIgnoreFilepaths(self._top_dir))
+    files = [
+      'test.py',
+      'test2.py',
+      'test3.py',
+    ]
+    expected = [
+      'test3.py',
+    ]
+    self._check_yapf_filtering(files, expected)
 
   def testYapfignoreWhitespace(self):
     self._make_yapfignore([' test.py '])
-    self._make_files({'.': ['test.py']})
-    self.assertEqual(
-        set(['test.py']), git_cl._GetYapfIgnoreFilepaths(self._top_dir))
-
-  def testYapfignoreMultiWildcard(self):
-    self._make_yapfignore(['*es*.py'])
-    self._make_files({
-        '.': ['test.py', 'test2.py'],
-    })
-    self.assertEqual(
-        set(['test.py', 'test2.py']),
-        git_cl._GetYapfIgnoreFilepaths(self._top_dir))
+    files = [
+      'test.py',
+      'test2.py',
+    ]
+    expected = [
+      'test2.py',
+    ]
+    self._check_yapf_filtering(files, expected)
 
-  def testYapfignoreRestoresDirectory(self):
+  def testYapfignoreNoFiles(self):
     self._make_yapfignore(['test.py'])
-    self._make_files({
-        '.': ['test.py'],
-    })
-    old_cwd = os.getcwd()
-    self.assertEqual(
-        set(['test.py']), git_cl._GetYapfIgnoreFilepaths(self._top_dir))
-    self.assertEqual(old_cwd, os.getcwd())
+    self._check_yapf_filtering([], [])
+
+  def testYapfignoreMissingYapfignore(self):
+    files = [
+      'test.py',
+    ]
+    expected = [
+      'test.py',
+    ]
+    self._check_yapf_filtering(files, expected)
 
 
 if __name__ == '__main__':