From 690a460c8e8da76153a6bfec1ddcbe524d2ea407 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 27 Jul 2018 12:42:25 -0700
Subject: [PATCH 01/18] Allow mailers to be explicitly marked as inbound or
 outbound

Summary:
See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail.

Allow individual mailers to be marked as not supporting inbound or outbound mail.

Test Plan:
  - Added and ran unit tests.
  - Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive.
  - Hit `bin/mail send-test` with a no-outbound mailer.
  - I'll hold this until after the release cut so it can soak on `secure` for a bit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19546
---
 .../PhabricatorMailImplementationAdapter.php  | 21 +++++
 ...ricatorMetaMTAMailgunReceiveController.php |  7 +-
 ...icatorMetaMTAPostmarkReceiveController.php |  7 +-
 ...icatorMetaMTASendGridReceiveController.php |  7 +-
 ...bricatorMailManagementSendTestWorkflow.php | 10 ++-
 .../storage/PhabricatorMetaMTAMail.php        | 79 +++++++++++++++----
 .../PhabricatorMailConfigTestCase.php         | 73 ++++++++++++++++-
 .../configuring_outbound_email.diviner        |  4 +
 .../PhabricatorClusterMailersConfigType.php   |  2 +
 9 files changed, 185 insertions(+), 25 deletions(-)

diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php
index dfbe891651..8a6b2d2275 100644
--- a/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php
+++ b/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php
@@ -6,6 +6,9 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject {
   private $priority;
   private $options = array();
 
+  private $supportsInbound = true;
+  private $supportsOutbound = true;
+
   final public function getAdapterType() {
     return $this->getPhobjectClassConstant('ADAPTERTYPE');
   }
@@ -67,6 +70,24 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject {
     return $this->priority;
   }
 
+  final public function setSupportsInbound($supports_inbound) {
+    $this->supportsInbound = $supports_inbound;
+    return $this;
+  }
+
+  final public function getSupportsInbound() {
+    return $this->supportsInbound;
+  }
+
+  final public function setSupportsOutbound($supports_outbound) {
+    $this->supportsOutbound = $supports_outbound;
+    return $this;
+  }
+
+  final public function getSupportsOutbound() {
+    return $this->supportsOutbound;
+  }
+
   final public function getOption($key) {
     if (!array_key_exists($key, $this->options)) {
       throw new Exception(
diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php
index 3ca2711dcf..91f656cf97 100644
--- a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php
+++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php
@@ -17,9 +17,12 @@ final class PhabricatorMetaMTAMailgunReceiveController
     // inbound mail from any of them. Test the signature to see if it matches
     // any configured Mailgun mailer.
 
-    $mailers = PhabricatorMetaMTAMail::newMailersWithTypes(
+    $mailers = PhabricatorMetaMTAMail::newMailers(
       array(
-        PhabricatorMailImplementationMailgunAdapter::ADAPTERTYPE,
+        'inbound' => true,
+        'types' => array(
+          PhabricatorMailImplementationMailgunAdapter::ADAPTERTYPE,
+        ),
       ));
     foreach ($mailers as $mailer) {
       $api_key = $mailer->getOption('api-key');
diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php
index 345cd93fe1..550a1366f2 100644
--- a/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php
+++ b/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php
@@ -12,9 +12,12 @@ final class PhabricatorMetaMTAPostmarkReceiveController
    */
   public function handleRequest(AphrontRequest $request) {
     // Don't process requests if we don't have a configured Postmark adapter.
-    $mailers = PhabricatorMetaMTAMail::newMailersWithTypes(
+    $mailers = PhabricatorMetaMTAMail::newMailers(
       array(
-        PhabricatorMailImplementationPostmarkAdapter::ADAPTERTYPE,
+        'inbound' => true,
+        'types' => array(
+          PhabricatorMailImplementationPostmarkAdapter::ADAPTERTYPE,
+        ),
       ));
     if (!$mailers) {
       return new Aphront404Response();
diff --git a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php
index 6651f85d6c..9ec32f60a1 100644
--- a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php
+++ b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php
@@ -11,9 +11,12 @@ final class PhabricatorMetaMTASendGridReceiveController
     // SendGrid doesn't sign payloads so we can't be sure that SendGrid
     // actually sent this request, but require a configured SendGrid mailer
     // before we activate this endpoint.
-    $mailers = PhabricatorMetaMTAMail::newMailersWithTypes(
+    $mailers = PhabricatorMetaMTAMail::newMailers(
       array(
-        PhabricatorMailImplementationSendGridAdapter::ADAPTERTYPE,
+        'inbound' => true,
+        'types' => array(
+          PhabricatorMailImplementationSendGridAdapter::ADAPTERTYPE,
+        ),
       ));
     if (!$mailers) {
       return new Aphront404Response();
diff --git a/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
index 152b62fd3f..8140d64bdd 100644
--- a/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
+++ b/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
@@ -168,7 +168,8 @@ final class PhabricatorMailManagementSendTestWorkflow
 
     $mailer_key = $args->getArg('mailer');
     if ($mailer_key !== null) {
-      $mailers = PhabricatorMetaMTAMail::newMailers();
+      $mailers = PhabricatorMetaMTAMail::newMailers(array());
+
       $mailers = mpull($mailers, null, 'getKey');
       if (!isset($mailers[$mailer_key])) {
         throw new PhutilArgumentUsageException(
@@ -178,6 +179,13 @@ final class PhabricatorMailManagementSendTestWorkflow
             implode(', ', array_keys($mailers))));
       }
 
+      if (!$mailers[$mailer_key]->getSupportsOutbound()) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Mailer ("%s") is not configured to support outbound mail.',
+            $mailer_key));
+      }
+
       $mail->setTryMailers(array($mailer_key));
     }
 
diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
index cca3dfa335..d757868483 100644
--- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
+++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
@@ -494,7 +494,10 @@ final class PhabricatorMetaMTAMail
       throw new Exception(pht('Trying to send an already-sent mail!'));
     }
 
-    $mailers = self::newMailers();
+    $mailers = self::newMailers(
+      array(
+        'outbound' => true,
+      ));
 
     $try_mailers = $this->getParam('mailers.try');
     if ($try_mailers) {
@@ -505,21 +508,15 @@ final class PhabricatorMetaMTAMail
     return $this->sendWithMailers($mailers);
   }
 
-  public static function newMailersWithTypes(array $types) {
-    $mailers = self::newMailers();
-    $types = array_fuse($types);
+  public static function newMailers(array $constraints) {
+    PhutilTypeSpec::checkMap(
+      $constraints,
+      array(
+        'types' => 'optional list<string>',
+        'inbound' => 'optional bool',
+        'outbound' => 'optional bool',
+      ));
 
-    foreach ($mailers as $key => $mailer) {
-      $mailer_type = $mailer->getAdapterType();
-      if (!isset($types[$mailer_type])) {
-        unset($mailers[$key]);
-      }
-    }
-
-    return array_values($mailers);
-  }
-
-  public static function newMailers() {
     $mailers = array();
 
     $config = PhabricatorEnv::getEnvConfig('cluster.mailers');
@@ -565,10 +562,45 @@ final class PhabricatorMetaMTAMail
         $options = idx($spec, 'options', array()) + $defaults;
         $mailer->setOptions($options);
 
+        $mailer->setSupportsInbound(idx($spec, 'inbound', true));
+        $mailer->setSupportsOutbound(idx($spec, 'outbound', true));
+
         $mailers[] = $mailer;
       }
     }
 
+    // Remove mailers with the wrong types.
+    if (isset($constraints['types'])) {
+      $types = $constraints['types'];
+      $types = array_fuse($types);
+      foreach ($mailers as $key => $mailer) {
+        $mailer_type = $mailer->getAdapterType();
+        if (!isset($types[$mailer_type])) {
+          unset($mailers[$key]);
+        }
+      }
+    }
+
+    // If we're only looking for inbound mailers, remove mailers with inbound
+    // support disabled.
+    if (!empty($constraints['inbound'])) {
+      foreach ($mailers as $key => $mailer) {
+        if (!$mailer->getSupportsInbound()) {
+          unset($mailers[$key]);
+        }
+      }
+    }
+
+    // If we're only looking for outbound mailers, remove mailers with outbound
+    // support disabled.
+    if (!empty($constraints['outbound'])) {
+      foreach ($mailers as $key => $mailer) {
+        if (!$mailer->getSupportsOutbound()) {
+          unset($mailers[$key]);
+        }
+      }
+    }
+
     $sorted = array();
     $groups = mgroup($mailers, 'getPriority');
     krsort($groups);
@@ -589,9 +621,24 @@ final class PhabricatorMetaMTAMail
 
   public function sendWithMailers(array $mailers) {
     if (!$mailers) {
+      $any_mailers = self::newMailers();
+
+      // NOTE: We can end up here with some custom list of "$mailers", like
+      // from a unit test. In that case, this message could be misleading. We
+      // can't really tell if the caller made up the list, so just assume they
+      // aren't tricking us.
+
+      if ($any_mailers) {
+        $void_message = pht(
+          'No configured mailers support sending outbound mail.');
+      } else {
+        $void_message = pht(
+          'No mailers are configured.');
+      }
+
       return $this
         ->setStatus(PhabricatorMailOutboundStatus::STATUS_VOID)
-        ->setMessage(pht('No mailers are configured.'))
+        ->setMessage($void_message)
         ->save();
     }
 
diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php
index 25984a2c1d..fb48925ed4 100644
--- a/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php
+++ b/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php
@@ -118,11 +118,80 @@ final class PhabricatorMailConfigTestCase
     $this->assertTrue($saw_a1 && $saw_a2);
   }
 
-  private function newMailersWithConfig(array $config) {
+  public function testMailerConstraints() {
+    $config = array(
+      array(
+        'key' => 'X1',
+        'type' => 'test',
+      ),
+      array(
+        'key' => 'X1-in',
+        'type' => 'test',
+        'outbound' => false,
+      ),
+      array(
+        'key' => 'X1-out',
+        'type' => 'test',
+        'inbound' => false,
+      ),
+      array(
+        'key' => 'X1-void',
+        'type' => 'test',
+        'inbound' => false,
+        'outbound' => false,
+      ),
+    );
+
+    $mailers = $this->newMailersWithConfig(
+      $config,
+      array());
+    $this->assertEqual(4, count($mailers));
+
+    $mailers = $this->newMailersWithConfig(
+      $config,
+      array(
+        'inbound' => true,
+      ));
+    $this->assertEqual(2, count($mailers));
+
+    $mailers = $this->newMailersWithConfig(
+      $config,
+      array(
+        'outbound' => true,
+      ));
+    $this->assertEqual(2, count($mailers));
+
+    $mailers = $this->newMailersWithConfig(
+      $config,
+      array(
+        'inbound' => true,
+        'outbound' => true,
+      ));
+    $this->assertEqual(1, count($mailers));
+
+    $mailers = $this->newMailersWithConfig(
+      $config,
+      array(
+        'types' => array('test'),
+      ));
+    $this->assertEqual(4, count($mailers));
+
+    $mailers = $this->newMailersWithConfig(
+      $config,
+      array(
+        'types' => array('duck'),
+      ));
+    $this->assertEqual(0, count($mailers));
+  }
+
+  private function newMailersWithConfig(
+    array $config,
+    array $constraints = array()) {
+
     $env = PhabricatorEnv::beginScopedEnv();
     $env->overrideEnvConfig('cluster.mailers', $config);
 
-    $mailers = PhabricatorMetaMTAMail::newMailers();
+    $mailers = PhabricatorMetaMTAMail::newMailers($constraints);
 
     unset($env);
     return $mailers;
diff --git a/src/docs/user/configuration/configuring_outbound_email.diviner b/src/docs/user/configuration/configuring_outbound_email.diviner
index 5de8429c13..a33134fe4e 100644
--- a/src/docs/user/configuration/configuring_outbound_email.diviner
+++ b/src/docs/user/configuration/configuring_outbound_email.diviner
@@ -82,6 +82,10 @@ The supported keys for each mailer are:
   - `priority`: Optional string. Advanced option which controls load balancing
     and failover behavior. See below for details.
   - `options`: Optional map. Additional options for the mailer type.
+  - `inbound`: Optional bool. Use `false` to prevent this mailer from being
+    used to receive inbound mail.
+  - `outbound`: Optional bool. Use `false` to prevent this mailer from being
+    used to send outbound mail.
 
 The `type` field can be used to select these third-party mailers:
 
diff --git a/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php b/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php
index 03f30506bd..387014289d 100644
--- a/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php
+++ b/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php
@@ -43,6 +43,8 @@ final class PhabricatorClusterMailersConfigType
             'type' => 'string',
             'priority' => 'optional int',
             'options' => 'optional wild',
+            'inbound' => 'optional bool',
+            'outbound' => 'optional bool',
           ));
       } catch (Exception $ex) {
         throw $this->newException(

From 9cf3b3bbf806bcafb1e10af270add71863d40e05 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 27 Jul 2018 13:22:36 -0700
Subject: [PATCH 02/18] Count lines in build log slices more cheaply

Summary:
See PHI766. Ref T13164. Build log chunk processing does a `preg_split()` on slices, but this isn't terribly efficient.

We can get the same count more cheaply by just using `substr_count()` a few times.

(I also tried `preg_match_all()`, which was between the two in speed.)

Test Plan:
- Used `bin/harbormaster rebuild-log --id X --force` to rebuild logs. Verified that the linemap is identical before/after this change.
- Saw local time for the 18MB log in PHI766 drop from ~1.7s to ~900ms, and `preg_split()` drop out of the profiler (we're now spending the biggest chunk of time on `gzdeflate()`).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19545
---
 .../harbormaster/storage/build/HarbormasterBuildLog.php   | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php
index 5bc358ccc8..2b160445d8 100644
--- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php
+++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php
@@ -643,7 +643,13 @@ final class HarbormasterBuildLog
       $pos += $slice_length;
 
       $map_bytes += $slice_length;
-      $line_count += count(preg_split("/\r\n|\r|\n/", $slice)) - 1;
+
+      // Count newlines in the slice. This goofy approach is meaningfully
+      // faster than "preg_match_all()" or "preg_split()". See PHI766.
+      $n_rn = substr_count($slice, "\r\n");
+      $n_r = substr_count($slice, "\r");
+      $n_n = substr_count($slice, "\n");
+      $line_count += ($n_rn) + ($n_r - $n_rn) + ($n_n - $n_rn);
 
       if ($map_bytes >= ($marker_distance - $max_utf8_width)) {
         $map[] = array(

From 81a7c9ac43f21267ada60d66b7451b966b8bcdc0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 09:40:47 -0700
Subject: [PATCH 03/18] Remove the execution time limit (if any) before sinking
 HTTP responses

Summary:
Ref T12907. At least part of the problem is that we can hit PHP's `max_execution_time` limit on the file download pathway.

We don't currently set this to anything in the application itself, but PHP often sets it to 30s by default (and we have it set to 30s in production).

When writing responses, remove this limit. This limit is mostly a protection against accidental loops/recurison/etc., not a process slot protection. It doesn't really protect process slots anyway, since it doesn't start counting until the request starts executing, so you can (by default) //send// the request as slowly as you want without hitting this limit.

By releasing the limit this late, hopefully all the loops and recursion issues have already been caught and we're left with mostly smooth sailing.

We already remove this limit when sending `git clone` responses in `DiffusionServeController` and nothing has blown up. This affects `git clone http://` and similar.

(I may have had this turned off locally and/or just be too impatient to wait 30s, which is why I haven't caught this previously.)

Test Plan:
  - Poked around and downloaded some files.
  - Will `curl ...` in production and see if that goes better.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12907

Differential Revision: https://secure.phabricator.com/D19547
---
 src/aphront/response/AphrontResponse.php | 11 ++++++++++-
 src/aphront/sink/AphrontHTTPSink.php     | 17 +++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php
index 2e6513c7a8..0eab91b2af 100644
--- a/src/aphront/response/AphrontResponse.php
+++ b/src/aphront/response/AphrontResponse.php
@@ -54,7 +54,16 @@ abstract class AphrontResponse extends Phobject {
 
 
   public function getContentIterator() {
-    return array($this->buildResponseString());
+    // By default, make sure responses are truly returning a string, not some
+    // kind of object that behaves like a string.
+
+    // We're going to remove the execution time limit before dumping the
+    // response into the sink, and want any rendering that's going to occur
+    // to happen BEFORE we release the limit.
+
+    return array(
+      (string)$this->buildResponseString(),
+    );
   }
 
   public function buildResponseString() {
diff --git a/src/aphront/sink/AphrontHTTPSink.php b/src/aphront/sink/AphrontHTTPSink.php
index 11c6466cf1..51c54df520 100644
--- a/src/aphront/sink/AphrontHTTPSink.php
+++ b/src/aphront/sink/AphrontHTTPSink.php
@@ -112,6 +112,23 @@ abstract class AphrontHTTPSink extends Phobject {
       $response->getHTTPResponseMessage());
     $this->writeHeaders($all_headers);
 
+    // Allow clients an unlimited amount of time to download the response.
+
+    // This allows clients to perform a "slow loris" attack, where they
+    // download a large response very slowly to tie up process slots. However,
+    // concurrent connection limits and "RequestReadTimeout" already prevent
+    // this attack. We could add our own minimum download rate here if we want
+    // to make this easier to configure eventually.
+
+    // For normal page responses, we've fully rendered the page into a string
+    // already so all that's left is writing it to the client.
+
+    // For unusual responses (like large file downloads) we may still be doing
+    // some meaningful work, but in theory that work is intrinsic to streaming
+    // the response.
+
+    set_time_limit(0);
+
     $abort = false;
     foreach ($data as $block) {
       if (!$this->isWritable()) {

From 13cac5c362bba4e69f3c1f820a920bd5a62ad602 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 15:32:55 -0700
Subject: [PATCH 04/18] Add Spaces to Projects

Summary:
See PHI774. Ref T13164. There is no reason projects //don't// support Spaces, just a vague concern that it's not hugely useful and might be a bit confusing.

However, it's at least somewhat useful (to improve consistency and reduce special casing) and doesn't necessarily seem more confusing than Projects are anyway. Support is trivial from a technical point of view, so just hook it up.

Test Plan: Created new projects, shifted projects between spaces. The support is all pretty much automatic.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19549
---
 .../sql/autopatches/20180730.project.01.spaces.sql  |  2 ++
 src/__phutil_library_map__.php                      |  1 +
 .../project/storage/PhabricatorProject.php          | 13 ++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 resources/sql/autopatches/20180730.project.01.spaces.sql

diff --git a/resources/sql/autopatches/20180730.project.01.spaces.sql b/resources/sql/autopatches/20180730.project.01.spaces.sql
new file mode 100644
index 0000000000..927ff3b677
--- /dev/null
+++ b/resources/sql/autopatches/20180730.project.01.spaces.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_project.project
+  ADD COLUMN spacePHID VARBINARY(64) DEFAULT NULL;
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 568c715ea1..981764dc7a 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -9752,6 +9752,7 @@ phutil_register_library_map(array(
       'PhabricatorFerretInterface',
       'PhabricatorConduitResultInterface',
       'PhabricatorColumnProxyInterface',
+      'PhabricatorSpacesInterface',
     ),
     'PhabricatorProjectAddHeraldAction' => 'PhabricatorProjectHeraldAction',
     'PhabricatorProjectApplication' => 'PhabricatorApplication',
diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php
index 4ec4b63675..aa035aedf5 100644
--- a/src/applications/project/storage/PhabricatorProject.php
+++ b/src/applications/project/storage/PhabricatorProject.php
@@ -11,7 +11,8 @@ final class PhabricatorProject extends PhabricatorProjectDAO
     PhabricatorFulltextInterface,
     PhabricatorFerretInterface,
     PhabricatorConduitResultInterface,
-    PhabricatorColumnProxyInterface {
+    PhabricatorColumnProxyInterface,
+    PhabricatorSpacesInterface {
 
   protected $name;
   protected $status = PhabricatorProjectStatus::STATUS_ACTIVE;
@@ -38,6 +39,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
   protected $projectPathKey;
 
   protected $properties = array();
+  protected $spacePHID;
 
   private $memberPHIDs = self::ATTACHABLE;
   private $watcherPHIDs = self::ATTACHABLE;
@@ -82,6 +84,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
       ->setViewPolicy($view_policy)
       ->setEditPolicy($edit_policy)
       ->setJoinPolicy($join_policy)
+      ->setSpacePHID($actor->getDefaultSpacePHID())
       ->setIsMembershipLocked(0)
       ->attachMemberPHIDs(array())
       ->attachSlugs(array())
@@ -697,6 +700,14 @@ final class PhabricatorProject extends PhabricatorProjectDAO
   }
 
 
+/* -(  PhabricatorSpacesInterface  )----------------------------------------- */
+
+
+  public function getSpacePHID() {
+    return $this->spacePHID;
+  }
+
+
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */
 
 

From d9b5b04950883eb7a349ebc80e49adc9a9b857d9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 15:59:11 -0700
Subject: [PATCH 05/18] Improve Space behavior for subprojects and milestones

Summary:
Depends on D19549. Ref T13164. See PHI774.

  - Make milestones inherit their parent project's space automatically, like they inherit their parent policies.
  - Make subprojects default to their parent project's space.

Test Plan: Created subprojects and milestones, got sensible default/effective Space behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19550
---
 .../engine/PhabricatorProjectEditEngine.php    |  7 ++++++-
 .../project/storage/PhabricatorProject.php     | 18 ++++++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/applications/project/engine/PhabricatorProjectEditEngine.php b/src/applications/project/engine/PhabricatorProjectEditEngine.php
index a7702c345f..5eb6b49b87 100644
--- a/src/applications/project/engine/PhabricatorProjectEditEngine.php
+++ b/src/applications/project/engine/PhabricatorProjectEditEngine.php
@@ -51,7 +51,11 @@ final class PhabricatorProjectEditEngine
   }
 
   protected function newEditableObject() {
-    return PhabricatorProject::initializeNewProject($this->getViewer());
+    $parent = nonempty($this->parentProject, $this->milestoneProject);
+
+    return PhabricatorProject::initializeNewProject(
+      $this->getViewer(),
+      $parent);
   }
 
   protected function newObjectQuery() {
@@ -112,6 +116,7 @@ final class PhabricatorProjectEditEngine
       PhabricatorTransactions::TYPE_VIEW_POLICY,
       PhabricatorTransactions::TYPE_EDIT_POLICY,
       PhabricatorTransactions::TYPE_JOIN_POLICY,
+      PhabricatorTransactions::TYPE_SPACE,
       PhabricatorProjectIconTransaction::TRANSACTIONTYPE,
       PhabricatorProjectColorTransaction::TRANSACTIONTYPE,
     );
diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php
index aa035aedf5..a049354ad7 100644
--- a/src/applications/project/storage/PhabricatorProject.php
+++ b/src/applications/project/storage/PhabricatorProject.php
@@ -61,7 +61,10 @@ final class PhabricatorProject extends PhabricatorProjectDAO
   const ITEM_MILESTONES = 'project.milestones';
   const ITEM_SUBPROJECTS = 'project.subprojects';
 
-  public static function initializeNewProject(PhabricatorUser $actor) {
+  public static function initializeNewProject(
+    PhabricatorUser $actor,
+    PhabricatorProject $parent = null) {
+
     $app = id(new PhabricatorApplicationQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withClasses(array('PhabricatorProjectApplication'))
@@ -74,6 +77,14 @@ final class PhabricatorProject extends PhabricatorProjectDAO
     $join_policy = $app->getPolicy(
       ProjectDefaultJoinCapability::CAPABILITY);
 
+    // If this is the child of some other project, default the Space to the
+    // Space of the parent.
+    if ($parent) {
+      $space_phid = $parent->getSpacePHID();
+    } else {
+      $space_phid = $actor->getDefaultSpacePHID();
+    }
+
     $default_icon = PhabricatorProjectIconSet::getDefaultIconKey();
     $default_color = PhabricatorProjectIconSet::getDefaultColorKey();
 
@@ -84,7 +95,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
       ->setViewPolicy($view_policy)
       ->setEditPolicy($edit_policy)
       ->setJoinPolicy($join_policy)
-      ->setSpacePHID($actor->getDefaultSpacePHID())
+      ->setSpacePHID($space_phid)
       ->setIsMembershipLocked(0)
       ->attachMemberPHIDs(array())
       ->attachSlugs(array())
@@ -704,6 +715,9 @@ final class PhabricatorProject extends PhabricatorProjectDAO
 
 
   public function getSpacePHID() {
+    if ($this->isMilestone()) {
+      return $this->getParentProject()->getSpacePHID();
+    }
     return $this->spacePHID;
   }
 

From 8374201620fea39666e3c6d03284f94b9376b37c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 15:59:48 -0700
Subject: [PATCH 06/18] Add a more specific CSS rule to make Spaces headers in
 projects colored red

Summary:
Depends on D19551. Ref T13164. Projects use a special kind of header setup that has a more specific CSS rule to make content black. Add an even more specific rule to make it red.

(This could probably be disentangled a bit and isn't necessarily the cleanest fix, but I poked at it for a few minutes and didn't come up with anything cleaner.)

Test Plan: Viewed projects in spaces, saw the space names colored red properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19552
---
 resources/celerity/map.php                 | 6 +++---
 webroot/rsrc/css/phui/phui-header-view.css | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 5c1c7e7bfe..9c0d0eda0a 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
-    'core.pkg.css' => 'e5233bff',
+    'core.pkg.css' => 'f515619b',
     'core.pkg.js' => '2058ec09',
     'differential.pkg.css' => '06dc617c',
     'differential.pkg.js' => 'ef19e026',
@@ -154,7 +154,7 @@ return array(
     'rsrc/css/phui/phui-form-view.css' => 'ae9f8d16',
     'rsrc/css/phui/phui-form.css' => '7aaa04e3',
     'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f',
-    'rsrc/css/phui/phui-header-view.css' => '31dc6c72',
+    'rsrc/css/phui/phui-header-view.css' => 'edeb9252',
     'rsrc/css/phui/phui-hovercard.css' => 'f0592bcf',
     'rsrc/css/phui/phui-icon-set-selector.css' => '87db8fee',
     'rsrc/css/phui/phui-icon.css' => 'cf24ceec',
@@ -821,7 +821,7 @@ return array(
     'phui-form-css' => '7aaa04e3',
     'phui-form-view-css' => 'ae9f8d16',
     'phui-head-thing-view-css' => 'fd311e5f',
-    'phui-header-view-css' => '31dc6c72',
+    'phui-header-view-css' => 'edeb9252',
     'phui-hovercard' => '1bd28176',
     'phui-hovercard-view-css' => 'f0592bcf',
     'phui-icon-set-selector-css' => '87db8fee',
diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css
index 478dde2153..5a8f06281d 100644
--- a/webroot/rsrc/css/phui/phui-header-view.css
+++ b/webroot/rsrc/css/phui/phui-header-view.css
@@ -294,7 +294,8 @@ body .phui-header-shell.phui-bleed-header
 }
 
 .spaces-name .phui-handle,
-.spaces-name a.phui-handle {
+.spaces-name a.phui-handle,
+.phui-profile-header.phui-header-shell .spaces-name .phui-handle {
   color: {$sh-redtext};
 }
 

From 8d8086fccf9635484ccb915018a7074f73f7d1d3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 16:22:46 -0700
Subject: [PATCH 07/18] Add Spaces support to Phriction

Summary:
Ref T13164. See PHI774. Fixes T12435.

Since Phriction is hierarchical, there isn't a super strong motivation to support Spaces: you can generally set policies on a small number of documents to get the desired effective policy behavior.

However, it still improves consistency and there's no reason //not// to support Spaces. In the case where you have some moderately weird/complex policy on one or more Spaces, using Spaces to define the policy behavior can make things a bit simpler and easier to understand.

This probably doesn't actually fix whatever the root problem in T12435 was (complicated, non-hierarchical access policies?). See also a bunch of discussion in T12442. So we might end up going beyond this to address other use cases, but I think this is reasonable regardless.

Test Plan: Created and edited Phriction documents and shifted them between Spaces. Searched by Space, etc.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164, T12435

Differential Revision: https://secure.phabricator.com/D19553
---
 .../20180730.phriction.01.spaces.sql          |  2 ++
 src/__phutil_library_map__.php                |  1 +
 .../controller/PhrictionEditController.php    |  9 +++++++
 .../query/PhrictionDocumentSearchEngine.php   |  1 +
 .../phriction/storage/PhrictionDocument.php   | 25 +++++++++++++++----
 5 files changed, 33 insertions(+), 5 deletions(-)
 create mode 100644 resources/sql/autopatches/20180730.phriction.01.spaces.sql

diff --git a/resources/sql/autopatches/20180730.phriction.01.spaces.sql b/resources/sql/autopatches/20180730.phriction.01.spaces.sql
new file mode 100644
index 0000000000..6d3e007258
--- /dev/null
+++ b/resources/sql/autopatches/20180730.phriction.01.spaces.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_phriction.phriction_document
+  ADD spacePHID VARBINARY(64) DEFAULT NULL;
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 981764dc7a..bd4cfef6c2 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -11129,6 +11129,7 @@ phutil_register_library_map(array(
       'PhabricatorApplicationTransactionInterface',
       'PhabricatorConduitResultInterface',
       'PhabricatorPolicyCodexInterface',
+      'PhabricatorSpacesInterface',
     ),
     'PhrictionDocumentAuthorHeraldField' => 'PhrictionDocumentHeraldField',
     'PhrictionDocumentContentHeraldField' => 'PhrictionDocumentHeraldField',
diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php
index 006488b274..ccd473931c 100644
--- a/src/applications/phriction/controller/PhrictionEditController.php
+++ b/src/applications/phriction/controller/PhrictionEditController.php
@@ -121,6 +121,8 @@ final class PhrictionEditController
       $v_projects = array_reverse($v_projects);
     }
 
+    $v_space = $document->getSpacePHID();
+
     if ($request->isFormPost()) {
 
       $title = $request->getStr('title');
@@ -131,6 +133,7 @@ final class PhrictionEditController
       $v_edit = $request->getStr('editPolicy');
       $v_cc = $request->getArr('cc');
       $v_projects = $request->getArr('projects');
+      $v_space = $request->getStr('spacePHID');
 
       $xactions = array();
       $xactions[] = id(new PhrictionTransaction())
@@ -146,6 +149,9 @@ final class PhrictionEditController
       $xactions[] = id(new PhrictionTransaction())
         ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY)
         ->setNewValue($v_edit);
+      $xactions[] = id(new PhrictionTransaction())
+        ->setTransactionType(PhabricatorTransactions::TYPE_SPACE)
+        ->setNewValue($v_space);
       $xactions[] = id(new PhrictionTransaction())
         ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
         ->setNewValue(array('=' => $v_cc));
@@ -192,6 +198,7 @@ final class PhrictionEditController
 
         $document->setViewPolicy($v_view);
         $document->setEditPolicy($v_edit);
+        $document->setSpacePHID($v_space);
       }
     }
 
@@ -267,7 +274,9 @@ final class PhrictionEditController
           ->setDatasource(new PhabricatorMetaMTAMailableDatasource()))
       ->appendChild(
         id(new AphrontFormPolicyControl())
+          ->setViewer($viewer)
           ->setName('viewPolicy')
+          ->setSpacePHID($v_space)
           ->setPolicyObject($document)
           ->setCapability($view_capability)
           ->setPolicies($policies)
diff --git a/src/applications/phriction/query/PhrictionDocumentSearchEngine.php b/src/applications/phriction/query/PhrictionDocumentSearchEngine.php
index e3d962146a..49a332268b 100644
--- a/src/applications/phriction/query/PhrictionDocumentSearchEngine.php
+++ b/src/applications/phriction/query/PhrictionDocumentSearchEngine.php
@@ -131,6 +131,7 @@ final class PhrictionDocumentSearchEngine
 
       $item = id(new PHUIObjectItemView())
         ->setHeader($content->getTitle())
+        ->setObject($document)
         ->setHref($slug_uri)
         ->addByline($byline)
         ->addIcon('none', $updated);
diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php
index f482579cea..6edb516e6e 100644
--- a/src/applications/phriction/storage/PhrictionDocument.php
+++ b/src/applications/phriction/storage/PhrictionDocument.php
@@ -12,7 +12,8 @@ final class PhrictionDocument extends PhrictionDAO
     PhabricatorProjectInterface,
     PhabricatorApplicationTransactionInterface,
     PhabricatorConduitResultInterface,
-    PhabricatorPolicyCodexInterface {
+    PhabricatorPolicyCodexInterface,
+    PhabricatorSpacesInterface {
 
   protected $slug;
   protected $depth;
@@ -21,6 +22,7 @@ final class PhrictionDocument extends PhrictionDAO
   protected $mailKey;
   protected $viewPolicy;
   protected $editPolicy;
+  protected $spacePHID;
 
   private $contentObject = self::ATTACHABLE;
   private $ancestors = array();
@@ -81,12 +83,16 @@ final class PhrictionDocument extends PhrictionDAO
     }
 
     if ($parent_doc) {
-      $document->setViewPolicy($parent_doc->getViewPolicy());
-      $document->setEditPolicy($parent_doc->getEditPolicy());
+      $document
+        ->setViewPolicy($parent_doc->getViewPolicy())
+        ->setEditPolicy($parent_doc->getEditPolicy())
+        ->setSpacePHID($parent_doc->getSpacePHID());
     } else {
       $default_view_policy = PhabricatorPolicies::getMostOpenPolicy();
-      $document->setViewPolicy($default_view_policy);
-      $document->setEditPolicy(PhabricatorPolicies::POLICY_USER);
+      $document
+        ->setViewPolicy($default_view_policy)
+        ->setEditPolicy(PhabricatorPolicies::POLICY_USER)
+        ->setSpacePHID($actor->getDefaultSpacePHID());
     }
 
     return $document;
@@ -202,6 +208,15 @@ final class PhrictionDocument extends PhrictionDAO
   }
 
 
+/* -(  PhabricatorSpacesInterface  )----------------------------------------- */
+
+
+  public function getSpacePHID() {
+    return $this->spacePHID;
+  }
+
+
+
 /* -(  PhabricatorSubscribableInterface  )----------------------------------- */
 
 

From 45babe82f36470f2c8d2f3c363500894645bf85c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 16:33:20 -0700
Subject: [PATCH 08/18] Add Spaces information to the project list UI

Summary: Depends on D19552. Ref T13164. We need this little `setObject(...)` hook to get the Space name into the search list UI.

Test Plan: Viewed project list, saw some Spaces listed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19554
---
 src/applications/project/view/PhabricatorProjectListView.php | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/applications/project/view/PhabricatorProjectListView.php b/src/applications/project/view/PhabricatorProjectListView.php
index 7496a401bb..645440d831 100644
--- a/src/applications/project/view/PhabricatorProjectListView.php
+++ b/src/applications/project/view/PhabricatorProjectListView.php
@@ -57,6 +57,7 @@ final class PhabricatorProjectListView extends AphrontView {
       $icon_name = $project->getDisplayIconName();
 
       $item = id(new PHUIObjectItemView())
+        ->setObject($project)
         ->setHeader($project->getName())
         ->setHref("/project/view/{$id}/")
         ->setImageURI($project->getProfileImageURI())

From 96e3c73159e602493c100ecf1bce14561c21c9c4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Jul 2018 15:59:39 -0700
Subject: [PATCH 09/18] Put "Subprojects" on top of "Milestones" in the Project
 UI

Summary:
Depends on D19550. Ref T13164. See T12144#226172, mostly. We get some requests to make milestones reorderable, but in most cases users probably wanted subprojects, not milestones.

One reason to end up here is that we put "Milestones" on top. Instead, put "Subprojects" on top, since they're the less specialized option and we aren't terribly consistent about it anyway.

Test Plan: Viewed project subprojects page, saw "Subprojects" above "Milestones".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19551
---
 ...habricatorProjectSubprojectsController.php | 63 +++++++++----------
 1 file changed, 31 insertions(+), 32 deletions(-)

diff --git a/src/applications/project/controller/PhabricatorProjectSubprojectsController.php b/src/applications/project/controller/PhabricatorProjectSubprojectsController.php
index 269e80a080..36736c78ba 100644
--- a/src/applications/project/controller/PhabricatorProjectSubprojectsController.php
+++ b/src/applications/project/controller/PhabricatorProjectSubprojectsController.php
@@ -106,8 +106,8 @@ final class PhabricatorProjectSubprojectsController
       ->addClass('project-view-people-home')
       ->setMainColumn(array(
           $info_view,
-          $milestone_list,
           $subproject_list,
+          $milestone_list,
       ));
 
     return $this->newPage()
@@ -132,28 +132,11 @@ final class PhabricatorProjectSubprojectsController
       $project,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-    $allows_milestones = $project->supportsMilestones();
     $allows_subprojects = $project->supportsSubprojects();
+    $allows_milestones = $project->supportsMilestones();
 
     $curtain = $this->newCurtainView();
 
-    if ($allows_milestones && $milestones) {
-      $milestone_text = pht('Create Next Milestone');
-    } else {
-      $milestone_text = pht('Create Milestone');
-    }
-
-    $can_milestone = ($can_create && $can_edit && $allows_milestones);
-    $milestone_href = "/project/edit/?milestone={$id}";
-
-    $curtain->addAction(
-      id(new PhabricatorActionView())
-        ->setName($milestone_text)
-        ->setIcon('fa-plus')
-        ->setHref($milestone_href)
-        ->setDisabled(!$can_milestone)
-        ->setWorkflow(!$can_milestone));
-
     $can_subproject = ($can_create && $can_edit && $allows_subprojects);
 
     // If we're offering to create the first subproject, we're going to warn
@@ -176,22 +159,22 @@ final class PhabricatorProjectSubprojectsController
         ->setDisabled($subproject_disabled)
         ->setWorkflow($subproject_workflow));
 
-
-    if (!$project->supportsMilestones()) {
-      $note = pht(
-        'This project is already a milestone, and milestones may not '.
-        'have their own milestones.');
+    if ($allows_milestones && $milestones) {
+      $milestone_text = pht('Create Next Milestone');
     } else {
-      if (!$milestones) {
-        $note = pht('Milestones can be created for this project.');
-      } else {
-        $note = pht('This project has milestones.');
-      }
+      $milestone_text = pht('Create Milestone');
     }
 
-    $curtain->newPanel()
-      ->setHeaderText(pht('Milestones'))
-      ->appendChild($note);
+    $can_milestone = ($can_create && $can_edit && $allows_milestones);
+    $milestone_href = "/project/edit/?milestone={$id}";
+
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setName($milestone_text)
+        ->setIcon('fa-plus')
+        ->setHref($milestone_href)
+        ->setDisabled(!$can_milestone)
+        ->setWorkflow(!$can_milestone));
 
     if (!$project->supportsSubprojects()) {
       $note = pht(
@@ -209,6 +192,22 @@ final class PhabricatorProjectSubprojectsController
       ->setHeaderText(pht('Subprojects'))
       ->appendChild($note);
 
+    if (!$project->supportsSubprojects()) {
+      $note = pht(
+        'This project is already a milestone, and milestones may not '.
+        'have their own milestones.');
+    } else {
+      if (!$milestones) {
+        $note = pht('Milestones can be created for this project.');
+      } else {
+        $note = pht('This project has milestones.');
+      }
+    }
+
+    $curtain->newPanel()
+      ->setHeaderText(pht('Milestones'))
+      ->appendChild($note);
+
     return $curtain;
   }
 

From d8834377be0ce2f1d41ad5588bc8b6588b8ebcd8 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 1 Aug 2018 08:39:08 -0700
Subject: [PATCH 10/18] When a Herald rule blocks a push, show which rule fired
 in the push log UI

Summary:
Ref T13164. See PHI765. We currently show "Rejected: Herald" in the push log UI, but don't show which rule rejected a push.

We store this data, and it's potentially useful: either for hunting down a particular issue, or for getting a general sense of how often a reject rule is triggering (maybe because you want to tune how aggressive it is).

Show this data in the web UI, and include it in the data export payload.

Test Plan:
  - Pushed to a hosted repository so that I got blocked by a Herald rule.
  - Viewed the push logs in the web UI, now saw which rule triggered things.
  - Exported logs to CSV, saw Herald rule PHIDs in the data.

{F5776211}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19555
---
 .../view/DiffusionPushLogListView.php         | 23 +++++++++++++++----
 ...abricatorRepositoryPushLogSearchEngine.php |  4 ++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/applications/diffusion/view/DiffusionPushLogListView.php b/src/applications/diffusion/view/DiffusionPushLogListView.php
index e101625752..12a5fa5494 100644
--- a/src/applications/diffusion/view/DiffusionPushLogListView.php
+++ b/src/applications/diffusion/view/DiffusionPushLogListView.php
@@ -14,6 +14,8 @@ final class DiffusionPushLogListView extends AphrontView {
     $logs = $this->logs;
     $viewer = $this->getViewer();
 
+    $reject_herald = PhabricatorRepositoryPushLog::REJECT_HERALD;
+
     $handle_phids = array();
     foreach ($logs as $log) {
       $handle_phids[] = $log->getPusherPHID();
@@ -21,9 +23,13 @@ final class DiffusionPushLogListView extends AphrontView {
       if ($device_phid) {
         $handle_phids[] = $device_phid;
       }
+
+      if ($log->getPushEvent()->getRejectCode() == $reject_herald) {
+        $handle_phids[] = $log->getPushEvent()->getRejectDetails();
+      }
     }
 
-    $handles = $viewer->loadHandles($handle_phids);
+    $viewer->loadHandles($handle_phids);
 
     // Only administrators can view remote addresses.
     $remotes_visible = $viewer->getIsAdmin();
@@ -74,10 +80,17 @@ final class DiffusionPushLogListView extends AphrontView {
         $flag_names);
 
       $reject_code = $log->getPushEvent()->getRejectCode();
-      $reject_label = idx(
-        $reject_map,
-        $reject_code,
-        pht('Unknown ("%s")', $reject_code));
+
+      if ($reject_code == $reject_herald) {
+        $rule_phid = $log->getPushEvent()->getRejectDetails();
+        $handle = $viewer->renderHandle($rule_phid);
+        $reject_label = pht('Blocked: %s', $handle);
+      } else {
+        $reject_label = idx(
+          $reject_map,
+          $reject_code,
+          pht('Unknown ("%s")', $reject_code));
+      }
 
       $rows[] = array(
         phutil_tag(
diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
index 7b626fff32..eb2bc6b45b 100644
--- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
+++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
@@ -149,6 +149,9 @@ final class PhabricatorRepositoryPushLogSearchEngine
       id(new PhabricatorStringExportField())
         ->setKey('resultName')
         ->setLabel(pht('Result Name')),
+      id(new PhabricatorStringExportField())
+        ->setKey('resultDetails')
+        ->setLabel(pht('Result Details')),
       id(new PhabricatorIntExportField())
         ->setKey('writeWait')
         ->setLabel(pht('Write Wait (us)')),
@@ -237,6 +240,7 @@ final class PhabricatorRepositoryPushLogSearchEngine
         'flagNames' => $flag_names,
         'result' => $result,
         'resultName' => $result_name,
+        'resultDetails' => $event->getRejectDetails(),
         'writeWait' => $event->getWriteWait(),
         'readWait' => $event->getReadWait(),
         'hostWait' => $event->getHostWait(),

From 06380e8079dec5170bcd3ec47e8e6825dad22d72 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 1 Aug 2018 12:39:56 -0700
Subject: [PATCH 11/18] Allow push events to be filtered by which Herald rule
 blocked the push

Summary: Depends on D19555. Ref T13164. See PHI765. An install is interested in getting a sense of the impact of a particular blocking rule, which seems reasonable. Support filtering for pushes blocked by a particular rule or set of rules.

Test Plan: {F5776385}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19556
---
 .../PhabricatorRepositoryPushLogQuery.php     | 58 ++++++++++++++++---
 ...abricatorRepositoryPushLogSearchEngine.php | 14 ++++-
 .../PhabricatorRepositoryPushEvent.php        |  3 +
 3 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php
index cb097fae2f..d4734f61e6 100644
--- a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php
+++ b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php
@@ -12,6 +12,7 @@ final class PhabricatorRepositoryPushLogQuery
   private $pushEventPHIDs;
   private $epochMin;
   private $epochMax;
+  private $blockingHeraldRulePHIDs;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -54,6 +55,11 @@ final class PhabricatorRepositoryPushLogQuery
     return $this;
   }
 
+  public function withBlockingHeraldRulePHIDs(array $phids) {
+    $this->blockingHeraldRulePHIDs = $phids;
+    return $this;
+  }
+
   public function newResultObject() {
     return new PhabricatorRepositoryPushLog();
   }
@@ -89,71 +95,105 @@ final class PhabricatorRepositoryPushLogQuery
     if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn,
-        'id IN (%Ld)',
+        'log.id IN (%Ld)',
         $this->ids);
     }
 
     if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
-        'phid IN (%Ls)',
+        'log.phid IN (%Ls)',
         $this->phids);
     }
 
     if ($this->repositoryPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'repositoryPHID IN (%Ls)',
+        'log.repositoryPHID IN (%Ls)',
         $this->repositoryPHIDs);
     }
 
     if ($this->pusherPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'pusherPHID in (%Ls)',
+        'log.pusherPHID in (%Ls)',
         $this->pusherPHIDs);
     }
 
     if ($this->pushEventPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'pushEventPHID in (%Ls)',
+        'log.pushEventPHID in (%Ls)',
         $this->pushEventPHIDs);
     }
 
     if ($this->refTypes !== null) {
       $where[] = qsprintf(
         $conn,
-        'refType IN (%Ls)',
+        'log.refType IN (%Ls)',
         $this->refTypes);
     }
 
     if ($this->newRefs !== null) {
       $where[] = qsprintf(
         $conn,
-        'refNew IN (%Ls)',
+        'log.refNew IN (%Ls)',
         $this->newRefs);
     }
 
     if ($this->epochMin !== null) {
       $where[] = qsprintf(
         $conn,
-        'epoch >= %d',
+        'log.epoch >= %d',
         $this->epochMin);
     }
 
     if ($this->epochMax !== null) {
       $where[] = qsprintf(
         $conn,
-        'epoch <= %d',
+        'log.epoch <= %d',
         $this->epochMax);
     }
 
+    if ($this->blockingHeraldRulePHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        '(event.rejectCode = %d AND event.rejectDetails IN (%Ls))',
+        PhabricatorRepositoryPushLog::REJECT_HERALD,
+        $this->blockingHeraldRulePHIDs);
+    }
+
     return $where;
   }
 
+  protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
+    $joins = parent::buildJoinClauseParts($conn);
+
+    if ($this->shouldJoinPushEventTable()) {
+      $joins[] = qsprintf(
+        $conn,
+        'JOIN %T event ON event.phid = log.pushEventPHID',
+        id(new PhabricatorRepositoryPushEvent())->getTableName());
+    }
+
+    return $joins;
+  }
+
+  private function shouldJoinPushEventTable() {
+    if ($this->blockingHeraldRulePHIDs !== null) {
+      return true;
+    }
+
+    return false;
+  }
+
   public function getQueryApplicationClass() {
     return 'PhabricatorDiffusionApplication';
   }
 
+  protected function getPrimaryTableAlias() {
+    return 'log';
+  }
+
+
 }
diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
index eb2bc6b45b..87b2e44740 100644
--- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
+++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
@@ -32,6 +32,10 @@ final class PhabricatorRepositoryPushLogSearchEngine
         $map['createdEnd']);
     }
 
+    if ($map['blockingHeraldRulePHIDs']) {
+      $query->withBlockingHeraldRulePHIDs($map['blockingHeraldRulePHIDs']);
+    }
+
     return $query;
   }
 
@@ -43,13 +47,19 @@ final class PhabricatorRepositoryPushLogSearchEngine
         ->setAliases(array('repository', 'repositories', 'repositoryPHID'))
         ->setLabel(pht('Repositories'))
         ->setDescription(
-          pht('Search for pull logs for specific repositories.')),
+          pht('Search for push logs for specific repositories.')),
       id(new PhabricatorUsersSearchField())
         ->setKey('pusherPHIDs')
         ->setAliases(array('pusher', 'pushers', 'pusherPHID'))
         ->setLabel(pht('Pushers'))
         ->setDescription(
-          pht('Search for pull logs by specific users.')),
+          pht('Search for push logs by specific users.')),
+      id(new PhabricatorSearchDatasourceField())
+        ->setDatasource(new HeraldRuleDatasource())
+        ->setKey('blockingHeraldRulePHIDs')
+        ->setLabel(pht('Blocked By'))
+        ->setDescription(
+          pht('Search for pushes blocked by particular Herald rules.')),
       id(new PhabricatorSearchDateField())
         ->setLabel(pht('Created After'))
         ->setKey('createdStart'),
diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php
index e44f99df4d..682b367926 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php
@@ -49,6 +49,9 @@ final class PhabricatorRepositoryPushEvent
         'key_identifier' => array(
           'columns' => array('requestIdentifier'),
         ),
+        'key_reject' => array(
+          'columns' => array('rejectCode', 'rejectDetails'),
+        ),
       ),
     ) + parent::getConfiguration();
   }

From e72296f9274d6ddfbcfb9e632986bdbb2d255c4b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 1 Aug 2018 12:45:00 -0700
Subject: [PATCH 12/18] Support querying Herald rules by monogram in typeahead
 datsources

Summary:
Depends on D19556. See PHI765. Ref T13164. Currently, if you type `H1` in this datasource, it isn't smart enough to pull up the right object.

Add support for querying by monogram. This is similar to existing support in Owners packages, etc.

Test Plan: Typed `H1` in the new push log filter, got the right object as a result in the typeahead.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19557
---
 .../herald/typeahead/HeraldRuleDatasource.php    | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/applications/herald/typeahead/HeraldRuleDatasource.php b/src/applications/herald/typeahead/HeraldRuleDatasource.php
index c2db05c9ea..88343bd433 100644
--- a/src/applications/herald/typeahead/HeraldRuleDatasource.php
+++ b/src/applications/herald/typeahead/HeraldRuleDatasource.php
@@ -19,10 +19,18 @@ final class HeraldRuleDatasource
     $viewer = $this->getViewer();
     $raw_query = $this->getRawQuery();
 
-    $rules = id(new HeraldRuleQuery())
-      ->setViewer($viewer)
-      ->withDatasourceQuery($raw_query)
-      ->execute();
+    $query = id(new HeraldRuleQuery())
+      ->setViewer($viewer);
+
+    if (preg_match('/^[hH]\d+\z/', $raw_query)) {
+      $id = trim($raw_query, 'hH');
+      $id = (int)$id;
+      $query->withIDs(array($id));
+    } else {
+      $query->withDatasourceQuery($raw_query);
+    }
+
+    $rules = $query->execute();
 
     $handles = id(new PhabricatorHandleQuery())
       ->setViewer($viewer)

From 4c09e88c95829ad629c86e4b58e1be069fd0d018 Mon Sep 17 00:00:00 2001
From: Arturas Moskvinas <arturas@uber.com>
Date: Wed, 1 Aug 2018 14:42:52 +0300
Subject: [PATCH 13/18] Add parsing for ssh options (-o) which are passed when
 using GIT v2 wire protocol by git command (SSH transport)

Summary:
Makes `ssh-connect` compatible with Git v2 wire protocol over SSH

More details about git V2 wire: https://opensource.googleblog.com/2018/05/introducing-git-protocol-version-2.html

`git` command (2.18+) passes extra options (`-o "SendEnv GIT_PROTOCOL"`) to underlying `ssh` command to enable v2 wire protocol (environment variable enabling new protocol).

Phabricator `ssh-connect` command doesn't understand `-o` options and interprets it as host parts hence when you enable git v2 all clones/ls-remotes crash with:
```
#0 ExecFuture::resolvex() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:525]
#1 PhabricatorRepository::execxRemoteCommand(string, PhutilOpaqueEnvelope) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:400]
#2 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343]
#3 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126]
#4 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40]
#5 PhabricatorRepositoryPullEngine::pullRepository() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59]
#6 PhabricatorRepositoryManagementUpdateWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:441]
#7 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:333]
#8 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/repository/manage_repositories.php:22]
COMMAND
git ls-remote '********'
STDOUT
(empty)
STDERR
ssh: Could not resolve hostname -o: Name or service not known
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
at [<phutil>/src/future/exec/ExecFuture.php:369]
```

Test Plan:
How to reproduce:
1. add repository to Phabricator which is accessed via `ssh`
2. Use git 2.18+
3. Enable wire protocol in `/etc/gitconfig`:
```
[protocol]
    version = 2
```
4. Try refreshing repository: `phabricator/bin/repository update somecallsing`
5. Repository update fails with `ssh: Could not resolve hostname -o: Name or service not known`

after this changes - updates will succeed

Reviewers: epriestley, Pawka, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19542
---
 scripts/ssh/ssh-connect.php | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/scripts/ssh/ssh-connect.php b/scripts/ssh/ssh-connect.php
index 9757f3cbbe..0d87f16973 100755
--- a/scripts/ssh/ssh-connect.php
+++ b/scripts/ssh/ssh-connect.php
@@ -27,7 +27,15 @@ $args->parsePartial(
       'param' => pht('port'),
       'help' => pht('Port number to connect to.'),
     ),
+    array(
+      'name' => 'options',
+      'short' => 'o',
+      'param' => pht('options'),
+      'repeat' => true,
+      'help' => pht('SSH options.'),
+    ),
   ));
+
 $unconsumed_argv = $args->getUnconsumedArgumentVector();
 
 if (function_exists('pcntl_signal')) {
@@ -113,6 +121,25 @@ if ($port) {
   $arguments[] = $port;
 }
 
+$options = $args->getArg('options');
+$allowed_ssh_options = array('SendEnv=GIT_PROTOCOL');
+
+if (!empty($options)) {
+  foreach ($options as $option) {
+    if (array_search($option, $allowed_ssh_options) !== false) {
+      $pattern[] = '-o %s';
+      $arguments[] = $option;
+    } else {
+      throw new Exception(
+        pht(
+          'Disallowed ssh option "%s" given with "-o". '.
+          'Allowed options are: %s.',
+          $option,
+          implode(', ', $allowed_ssh_options)));
+    }
+  }
+}
+
 $pattern[] = '--';
 
 $pattern[] = '%s';

From 356b2781bcadf43aaf7e4d8086fcc8ab867a38cb Mon Sep 17 00:00:00 2001
From: Arturas Moskvinas <arturas@uber.com>
Date: Thu, 2 Aug 2018 17:41:40 +0300
Subject: [PATCH 14/18] Gracefully fail request if non existing callsign is
 passed to getrecentcommitsbypath instead of crashing

Summary:
`diffusion.getrecentcommitsbypath` fails with 500 error when non existing callsign is passed:
```
>>> UNRECOVERABLE FATAL ERROR <<<

Call to a member function getCommit() on null

```

Expected Behavior:
Return more graceful error notifying caller that such callsign/repository does not exist

Reproduction steps:
Open conduit: https://secure.phabricator.com/conduit/method/diffusion.getrecentcommitsbypath/
Enter:
callsign: "obviouslynotexisting"
path: "/random"
Click call method

Test Plan: after applying patch - call no longer fails with 500s

Reviewers: Pawka, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19558
---
 ...DiffusionGetRecentCommitsByPathConduitAPIMethod.php | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php
index 002805ac4e..65bc5f69da 100644
--- a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php
@@ -23,6 +23,12 @@ final class DiffusionGetRecentCommitsByPathConduitAPIMethod
     );
   }
 
+  protected function defineErrorTypes() {
+    return array(
+      'ERR_NOT_FOUND' => pht('Repository was not found.'),
+    );
+  }
+
   protected function defineReturnType() {
     return 'nonempty list<string>';
   }
@@ -36,6 +42,10 @@ final class DiffusionGetRecentCommitsByPathConduitAPIMethod
         'branch' => $request->getValue('branch'),
       ));
 
