From d2316e8025689dd14df645367c1ed22cbad3fd87 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Nov 2018 04:38:11 -0800
Subject: [PATCH 01/16] Fix an errant "switch ... continue"

Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-on-create-task/2062>.

This construction has the same behavior as "switch ... break" but is unconventional. PHP 7.3 started warning about it because it's likely a mistake.

Test Plan: Created a task, edited a task owner. The new code is functionally identical to the old code.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19772
---
 .../maniphest/editor/ManiphestTransactionEditor.php             | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
index 7c77e4acee..66c523573f 100644
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -290,7 +290,7 @@ final class ManiphestTransactionEditor
           $copy->setOwnerPHID($xaction->getNewValue());
           break;
         default:
-          continue;
+          break;
       }
     }
 

From 798a391e5a9031876c8e9fef471ad156428ae439 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 26 Oct 2018 07:56:15 -0700
Subject: [PATCH 02/16] Add test coverage for "%R" in qsprintf() and convert
 LiskDAO to support it

Summary:
Ref T13210. Ref T11908. Add some basic test coverage for the new "%R" introduced in D19764, then convert LiskDAO to implement the "Database + Table Ref" interface.

To move forward, we need to convert all of these (where `%T` is not a table alias):

```counterexample
qsprintf($conn, '... %T ...', $thing->getTableName());
```

...to these:

```
qsprintf($conn, '... %R ...', $thing);
```

The new code is a little simpler (no `->getTableName()` call) which is sort of nice. But we also have a //lot// of `%T` so this is probably going to take a while.

(I'll hold this until after the release cut.)

Test Plan:
  - Ran unit tests.
  - Browsed around and edited some objects without issues. This change causes a reasonably large percentage of our total queries to use the new code since the LiskDAO builtin queries are some of the most commonly-constructed queries, although there are still ~700 callsites which need to be examined for possible conversion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13210, T11908

Differential Revision: https://secure.phabricator.com/D19765
---
 src/__phutil_library_map__.php                |  5 ++-
 .../__tests__/QueryFormattingTestCase.php     | 35 ++++++++++++------
 src/infrastructure/storage/lisk/LiskDAO.php   | 37 ++++++++++++++-----
 .../storage/lisk/PhabricatorLiskDAO.php       |  3 +-
 .../lisk/__tests__/LiskIsolationTestDAO.php   |  2 +-
 5 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 7a9ea1df70..11a939c489 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -7131,7 +7131,10 @@ phutil_register_library_map(array(
     'LegalpadTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'LegalpadTransactionView' => 'PhabricatorApplicationTransactionView',
     'LiskChunkTestCase' => 'PhabricatorTestCase',
-    'LiskDAO' => 'Phobject',
+    'LiskDAO' => array(
+      'Phobject',
+      'AphrontDatabaseTableRefInterface',
+    ),
     'LiskDAOSet' => 'Phobject',
     'LiskDAOTestCase' => 'PhabricatorTestCase',
     'LiskEphemeralObjectException' => 'Exception',
diff --git a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php b/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php
index 6bd0eeedaf..53d16a7948 100644
--- a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php
+++ b/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php
@@ -3,23 +3,23 @@
 final class QueryFormattingTestCase extends PhabricatorTestCase {
 
   public function testQueryFormatting() {
-    $conn_r = id(new PhabricatorUser())->establishConnection('r');
+    $conn = id(new PhabricatorUser())->establishConnection('r');
 
     $this->assertEqual(
       'NULL',
-      qsprintf($conn_r, '%nd', null));
+      qsprintf($conn, '%nd', null));
 
     $this->assertEqual(
       '0',
-      qsprintf($conn_r, '%nd', 0));
+      qsprintf($conn, '%nd', 0));
 
     $this->assertEqual(
       '0',
-      qsprintf($conn_r, '%d', 0));
+      qsprintf($conn, '%d', 0));
 
     $raised = null;
     try {
-      qsprintf($conn_r, '%d', 'derp');
+      qsprintf($conn, '%d', 'derp');
     } catch (Exception $ex) {
       $raised = $ex;
     }
@@ -29,27 +29,40 @@ final class QueryFormattingTestCase extends PhabricatorTestCase {
 
     $this->assertEqual(
       "'<S>'",
-      qsprintf($conn_r, '%s', null));
+      qsprintf($conn, '%s', null));
 
     $this->assertEqual(
       'NULL',
-      qsprintf($conn_r, '%ns', null));
+      qsprintf($conn, '%ns', null));
 
     $this->assertEqual(
       "'<S>', '<S>'",
-      qsprintf($conn_r, '%Ls', array('x', 'y')));
+      qsprintf($conn, '%Ls', array('x', 'y')));
 
     $this->assertEqual(
       "'<B>'",
-      qsprintf($conn_r, '%B', null));
+      qsprintf($conn, '%B', null));
 
     $this->assertEqual(
       'NULL',
-      qsprintf($conn_r, '%nB', null));
+      qsprintf($conn, '%nB', null));
 
     $this->assertEqual(
       "'<B>', '<B>'",
-      qsprintf($conn_r, '%LB', array('x', 'y')));
+      qsprintf($conn, '%LB', array('x', 'y')));
+
+    $this->assertEqual(
+      '<C>',
+      qsprintf($conn, '%T', 'x'));
+
+    $this->assertEqual(
+      '<C>',
+      qsprintf($conn, '%C', 'y'));
+
+    $this->assertEqual(
+      '<C>.<C>',
+      qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y')));
+
   }
 
 
diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php
index 0c940ef5f0..583d8226f6 100644
--- a/src/infrastructure/storage/lisk/LiskDAO.php
+++ b/src/infrastructure/storage/lisk/LiskDAO.php
@@ -162,7 +162,8 @@
  * @task   xaction Managing Transactions
  * @task   isolate Isolation for Unit Testing
  */
