From 2c3d071a26a938d777a516bbf0905e2aa4daf214 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 23 Oct 2013 08:28:58 -0700
Subject: [PATCH] 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
---
 .../herald/adapter/HeraldCommitAdapter.php    | 88 ++++++++++---------
 1 file changed, 47 insertions(+), 41 deletions(-)

diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php
index c1199c9f2c..5b56446ccf 100644
--- a/src/applications/herald/adapter/HeraldCommitAdapter.php
+++ b/src/applications/herald/adapter/HeraldCommitAdapter.php
@@ -17,6 +17,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
   protected $repository;
   protected $commit;
   protected $commitData;
+  private $commitDiff;
 
   protected $emailPHIDs = array();
   protected $addCCPHIDs = array();
@@ -272,15 +273,50 @@ final class HeraldCommitAdapter extends HeraldAdapter {
     return $diff;
   }
 
-  private function loadChangesets() {
-    try {
-      $diff = $this->loadCommitDiff();
-    } catch (Exception $ex) {
-      return array(
-        '<<< Failed to load diff, this may mean the change was '.
-        'unimaginably enormous. >>>');
+  private function getDiffContent($type) {
+    if ($this->commitDiff === null) {
+      try {
+        $this->commitDiff = $this->loadCommitDiff();
+      } catch (Exception $ex) {
+        $this->commitDiff = $ex;
+        phlog($ex);
+      }
     }
-    return $diff->getChangesets();
+
+    if ($this->commitDiff instanceof Exception) {
+      $ex = $this->commitDiff;
+      $ex_class = get_class($ex);
+      $ex_message = pht('Failed to load changes: %s', $ex->getMessage());
+
+      return array(
+        '<'.$ex_class.'>' => $ex_message,
+      );
+    }
+
+    $changes = $this->commitDiff->getChangesets();
+
+    $result = array();
+    foreach ($changes as $change) {
+      $lines = array();
+      foreach ($change->getHunks() as $hunk) {
+        switch ($type) {
+          case '-':
+            $lines[] = $hunk->makeOldFile();
+            break;
+          case '+':
+            $lines[] = $hunk->makeNewFile();
+            break;
+          case '*':
+            $lines[] = $hunk->makeChanges();
+            break;
+          default:
+            throw new Exception("Unknown content selection '{$type}'!");
+        }
+      }
+      $result[$change->getFilename()] = implode("\n", $lines);
+    }
+
+    return $result;
   }
 
   public function getHeraldField($field) {
@@ -299,41 +335,11 @@ final class HeraldCommitAdapter extends HeraldAdapter {
       case self::FIELD_REPOSITORY:
         return $this->repository->getPHID();
       case self::FIELD_DIFF_CONTENT:
-        $dict = array();
-        $lines = array();
-        $changes = $this->loadChangesets();
-        foreach ($changes as $change) {
-          $lines = array();
-          foreach ($change->getHunks() as $hunk) {
-            $lines[] = $hunk->makeChanges();
-          }
-          $dict[$change->getFilename()] = implode("\n", $lines);
-        }
-        return $dict;
+        return $this->getDiffContent('*');
       case self::FIELD_DIFF_ADDED_CONTENT:
-        $dict = array();
-        $lines = array();
-        $changes = $this->loadChangesets();
-        foreach ($changes as $change) {
-          $lines = array();
-          foreach ($change->getHunks() as $hunk) {
-            $lines[] = implode('', $hunk->getAddedLines());
-          }
-          $dict[$change->getFilename()] = implode("\n", $lines);
-        }
-        return $dict;
+        return $this->getDiffContent('+');
       case self::FIELD_DIFF_REMOVED_CONTENT:
-        $dict = array();
-        $lines = array();
-        $changes = $this->loadChangesets();
-        foreach ($changes as $change) {
-          $lines = array();
-          foreach ($change->getHunks() as $hunk) {
-            $lines[] = implode('', $hunk->getRemovedLines());
-          }
-          $dict[$change->getFilename()] = implode("\n", $lines);
-        }
-        return $dict;
+        return $this->getDiffContent('-');
       case self::FIELD_AFFECTED_PACKAGE:
         $packages = $this->loadAffectedPackages();
         return mpull($packages, 'getPHID');