+    if ($drequest === null) {
+        throw new ConduitException('ERR_NOT_FOUND');
+    }
+
     $limit = nonempty(
       $request->getValue('limit'),
       self::DEFAULT_LIMIT);

From 3574a55a95bda7d852dacd3082bfa312d610f615 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 2 Aug 2018 16:33:41 -0700
Subject: [PATCH 15/18] Deprecate Conduit method
 "diffusion.getrecentcommitsbypath"

Summary:
See D19558. This method has no callers and just wraps `diffusion.historyquery`, since D5960 (2013).

This was introduced in D315 (which didn't make it out of FB, I think) inside Facebook for unclear purposes in 2011.

Test Plan: Grepped for callers, found none.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: artms

Differential Revision: https://secure.phabricator.com/D19559
---
 .../DiffusionGetRecentCommitsByPathConduitAPIMethod.php   | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php
index 65bc5f69da..286831f82b 100644
--- a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php
@@ -9,6 +9,14 @@ final class DiffusionGetRecentCommitsByPathConduitAPIMethod
     return 'diffusion.getrecentcommitsbypath';
   }
 
+  public function getMethodStatus() {
+    return self::METHOD_STATUS_DEPRECATED;
+  }
+
+  public function getMethodStatusDescription() {
+    return pht('Obsoleted by "diffusion.historyquery".');
+  }
+
   public function getMethodDescription() {
     return pht(
       'Get commit identifiers for recent commits affecting a given path.');

From f3fa164882eab06ef10cfdfcacb82248962d9f80 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 3 Aug 2018 12:15:29 -0700
Subject: [PATCH 16/18] Add a "Last Edited" property to Wiki pages

Summary:
Ref T13164. See PHI797. The last edit is available in the page header, but it's not precise (just says "180 days ago") and a little weird (it's unusual for us to put that kind of information in the header).