-abstract class LiskDAO extends Phobject {
+abstract class LiskDAO extends Phobject
+  implements AphrontDatabaseTableRefInterface {
 
   const CONFIG_IDS                  = 'id-mechanism';
   const CONFIG_TIMESTAMPS           = 'timestamps';
@@ -235,8 +236,11 @@ abstract class LiskDAO extends Phobject {
    * @return string Connection namespace for cache
    * @task conn
    */
-  abstract protected function getConnectionNamespace();
+  protected function getConnectionNamespace() {
+    return $this->getDatabaseName();
+  }
 
+  abstract protected function getDatabaseName();
 
   /**
    * Get an existing, cached connection for this object.
@@ -525,8 +529,8 @@ abstract class LiskDAO extends Phobject {
     $args = func_get_args();
     $args = array_slice($args, 1);
 
-    $pattern = 'SELECT * FROM %T WHERE '.$pattern.' %Q';
-    array_unshift($args, $this->getTableName());
+    $pattern = 'SELECT * FROM %R WHERE '.$pattern.' %Q';
+    array_unshift($args, $this);
     array_push($args, $lock_clause);
     array_unshift($args, $pattern);
 
@@ -1150,8 +1154,8 @@ abstract class LiskDAO extends Phobject {
 
     $id = $this->getID();
     $conn->query(
-      'UPDATE %T SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'),
-      $this->getTableName(),
+      'UPDATE %R SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'),
+      $this,
       $map,
       $this->getIDKeyForUse(),
       $id);
@@ -1178,8 +1182,8 @@ abstract class LiskDAO extends Phobject {
 
     $conn = $this->establishConnection('w');
     $conn->query(
-      'DELETE FROM %T WHERE %C = %d',
-      $this->getTableName(),
+      'DELETE FROM %R WHERE %C = %d',
+      $this,
       $this->getIDKeyForUse(),
       $this->getID());
 
@@ -1255,9 +1259,9 @@ abstract class LiskDAO extends Phobject {
     $data = implode(', ', $data);
 
     $conn->query(
-      '%Q INTO %T (%LC) VALUES (%Q)',
+      '%Q INTO %R (%LC) VALUES (%Q)',
       $mode,
-      $this->getTableName(),
+      $this,
       $columns,
       $data);
 
@@ -2019,4 +2023,17 @@ abstract class LiskDAO extends Phobject {
       ->getMaximumByteLengthForDataType($data_type);
   }
 
+
+/* -(  AphrontDatabaseTableRefInterface  )----------------------------------- */
+
+
+  public function getAphrontRefDatabaseName() {
+    return $this->getDatabaseName();
+  }
+
+  public function getAphrontRefTableName() {
+    return $this->getTableName();
+  }
+
+
 }
diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
index b3b324e951..b300efbf4b 100644
--- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
+++ b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
@@ -187,11 +187,10 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
    */
   abstract public function getApplicationName();
 
-  protected function getConnectionNamespace() {
+  protected function getDatabaseName() {
     return self::getStorageNamespace().'_'.$this->getApplicationName();
   }
 
-
   /**
    * Break a list of escaped SQL statement fragments (e.g., VALUES lists for
    * INSERT, previously built with @{function:qsprintf}) into chunks which will
diff --git a/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php b/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php
index d9070753f8..8008fb8e46 100644
--- a/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php
+++ b/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php
@@ -22,7 +22,7 @@ final class LiskIsolationTestDAO extends LiskDAO {
         'resource!'));
   }
 
-  protected function getConnectionNamespace() {
+  protected function getDatabaseName() {
     return 'test';
   }
 

From 5e1d94f33651e11079a1fd331637c890717d2538 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Nov 2018 10:08:20 -0800
Subject: [PATCH 03/16] Remove nonfunctional AJAX embed behavior for Slowvote

Summary:
See <https://hackerone.com/reports/434116>. Slowvote has a piece of Javascript that attempts to let you vote on `{V123}` polls inline.

It does not work: nothing ever triggers it (nothing renders a control with a `slowvote-option` sigil).

At least for now, just remove it. It has a completely separate pathway in the controller and both pathways are buggy, so this makes fixing them easier.

Test Plan: Voted in plurality and approval polls via Slowvote and the embedded widget.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19773
---
 resources/celerity/map.php                    |  8 ----
 .../PhabricatorSlowvoteVoteController.php     | 38 ----------------
 .../slowvote/view/SlowvoteEmbedView.php       |  6 ---
 .../slowvote/behavior-slowvote-embed.js       | 43 -------------------
 4 files changed, 95 deletions(-)
 delete mode 100644 webroot/rsrc/js/application/slowvote/behavior-slowvote-embed.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index b6817f97eb..f812e17517 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -422,7 +422,6 @@ return array(
     'rsrc/js/application/repository/repository-crossreference.js' => '9a860428',
     'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072',
     'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08',
-    'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f',
     'rsrc/js/application/transactions/behavior-comment-actions.js' => '038bf27f',
     'rsrc/js/application/transactions/behavior-reorder-configs.js' => 'd7a74243',
     'rsrc/js/application/transactions/behavior-reorder-fields.js' => 'b59e1e96',
@@ -674,7 +673,6 @@ return array(
     'javelin-behavior-select-content' => 'bf5374ef',
     'javelin-behavior-select-on-click' => '4e3e79a6',
     'javelin-behavior-setup-check-https' => '491416b3',
-    'javelin-behavior-slowvote-embed' => '887ad43f',
     'javelin-behavior-stripe-payment-form' => 'a6b98425',
     'javelin-behavior-test-payment-form' => 'fc91ab6c',
     'javelin-behavior-time-typeahead' => '522431f7',
@@ -1550,12 +1548,6 @@ return array(
       'phabricator-keyboard-shortcut',
       'javelin-stratcom',
     ),
-    '887ad43f' => array(
-      'javelin-behavior',
-      'javelin-request',
-      'javelin-stratcom',
-      'javelin-dom',
-    ),
     '8935deef' => array(
       'javelin-install',
       'javelin-dom',
diff --git a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
index c69a825c3a..53fbc0102d 100644
--- a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
+++ b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
@@ -25,44 +25,6 @@ final class PhabricatorSlowvoteVoteController
 
     $old_votes = mpull($viewer_choices, null, 'getOptionID');
 
-    if ($request->isAjax()) {
-      $vote = $request->getInt('vote');
-      $votes = array_keys($old_votes);
-      $votes = array_fuse($votes);
-
-      if ($poll->getMethod() == PhabricatorSlowvotePoll::METHOD_PLURALITY) {
-        if (idx($votes, $vote, false)) {
-          $votes = array();
-        } else {
-          $votes = array($vote);
-        }
-      } else {
-        if (idx($votes, $vote, false)) {
-          unset($votes[$vote]);
-        } else {
-          $votes[$vote] = $vote;
-        }
-      }
-
-      $this->updateVotes($viewer, $poll, $old_votes, $votes);
-
-      $updated_choices = id(new PhabricatorSlowvoteChoice())->loadAllWhere(
-        'pollID = %d AND authorPHID = %s',
-        $poll->getID(),
-        $viewer->getPHID());
-
-      $embed = id(new SlowvoteEmbedView())
-        ->setPoll($poll)
-        ->setOptions($options)
-        ->setViewerChoices($updated_choices);
-
-      return id(new AphrontAjaxResponse())
-        ->setContent(array(
-          'pollID' => $poll->getID(),
-          'contentHTML' => $embed->render(),
-        ));
-    }
-
     if (!$request->isFormPost()) {
       return id(new Aphront404Response());
     }
diff --git a/src/applications/slowvote/view/SlowvoteEmbedView.php b/src/applications/slowvote/view/SlowvoteEmbedView.php
index 452cfbe9eb..1183f3b1cd 100644
--- a/src/applications/slowvote/view/SlowvoteEmbedView.php
+++ b/src/applications/slowvote/view/SlowvoteEmbedView.php
@@ -39,12 +39,6 @@ final class SlowvoteEmbedView extends AphrontView {
     }
 
     require_celerity_resource('phabricator-slowvote-css');
-    require_celerity_resource('javelin-behavior-slowvote-embed');
-
-    $config = array(
-      'pollID' => $poll->getID(),
-    );
-    Javelin::initBehavior('slowvote-embed', $config);
 
     $user_choices = $poll->getViewerChoices($this->getUser());
     $user_choices = mpull($user_choices, 'getOptionID', 'getOptionID');
diff --git a/webroot/rsrc/js/application/slowvote/behavior-slowvote-embed.js b/webroot/rsrc/js/application/slowvote/behavior-slowvote-embed.js
deleted file mode 100644
index 72e52e1f0c..0000000000
--- a/webroot/rsrc/js/application/slowvote/behavior-slowvote-embed.js
+++ /dev/null
@@ -1,43 +0,0 @@
-/**
- * @provides javelin-behavior-slowvote-embed
- * @requires javelin-behavior
- *           javelin-request
- *           javelin-stratcom
- *           javelin-dom
- */
-JX.behavior('slowvote-embed', function() {
-  JX.Stratcom.listen(
-    ['click'],
-    'slowvote-option',
-    function(e) {
-      if (!e.isNormalMouseEvent()) {
-        return;
-      }
-      e.kill();
-
-      var pollID = e.getNodeData('slowvote-embed').pollID;
-      var voteURI = '/vote/' + pollID + '/';
-
-      var request = new JX.Request(voteURI, function(r) {
-        var updated_poll = JX.$H(r.contentHTML);
-        var root = JX.$('phabricator-standard-page');
-
-        var polls = JX.DOM.scry(root, 'div', 'slowvote-embed');
-
-        for(var i = 0; i < polls.length; i++) {
-          var data = JX.Stratcom.getData(polls[i]);
-
-          if (data.pollID == pollID) {
-            JX.DOM.replace(polls[i], updated_poll);
-          }
-
-        }
-
-      });
-
-      request.addData({vote: e.getNodeData('slowvote-option').optionID});
-      request.send();
-
-    });
-
-});

From d38e768ed87787016eea78612ef79ac4cc119392 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Nov 2018 10:19:50 -0800
Subject: [PATCH 04/16] Prevent users from voting for invalid Slowvote options

Summary:
Depends on D19773. See <https://hackerone.com/reports/434116>. You can currently vote for invalid options by submitting, e.g., `vote[]=12345`.

By doing this, you can see the responses, which is sort of theoretically a security problem? This is definitely a bug, regardless.

Instead, only allow users to vote for options which are actually part of the poll.

Test Plan:
  - Tried to vote for invalid options by editing the form to `vote[]=12345` (got error).
  - Tried to vote for invalid options by editing the radio buttons on a plurality poll into checkboxes, checking multiple boxes, and submitting (got error).
  - Voted in approval and plurality polls the right way, from the main web UI and from the embed (`{V...}`) UI.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19774
---
 .../PhabricatorSlowvoteVoteController.php     | 38 ++++++++++++-------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
index 53fbc0102d..62913b09e3 100644
--- a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
+++ b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
@@ -7,6 +7,10 @@ final class PhabricatorSlowvoteVoteController
     $viewer = $request->getViewer();
     $id = $request->getURIData('id');
 
+    if (!$request->isFormPost()) {
+      return id(new Aphront404Response());
+    }
+
     $poll = id(new PhabricatorSlowvoteQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
@@ -16,31 +20,36 @@ final class PhabricatorSlowvoteVoteController
     if (!$poll) {
       return new Aphront404Response();
     }
+
     if ($poll->getIsClosed()) {
       return new Aphront400Response();
     }
 
     $options = $poll->getOptions();
-    $viewer_choices = $poll->getViewerChoices($viewer);
+    $options = mpull($options, null, 'getID');
 
-    $old_votes = mpull($viewer_choices, null, 'getOptionID');
-
-    if (!$request->isFormPost()) {
-      return id(new Aphront404Response());
-    }
+    $old_votes = $poll->getViewerChoices($viewer);
+    $old_votes = mpull($old_votes, null, 'getOptionID');
 
     $votes = $request->getArr('vote');
     $votes = array_fuse($votes);
 
-    $this->updateVotes($viewer, $poll, $old_votes, $votes);
+    $method = $poll->getMethod();
+    $is_plurality = ($method == PhabricatorSlowvotePoll::METHOD_PLURALITY);
 
-    return id(new AphrontRedirectResponse())->setURI('/V'.$poll->getID());
-  }
+    if ($is_plurality && count($votes) > 1) {
+      throw new Exception(
+        pht('In this poll, you may only vote for one option.'));
+    }
 
-  private function updateVotes($viewer, $poll, $old_votes, $votes) {
-    if (!empty($votes) && count($votes) > 1 &&
-        $poll->getMethod() == PhabricatorSlowvotePoll::METHOD_PLURALITY) {
-      return id(new Aphront400Response());
+    foreach ($votes as $vote) {
+      if (!isset($options[$vote])) {
+        throw new Exception(
+          pht(
+            'Option ("%s") is not a valid poll option. You may only '.
+            'vote for valid options.',
+            $vote));
+      }
     }
 
     foreach ($old_votes as $old_vote) {
@@ -60,6 +69,9 @@ final class PhabricatorSlowvoteVoteController
         ->setOptionID($vote)
         ->save();
     }
+
+    return id(new AphrontRedirectResponse())
+      ->setURI($poll->getURI());
   }
 
 }

From bbfc860c6359a419737f2ad782aa0a00dd9e1b48 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Nov 2018 04:52:59 -0800
Subject: [PATCH 05/16] Improve aesthetics of commit hook rejection message

Summary: See PHI939. Ref T13216. Make the dragon's companion animal more clearly cow-like.

Test Plan:
Before:

```
\     \__/
 \____(Oo)
 (    (--)
 //__\\
//    \\
```

After:

```
*     \__/
 \____(Oo)
 (    (..)
 //___\\
//     \\
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19775
---
 scripts/repository/commit_hook.php | 34 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php
index 07d6d7cfa2..64b7b0ec24 100755
--- a/scripts/repository/commit_hook.php
+++ b/scripts/repository/commit_hook.php
@@ -204,23 +204,23 @@ try {
 +---------------------------------------------------------------+
 |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
 +---------------------------------------------------------------+
-            \
-             \                    ^    /^
-              \                  / \  // \
-               \   |\___/|      /   \//  .\
-                \  /V  V  \__  /    //  | \ \           *----*
-                  /     /  \/_/    //   |  \  \          \   |
-                  @___@`    \/_   //    |   \   \         \/\ \
-                 0/0/|       \/_ //     |    \    \         \  \
-             0/0/0/0/|        \///      |     \     \       |  |
-          0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
-       0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
-                   ,-}        _      *-.|.-~-.           .~    ~
-  \     \__/        `/\      /                 ~-. _ .-~      /
-   \____(Oo)           *.   }            {                   /
-   (    (--)          .----~-.\        \-`                 .~
-   //__\\\\  \ DENIED!  ///.----..<        \             _ -~
-  //    \\\\               ///-._ _ _ _ _ _ _{^ - - - - ~
+             \
+              \                    ^    /^
+               \                  / \  // \
+                \   |\___/|      /   \//  .\
+                 \  /V  V  \__  /    //  | \ \           *----*
+                   /     /  \/_/    //   |  \  \          \   |
+                   @___@`    \/_   //    |   \   \         \/\ \
+                  0/0/|       \/_ //     |    \    \         \  \
+              0/0/0/0/|        \///      |     \     \       |  |
+           0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
+        0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
+                    ,-}        _      *-.|.-~-.           .~    ~
+  *     \__/         `/\      /                 ~-. _ .-~      /
+   \____(Oo)            *.   }            {                   /
+   (    (..)           .----~-.\        \-`                 .~
+   //___\\\\  \ DENIED!  ///.----..<        \             _ -~
+  //     \\\\                ///-._ _ _ _ _ _ _{^ - - - - ~
 
 EOTXT
 );

From c206b066df386fd6724df1e6eb1500ad46f0e535 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Nov 2018 09:07:30 -0800
Subject: [PATCH 06/16] When `{meme ...}` embed has no text, just use the raw
 file data unmodified

Summary:
Ref T13216. See PHI948. When you use the remarkup hint button to embed a meme with no text, you get `{meme src=X}`.

If the source is a GIF, we currently split the source apart into frame-by-frame images, process them, and stitch them back together. The end result is the same image we started with, but this process can be slow/expensive, and may timeout for sufficiently large GIFs.

Instead: when there's no text, just return the original image data.

Test Plan:
  - Used `{meme src=X}` with no text, got an image faster.
  - Used `{meme src=X, above=...}` to add text, got an attempt to add text (which didn't get very far locally since I don't have GD configured).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19777
---
 src/applications/macro/engine/PhabricatorMemeEngine.php | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php
index 3e87a304c9..7433a4e8bc 100644
--- a/src/applications/macro/engine/PhabricatorMemeEngine.php
+++ b/src/applications/macro/engine/PhabricatorMemeEngine.php
@@ -174,6 +174,15 @@ final class PhabricatorMemeEngine extends Phobject {
   private function newAssetData(PhabricatorFile $template) {
     $template_data = $template->loadFileData();
 
+    // When we aren't adding text, just return the data unmodified. This saves
+    // us from doing expensive stitching when we aren't actually making any
+    // changes to the image.
+    $above_text = $this->getAboveText();
+    $below_text = $this->getBelowText();
+    if (!strlen(trim($above_text)) && !strlen(trim($below_text))) {
+      return $template_data;
+    }
+
     $result = $this->newImagemagickAsset($template, $template_data);
     if ($result) {
       return $result;

From 1f6a4cfffe5894e7eb168813353eb3e9d59a43b3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Nov 2018 06:27:36 -0800
Subject: [PATCH 07/16] Prevent users from selecting excessively bad passwords
 based on their username or email address

Summary:
Ref T13216. We occasionally receive HackerOne reports concerned that you can select your username as a password. I suspect very few users actually do this and that this is mostly a compliance/checklist sort of issue, not a real security issue.

On this install, we have about 41,000 user accounts. Of these, 100 have their username as a password (account or VCS). A substantial subset of these are either explicitly intentional ("demo", "bugmenot") or obvious test accounts ("test" in name, or name is a nonsensical string of gibberish, or looks like "tryphab" or similar) or just a bunch of numbers (?), or clearly a "researcher" doing this on purpose (e.g., name includes "pentest" or "xss" or similar).

So I'm not sure real users are actually very inclined to do this, and we can't really ever stop them from picking awful passwords anyway. But we //can// stop researchers from reporting that this is an issue.

Don't allow users to select passwords which contain words in a blocklist: their username, real name, email addresses, or the install's domain name. These words also aren't allowed to contain the password (that is, neither your password nor your username may be a substring of the other one). We also do a little normalization to try to split apart email addresses, domains, and real names, so I can't have "evan1234" as my password.

Test Plan:
  - Added unit tests and made them pass.
  - Tried to set my password to a bunch of variations of my username / email / domain name / real name / etc, got rejected.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19776
---
 .../PhabricatorAuthPasswordTestCase.php       | 76 +++++++++++++++++++
 .../engine/PhabricatorAuthPasswordEngine.php  | 64 +++++++++++++++-
 .../PhabricatorAuthPasswordHashInterface.php  | 18 +++++
 .../people/storage/PhabricatorUser.php        | 18 +++++
 4 files changed, 175 insertions(+), 1 deletion(-)

diff --git a/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php b/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php
index 6ac616b2a1..794b0b5e22 100644
--- a/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php
+++ b/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php
@@ -99,6 +99,82 @@ final class PhabricatorAuthPasswordTestCase extends PhabricatorTestCase {
     $this->assertTrue($account_engine->isUniquePassword($password2));
   }
 
+  public function testPasswordBlocklisting() {
+    $user = $this->generateNewTestUser();
+
+    $user
+      ->setUsername('iasimov')
+      ->setRealName('Isaac Asimov')
+      ->save();
+
+    $test_type = PhabricatorAuthPassword::PASSWORD_TYPE_TEST;
+    $content_source = $this->newContentSource();
+
+    $engine = id(new PhabricatorAuthPasswordEngine())
+      ->setViewer($user)
+      ->setContentSource($content_source)
+      ->setPasswordType($test_type)
+      ->setObject($user);
+
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('account.minimum-password-length', 4);
+
+    $passwords = array(
+      'a23li432m9mdf' => true,
+
+      // Empty.
+      '' => false,
+
+      // Password length tests.
+      'xh3' => false,
+      'xh32' => true,
+
+      // In common password blocklist.
+      'password1' => false,
+
+      // Tests for the account identifier blocklist.
+      'isaac' => false,
+      'iasimov' => false,
+      'iasimov1' => false,
+      'asimov' => false,
+      'iSaAc' => false,
+      '32IASIMOV' => false,
+      'i-am-iasimov-this-is-my-long-strong-password' => false,
+      'iasimo' => false,
+
+      // These are okay: although they're visually similar, they aren't mutual
+      // substrings of any identifier.
+      'iasimo1' => true,
+      'isa1mov' => true,
+    );
+
+    foreach ($passwords as $password => $expect) {
+      $this->assertBlocklistedPassword($engine, $password, $expect);
+    }
+  }
+
+  private function assertBlocklistedPassword(
+    PhabricatorAuthPasswordEngine $engine,
+    $raw_password,
+    $expect_valid) {
+
+    $envelope_1 = new PhutilOpaqueEnvelope($raw_password);
+    $envelope_2 = new PhutilOpaqueEnvelope($raw_password);
+
+    $caught = null;
+    try {
+      $engine->checkNewPassword($envelope_1, $envelope_2);
+    } catch (PhabricatorAuthPasswordException $exception) {
+      $caught = $exception;
+    }
+
+    $this->assertEqual(
+      $expect_valid,
+      !($caught instanceof PhabricatorAuthPasswordException),
+      pht('Validity of password "%s".', $raw_password));
+  }
+
+
   public function testPasswordUpgrade() {
     $weak_hasher = new PhabricatorIteratedMD5PasswordHasher();
 
diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
index 067cea30e7..f763b0987f 100644
--- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
@@ -115,7 +115,9 @@ final class PhabricatorAuthPasswordEngine
     // revoked passwords or colliding passwords either, so we can skip these
     // checks.
 
-    if ($this->getObject()->getPHID()) {
+    $object = $this->getObject();
+
+    if ($object->getPHID()) {
       if ($this->isRevokedPassword($password)) {
         throw new PhabricatorAuthPasswordException(
           pht(
@@ -132,6 +134,66 @@ final class PhabricatorAuthPasswordEngine
           pht('Not Unique'));
       }
     }
+
+    // Prevent use of passwords which are similar to any object identifier.
+    // For example, if your username is "alincoln", your password may not be
+    // "alincoln", "lincoln", or "alincoln1".
+    $viewer = $this->getViewer();
+    $blocklist = $object->newPasswordBlocklist($viewer, $this);
+
+    // Smallest number of overlapping characters that we'll consider to be
+    // too similar.
+    $minimum_similarity = 4;
+
+    // Add the domain name to the blocklist.
+    $base_uri = PhabricatorEnv::getAnyBaseURI();
+    $base_uri = new PhutilURI($base_uri);
+    $blocklist[] = $base_uri->getDomain();
+
+    // Generate additional subterms by splitting the raw blocklist on
+    // characters like "@", " " (space), and "." to break up email addresses,
+    // readable names, and domain names into components.
+    $terms_map = array();
+    foreach ($blocklist as $term) {
+      $terms_map[$term] = $term;
+      foreach (preg_split('/[ @.]/', $term) as $subterm) {
+        $terms_map[$subterm] = $term;
+      }
+    }
+
+    // Skip very short terms: it's okay if your password has the substring
+    // "com" in it somewhere even if the install is on "mycompany.com".
+    foreach ($terms_map as $term => $source) {
+      if (strlen($term) < $minimum_similarity) {
+        unset($terms_map[$term]);
+      }
+    }
+
+    // Normalize terms for comparison.
+    $normal_map = array();
+    foreach ($terms_map as $term => $source) {
+      $term = phutil_utf8_strtolower($term);
+      $normal_map[$term] = $source;
+    }
+
+    // Finally, make sure that none of the terms appear in the password,
+    // and that the password does not appear in any of the terms.
+    $normal_password = phutil_utf8_strtolower($raw_password);
+    if (strlen($normal_password) >= $minimum_similarity) {
+      foreach ($normal_map as $term => $source) {
+        if (strpos($term, $normal_password) === false &&
+            strpos($normal_password, $term) === false) {
+          continue;
+        }
+
+        throw new PhabricatorAuthPasswordException(
+          pht(
+            'The password you entered is very similar to a nonsecret account '.
+            'identifier (like a username or email address). Choose a more '.
+            'distinct password.'),
+          pht('Not Distinct'));
+      }
+    }
   }
 
   public function isValidPassword(PhutilOpaqueEnvelope $envelope) {
diff --git a/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php b/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php
index 36a296b209..af20db3ed4 100644
--- a/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php
+++ b/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php
@@ -6,4 +6,22 @@ interface PhabricatorAuthPasswordHashInterface {
     PhutilOpaqueEnvelope $envelope,
     PhabricatorAuthPassword $password);
 
+  /**
+   * Return a list of strings which passwords associated with this object may
+   * not be similar to.
+   *
+   * This method allows you to prevent users from selecting their username
+   * as their password or picking other passwords which are trivially similar
+   * to an account or object identifier.
+   *
+   * @param PhabricatorUser The user selecting the password.
+   * @param PhabricatorAuthPasswordEngine The password engine updating a
+   *  password.
+   * @return list<string> Blocklist of nonsecret identifiers which the password
+   *  should not be similar to.
+   */
+  public function newPasswordBlocklist(
+    PhabricatorUser $viewer,
+    PhabricatorAuthPasswordEngine $engine);
+
 }
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index cd85800ab1..e2a0f9a2a2 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -1665,5 +1665,23 @@ final class PhabricatorUser
     return new PhutilOpaqueEnvelope($digest);
   }
 
+  public function newPasswordBlocklist(
+    PhabricatorUser $viewer,
+    PhabricatorAuthPasswordEngine $engine) {
+
+    $list = array();
+    $list[] = $this->getUsername();
+    $list[] = $this->getRealName();
+
+    $emails = id(new PhabricatorUserEmail())->loadAllWhere(
+      'userPHID = %s',
+      $this->getPHID());
+    foreach ($emails as $email) {
+      $list[] = $email->getAddress();
+    }
+
+    return $list;
+  }
+
 
 }

From 8a4bf386552f976b0f16907b309d6c3119f85f01 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Nov 2018 09:07:02 -0800
Subject: [PATCH 08/16] Use 160-bit TOTP keys rather than 80-bit TOTP keys

Summary:
See <https://hackerone.com/reports/435648>. We currently use 80-bit TOTP keys. The RFC suggests 128 as a minimum and recommends 160.

The math suggests that doing the hashing for an 80-bit key is hard (slightly beyond the reach of a highly motivated state actor, today) but there's no reason not to use 160 bits instead to put this completely out of reach.

See some additional discussion on the HackerOne report about enormous key sizes, number of required observations, etc.

Test Plan: Added a new 160-bit TOTP factor to Google Authenticator without issue.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19792
---
 src/applications/auth/factor/PhabricatorTOTPAuthFactor.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
index 373adfbb54..7d7aec921f 100644
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
@@ -185,7 +185,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
 
 
   public static function generateNewTOTPKey() {
-    return strtoupper(Filesystem::readRandomCharacters(16));
+    return strtoupper(Filesystem::readRandomCharacters(32));
   }
 
   public static function verifyTOTPCode(

From b645af981bbed044368d67130a2233954f7d814e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Nov 2018 05:26:30 -0800
Subject: [PATCH 09/16] Correct a missing parameter in "Outbound Mail"
 documentation

Summary: See <https://discourse.phabricator-community.org/t/documentation-error/2079>. This command is missing the configuration key.

Test Plan: Ran `bin/config set --stdin` with no other arguments, got an error about missing key. Ran with `... cluster.mailers`, got read from stdin.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19791
---
 src/docs/user/configuration/configuring_outbound_email.diviner | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/docs/user/configuration/configuring_outbound_email.diviner b/src/docs/user/configuration/configuring_outbound_email.diviner
index a33134fe4e..db04c21877 100644
--- a/src/docs/user/configuration/configuring_outbound_email.diviner
+++ b/src/docs/user/configuration/configuring_outbound_email.diviner
@@ -133,7 +133,7 @@ tricky because of shell escaping. The easiest way to do it is to use the
 Then set the value like this:
 
 ```
-phabricator/ $ ./bin/config set --stdin < mailers.json
+phabricator/ $ ./bin/config set --stdin cluster.mailers < mailers.json
 ```
 
 For alternatives and more information on configuration, see

From e09d29fb1a3eb6a4fa0f6c4e51a02747d7dd968c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Nov 2018 13:14:12 -0800
Subject: [PATCH 10/16] Clean up the workflow for some post-push logging code

Summary:
Ref T13216. When a repository is clustered, we run this cleanup code (to tell the repository to update, and log some timing information) on both nodes. Currently, we do slightly too much work, which is unnecessary and can be a bit confusing to human readers.

The double update message doesn't hurt anything, but there's no reason to write it twice.

Likewise, the second timing information update query doesn't do anything: there's no PushEvent object with the right identifier, so it just updates nothing. We don't need to run it, and it's confusing that we do.

Instead, only do these writes if we're actually the final node with the repository on it.

Test Plan: Added some logging, saw double writes/updates before the change and no doubles afterwards, with no other behavioral changes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19778
---
 src/applications/almanac/util/AlmanacKeys.php |  2 +-
 .../DiffusionGitReceivePackSSHWorkflow.php    | 27 +++++++++++--------
 .../command/DrydockSSHCommandInterface.php    |  4 +--
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/applications/almanac/util/AlmanacKeys.php b/src/applications/almanac/util/AlmanacKeys.php
index 7ed9098e97..83a4f4021b 100644
--- a/src/applications/almanac/util/AlmanacKeys.php
+++ b/src/applications/almanac/util/AlmanacKeys.php
@@ -63,7 +63,7 @@ final class AlmanacKeys extends Phobject {
     // protocol does not have a mechanism like a "Host" header.
     $username = PhabricatorEnv::getEnvConfig('cluster.instance');
     if (strlen($username)) {
-      return $username;
+//      return $username;
     }
 
     $username = PhabricatorEnv::getEnvConfig('diffusion.ssh-user');
diff --git a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php
index d48abc159e..54220f0776 100644
--- a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php
+++ b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php
@@ -30,7 +30,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow {
 
     if ($this->shouldProxy()) {
       $command = $this->getProxyCommand(true);
-      $did_synchronize = false;
+      $did_write = false;
 
       if ($device) {
         $this->writeClusterEngineLogMessage(
@@ -40,7 +40,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow {
       }
     } else {
       $command = csprintf('git-receive-pack %s', $repository->getLocalPath());
-      $did_synchronize = true;
+      $did_write = true;
       $cluster_engine->synchronizeWorkingCopyBeforeWrite();
 
       if ($device) {
@@ -60,7 +60,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow {
 
     // We've committed the write (or rejected it), so we can release the lock
     // without waiting for the client to receive the acknowledgement.
-    if ($did_synchronize) {
+    if ($did_write) {
       $cluster_engine->synchronizeWorkingCopyAfterWrite();
     }
 
@@ -69,18 +69,23 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow {
     }
 
     if (!$err) {
-      $repository->writeStatusMessage(
-        PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
-        PhabricatorRepositoryStatusMessage::CODE_OKAY);
       $this->waitForGitClient();
 
-      $host_wait_end = microtime(true);
+      // When a repository is clustered, we reach this cleanup code on both
+      // the proxy and the actual final endpoint node. Don't do more cleanup
+      // or logging than we need to.
+      if ($did_write) {
+        $repository->writeStatusMessage(
+          PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
+          PhabricatorRepositoryStatusMessage::CODE_OKAY);
 
-      $this->updatePushLogWithTimingInformation(
-        $this->getClusterEngineLogProperty('writeWait'),
-        $this->getClusterEngineLogProperty('readWait'),
-        ($host_wait_end - $host_wait_start));
+        $host_wait_end = microtime(true);
 
+        $this->updatePushLogWithTimingInformation(
+          $this->getClusterEngineLogProperty('writeWait'),
+          $this->getClusterEngineLogProperty('readWait'),
+          ($host_wait_end - $host_wait_start));
+      }
     }
 
     return $err;
diff --git a/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php b/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
index 1aab14b57b..7ebd95938b 100644
--- a/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
+++ b/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
@@ -30,8 +30,8 @@ final class DrydockSSHCommandInterface extends DrydockCommandInterface {
     $full_command = call_user_func_array('csprintf', $argv);
 
     $flags = array();
-    $flags[] = '-o';
-    $flags[] = 'LogLevel=quiet';
+    // $flags[] = '-o';
+    // $flags[] = 'LogLevel=quiet';
 
     $flags[] = '-o';
     $flags[] = 'StrictHostKeyChecking=no';

From 966db4d38ec8697e72ffb79d43810402c3061036 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Nov 2018 14:30:09 -0800
Subject: [PATCH 11/16] Add an intracluster synchronization log for cluster
 repositories

Summary:
Depends on D19778. Ref T13216. See PHI943, PHI889, et al.

We currently have a push log and a pull log, but do not separately log intracluster synchronization events. We've encountered several specific cases where having this kind of log would be helpful:

  - In PHI943, an install was accidentally aborting locks early. Having timing information in the sync log would let us identify this more quickly.
  - In PHI889, an install hit an issue with `MaxStartups` configuration in `sshd`. A log would let us identify when this is an issue.
  - In PHI889, I floated a "push the linux kernel + fetch timeout" theory. A sync log would let us see sync/fetch timeouts to confirm this being a problem in practice.
  - A sync log will help us assess, develop, test, and monitor intracluster routing sync changes (likely those in T13211) in the future.

Some of these events are present in the pull log already, but only if they make it as far as running a `git upload-pack` subprocess (not the case with `MaxStartups` problems) -- and they can't record end-to-end timing.

No UI yet, I'll add that in a future change.

Test Plan:
  - Forced all operations to synchronize by adding `|| true` to the version check.
  - Pulled, got a sync log in the database.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19779
---
 .../sql/autopatches/20181106.repo.01.sync.sql |  14 +++
 src/__phutil_library_map__.php                |   9 ++
 .../DiffusionRepositoryClusterEngine.php      |  68 ++++++++++-
 ...PhabricatorRepositorySyncEventPHIDType.php |  39 ++++++
 .../PhabricatorRepositorySyncEventQuery.php   | 115 ++++++++++++++++++
 .../PhabricatorRepositorySyncEvent.php        |  99 +++++++++++++++
 6 files changed, 338 insertions(+), 6 deletions(-)
 create mode 100644 resources/sql/autopatches/20181106.repo.01.sync.sql
 create mode 100644 src/applications/repository/phid/PhabricatorRepositorySyncEventPHIDType.php
 create mode 100644 src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php
 create mode 100644 src/applications/repository/storage/PhabricatorRepositorySyncEvent.php

diff --git a/resources/sql/autopatches/20181106.repo.01.sync.sql b/resources/sql/autopatches/20181106.repo.01.sync.sql
new file mode 100644
index 0000000000..3302ad8ff1
--- /dev/null
+++ b/resources/sql/autopatches/20181106.repo.01.sync.sql
@@ -0,0 +1,14 @@
+CREATE TABLE {$NAMESPACE}_repository.repository_syncevent (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  phid VARBINARY(64) NOT NULL,
+  repositoryPHID VARBINARY(64) NOT NULL,
+  epoch INT UNSIGNED NOT NULL,
+  devicePHID VARBINARY(64) NOT NULL,
+  fromDevicePHID VARBINARY(64) NOT NULL,
+  deviceVersion INT UNSIGNED,
+  fromDeviceVersion INT UNSIGNED,
+  resultType VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT},
+  resultCode INT UNSIGNED NOT NULL,
+  syncWait BIGINT UNSIGNED NOT NULL,
+  properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 11a939c489..c19d29d052 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -4163,6 +4163,9 @@ phutil_register_library_map(array(
     'PhabricatorRepositorySvnCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php',
     'PhabricatorRepositorySvnCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php',
     'PhabricatorRepositorySymbol' => 'applications/repository/storage/PhabricatorRepositorySymbol.php',
+    'PhabricatorRepositorySyncEvent' => 'applications/repository/storage/PhabricatorRepositorySyncEvent.php',
+    'PhabricatorRepositorySyncEventPHIDType' => 'applications/repository/phid/PhabricatorRepositorySyncEventPHIDType.php',
+    'PhabricatorRepositorySyncEventQuery' => 'applications/repository/query/PhabricatorRepositorySyncEventQuery.php',
     'PhabricatorRepositoryTestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php',
     'PhabricatorRepositoryTransaction' => 'applications/repository/storage/PhabricatorRepositoryTransaction.php',
     'PhabricatorRepositoryTransactionQuery' => 'applications/repository/query/PhabricatorRepositoryTransactionQuery.php',
@@ -10111,6 +10114,12 @@ phutil_register_library_map(array(
     'PhabricatorRepositorySvnCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
     'PhabricatorRepositorySvnCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
     'PhabricatorRepositorySymbol' => 'PhabricatorRepositoryDAO',
+    'PhabricatorRepositorySyncEvent' => array(
+      'PhabricatorRepositoryDAO',
+      'PhabricatorPolicyInterface',
+    ),
+    'PhabricatorRepositorySyncEventPHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorRepositorySyncEventQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase',
     'PhabricatorRepositoryTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
index 2e5d4215db..cac3351ef1 100644
--- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
@@ -206,7 +206,10 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
             }
           }
 
-          $this->synchronizeWorkingCopyFromDevices($fetchable);
+          $this->synchronizeWorkingCopyFromDevices(
+            $fetchable,
+            $this_version,
+            $max_version);
         } else {
           $this->synchronizeWorkingCopyFromRemote();
         }
@@ -653,7 +656,11 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
   /**
    * @task internal
    */
-  private function synchronizeWorkingCopyFromDevices(array $device_phids) {
+  private function synchronizeWorkingCopyFromDevices(
+    array $device_phids,
+    $local_version,
+    $remote_version) {
+
     $repository = $this->getRepository();
 
     $service = $repository->loadAlmanacService();
@@ -694,7 +701,10 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
     $caught = null;
     foreach ($fetchable as $binding) {
       try {
-        $this->synchronizeWorkingCopyFromBinding($binding);
+        $this->synchronizeWorkingCopyFromBinding(
+          $binding,
+          $local_version,
+          $remote_version);
         $caught = null;
         break;
       } catch (Exception $ex) {
@@ -711,14 +721,17 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
   /**
    * @task internal
    */
-  private function synchronizeWorkingCopyFromBinding($binding) {
+  private function synchronizeWorkingCopyFromBinding(
+    AlmanacBinding $binding,
+    $local_version,
+    $remote_version) {
+
     $repository = $this->getRepository();
     $device = AlmanacKeys::getLiveDevice();
 
     $this->logLine(
       pht(
-        'Synchronizing this device ("%s") from cluster leader ("%s") before '.
-        'read.',
+        'Synchronizing this device ("%s") from cluster leader ("%s").',
         $device->getName(),
         $binding->getDevice()->getName()));
 
@@ -746,17 +759,60 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
 
     $future->setCWD($local_path);
 
+    $log = PhabricatorRepositorySyncEvent::initializeNewEvent()
+      ->setRepositoryPHID($repository->getPHID())
+      ->setEpoch(PhabricatorTime::getNow())
+      ->setDevicePHID($device->getPHID())
+      ->setFromDevicePHID($binding->getDevice()->getPHID())
+      ->setDeviceVersion($local_version)
+      ->setFromDeviceVersion($remote_version);
+
+    $sync_start = microtime(true);
+
     try {
       $future->resolvex();
     } catch (Exception $ex) {
+      $sync_end = microtime(true);
+      $log->setSyncWait((int)(1000000 * ($sync_end - $sync_start)));
+
+      if ($ex instanceof CommandException) {
+        if ($future->getWasKilledByTimeout()) {
+          $result_type = PhabricatorRepositorySyncEvent::RESULT_TIMEOUT;
+        } else {
+          $result_type = PhabricatorRepositorySyncEvent::RESULT_ERROR;
+        }
+
+       $log
+         ->setResultCode($ex->getError())
+         ->setResultType($result_type)
+         ->setProperty('stdout', $ex->getStdout())
+         ->setProperty('stderr', $ex->getStderr());
+      } else {
+        $log
+          ->setResultCode(1)
+          ->setResultType(PhabricatorRepositorySyncEvent::RESULT_EXCEPTION)
+          ->setProperty('message', $ex->getMessage());
+      }
+
+      $log->save();
+
       $this->logLine(
         pht(
           'Synchronization of "%s" from leader "%s" failed: %s',
           $device->getName(),
           $binding->getDevice()->getName(),
           $ex->getMessage()));
+
       throw $ex;
     }
+
+    $sync_end = microtime(true);
+
+    $log
+      ->setSyncWait((int)(1000000 * ($sync_end - $sync_start)))
+      ->setResultCode(0)
+      ->setResultType(PhabricatorRepositorySyncEvent::RESULT_SYNC)
+      ->save();
   }
 
 
diff --git a/src/applications/repository/phid/PhabricatorRepositorySyncEventPHIDType.php b/src/applications/repository/phid/PhabricatorRepositorySyncEventPHIDType.php
new file mode 100644
index 0000000000..ade9560a93
--- /dev/null
+++ b/src/applications/repository/phid/PhabricatorRepositorySyncEventPHIDType.php
@@ -0,0 +1,39 @@
+<?php
+
+final class PhabricatorRepositorySyncEventPHIDType extends PhabricatorPHIDType {
+
+  const TYPECONST = 'SYNE';
+
+  public function getTypeName() {
+    return pht('Sync Event');
+  }
+
+  public function newObject() {
+    return new PhabricatorRepositorySyncEvent();
+  }
+
+  public function getPHIDTypeApplicationClass() {
+    return 'PhabricatorDiffusionApplication';
+  }
+
+  protected function buildQueryForObjects(
+    PhabricatorObjectQuery $query,
+    array $phids) {
+
+    return id(new PhabricatorRepositorySyncEventQuery())
+      ->withPHIDs($phids);
+  }
+
+  public function loadHandles(
+    PhabricatorHandleQuery $query,
+    array $handles,
+    array $objects) {
+
+    foreach ($handles as $phid => $handle) {
+      $event = $objects[$phid];
+
+      $handle->setName(pht('Sync Event %d', $event->getID()));
+    }
+  }
+
+}
diff --git a/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php b/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php
new file mode 100644
index 0000000000..e7b157e9fd
--- /dev/null
+++ b/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php
@@ -0,0 +1,115 @@
+<?php
+
+final class PhabricatorRepositorySyncEventQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
+
+  private $ids;
+  private $phids;
+  private $repositoryPHIDs;
+  private $epochMin;
+  private $epochMax;
+
+  public function withIDs(array $ids) {
+    $this->ids = $ids;
+    return $this;
+  }
+
+  public function withPHIDs(array $phids) {
+    $this->phids = $phids;
+    return $this;
+  }
+
+  public function withRepositoryPHIDs(array $repository_phids) {
+    $this->repositoryPHIDs = $repository_phids;
+    return $this;
+  }
+
+  public function withEpochBetween($min, $max) {
+    $this->epochMin = $min;
+    $this->epochMax = $max;
+    return $this;
+  }
+
+  public function newResultObject() {
+    return new PhabricatorRepositoryPullEvent();
+  }
+
+  protected function loadPage() {
+    return $this->loadStandardPage($this->newResultObject());
+  }
+
+  protected function willFilterPage(array $events) {
+    $repository_phids = mpull($events, 'getRepositoryPHID');
+    $repository_phids = array_filter($repository_phids);
+
+    if ($repository_phids) {
+      $repositories = id(new PhabricatorRepositoryQuery())
+        ->setViewer($this->getViewer())
+        ->withPHIDs($repository_phids)
+        ->execute();
+      $repositories = mpull($repositories, null, 'getPHID');
+    } else {
+      $repositories = array();
+    }
+
+    foreach ($events as $key => $event) {
+      $phid = $event->getRepositoryPHID();
+
+      if (empty($repositories[$phid])) {
+        unset($events[$key]);
+        $this->didRejectResult($event);
+        continue;
+      }
+
+      $event->attachRepository($repositories[$phid]);
+    }
+
+    return $events;
+  }
+
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
+
+    if ($this->ids !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'id IN (%Ld)',
+        $this->ids);
+    }
+
+    if ($this->phids !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'phid IN (%Ls)',
+        $this->phids);
+    }
+
+    if ($this->repositoryPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'repositoryPHID IN (%Ls)',
+        $this->repositoryPHIDs);
+    }
+
+    if ($this->epochMin !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'epoch >= %d',
+        $this->epochMin);
+    }
+
+    if ($this->epochMax !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'epoch <= %d',
+        $this->epochMax);
+    }
+
+    return $where;
+  }
+
+  public function getQueryApplicationClass() {
+    return 'PhabricatorDiffusionApplication';
+  }
+
+}
diff --git a/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php b/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php
new file mode 100644
index 0000000000..767f9f00c7
--- /dev/null
+++ b/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php
@@ -0,0 +1,99 @@
+<?php
+
+final class PhabricatorRepositorySyncEvent
+  extends PhabricatorRepositoryDAO
+  implements PhabricatorPolicyInterface {
+
+  protected $repositoryPHID;
+  protected $epoch;
+  protected $devicePHID;
+  protected $fromDevicePHID;
+  protected $deviceVersion;
+  protected $fromDeviceVersion;
+  protected $resultType;
+  protected $resultCode;
+  protected $syncWait;
+  protected $properties = array();
+
+  private $repository = self::ATTACHABLE;
+
+  const RESULT_SYNC = 'sync';
+  const RESULT_ERROR = 'error';
+  const RESULT_TIMEOUT = 'timeout';
+  const RESULT_EXCEPTION = 'exception';
+
+  public static function initializeNewEvent() {
+    return new self();
+  }
+
+  protected function getConfiguration() {
+    return array(
+      self::CONFIG_AUX_PHID => true,
+      self::CONFIG_TIMESTAMPS => false,
+      self::CONFIG_SERIALIZATION => array(
+        'properties' => self::SERIALIZATION_JSON,
+      ),
+      self::CONFIG_COLUMN_SCHEMA => array(
+        'deviceVersion' => 'uint32?',
+        'fromDeviceVersion' => 'uint32?',
+        'resultType' => 'text32',
+        'resultCode' => 'uint32',
+        'syncWait' => 'uint64',
+      ),
+      self::CONFIG_KEY_SCHEMA => array(
+        'key_repository' => array(
+          'columns' => array('repositoryPHID'),
+        ),
+        'key_epoch' => array(
+          'columns' => array('epoch'),
+        ),
+      ),
+    ) + parent::getConfiguration();
+  }
+
+  public function getPHIDType() {
+    return PhabricatorRepositorySyncEventPHIDType::TYPECONST;
+  }
+
+  public function attachRepository(PhabricatorRepository $repository) {
+    $this->repository = $repository;
+    return $this;
+  }
+
+  public function getRepository() {
+    return $this->assertAttached($this->repository);
+  }
+
+  public function setProperty($key, $value) {
+    $this->properites[$key] = $value;
+    return $this;
+  }
+
+  public function getProperty($key, $default = null) {
+    return idx($this->properties, $key, $default);
+  }
+
+/* -(  PhabricatorPolicyInterface  )----------------------------------------- */
+
+
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+  public function getPolicy($capability) {
+    return $this->getRepository()->getPolicy($capability);
+  }
+
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return $this->getRepository()->hasAutomaticCapability($capability, $viewer);
+  }
+
+  public function describeAutomaticCapability($capability) {
+    return pht(
+      "A repository's sync events are visible to users who can see the ".
+      "repository.");
+  }
+
+}

From b12e92e6e2dd6b0ec90b88c68fcf1a2b84db4fa7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Nov 2018 14:51:08 -0800
Subject: [PATCH 12/16] Add timing information for commit hooks to push logs

Summary:
Depends on D19779. Ref T13216. The push logs currently record the "hostWait", which is roughly "locking + subprocess cost". We also record locking separately, so we can figure out "subprocess cost" alone by subtracting the lock costs.

However, the subprocess (normally `git receive-pack`) runs hooks, and we don't have an easy way to figure out how much time was spent doing actual `git` stuff vs spent doing commit hook processing. This would have been useful in diagnosing at least one recent issue.

Track at least a rough hook cost and record it in the push logs.

Test Plan: Pushed to a repository, saw a reasonable hook cost appear in the database table.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19780
---
 .../sql/autopatches/20181106.repo.02.hook.sql    |  2 ++
 scripts/repository/commit_hook.php               |  5 ++++-
 .../engine/DiffusionCommitHookEngine.php         | 16 +++++++++++++++-
 .../storage/PhabricatorRepositoryPushEvent.php   |  2 ++
 4 files changed, 23 insertions(+), 2 deletions(-)
 create mode 100644 resources/sql/autopatches/20181106.repo.02.hook.sql

diff --git a/resources/sql/autopatches/20181106.repo.02.hook.sql b/resources/sql/autopatches/20181106.repo.02.hook.sql
new file mode 100644
index 0000000000..be06923044
--- /dev/null
+++ b/resources/sql/autopatches/20181106.repo.02.hook.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_pushevent
+  ADD hookWait BIGINT UNSIGNED;
diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php
index 64b7b0ec24..df49aa7b00 100755
--- a/scripts/repository/commit_hook.php
+++ b/scripts/repository/commit_hook.php
@@ -17,6 +17,8 @@
 // subclasses of PhabricatorConfigSiteSource to read it and build an instance
 // environment.
 
+$hook_start = microtime(true);
+
 if ($argc > 1) {
   $context = $argv[1];
   $context = explode(':', $context, 2);
@@ -35,7 +37,8 @@ if ($argc < 2) {
   throw new Exception(pht('usage: commit-hook <repository>'));
 }
 
-$engine = new DiffusionCommitHookEngine();
+$engine = id(new DiffusionCommitHookEngine())
+  ->setStartTime($hook_start);
 
 $repository = id(new PhabricatorRepositoryQuery())
   ->setViewer(PhabricatorUser::getOmnipotentUser())
diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
index 59fb4b5e12..d22635e859 100644
--- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
+++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
@@ -31,6 +31,7 @@ final class DiffusionCommitHookEngine extends Phobject {
   private $mercurialHook;
   private $mercurialCommits = array();
   private $gitCommits = array();
+  private $startTime;
 
   private $heraldViewerProjects;
   private $rejectCode = PhabricatorRepositoryPushLog::REJECT_BROKEN;
@@ -70,6 +71,15 @@ final class DiffusionCommitHookEngine extends Phobject {
     return $this->requestIdentifier;
   }
 
+  public function setStartTime($start_time) {
+    $this->startTime = $start_time;
+    return $this;
+  }
+
+  public function getStartTime() {
+    return $this->startTime;
+  }
+
   public function setSubversionTransactionInfo($transaction, $repository) {
     $this->subversionTransaction = $transaction;
     $this->subversionRepository = $repository;
@@ -1102,11 +1112,15 @@ final class DiffusionCommitHookEngine extends Phobject {
   private function newPushEvent() {
     $viewer = $this->getViewer();
 
+    $hook_start = $this->getStartTime();
+    $hook_end = microtime(true);
+
     $event = PhabricatorRepositoryPushEvent::initializeNewEvent($viewer)
       ->setRepositoryPHID($this->getRepository()->getPHID())
       ->setRemoteAddress($this->getRemoteAddress())
       ->setRemoteProtocol($this->getRemoteProtocol())
-      ->setEpoch(PhabricatorTime::getNow());
+      ->setEpoch(PhabricatorTime::getNow())
+      ->setHookWait((int)(1000000 * ($hook_end - $hook_start)));
 
     $identifier = $this->getRequestIdentifier();
     if (strlen($identifier)) {
diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php
index 682b367926..ac97aa2bcf 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php
@@ -19,6 +19,7 @@ final class PhabricatorRepositoryPushEvent
   protected $writeWait;
   protected $readWait;
   protected $hostWait;
+  protected $hookWait;
 
   private $repository = self::ATTACHABLE;
   private $logs = self::ATTACHABLE;
@@ -41,6 +42,7 @@ final class PhabricatorRepositoryPushEvent
         'writeWait' => 'uint64?',
         'readWait' => 'uint64?',
         'hostWait' => 'uint64?',
+        'hookWait' => 'uint64?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'key_repository' => array(

From c32fa0626610e9592fabbcb9281918175c2d3888 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Nov 2018 07:30:02 -0800
Subject: [PATCH 13/16] Use phutil_microseconds_since(...) to simplify some
 timing arithmetic

Summary: Depends on D19796. Simplify some timing code by using phutil_microseconds_since() instead of duplicate casting and arithmetic.

Test Plan: Grepped for `1000000` to find these. Pulled, pushed, made a conduit call. This isn't exhaustive but it should be hard for these to break in a bad way since they're all just diagnostic.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19797
---
 scripts/ssh/ssh-exec.php                                   | 2 +-
 .../conduit/controller/PhabricatorConduitAPIController.php | 4 +---
 src/applications/conduit/ssh/ConduitSSHWorkflow.php        | 4 +---
 .../diffusion/engine/DiffusionCommitHookEngine.php         | 3 +--
 .../protocol/DiffusionRepositoryClusterEngine.php          | 7 ++-----
 .../daemon/workers/storage/PhabricatorWorkerActiveTask.php | 3 +--
 support/startup/PhabricatorStartup.php                     | 2 ++
 7 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php
index 0f2275cda8..eea2db4712 100755
--- a/scripts/ssh/ssh-exec.php
+++ b/scripts/ssh/ssh-exec.php
@@ -307,7 +307,7 @@ try {
 $ssh_log->setData(
   array(
     'c' => $err,
-    'T' => (int)(1000000 * (microtime(true) - $ssh_start_time)),
+    'T' => phutil_microseconds_since($ssh_start_time),
   ));
 
 exit($err);
diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
index aea3fc0908..f217adf154 100644
--- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
@@ -109,15 +109,13 @@ final class PhabricatorConduitAPIController
       $error_info = $ex->getMessage();
     }
 
-    $time_end = microtime(true);
-
     $log
       ->setCallerPHID(
         isset($conduit_user)
           ? $conduit_user->getPHID()
           : null)
       ->setError((string)$error_code)
-      ->setDuration(1000000 * ($time_end - $time_start));
+      ->setDuration(phutil_microseconds_since($time_start));
 
     if (!PhabricatorEnv::isReadOnly()) {
       $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
diff --git a/src/applications/conduit/ssh/ConduitSSHWorkflow.php b/src/applications/conduit/ssh/ConduitSSHWorkflow.php
index 603a479ea0..0b4bd3cdef 100644
--- a/src/applications/conduit/ssh/ConduitSSHWorkflow.php
+++ b/src/applications/conduit/ssh/ConduitSSHWorkflow.php
@@ -73,15 +73,13 @@ final class ConduitSSHWorkflow extends PhabricatorSSHWorkflow {
     // if the response is large and the receiver is slow to read it.
     $this->getIOChannel()->flush();
 
-    $time_end = microtime(true);
-
     $connection_id = idx($metadata, 'connectionID');
     $log = id(new PhabricatorConduitMethodCallLog())
       ->setCallerPHID($this->getSSHUser()->getPHID())
       ->setConnectionID($connection_id)
       ->setMethod($method)
       ->setError((string)$error_code)
-      ->setDuration(1000000 * ($time_end - $time_start))
+      ->setDuration(phutil_microseconds_since($time_start))
       ->save();
   }
 }
diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
index d22635e859..236e289ec0 100644
--- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
+++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
@@ -1113,14 +1113,13 @@ final class DiffusionCommitHookEngine extends Phobject {
     $viewer = $this->getViewer();
 
     $hook_start = $this->getStartTime();
-    $hook_end = microtime(true);
 
     $event = PhabricatorRepositoryPushEvent::initializeNewEvent($viewer)
       ->setRepositoryPHID($this->getRepository()->getPHID())
       ->setRemoteAddress($this->getRemoteAddress())
       ->setRemoteProtocol($this->getRemoteProtocol())
       ->setEpoch(PhabricatorTime::getNow())
-      ->setHookWait((int)(1000000 * ($hook_end - $hook_start)));
+      ->setHookWait(phutil_microseconds_since($hook_start));
 
     $identifier = $this->getRequestIdentifier();
     if (strlen($identifier)) {
diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
index cac3351ef1..1de58f9240 100644
--- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
@@ -772,8 +772,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
     try {
       $future->resolvex();
     } catch (Exception $ex) {
-      $sync_end = microtime(true);
-      $log->setSyncWait((int)(1000000 * ($sync_end - $sync_start)));
+      $log->setSyncWait(phutil_microseconds_since($sync_start));
 
       if ($ex instanceof CommandException) {
         if ($future->getWasKilledByTimeout()) {
@@ -806,10 +805,8 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
       throw $ex;
     }
 
-    $sync_end = microtime(true);
-
     $log
-      ->setSyncWait((int)(1000000 * ($sync_end - $sync_start)))
+      ->setSyncWait(phutil_microseconds_since($sync_start))
       ->setResultCode(0)
       ->setResultType(PhabricatorRepositorySyncEvent::RESULT_SYNC)
       ->save();
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
index e1a6ef1500..ed1eeaea63 100644
--- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
+++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
@@ -169,8 +169,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask {
 
       $t_start = microtime(true);
         $worker->executeTask();
-      $t_end = microtime(true);
-      $duration = (int)(1000000 * ($t_end - $t_start));
+      $duration = phutil_microseconds_since($t_start);
 
       $result = $this->archiveTask(
         PhabricatorWorkerArchiveTask::RESULT_SUCCESS,
diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php
index 212b057376..1bfb74d886 100644
--- a/support/startup/PhabricatorStartup.php
+++ b/support/startup/PhabricatorStartup.php
@@ -64,6 +64,8 @@ final class PhabricatorStartup {
    * @task info
    */
   public static function getMicrosecondsSinceStart() {
+    // This is the same as "phutil_microseconds_since()", but we may not have
+    // loaded libphutil yet.
     return (int)(1000000 * (microtime(true) - self::getStartTime()));
   }
 

From 2a7ac8e3882fcdaa7ad92308ddae462ff37375ab Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Nov 2018 10:08:51 -0800
Subject: [PATCH 14/16] Make "bin/repository thaw" workflow more clear when
 devices are disabled

Summary:
Ref T13216. See PHI943. If autoscale lightning strikes all your servers at once and destroys them, the path to recovery can be unclear. You're "supposed" to:

  - demote all the devices;
  - disable the bindings;
  - bind the new servers;
  - put whatever working copies you can scrape up back on disk;
  - promote one of the new servers.

However, the documentation is a bit misleading (it was sort of written with "you lost one or two devices" in mind, not "you lost every device") and demote-before-disable is unnecessary and slightly risky if servers come back online. There's also a missing guardrail before the promote step which lets you accidentally skip the demotion step and end up in a confusing state. Instead:

  - Add a guard rail: when you try to promote a new server, warn if inactive devices still have versions and tell the user to demote them.
  - Allow demotion of inactive devices: the order "disable, demote" is safer and more intuitive than "demote, disable" and there's no reason to require the unintuitive order.
  - Make the "cluster already has leaders" message more clear.
  - Make the documentation more clear.

Test Plan:
  - Bound a repository to two devices.
  - Wrote to A to make it a leader, then disabled it (simulating a lightning strike).
  - Tried to promote B. Got a new, useful error ("demote A first").
  - Demoted A (before: error about demoting inactive devices; now: works fine).
  - Promoted B. This worked.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19793
---
 ...icatorRepositoryManagementThawWorkflow.php | 90 +++++++++++++------
 .../user/cluster/cluster_repositories.diviner | 24 +++--
 2 files changed, 74 insertions(+), 40 deletions(-)

diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php
index 71076314da..4c4eed85b9 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php
@@ -120,33 +120,71 @@ final class PhabricatorRepositoryManagementThawWorkflow
               $repository->getDisplayName()));
         }
 
-        $bindings = $service->getActiveBindings();
-        $bindings = mpull($bindings, null, 'getDevicePHID');
-        if (empty($bindings[$device->getPHID()])) {
-          throw new PhutilArgumentUsageException(
-            pht(
-              'Repository "%s" has no active binding to device "%s". Only '.
-              'actively bound devices can be promoted or demoted.',
-              $repository->getDisplayName(),
-              $device->getName()));
-        }
-
-        $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions(
-          $repository->getPHID());
-
-        $versions = mpull($versions, null, 'getDevicePHID');
-        $versions = array_select_keys($versions, array_keys($bindings));
-
-        if ($versions && $promote) {
-          throw new PhutilArgumentUsageException(
-            pht(
-              'Unable to promote "%s" for repository "%s": the leaders for '.
-              'this cluster are not ambiguous.',
-              $device->getName(),
-              $repository->getDisplayName()));
-        }
-
         if ($promote) {
+          // You can only promote active devices. (You may demote active or
+          // inactive devices.)
+          $bindings = $service->getActiveBindings();
+          $bindings = mpull($bindings, null, 'getDevicePHID');
+          if (empty($bindings[$device->getPHID()])) {
+            throw new PhutilArgumentUsageException(
+              pht(
+                'Repository "%s" has no active binding to device "%s". Only '.
+                'actively bound devices can be promoted.',
+                $repository->getDisplayName(),
+                $device->getName()));
+          }
+
+          $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions(
+            $repository->getPHID());
+          $versions = mpull($versions, null, 'getDevicePHID');
+
+          // Before we promote, make sure there are no outstanding versions on
+          // devices with inactive bindings. If there are, you need to demote
+          // these first.
+          $inactive = array();
+          foreach ($versions as $device_phid => $version) {
+            if (isset($bindings[$device_phid])) {
+              continue;
+            }
+            $inactive[$device_phid] = $version;
+          }
+
+          if ($inactive) {
+            $handles = $viewer->loadHandles(array_keys($inactive));
+
+            $handle_list = iterator_to_array($handles);
+            $handle_list = mpull($handle_list, 'getName');
+            $handle_list = implode(', ', $handle_list);
+
+            throw new PhutilArgumentUsageException(
+              pht(
+                'Repository "%s" has versions on inactive devices. Demote '.
+                '(or reactivate) these devices before promoting a new '.
+                'leader: %s.',
+                $repository->getDisplayName(),
+                $handle_list));
+          }
+
+          // Now, make sure there are no outstanding versions on devices with
+          // active bindings. These also need to be demoted (or promoting is a
+          // mistake or already happened).
+          $active = array_select_keys($versions, array_keys($bindings));
+          if ($active) {
+            $handles = $viewer->loadHandles(array_keys($active));
+
+            $handle_list = iterator_to_array($handles);
+            $handle_list = mpull($handle_list, 'getName');
+            $handle_list = implode(', ', $handle_list);
+
+            throw new PhutilArgumentUsageException(
+              pht(
+                'Unable to promote "%s" for repository "%s" because this '.
+                'cluster already has one or more unambiguous leaders: %s.',
+                $device->getName(),
+                $repository->getDisplayName(),
+                $handle_list));
+          }
+
           PhabricatorRepositoryWorkingCopyVersion::updateVersion(
             $repository->getPHID(),
             $device->getPHID(),
diff --git a/src/docs/user/cluster/cluster_repositories.diviner b/src/docs/user/cluster/cluster_repositories.diviner
index 8d764c2e3e..cdf2641933 100644
--- a/src/docs/user/cluster/cluster_repositories.diviner
+++ b/src/docs/user/cluster/cluster_repositories.diviner
@@ -414,28 +414,24 @@ present on the leaders but not present on the followers by examining the
 push logs.
 
 If you are comfortable discarding these changes, you can instruct Phabricator
-that it can forget about the leaders in two ways: disable the service bindings
-to all of the leader devices so they are no longer part of the cluster, or use
-`bin/repository thaw` to `--demote` the leaders explicitly.
+that it can forget about the leaders by doing this:
 
-If you do this, **you will lose data**. Either action will discard any changes
-on the affected leaders which have not replicated to other devices in the
-cluster.
+  - Disable the service bindings to all of the leader devices so they are no
+    longer part of the cluster.
+  - Then, use `bin/repository thaw` to `--demote` the leaders explicitly.
 
-To remove a device from the cluster, disable all of the bindings to it
-in Almanac, using the web UI.
-
-{icon exclamation-triangle, color="red"} Any data which is only present on
-the disabled device will be lost.
-
-To demote a device without removing it from the cluster, run this command:
+To demote a device, run this command:
 
 ```
 phabricator/ $ ./bin/repository thaw rXYZ --demote repo002.corp.net
 ```
 
 {icon exclamation-triangle, color="red"} Any data which is only present on
-**this** device will be lost.
+the demoted device will be lost.
+
+If you do this, **you will lose unreplicated data**. You will discard any
+changes on the affected leaders which have not replicated to other devices
+in the cluster.
 
 
 Ambiguous Leaders

From 1d7c960531becd2c0f96febd27b7f2e16242a70f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Nov 2018 09:36:50 -0800
Subject: [PATCH 15/16] Put push log "hookWait" to data export and add all wait
 values to UI

Summary:
Depends on D19797. Ref T13216.

  - Put the new `hookWait` in the export and the UI.
  - Put the existing waits in the UI, not just the export.
  - Make order consistent: host, write, read, hook (this is the order the timers start in).

Test Plan: Pushed some stuff, viewed web UI and saw sensible numbers, exported data and got the same values.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19798
---
 .../DiffusionRepositoryClusterEngine.php      |  2 +-
 .../view/DiffusionPushLogListView.php         | 34 ++++++++++++++++---
 ...abricatorRepositoryPushLogSearchEngine.php | 10 ++++--
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
index 1de58f9240..c72021f0c1 100644
--- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
@@ -410,8 +410,8 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
 
     $log = $this->logger;
     if ($log) {
-      $log->writeClusterEngineLogProperty('readWait', $read_wait);
       $log->writeClusterEngineLogProperty('writeWait', $write_wait);
+      $log->writeClusterEngineLogProperty('readWait', $read_wait);
     }
   }
 
diff --git a/src/applications/diffusion/view/DiffusionPushLogListView.php b/src/applications/diffusion/view/DiffusionPushLogListView.php
index 12a5fa5494..d84667ffb5 100644
--- a/src/applications/diffusion/view/DiffusionPushLogListView.php
+++ b/src/applications/diffusion/view/DiffusionPushLogListView.php
@@ -41,9 +41,10 @@ final class DiffusionPushLogListView extends AphrontView {
     $any_host = false;
     foreach ($logs as $log) {
       $repository = $log->getRepository();
+      $event = $log->getPushEvent();
 
       if ($remotes_visible) {
-        $remote_address = $log->getPushEvent()->getRemoteAddress();
+        $remote_address = $event->getRemoteAddress();
       } else {
         $remote_address = null;
       }
@@ -79,10 +80,10 @@ final class DiffusionPushLogListView extends AphrontView {
         phutil_tag('br'),
         $flag_names);
 
-      $reject_code = $log->getPushEvent()->getRejectCode();
+      $reject_code = $event->getRejectCode();
 
       if ($reject_code == $reject_herald) {
-        $rule_phid = $log->getPushEvent()->getRejectDetails();
+        $rule_phid = $event->getRejectDetails();
         $handle = $viewer->renderHandle($rule_phid);
         $reject_label = pht('Blocked: %s', $handle);
       } else {
@@ -92,6 +93,11 @@ final class DiffusionPushLogListView extends AphrontView {
           pht('Unknown ("%s")', $reject_code));
       }
 
+      $host_wait = $this->formatMicroseconds($event->getHostWait());
+      $write_wait = $this->formatMicroseconds($event->getWriteWait());
+      $read_wait = $this->formatMicroseconds($event->getReadWait());
+      $hook_wait = $this->formatMicroseconds($event->getHookWait());
+
       $rows[] = array(
         phutil_tag(
           'a',
@@ -107,7 +113,7 @@ final class DiffusionPushLogListView extends AphrontView {
           $repository->getDisplayName()),
         $viewer->renderHandle($log->getPusherPHID()),
         $remote_address,
-        $log->getPushEvent()->getRemoteProtocol(),
+        $event->getRemoteProtocol(),
         $device,
         $log->getRefType(),
         $log->getRefName(),
@@ -121,6 +127,10 @@ final class DiffusionPushLogListView extends AphrontView {
         $flag_names,
         $reject_label,
         $viewer->formatShortDateTime($log->getEpoch()),
+        $host_wait,
+        $write_wait,
+        $read_wait,
+        $hook_wait,
       );
     }
 
@@ -140,6 +150,10 @@ final class DiffusionPushLogListView extends AphrontView {
           pht('Flags'),
           pht('Result'),
           pht('Date'),
+          pht('Host Wait'),
+          pht('Write Wait'),
+          pht('Read Wait'),
+          pht('Hook Wait'),
         ))
       ->setColumnClasses(
         array(
@@ -156,6 +170,10 @@ final class DiffusionPushLogListView extends AphrontView {
           '',
           '',
           'right',
+          'n right',
+          'n right',
+          'n right',
+          'n right',
         ))
       ->setColumnVisibility(
         array(
@@ -170,4 +188,12 @@ final class DiffusionPushLogListView extends AphrontView {
     return $table;
   }
 
+  private function formatMicroseconds($duration) {
+    if ($duration === null) {
+      return null;
+    }
+
+    return pht('%sus', new PhutilNumber($duration));
+  }
+
 }
diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
index 87b2e44740..271d8a9c14 100644
--- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
+++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php
@@ -162,6 +162,9 @@ final class PhabricatorRepositoryPushLogSearchEngine
       id(new PhabricatorStringExportField())
         ->setKey('resultDetails')
         ->setLabel(pht('Result Details')),
+      id(new PhabricatorIntExportField())
+        ->setKey('hostWait')
+        ->setLabel(pht('Host Wait (us)')),
       id(new PhabricatorIntExportField())
         ->setKey('writeWait')
         ->setLabel(pht('Write Wait (us)')),
@@ -169,8 +172,8 @@ final class PhabricatorRepositoryPushLogSearchEngine
         ->setKey('readWait')
         ->setLabel(pht('Read Wait (us)')),
       id(new PhabricatorIntExportField())
-        ->setKey('hostWait')
-        ->setLabel(pht('Host Wait (us)')),
+        ->setKey('hookWait')
+        ->setLabel(pht('Hook Wait (us)')),
     );
 
     if ($viewer->getIsAdmin()) {
@@ -251,9 +254,10 @@ final class PhabricatorRepositoryPushLogSearchEngine
         'result' => $result,
         'resultName' => $result_name,
         'resultDetails' => $event->getRejectDetails(),
+        'hostWait' => $event->getHostWait(),
         'writeWait' => $event->getWriteWait(),
         'readWait' => $event->getReadWait(),
-        'hostWait' => $event->getHostWait(),
+        'hookWait' => $event->getHookWait(),
       );
 
       if ($viewer->getIsAdmin()) {

From 315d857a8ad63a931342f6c5ac491f368f278635 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Nov 2018 10:14:46 -0800
Subject: [PATCH 16/16] Add a basic web UI for intracluster sync logs

Summary: Depends on D19798. Ref T13216. This puts at least a basic UI on top of sync logs.

Test Plan:
Viewed logs from the web UI and exported data. Note that these syncs are somewhat simulated since I my local cluster is somewhat-faked (i.e., not actually multiple machines).

{F5995899}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19799
---
 src/__phutil_library_map__.php                |   6 +
 .../PhabricatorDiffusionApplication.php       |   3 +
 .../DiffusionRepositoryController.php         |  13 +-
 .../DiffusionSyncLogListController.php        |  17 ++
 .../query/DiffusionSyncLogSearchEngine.php    | 154 ++++++++++++++++++
 .../view/DiffusionSyncLogListView.php         |  79 +++++++++
 .../PhabricatorRepositorySyncEventQuery.php   |   2 +-
 .../PhabricatorRepositorySyncEvent.php        |   2 +-
 8 files changed, 272 insertions(+), 4 deletions(-)
 create mode 100644 src/applications/diffusion/controller/DiffusionSyncLogListController.php
 create mode 100644 src/applications/diffusion/query/DiffusionSyncLogSearchEngine.php
 create mode 100644 src/applications/diffusion/view/DiffusionSyncLogListView.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index c19d29d052..4c784571b5 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -992,6 +992,9 @@ phutil_register_library_map(array(
     'DiffusionSymbolController' => 'applications/diffusion/controller/DiffusionSymbolController.php',
     'DiffusionSymbolDatasource' => 'applications/diffusion/typeahead/DiffusionSymbolDatasource.php',
     'DiffusionSymbolQuery' => 'applications/diffusion/query/DiffusionSymbolQuery.php',
+    'DiffusionSyncLogListController' => 'applications/diffusion/controller/DiffusionSyncLogListController.php',
+    'DiffusionSyncLogListView' => 'applications/diffusion/view/DiffusionSyncLogListView.php',
+    'DiffusionSyncLogSearchEngine' => 'applications/diffusion/query/DiffusionSyncLogSearchEngine.php',
     'DiffusionTagListController' => 'applications/diffusion/controller/DiffusionTagListController.php',
     'DiffusionTagListView' => 'applications/diffusion/view/DiffusionTagListView.php',
     'DiffusionTagTableView' => 'applications/diffusion/view/DiffusionTagTableView.php',
@@ -6367,6 +6370,9 @@ phutil_register_library_map(array(
     'DiffusionSymbolController' => 'DiffusionController',
     'DiffusionSymbolDatasource' => 'PhabricatorTypeaheadDatasource',
     'DiffusionSymbolQuery' => 'PhabricatorOffsetPagedQuery',
+    'DiffusionSyncLogListController' => 'DiffusionLogController',
+    'DiffusionSyncLogListView' => 'AphrontView',
+    'DiffusionSyncLogSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'DiffusionTagListController' => 'DiffusionController',
     'DiffusionTagListView' => 'DiffusionView',
     'DiffusionTagTableView' => 'DiffusionView',
diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
index a7baea1968..d635f5d559 100644
--- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
+++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
@@ -118,6 +118,9 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
           $this->getQueryRoutePattern() => 'DiffusionPushLogListController',
           'view/(?P<id>\d+)/' => 'DiffusionPushEventViewController',
         ),
+        'synclog/' => array(
+          $this->getQueryRoutePattern() => 'DiffusionSyncLogListController',
+        ),
         'pulllog/' => array(
           $this->getQueryRoutePattern() => 'DiffusionPullLogListController',
         ),
diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php
index 89c677c081..268eedbcc7 100644
--- a/src/applications/diffusion/controller/DiffusionRepositoryController.php
+++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php
@@ -370,8 +370,17 @@ final class DiffusionRepositoryController extends DiffusionController {
       $action_view->addAction(
         id(new PhabricatorActionView())
           ->setName(pht('View Push Logs'))
-          ->setIcon('fa-list-alt')
+          ->setIcon('fa-upload')
           ->setHref($push_uri));
+
+      $pull_uri = $this->getApplicationURI(
+        'synclog/?repositories='.$repository->getPHID());
+
+      $action_view->addAction(
+        id(new PhabricatorActionView())
+          ->setName(pht('View Sync Logs'))
+          ->setIcon('fa-exchange')
+          ->setHref($pull_uri));
     }
 
     $pull_uri = $this->getApplicationURI(
@@ -380,7 +389,7 @@ final class DiffusionRepositoryController extends DiffusionController {
     $action_view->addAction(
       id(new PhabricatorActionView())
         ->setName(pht('View Pull Logs'))
-        ->setIcon('fa-list-alt')
+        ->setIcon('fa-download')
         ->setHref($pull_uri));
 
     return $action_view;
diff --git a/src/applications/diffusion/controller/DiffusionSyncLogListController.php b/src/applications/diffusion/controller/DiffusionSyncLogListController.php
new file mode 100644
index 0000000000..a6281f9bf0
--- /dev/null
+++ b/src/applications/diffusion/controller/DiffusionSyncLogListController.php
@@ -0,0 +1,17 @@
+<?php
+
+final class DiffusionSyncLogListController
+  extends DiffusionLogController {
+
+  public function handleRequest(AphrontRequest $request) {
+    return id(new DiffusionSyncLogSearchEngine())
+      ->setController($this)
+      ->buildResponse();
+  }
+
+  protected function buildApplicationCrumbs() {
+    return parent::buildApplicationCrumbs()
+      ->addTextCrumb(pht('Sync Logs'), $this->getApplicationURI('synclog/'));
+  }
+
+}
diff --git a/src/applications/diffusion/query/DiffusionSyncLogSearchEngine.php b/src/applications/diffusion/query/DiffusionSyncLogSearchEngine.php
new file mode 100644
index 0000000000..6b321189f4
--- /dev/null
+++ b/src/applications/diffusion/query/DiffusionSyncLogSearchEngine.php
@@ -0,0 +1,154 @@
+<?php
+
+final class DiffusionSyncLogSearchEngine
+  extends PhabricatorApplicationSearchEngine {
+
+  public function getResultTypeDescription() {
+    return pht('Sync Logs');
+  }
+
+  public function getApplicationClassName() {
+    return 'PhabricatorDiffusionApplication';
+  }
+
+  public function newQuery() {
+    return new PhabricatorRepositorySyncEventQuery();
+  }
+
+  protected function buildQueryFromParameters(array $map) {
+    $query = $this->newQuery();
+
+    if ($map['repositoryPHIDs']) {
+      $query->withRepositoryPHIDs($map['repositoryPHIDs']);
+    }
+
+    if ($map['createdStart'] || $map['createdEnd']) {
+      $query->withEpochBetween(
+        $map['createdStart'],
+        $map['createdEnd']);
+    }
+
+    return $query;
+  }
+
+  protected function buildCustomSearchFields() {
+    return array(
+      id(new PhabricatorSearchDatasourceField())
+        ->setDatasource(new DiffusionRepositoryDatasource())
+        ->setKey('repositoryPHIDs')
+        ->setAliases(array('repository', 'repositories', 'repositoryPHID'))
+        ->setLabel(pht('Repositories'))
+        ->setDescription(
+          pht('Search for sync logs for specific repositories.')),
+      id(new PhabricatorSearchDateField())
+        ->setLabel(pht('Created After'))
+        ->setKey('createdStart'),
+      id(new PhabricatorSearchDateField())
+        ->setLabel(pht('Created Before'))
+        ->setKey('createdEnd'),
+    );
+  }
+
+  protected function newExportFields() {
+    $viewer = $this->requireViewer();
+
+    $fields = array(
+      id(new PhabricatorPHIDExportField())
+        ->setKey('repositoryPHID')
+        ->setLabel(pht('Repository PHID')),
+      id(new PhabricatorStringExportField())
+        ->setKey('repository')
+        ->setLabel(pht('Repository')),
+      id(new PhabricatorPHIDExportField())
+        ->setKey('devicePHID')
+        ->setLabel(pht('Device PHID')),
+      id(new PhabricatorPHIDExportField())
+        ->setKey('fromDevicePHID')
+        ->setLabel(pht('From Device PHID')),
+      id(new PhabricatorIntExportField())
+        ->setKey('deviceVersion')
+        ->setLabel(pht('Device Version')),
+      id(new PhabricatorIntExportField())
+        ->setKey('fromDeviceVersion')
+        ->setLabel(pht('From Device Version')),
+      id(new PhabricatorStringExportField())
+        ->setKey('result')
+        ->setLabel(pht('Result')),
+      id(new PhabricatorIntExportField())
+        ->setKey('code')
+        ->setLabel(pht('Code')),
+      id(new PhabricatorEpochExportField())
+        ->setKey('date')
+        ->setLabel(pht('Date')),
+      id(new PhabricatorIntExportField())
+        ->setKey('syncWait')
+        ->setLabel(pht('Sync Wait')),
+    );
+
+    return $fields;
+  }
+
+  protected function newExportData(array $events) {
+    $viewer = $this->requireViewer();
+
+    $export = array();
+    foreach ($events as $event) {
+      $repository = $event->getRepository();
+      $repository_phid = $repository->getPHID();
+      $repository_name = $repository->getDisplayName();
+
+      $map = array(
+        'repositoryPHID' => $repository_phid,
+        'repository' => $repository_name,
+        'devicePHID' => $event->getDevicePHID(),
+        'fromDevicePHID' => $event->getFromDevicePHID(),
+        'deviceVersion' => $event->getDeviceVersion(),
+        'fromDeviceVersion' => $event->getFromDeviceVersion(),
+        'result' => $event->getResultType(),
+        'code' => $event->getResultCode(),
+        'date' => $event->getEpoch(),
+        'syncWait' => $event->getSyncWait(),
+      );
+
+      $export[] = $map;
+    }
+
+    return $export;
+  }
+
+  protected function getURI($path) {
+    return '/diffusion/synclog/'.$path;
+  }
+
+  protected function getBuiltinQueryNames() {
+    return array(
+      'all' => pht('All Sync Logs'),
+    );
+  }
+
+  public function buildSavedQueryFromBuiltin($query_key) {
+    $query = $this->newSavedQuery();
+    $query->setQueryKey($query_key);
+
+    switch ($query_key) {
+      case 'all':
+        return $query;
+    }
+
+    return parent::buildSavedQueryFromBuiltin($query_key);
+  }
+
+  protected function renderResultList(
+    array $logs,
+    PhabricatorSavedQuery $query,
+    array $handles) {
+
+    $table = id(new DiffusionSyncLogListView())
+      ->setViewer($this->requireViewer())
+      ->setLogs($logs);
+
+    return id(new PhabricatorApplicationSearchResultView())
+      ->setTable($table);
+  }
+
+}
diff --git a/src/applications/diffusion/view/DiffusionSyncLogListView.php b/src/applications/diffusion/view/DiffusionSyncLogListView.php
new file mode 100644
index 0000000000..b4e195122e
--- /dev/null
+++ b/src/applications/diffusion/view/DiffusionSyncLogListView.php
@@ -0,0 +1,79 @@
+<?php
+
+final class DiffusionSyncLogListView extends AphrontView {
+
+  private $logs;
+
+  public function setLogs(array $logs) {
+    assert_instances_of($logs, 'PhabricatorRepositorySyncEvent');
+    $this->logs = $logs;
+    return $this;
+  }
+
+  public function render() {
+    $events = $this->logs;
+    $viewer = $this->getViewer();
+
+    $rows = array();
+    foreach ($events as $event) {
+      $repository = $event->getRepository();
+      $repository_link = phutil_tag(
+        'a',
+        array(
+          'href' => $repository->getURI(),
+        ),
+        $repository->getDisplayName());
+
+      $event_id = $event->getID();
+
+      $sync_wait = pht('%sus', new PhutilNumber($event->getSyncWait()));
+
+      $device_link = $viewer->renderHandle($event->getDevicePHID());
+      $from_device_link = $viewer->renderHandle($event->getFromDevicePHID());
+
+      $rows[] = array(
+        $event_id,
+        $repository_link,
+        $device_link,
+        $from_device_link,
+        $event->getDeviceVersion(),
+        $event->getFromDeviceVersion(),
+        $event->getResultType(),
+        $event->getResultCode(),
+        phabricator_datetime($event->getEpoch(), $viewer),
+        $sync_wait,
+      );
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setHeaders(
+        array(
+          pht('Sync'),
+          pht('Repository'),
+          pht('Device'),
+          pht('From Device'),
+          pht('Version'),
+          pht('From Version'),
+          pht('Result'),
+          pht('Code'),
+          pht('Date'),
+          pht('Sync Wait'),
+        ))
+      ->setColumnClasses(
+        array(
+          'n',
+          '',
+          '',
+          '',
+          'n',
+          'n',
+          'wide right',
+          'n',
+          'right',
+          'n right',
+        ));
+
+    return $table;
+  }
+
+}
diff --git a/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php b/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php
index e7b157e9fd..542cb5cdc0 100644
--- a/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php
+++ b/src/applications/repository/query/PhabricatorRepositorySyncEventQuery.php
@@ -31,7 +31,7 @@ final class PhabricatorRepositorySyncEventQuery
   }
 
   public function newResultObject() {
-    return new PhabricatorRepositoryPullEvent();
+    return new PhabricatorRepositorySyncEvent();
   }
 
   protected function loadPage() {
diff --git a/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php b/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php
index 767f9f00c7..dd5765985e 100644
--- a/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php
+++ b/src/applications/repository/storage/PhabricatorRepositorySyncEvent.php
@@ -65,7 +65,7 @@ final class PhabricatorRepositorySyncEvent
   }
 
   public function setProperty($key, $value) {
-    $this->properites[$key] = $value;
+    $this->properties[$key] = $value;
     return $this;
   }