Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(123)

Side by Side Diff: owners.py

Issue 12712002: An interactive tool to help find owners covering current change list. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/tools/depot_tools
Patch Set: Fix nits Created 7 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch | Annotate | Revision Log
« no previous file with comments | « git_cl.py ('k') | owners_finder.py » ('j') | owners_finder.py » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 # Copyright (c) 2012 The Chromium Authors. All rights reserved. 1 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
2 # Use of this source code is governed by a BSD-style license that can be 2 # Use of this source code is governed by a BSD-style license that can be
3 # found in the LICENSE file. 3 # found in the LICENSE file.
4 4
5 """A database of OWNERS files. 5 """A database of OWNERS files.
6 6
7 OWNERS files indicate who is allowed to approve changes in a specific directory 7 OWNERS files indicate who is allowed to approve changes in a specific directory
8 (or who is allowed to make changes without needing approval of another OWNER). 8 (or who is allowed to make changes without needing approval of another OWNER).
9 Note that all changes must still be reviewed by someone familiar with the code, 9 Note that all changes must still be reviewed by someone familiar with the code,
10 so you may need approval from both an OWNER and a reviewer in many cases. 10 so you may need approval from both an OWNER and a reviewer in many cases.
(...skipping 60 matching lines...) Expand 10 before | Expand all | Expand 10 after
71 71
72 72
73 class SyntaxErrorInOwnersFile(Exception): 73 class SyntaxErrorInOwnersFile(Exception):
74 def __init__(self, path, lineno, msg): 74 def __init__(self, path, lineno, msg):
75 super(SyntaxErrorInOwnersFile, self).__init__((path, lineno, msg)) 75 super(SyntaxErrorInOwnersFile, self).__init__((path, lineno, msg))
76 self.path = path 76 self.path = path
77 self.lineno = lineno 77 self.lineno = lineno
78 self.msg = msg 78 self.msg = msg
79 79
80 def __str__(self): 80 def __str__(self):
81 return "%s:%d syntax error: %s" % (self.path, self.lineno, self.msg) 81 return '%s:%d syntax error: %s' % (self.path, self.lineno, self.msg)
82 82
83 83
84 class Database(object): 84 class Database(object):
85 """A database of OWNERS files for a repository. 85 """A database of OWNERS files for a repository.
86 86
87 This class allows you to find a suggested set of reviewers for a list 87 This class allows you to find a suggested set of reviewers for a list
88 of changed files, and see if a list of changed files is covered by a 88 of changed files, and see if a list of changed files is covered by a
89 list of reviewers.""" 89 list of reviewers."""
90 90
91 def __init__(self, root, fopen, os_path, glob): 91 def __init__(self, root, fopen, os_path, glob):
(...skipping 12 matching lines...) Expand all
104 104
105 # Pick a default email regexp to use; callers can override as desired. 105 # Pick a default email regexp to use; callers can override as desired.
106 self.email_regexp = re.compile(BASIC_EMAIL_REGEXP) 106 self.email_regexp = re.compile(BASIC_EMAIL_REGEXP)
107 107
108 # Mapping of owners to the paths they own. 108 # Mapping of owners to the paths they own.
109 self.owned_by = {EVERYONE: set()} 109 self.owned_by = {EVERYONE: set()}
110 110
111 # Mapping of paths to authorized owners. 111 # Mapping of paths to authorized owners.
112 self.owners_for = {} 112 self.owners_for = {}
113 113
114 # Mapping reviewers to the preceding comment per file in the OWNERS files.
115 self.comments = {}
116
114 # Set of paths that stop us from looking above them for owners. 117 # Set of paths that stop us from looking above them for owners.
115 # (This is implicitly true for the root directory). 118 # (This is implicitly true for the root directory).
116 self.stop_looking = set(['']) 119 self.stop_looking = set([''])
117 120
118 def reviewers_for(self, files, author): 121 def reviewers_for(self, files, author):
119 """Returns a suggested set of reviewers that will cover the files. 122 """Returns a suggested set of reviewers that will cover the files.
120 123
121 files is a sequence of paths relative to (and under) self.root. 124 files is a sequence of paths relative to (and under) self.root.
122 If author is nonempty, we ensure it is not included in the set returned 125 If author is nonempty, we ensure it is not included in the set returned
123 in order avoid suggesting the author as a reviewer for their own changes.""" 126 in order avoid suggesting the author as a reviewer for their own changes."""
124 self._check_paths(files) 127 self._check_paths(files)
125 self._load_data_needed_for(files) 128 self.load_data_needed_for(files)
126 suggested_owners = self._covering_set_of_owners_for(files, author) 129 suggested_owners = self._covering_set_of_owners_for(files, author)
127 if EVERYONE in suggested_owners: 130 if EVERYONE in suggested_owners:
128 if len(suggested_owners) > 1: 131 if len(suggested_owners) > 1:
129 suggested_owners.remove(EVERYONE) 132 suggested_owners.remove(EVERYONE)
130 else: 133 else:
131 suggested_owners = set(['<anyone>']) 134 suggested_owners = set(['<anyone>'])
132 return suggested_owners 135 return suggested_owners
133 136
134 def files_not_covered_by(self, files, reviewers): 137 def files_not_covered_by(self, files, reviewers):
135 """Returns the files not owned by one of the reviewers. 138 """Returns the files not owned by one of the reviewers.
136 139
137 Args: 140 Args:
138 files is a sequence of paths relative to (and under) self.root. 141 files is a sequence of paths relative to (and under) self.root.
139 reviewers is a sequence of strings matching self.email_regexp. 142 reviewers is a sequence of strings matching self.email_regexp.
140 """ 143 """
141 self._check_paths(files) 144 self._check_paths(files)
142 self._check_reviewers(reviewers) 145 self._check_reviewers(reviewers)
143 self._load_data_needed_for(files) 146 self.load_data_needed_for(files)
144 147
145 covered_objs = self._objs_covered_by(reviewers) 148 covered_objs = self._objs_covered_by(reviewers)
146 uncovered_files = [f for f in files 149 uncovered_files = [f for f in files
147 if not self._is_obj_covered_by(f, covered_objs)] 150 if not self._is_obj_covered_by(f, covered_objs)]
148 151
149 return set(uncovered_files) 152 return set(uncovered_files)
150 153
151 def _check_paths(self, files): 154 def _check_paths(self, files):
152 def _is_under(f, pfx): 155 def _is_under(f, pfx):
153 return self.os_path.abspath(self.os_path.join(pfx, f)).startswith(pfx) 156 return self.os_path.abspath(self.os_path.join(pfx, f)).startswith(pfx)
(...skipping 21 matching lines...) Expand all
175 178
176 def _enclosing_dir_with_owners(self, objname): 179 def _enclosing_dir_with_owners(self, objname):
177 """Returns the innermost enclosing directory that has an OWNERS file.""" 180 """Returns the innermost enclosing directory that has an OWNERS file."""
178 dirpath = objname 181 dirpath = objname
179 while not dirpath in self.owners_for: 182 while not dirpath in self.owners_for:
180 if self._stop_looking(dirpath): 183 if self._stop_looking(dirpath):
181 break 184 break
182 dirpath = self.os_path.dirname(dirpath) 185 dirpath = self.os_path.dirname(dirpath)
183 return dirpath 186 return dirpath
184 187
185 def _load_data_needed_for(self, files): 188 def load_data_needed_for(self, files):
186 for f in files: 189 for f in files:
187 dirpath = self.os_path.dirname(f) 190 dirpath = self.os_path.dirname(f)
188 while not dirpath in self.owners_for: 191 while not dirpath in self.owners_for:
189 self._read_owners_in_dir(dirpath) 192 self._read_owners_in_dir(dirpath)
190 if self._stop_looking(dirpath): 193 if self._stop_looking(dirpath):
191 break 194 break
192 dirpath = self.os_path.dirname(dirpath) 195 dirpath = self.os_path.dirname(dirpath)
193 196
194 def _read_owners_in_dir(self, dirpath): 197 def _read_owners_in_dir(self, dirpath):
195 owners_path = self.os_path.join(self.root, dirpath, 'OWNERS') 198 owners_path = self.os_path.join(self.root, dirpath, 'OWNERS')
196 if not self.os_path.exists(owners_path): 199 if not self.os_path.exists(owners_path):
197 return 200 return
198 201 comment = []
202 in_comment = False
199 lineno = 0 203 lineno = 0
200 for line in self.fopen(owners_path): 204 for line in self.fopen(owners_path):
201 lineno += 1 205 lineno += 1
202 line = line.strip() 206 line = line.strip()
203 if line.startswith('#') or line == '': 207 if line.startswith('#'):
208 if not in_comment:
209 comment = []
210 comment.append(line[1:].strip())
211 in_comment = True
204 continue 212 continue
213 if line == '':
214 continue
215 in_comment = False
216
205 if line == 'set noparent': 217 if line == 'set noparent':
206 self.stop_looking.add(dirpath) 218 self.stop_looking.add(dirpath)
207 continue 219 continue
208 220
209 m = re.match("per-file (.+)=(.+)", line) 221 m = re.match('per-file (.+)=(.+)', line)
210 if m: 222 if m:
211 glob_string = m.group(1).strip() 223 glob_string = m.group(1).strip()
212 directive = m.group(2).strip() 224 directive = m.group(2).strip()
213 full_glob_string = self.os_path.join(self.root, dirpath, glob_string) 225 full_glob_string = self.os_path.join(self.root, dirpath, glob_string)
214 if '/' in glob_string or '\\' in glob_string: 226 if '/' in glob_string or '\\' in glob_string:
215 raise SyntaxErrorInOwnersFile(owners_path, lineno, 227 raise SyntaxErrorInOwnersFile(owners_path, lineno,
216 'per-file globs cannot span directories or use escapes: "%s"' % 228 'per-file globs cannot span directories or use escapes: "%s"' %
217 line) 229 line)
218 baselines = self.glob(full_glob_string) 230 baselines = self.glob(full_glob_string)
219 for baseline in (self.os_path.relpath(b, self.root) for b in baselines): 231 for baseline in (self.os_path.relpath(b, self.root) for b in baselines):
220 self._add_entry(baseline, directive, "per-file line", 232 self._add_entry(baseline, directive, 'per-file line',
221 owners_path, lineno) 233 owners_path, lineno, '\n'.join(comment))
222 continue 234 continue
223 235
224 if line.startswith('set '): 236 if line.startswith('set '):
225 raise SyntaxErrorInOwnersFile(owners_path, lineno, 237 raise SyntaxErrorInOwnersFile(owners_path, lineno,
226 'unknown option: "%s"' % line[4:].strip()) 238 'unknown option: "%s"' % line[4:].strip())
227 239
228 self._add_entry(dirpath, line, "line", owners_path, lineno) 240 self._add_entry(dirpath, line, 'line', owners_path, lineno,
241 ' '.join(comment))
229 242
230 def _add_entry(self, path, directive, line_type, owners_path, lineno): 243 def _add_entry(self, path, directive,
231 if directive == "set noparent": 244 line_type, owners_path, lineno, comment):
245 if directive == 'set noparent':
232 self.stop_looking.add(path) 246 self.stop_looking.add(path)
233 elif self.email_regexp.match(directive) or directive == EVERYONE: 247 elif self.email_regexp.match(directive) or directive == EVERYONE:
248 self.comments.setdefault(directive, {})
249 self.comments[directive][path] = comment
234 self.owned_by.setdefault(directive, set()).add(path) 250 self.owned_by.setdefault(directive, set()).add(path)
235 self.owners_for.setdefault(path, set()).add(directive) 251 self.owners_for.setdefault(path, set()).add(directive)
236 else: 252 else:
237 raise SyntaxErrorInOwnersFile(owners_path, lineno, 253 raise SyntaxErrorInOwnersFile(owners_path, lineno,
238 ('%s is not a "set" directive, "*", ' 254 ('%s is not a "set" directive, "*", '
239 'or an email address: "%s"' % (line_type, directive))) 255 'or an email address: "%s"' % (line_type, directive)))
240 256
241 def _covering_set_of_owners_for(self, files, author): 257 def _covering_set_of_owners_for(self, files, author):
242 dirs_remaining = set(self._enclosing_dir_with_owners(f) for f in files) 258 dirs_remaining = set(self._enclosing_dir_with_owners(f) for f in files)
243 all_possible_owners = self._all_possible_owners(dirs_remaining, author) 259 all_possible_owners = self.all_possible_owners(dirs_remaining, author)
244 suggested_owners = set() 260 suggested_owners = set()
245 while dirs_remaining: 261 while dirs_remaining:
246 owner = self.lowest_cost_owner(all_possible_owners, dirs_remaining) 262 owner = self.lowest_cost_owner(all_possible_owners, dirs_remaining)
247 suggested_owners.add(owner) 263 suggested_owners.add(owner)
248 dirs_to_remove = set(el[0] for el in all_possible_owners[owner]) 264 dirs_to_remove = set(el[0] for el in all_possible_owners[owner])
249 dirs_remaining -= dirs_to_remove 265 dirs_remaining -= dirs_to_remove
250 return suggested_owners 266 return suggested_owners
251 267
252 def _all_possible_owners(self, dirs, author): 268 def all_possible_owners(self, dirs, author):
253 """Returns a list of (potential owner, distance-from-dir) tuples; a 269 """Returns a list of (potential owner, distance-from-dir) tuples; a
254 distance of 1 is the lowest/closest possible distance (which makes the 270 distance of 1 is the lowest/closest possible distance (which makes the
255 subsequent math easier).""" 271 subsequent math easier)."""
256 all_possible_owners = {} 272 all_possible_owners = {}
257 for current_dir in dirs: 273 for current_dir in dirs:
258 dirname = current_dir 274 dirname = current_dir
259 distance = 1 275 distance = 1
260 while True: 276 while True:
261 for owner in self.owners_for.get(dirname, []): 277 for owner in self.owners_for.get(dirname, []):
262 if author and owner == author: 278 if author and owner == author:
263 continue 279 continue
264 all_possible_owners.setdefault(owner, []) 280 all_possible_owners.setdefault(owner, [])
265 # If the same person is in multiple OWNERS files above a given 281 # If the same person is in multiple OWNERS files above a given
266 # directory, only count the closest one. 282 # directory, only count the closest one.
267 if not any(current_dir == el[0] for el in all_possible_owners[owner]): 283 if not any(current_dir == el[0] for el in all_possible_owners[owner]):
268 all_possible_owners[owner].append((current_dir, distance)) 284 all_possible_owners[owner].append((current_dir, distance))
269 if self._stop_looking(dirname): 285 if self._stop_looking(dirname):
270 break 286 break
271 dirname = self.os_path.dirname(dirname) 287 dirname = self.os_path.dirname(dirname)
272 distance += 1 288 distance += 1
273 return all_possible_owners 289 return all_possible_owners
274 290
275 @staticmethod 291 @staticmethod
276 def lowest_cost_owner(all_possible_owners, dirs): 292 def total_costs_by_owner(all_possible_owners, dirs):
277 # We want to minimize both the number of reviewers and the distance 293 # We want to minimize both the number of reviewers and the distance
278 # from the files/dirs needing reviews. The "pow(X, 1.75)" below is 294 # from the files/dirs needing reviews. The "pow(X, 1.75)" below is
279 # an arbitrarily-selected scaling factor that seems to work well - it 295 # an arbitrarily-selected scaling factor that seems to work well - it
280 # will select one reviewer in the parent directory over three reviewers 296 # will select one reviewer in the parent directory over three reviewers
281 # in subdirs, but not one reviewer over just two. 297 # in subdirs, but not one reviewer over just two.
282 total_costs_by_owner = {} 298 result = {}
283 for owner in all_possible_owners: 299 for owner in all_possible_owners:
284 total_distance = 0 300 total_distance = 0
285 num_directories_owned = 0 301 num_directories_owned = 0
286 for dirname, distance in all_possible_owners[owner]: 302 for dirname, distance in all_possible_owners[owner]:
287 if dirname in dirs: 303 if dirname in dirs:
288 total_distance += distance 304 total_distance += distance
289 num_directories_owned += 1 305 num_directories_owned += 1
290 if num_directories_owned: 306 if num_directories_owned:
291 total_costs_by_owner[owner] = (total_distance / 307 result[owner] = (total_distance /
292 pow(num_directories_owned, 1.75)) 308 pow(num_directories_owned, 1.75))
309 return result
293 310
311 @staticmethod
312 def lowest_cost_owner(all_possible_owners, dirs):
313 total_costs_by_owner = Database.total_costs_by_owner(all_possible_owners,
314 dirs)
294 # Return the lowest cost owner. In the case of a tie, pick one randomly. 315 # Return the lowest cost owner. In the case of a tie, pick one randomly.
295 lowest_cost = min(total_costs_by_owner.itervalues()) 316 lowest_cost = min(total_costs_by_owner.itervalues())
296 lowest_cost_owners = filter( 317 lowest_cost_owners = filter(
297 lambda owner: total_costs_by_owner[owner] == lowest_cost, 318 lambda owner: total_costs_by_owner[owner] == lowest_cost,
298 total_costs_by_owner) 319 total_costs_by_owner)
299 return random.Random().choice(lowest_cost_owners) 320 return random.Random().choice(lowest_cost_owners)
OLDNEW
« no previous file with comments | « git_cl.py ('k') | owners_finder.py » ('j') | owners_finder.py » ('J')

Powered by Google App Engine
This is Rietveld 408576698