Add a precise timestamp to the footer for now. I'd imagine re-examining this the next time Phriction gets some UI work and maybe trying to integrate timeline/transactions more cleanly (see also T1894).

Test Plan: Looked at a wiki page, then edited it. Saw precise "Last Edit" timestamp adjacent to "Last Author".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19560
---
 .../phriction/controller/PhrictionDocumentController.php   | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index 8377cf7d29..c8ffb86ce8 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -254,7 +254,8 @@ final class PhrictionDocumentController
     PhrictionContent $content,
     $slug) {
 
-    $viewer = $this->getRequest()->getUser();
+    $viewer = $this->getViewer();
+
     $view = id(new PHUIPropertyListView())
       ->setUser($viewer)
       ->setObject($document);
@@ -263,6 +264,10 @@ final class PhrictionDocumentController
       pht('Last Author'),
       $viewer->renderHandle($content->getAuthorPHID()));
 
+    $view->addProperty(
+      pht('Last Edited'),
+      phabricator_datetime($content->getDateCreated(), $viewer));
+
     return $view;
   }
 

From 5839a54b604d972c11192105896124da38fb702a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 3 Aug 2018 13:02:23 -0700
Subject: [PATCH 17/18] Raise a tailored error when calling
 "transaction.search" with empty "phids" constraint

