From 7a0b07a8a5431d8288ebee57dbba2a5d3019f8d7 Mon Sep 17 00:00:00 2001 From: Bruce Dawson Date: Thu, 23 Apr 2020 17:14:40 +0000 Subject: [PATCH] Fix slash direction sensitivity in git cl owners Slashes and backslashes are interchangeable on Windows in many contexts. But not all. In particular, if you add a path to a dictionary and then try looking it up with an equivalent path that has different slashes then - not surprisingly - the lookup will fail. Therefore it is important to always use slashes when populating _paths_to_owners. This is now done, and documented. Additionally, while fnmatch.fnmatch handles comparing slash separated paths to backslash separated paths, fnmatch.translate does not. This is arguably a bug in fnmatch.translate, or at least something that should be documented. This bug is worked around by having _fnmatch sanitize file names to always use slashes, and asserting that the patterns do not contain backslashes. With these changes this command: git cl owners --ignore-current --show-all chrome/browser/BUILD.gn now correctly gives the same results as this command on Windows: git cl owners --ignore-current --show-all chrome\browser\BUILD.gn The modified test fails without the other changes, passes with them. Bug: 1009104 Change-Id: I416c7131281f00e352c1d2b85c30fcc463417fa5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1915410 Commit-Queue: Bruce Dawson Reviewed-by: Edward Lesmes --- owners.py | 14 ++++++++++++++ presubmit_support.py | 3 ++- tests/owners_finder_test.py | 11 +++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) 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):