@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Make Owners behavior when multiple packages own the same path with different dominion rules more consistent
Summary:
Fixes T12789. See that task for discussion. Currently, when multiple packages own the same path but have different dominion rules we get some weird/aribtrary/inconsistent results.
Instead, implement these rules:
- If zero or more weak and one or more strong packages claim a path, the strong packages (exactly) all own it.
- If one or more weak packages and zero strong packages claim a path, the weak packages all own it.
The major change here is that instead of keeping the //first// weak package we run into, we keep all the weak packages with the longest claim that we run into.
This needs to be implemented twice because Owners has two different near-copies of this logic, only one of which has test coverage. Some day maybe this will get fixed.
Test Plan:
- Added failing unit tests, made them pass.
- Viewed all A/B strong/weak combinations in Diffusion, saw sensible ownership results.
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Maniphest Tasks: T12789
Differential Revision: https://secure.phabricator.com/D18064
+154
-15
+26
-3
src/applications/owners/query/PhabricatorOwnersPackageQuery.php
+26
-3
src/applications/owners/query/PhabricatorOwnersPackageQuery.php
···
398
398
}
399
399
}
400
400
401
+
// At each strength level, drop weak packages if there are also strong
402
+
// packages of the same strength.
403
+
$strength_map = igroup($matches, 'strength');
404
+
foreach ($strength_map as $strength => $package_list) {
405
+
$any_strong = false;
406
+
foreach ($package_list as $package_id => $package) {
407
+
if (!$package['weak']) {
408
+
$any_strong = true;
409
+
break;
410
+
}
411
+
}
412
+
if ($any_strong) {
413
+
foreach ($package_list as $package_id => $package) {
414
+
if ($package['weak']) {
415
+
unset($matches[$package_id]);
416
+
}
417
+
}
418
+
}
419
+
}
420
+
401
421
$matches = isort($matches, 'strength');
402
422
$matches = array_reverse($matches);
403
423
404
-
$first_id = null;
424
+
$strongest = null;
405
425
foreach ($matches as $package_id => $match) {
406
-
if ($first_id === null) {
407
-
$first_id = $package_id;
426
+
if ($strongest === null) {
427
+
$strongest = $match['strength'];
428
+
}
429
+
430
+
if ($match['strength'] === $strongest) {
408
431
continue;
409
432
}
410
433
+39
-12
src/applications/owners/storage/PhabricatorOwnersPackage.php
+39
-12
src/applications/owners/storage/PhabricatorOwnersPackage.php
···
297
297
// a more specific package.
298
298
if ($weak) {
299
299
foreach ($path_packages as $match => $packages) {
300
+
301
+
// Group packages by length.
302
+
$length_map = array();
303
+
foreach ($packages as $package_id => $package) {
304
+
$length_map[$package['length']][$package_id] = $package;
305
+
}
306
+
307
+
// For each path length, remove all weak packages if there are any
308
+
// strong packages of the same length. This makes sure that if there
309
+
// are one or more strong claims on a particular path, only those
310
+
// claims stand.
311
+
foreach ($length_map as $package_list) {
312
+
$any_strong = false;
313
+
foreach ($package_list as $package_id => $package) {
314
+
if (!isset($weak[$package_id])) {
315
+
$any_strong = true;
316
+
break;
317
+
}
318
+
}
319
+
320
+
if ($any_strong) {
321
+
foreach ($package_list as $package_id => $package) {
322
+
if (isset($weak[$package_id])) {
323
+
unset($packages[$package_id]);
324
+
}
325
+
}
326
+
}
327
+
}
328
+
300
329
$packages = isort($packages, 'length');
301
330
$packages = array_reverse($packages, true);
302
331
303
-
$first = null;
332
+
$best_length = null;
304
333
foreach ($packages as $package_id => $package) {
305
-
// If this is the first package we've encountered, note it and
306
-
// continue. We're iterating over the packages from longest to
307
-
// shortest match, so this package always has the strongest claim
308
-
// on the path.
309
-
if ($first === null) {
310
-
$first = $package_id;
311
-
continue;
334
+
// If this is the first package we've encountered, note its length.
335
+
// We're iterating over the packages from longest to shortest match,
336
+
// so packages of this length always have the best claim on the path.
337
+
if ($best_length === null) {
338
+
$best_length = $package['length'];
312
339
}
313
340
314
-
// If this is the first package we saw, its claim stands even if it
315
-
// is a weak package.
316
-
if ($first === $package_id) {
341
+
// If this package has the same length as the best length, its claim
342
+
// stands.
343
+
if ($package['length'] === $best_length) {
317
344
continue;
318
345
}
319
346
320
-
// If this is a weak package and not the first package we saw,
347
+
// If this is a weak package and does not have the best length,
321
348
// cede its claim to the stronger package.
322
349
if (isset($weak[$package_id])) {
323
350
unset($packages[$package_id]);
+89
src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php
+89
src/applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php
···
100
100
PhabricatorOwnersPackage::findLongestPathsPerPackage($rows, $paths));
101
101
102
102
103
+
// Test cases where multiple packages own the same path, with various
104
+
// dominion rules.
105
+
106
+
$main_c = 'src/applications/main/main.c';
107
+
108
+
$rules = array(
109
+
// All claims strong.
110
+
array(
111
+
PhabricatorOwnersPackage::DOMINION_STRONG,
112
+
PhabricatorOwnersPackage::DOMINION_STRONG,
113
+
PhabricatorOwnersPackage::DOMINION_STRONG,
114
+
),
115
+
// All claims weak.
116
+
array(
117
+
PhabricatorOwnersPackage::DOMINION_WEAK,
118
+
PhabricatorOwnersPackage::DOMINION_WEAK,
119
+
PhabricatorOwnersPackage::DOMINION_WEAK,
120
+
),
121
+
// Mixture of strong and weak claims, strong first.
122
+
array(
123
+
PhabricatorOwnersPackage::DOMINION_STRONG,
124
+
PhabricatorOwnersPackage::DOMINION_STRONG,
125
+
PhabricatorOwnersPackage::DOMINION_WEAK,
126
+
),
127
+
// Mixture of strong and weak claims, weak first.
128
+
array(
129
+
PhabricatorOwnersPackage::DOMINION_WEAK,
130
+
PhabricatorOwnersPackage::DOMINION_STRONG,
131
+
PhabricatorOwnersPackage::DOMINION_STRONG,
132
+
),
133
+
);
134
+
135
+
foreach ($rules as $rule_idx => $rule) {
136
+
$rows = array(
137
+
array(
138
+
'id' => 1,
139
+
'excluded' => 0,
140
+
'dominion' => $rule[0],
141
+
'path' => $main_c,
142
+
),
143
+
array(
144
+
'id' => 2,
145
+
'excluded' => 0,
146
+
'dominion' => $rule[1],
147
+
'path' => $main_c,
148
+
),
149
+
array(
150
+
'id' => 3,
151
+
'excluded' => 0,
152
+
'dominion' => $rule[2],
153
+
'path' => $main_c,
154
+
),
155
+
);
156
+
157
+
$paths = array(
158
+
$main_c => $pvalue,
159
+
);
160
+
161
+
// If one or more packages have strong dominion, they should own the
162
+
// path. If not, all the packages with weak dominion should own the
163
+
// path.
164
+
$strong = array();
165
+
$weak = array();
166
+
foreach ($rule as $idx => $dominion) {
167
+
if ($dominion == PhabricatorOwnersPackage::DOMINION_STRONG) {
168
+
$strong[] = $idx + 1;
169
+
} else {
170
+
$weak[] = $idx + 1;
171
+
}
172
+
}
173
+
174
+
if ($strong) {
175
+
$expect = $strong;
176
+
} else {
177
+
$expect = $weak;
178
+
}
179
+
180
+
$expect = array_fill_keys($expect, strlen($main_c));
181
+
$actual = PhabricatorOwnersPackage::findLongestPathsPerPackage(
182
+
$rows,
183
+
$paths);
184
+
185
+
ksort($actual);
186
+
187
+
$this->assertEqual(
188
+
$expect,
189
+
$actual,
190
+
pht('Ruleset "%s" for Identical Ownership', $rule_idx));
191
+
}
103
192
}
104
193
105
194
}