@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
Improve exception behavior for Herald commit rules which fail to load diff context
Summary:
This code is a little funky right now, and can return `array("error message")` and then try to call `getHunks()` on it. Additionally, each field loads the commit's changes separately.
Instead, load the commit's changes once and cache them, and handle exceptions appropriately.
Test Plan:
- Created a rule like "changed, added, removed content all match /.*/" to force all fields to generate.
- Ran it successfully.
- Faked an error and ran it, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: bigo, aran
Differential Revision: https://secure.phabricator.com/D7384
+46
-40
+46
-40
src/applications/herald/adapter/HeraldCommitAdapter.php
+46
-40
src/applications/herald/adapter/HeraldCommitAdapter.php
···
17
17
protected $repository;
18
18
protected $commit;
19
19
protected $commitData;
20
+
private $commitDiff;
20
21
21
22
protected $emailPHIDs = array();
22
23
protected $addCCPHIDs = array();
···
272
273
return $diff;
273
274
}
274
275
275
-
private function loadChangesets() {
276
-
try {
277
-
$diff = $this->loadCommitDiff();
278
-
} catch (Exception $ex) {
276
+
private function getDiffContent($type) {
277
+
if ($this->commitDiff === null) {
278
+
try {
279
+
$this->commitDiff = $this->loadCommitDiff();
280
+
} catch (Exception $ex) {
281
+
$this->commitDiff = $ex;
282
+
phlog($ex);
283
+
}
284
+
}
285
+
286
+
if ($this->commitDiff instanceof Exception) {
287
+
$ex = $this->commitDiff;
288
+
$ex_class = get_class($ex);
289
+
$ex_message = pht('Failed to load changes: %s', $ex->getMessage());
290
+
279
291
return array(
280
-
'<<< Failed to load diff, this may mean the change was '.
281
-
'unimaginably enormous. >>>');
292
+
'<'.$ex_class.'>' => $ex_message,
293
+
);
294
+
}
295
+
296
+
$changes = $this->commitDiff->getChangesets();
297
+
298
+
$result = array();
299
+
foreach ($changes as $change) {
300
+
$lines = array();
301
+
foreach ($change->getHunks() as $hunk) {
302
+
switch ($type) {
303
+
case '-':
304
+
$lines[] = $hunk->makeOldFile();
305
+
break;
306
+
case '+':
307
+
$lines[] = $hunk->makeNewFile();
308
+
break;
309
+
case '*':
310
+
$lines[] = $hunk->makeChanges();
311
+
break;
312
+
default:
313
+
throw new Exception("Unknown content selection '{$type}'!");
314
+
}
315
+
}
316
+
$result[$change->getFilename()] = implode("\n", $lines);
282
317
}
283
-
return $diff->getChangesets();
318
+
319
+
return $result;
284
320
}
285
321
286
322
public function getHeraldField($field) {
···
299
335
case self::FIELD_REPOSITORY:
300
336
return $this->repository->getPHID();
301
337
case self::FIELD_DIFF_CONTENT:
302
-
$dict = array();
303
-
$lines = array();
304
-
$changes = $this->loadChangesets();
305
-
foreach ($changes as $change) {
306
-
$lines = array();
307
-
foreach ($change->getHunks() as $hunk) {
308
-
$lines[] = $hunk->makeChanges();
309
-
}
310
-
$dict[$change->getFilename()] = implode("\n", $lines);
311
-
}
312
-
return $dict;
338
+
return $this->getDiffContent('*');
313
339
case self::FIELD_DIFF_ADDED_CONTENT:
314
-
$dict = array();
315
-
$lines = array();
316
-
$changes = $this->loadChangesets();
317
-
foreach ($changes as $change) {
318
-
$lines = array();
319
-
foreach ($change->getHunks() as $hunk) {
320
-
$lines[] = implode('', $hunk->getAddedLines());
321
-
}
322
-
$dict[$change->getFilename()] = implode("\n", $lines);
323
-
}
324
-
return $dict;
340
+
return $this->getDiffContent('+');
325
341
case self::FIELD_DIFF_REMOVED_CONTENT:
326
-
$dict = array();
327
-
$lines = array();
328
-
$changes = $this->loadChangesets();
329
-
foreach ($changes as $change) {
330
-
$lines = array();
331
-
foreach ($change->getHunks() as $hunk) {
332
-
$lines[] = implode('', $hunk->getRemovedLines());
333
-
}
334
-
$dict[$change->getFilename()] = implode("\n", $lines);
335
-
}
336
-
return $dict;
342
+
return $this->getDiffContent('-');
337
343
case self::FIELD_AFFECTED_PACKAGE:
338
344
$packages = $this->loadAffectedPackages();
339
345
return mpull($packages, 'getPHID');