Match the longest dirs entries.

Author: ehuss

highfive/newpr.py

@@ -274,44 +274,47 @@ def choose_reviewer(self, repo, owner, diff, exclude):
         dirs = self.repo_config.get('dirs', {})
         groups = self.get_groups()
 
-        most_changed = None
+        # Map of `dirs` path to the number of changes found in that path.
+        counts = {}
         # If there's directories with specially assigned groups/users
         # inspect the diff to find the directory with the most additions
         if dirs:
-            counts = {}
-            cur_dir = None
+            # List of the longest `dirs` paths that match the current path.
+            # This is a list to handle the situation if multiple paths of the
+            # same length match.
+            longest_dir_paths = []
             for line in diff.split('\n'):
                 if line.startswith("diff --git "):
-                    # update cur_dir
-                    cur_dir = None
+                    # update longest_dir_paths
+                    longest_dir_paths = []
                     parts = line[line.find(" b/") + len(" b/"):].split("/")
                     if not parts:
                         continue
-                    cur_dir = "/".join(parts[:2])
-
-                    # A few heuristics to get better reviewers
-                    if cur_dir.startswith('compiler/'):
-                        cur_dir = 'compiler'
-                    if cur_dir == 'src/test':
-                        cur_dir = None
-                    if cur_dir and cur_dir not in counts:
-                        counts[cur_dir] = 0
-                    continue
 
-                if cur_dir and (not line.startswith('+++')) and line.startswith('+'):
-                    counts[cur_dir] += 1
+                    # Find the longest `dirs` entries that match this path.
+                    longest = {}
+                    for dir_path in dirs:
+                        dir_parts = dir_path.split('/')
+                        if parts[:len(dir_parts)] == dir_parts:
+                            longest[dir_path] = len(dir_parts)
+                    max_count = max(longest.values(), default=0)
+                    longest_dir_paths = [
+                        path for (path, count) in longest.items()
+                            if count == max_count
+                    ]
+                    continue
 
-            # Find the largest count.
-            most_changes = 0
-            for directory, changes in counts.items():
-                if changes > most_changes:
-                    most_changes = changes
-                    most_changed = directory
+                if (not line.startswith('+++')) and line.startswith('+'):
+                    for path in longest_dir_paths:
+                        counts[path] = counts.get(path, 0) + 1
 
-        # lookup that directory in the json file to find the potential reviewers
+        # `all` is always included.
         potential = groups['all']
-        if most_changed and most_changed in dirs:
-            potential.extend(dirs[most_changed])
+        # Include the `dirs` entries with the maximum number of matches.
+        max_count = max(counts.values(), default=0)
+        max_paths = [path for (path, count) in counts.items() if count == max_count]
+        for path in max_paths:
+            potential.extend(dirs[path])
         if not potential:
             potential = groups['core']
 

highfive/tests/fakes.py

@@ -87,7 +87,19 @@ def get_repo_configs():
         },
         'teams': {
             "groups": {"all": ["@ehuss"], "a": ["@pnkfelix"], "d": ["@e"], "compiler-team": ["@niko"], "b/c": ["@nrc"]}
-        }
+        },
+        'prefixed-dirs': {
+            "groups": {
+                "all": [],
+                "compiler": ["@compiler"],
+            },
+            "dirs": {
+                "compiler": ["compiler"],
+                "compiler/rustc_llvm": ["@llvm"],
+                "compiler/rustc_parse": ["@parser"],
+                "compiler/rustc_parse/src/parse/lexer": ["@lexer"],
+            }
+        },
     }
 
 
@@ -127,3 +139,28 @@ def new_pr(
         p['pull_request']['user']['login'] = pr_author
 
         return payload.Payload(p)
+
+
+def make_fake_diff(paths):
+    """Generates a fake diff that touches the given files.
+
+    :param paths: A sequence of `(path, added, removed)` tuples where `added`
+        is the number of lines added, and `removed` is the number of lines
+        removed.
+    :returns: A string of the fake diff.
+    """
+    # This isn't a properly structured diff, but it has approximately enough
+    # information for what highfive looks at.
+    result = []
+    for (path, added, removed) in paths:
+        result.append(f'diff --git a/{path} b/{path}')
+        result.append('index 1677422122e..1108c1f4d4c 100644')
+        result.append(f'--- a/{path}')
+        result.append(f'+++ b/{path}')
+        result.append('@@ -0,0 +1 @@')
+        for n in range(added):
+            result.append(f'+Added line {n}')
+        for n in range(removed):
+            result.append(f'-Removed line {n}')
+    result.append('')
+    return '\n'.join(result)

highfive/tests/test_newpr.py

@@ -1310,6 +1310,46 @@ def test_with_team_ping(self):
             assert handler.find_reviewer(msg, None) is None, \
                 "expected '%s' to have no reviewer extracted" % msg
 
+    def test_prefixed_dirs(self):
+        """Test dirs with multiple overlapping prefixes."""
+        self.handler = HighfiveHandlerMock(
+            Payload({}), repo_config=self.fakes['config']['prefixed-dirs']
+        ).handler
+        # Base compiler rule should catch anything in compiler/
+        diff = fakes.make_fake_diff([('compiler/foo', 1, 1)])
+        (chosen_reviewers, _) = self.choose_reviewers(diff, 'ehuss')
+        assert set(['compiler']) == chosen_reviewers
+
+        # Anything in rustc_llvm should go to llvm.
+        diff = fakes.make_fake_diff([('compiler/rustc_llvm/foo', 1, 1)])
+        (chosen_reviewers, _) = self.choose_reviewers(diff, 'ehuss')
+        assert set(['llvm']) == chosen_reviewers
+
+        # 1 change in rustc_llvm, multiple changes in other directories, the
+        # other directories win because they have more changes.
+        diff = fakes.make_fake_diff([
+            ('compiler/rustc_llvm/foo', 1, 1),
+            ('compiler/rustc_traits/src/foo.rs', 0, 1),
+            ('compiler/rustc_macros//src/foo.rs', 2, 3),
+        ])
+        (chosen_reviewers, _) = self.choose_reviewers(diff, 'ehuss')
+        assert set(['compiler']) == chosen_reviewers
+
+        # Change in a deeply nested directory should win over its parent.
+        diff = fakes.make_fake_diff([
+            ('compiler/rustc_parse/src/parse/lexer/foo.rs', 1, 1)
+        ])
+        (chosen_reviewers, _) = self.choose_reviewers(diff, 'ehuss')
+        assert set(['lexer']) == chosen_reviewers
+
+        # Most changes in one component should win over the base compiler.
+        diff = fakes.make_fake_diff([
+            ('compiler/rustc_parse/src/foo.rs', 5, 10),
+            ('compiler/rustc_llvm/src/foo.rs', 1, 1),
+        ])
+        (chosen_reviewers, _) = self.choose_reviewers(diff, 'ehuss')
+        assert set(['parser']) == chosen_reviewers
+
 
 class TestRun(TestNewPR):
     @pytest.fixture(autouse=True)