Summary:
Ref T13164. See PHI725. For real "*.search" methods, parameters get validated and you get an error if you use an empty list as a constraint.

Since "transaction.search" isn't really a normal "*.search" method, it doesn't benefit from this. Just do the check manually for now.

Test Plan: Made `transaction.search` calls with no constraints (got results); a valid costraint (got fewer results); and an invalid empty constraint (got an exception).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19562
---
 .../conduit/TransactionSearchConduitAPIMethod.php         | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
index 3b040ea2c3..0394432ff3 100644
--- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
+++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
@@ -79,6 +79,14 @@ final class TransactionSearchConduitAPIMethod
       ));
 
     $with_phids = idx($constraints, 'phids');
+
+    if ($with_phids === array()) {
+      throw new Exception(
+        pht(
+          'Constraint "phids" to "transaction.search" requires nonempty list, '.
+          'empty list provided.'));
+    }
+
     if ($with_phids) {
       $xaction_query->withPHIDs($with_phids);
     }

From 91abc0f027209908c4d9eff824ea7445e0ca5555 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 3 Aug 2018 13:38:24 -0700
Subject: [PATCH 18/18] Stop indexing the chunk data objects for large Files
 stored in multiple chunks

Summary:
Ref T13164. See PHI766. Currently, when file data is stored in small chunks, we submit each chunk to the indexing engine.

However, chunks are never surfaced directly and can never be found via any search/query, so this work is pointless. Just skip it.

(It would be nice to do this a little more formally on `IndexableInterface` or similar as `isThisAnIndexableObject()`, but we'd have to add like a million empty "yes, index this always" methods to do that, and it seems unlikely that we'll end up with too many other objects like these.)

Test Plan:
  - Ran `bin/harbormaster rebuild-log --id ... --force` before and after change, saw about 200 fewer queries after the change.
  - Uploaded a uniquely named file and searched for it to make sure I didn't break that.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19563
---
 src/applications/files/storage/PhabricatorFile.php | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 1517b4435b..5109202a33 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -159,10 +159,22 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
   public function saveAndIndex() {
     $this->save();
-    PhabricatorSearchWorker::queueDocumentForIndexing($this->getPHID());
+
+    if ($this->isIndexableFile()) {
+      PhabricatorSearchWorker::queueDocumentForIndexing($this->getPHID());
+    }
+
     return $this;
   }
 
+  private function isIndexableFile() {
+    if ($this->getIsChunk()) {
+      return false;
+    }
+
+    return true;
+  }
+
   public function getMonogram() {
     return 'F'.$this->getID();
   }