diff --git a/owners.py b/owners.py index b001d3a649..498147d0df 100644 --- a/owners.py +++ b/owners.py @@ -59,6 +59,7 @@ Examples for all of these combinations can be found in tests/owners_unittest.py. import collections import fnmatch +import os import random import re @@ -133,6 +134,7 @@ class Database(object): # Mappings of directories -> globs in the directory -> owners # Example: "chrome/browser" -> "chrome/browser/*.h" -> ("john", "maria") + # Paths used as keys must use slashes as the separator, even on Windows. self._paths_to_owners = {} # Mapping reviewers to the preceding comment per file in the OWNERS files. @@ -234,6 +236,8 @@ class Database(object): self._read_global_comments() visited_dirs = set() for f in files: + # Always use slashes as separators. + f = f.replace(os.sep, '/') dirpath = self.os_path.dirname(f) while dirpath not in visited_dirs: visited_dirs.add(dirpath) @@ -413,6 +417,9 @@ class Database(object): 'cannot parse status entry: "%s"' % line.strip()) def _add_entry(self, owned_paths, directive, owners_path, lineno, comment): + # Consistently uses paths with slashes as the keys or else Windows will + # break in surprising and untested ways. + owned_paths = owned_paths.replace(os.sep, '/') if directive == 'set noparent': self._stop_looking.setdefault( self._get_root_affected_dir(owned_paths), set()).add(owned_paths) @@ -567,6 +574,8 @@ class Database(object): all_possible_owners_for_dir_or_file_cache = {} all_possible_owners = {} for current_dir in dirs_and_files: + # Always use slashes as separators. + current_dir = current_dir.replace(os.sep, '/') dir_owners = self._all_possible_owners_for_dir_or_file( current_dir, author, all_possible_owners_for_dir_or_file_cache) @@ -580,6 +589,11 @@ class Database(object): def _fnmatch(self, filename, pattern): """Same as fnmatch.fnmatch(), but internally caches the compiled regexes.""" + # Make sure backslashes are never used in the filename. The regex + # expressions generated by fnmatch.translate don't handle matching slashes + # to backslashes. + filename = filename.replace(os.sep, '/') + assert pattern.count('\\') == 0, 'Backslashes found in %s' % pattern matcher = self._fnmatch_cache.get(pattern) if matcher is None: matcher = re.compile(fnmatch.translate(pattern)).match diff --git a/presubmit_support.py b/presubmit_support.py index 197d9ce469..ffcfe9f3fd 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -634,7 +634,8 @@ class InputApi(object): def AffectedFiles(self, include_deletes=True, file_filter=None): """Same as input_api.change.AffectedFiles() except only lists files (and optionally directories) in the same directory as the current presubmit - script, or subdirectories thereof. + script, or subdirectories thereof. Note that files are listed using the OS + path separator, so backslashes are used as separators on Windows. """ dir_with_slash = normpath('%s/' % self.PresubmitLocalPath()) if len(dir_with_slash) == 1: diff --git a/tests/owners_finder_test.py b/tests/owners_finder_test.py index 126cc3a2e4..7d9b952376 100755 --- a/tests/owners_finder_test.py +++ b/tests/owners_finder_test.py @@ -150,6 +150,17 @@ class OwnersFinderTests(_BaseTestCase): finder = self.ownersFinder(files, author=brett) self.assertEqual(finder.unreviewed_files, {'content/bar/foo.cc'}) + def test_native_path_sep(self): + # Create a path with backslashes on Windows to make sure these are handled. + # This test is a harmless duplicate on other platforms. + native_slashes_path = 'chrome/browser/defaults.h'.replace('/', os.sep) + files = [ + native_slashes_path, # owned by brett + 'content/bar/foo.cc', # not owned by brett + ] + finder = self.ownersFinder(files, reviewers=[brett]) + self.assertEqual(finder.unreviewed_files, {'content/bar/foo.cc'}) + def test_reset(self): finder = self.defaultFinder() for _ in range(2):