From 69a7d57c3fdaeecd3b7ec8f98ba4a02b5438b8c3 Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Mon, 24 Jul 2017 13:27:19 -0700
Subject: [PATCH 01/12] Add a branch selector to Diffusion

Summary: Fixes T12931. Adds a branch selector that's always visible if the repo has commits.

Test Plan:
Test a plain hg, svn, git repository. Test setting a bad default branch. Test a good default branch. Test on desktop, mobile layouts.

{F5058061}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12931

Differential Revision: https://secure.phabricator.com/D18267
---
 resources/celerity/map.php                    |  10 +-
 .../DiffusionRepositoryController.php         | 128 ++++++++++++++++--
 .../css/application/diffusion/diffusion.css   |  17 +++
 webroot/rsrc/css/phui/button/phui-button.css  |   8 ++
 4 files changed, 147 insertions(+), 16 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index a9c633d7ad..c724382016 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => 'b5b51108',
-    'core.pkg.css' => 'f1c7630f',
+    'core.pkg.css' => 'c0a7ecfd',
     'core.pkg.js' => '5d80e0db',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '45951e9e',
@@ -75,7 +75,7 @@ return array(
     'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6',
     'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec',
     'rsrc/css/application/diffusion/diffusion-source.css' => '750add59',
-    'rsrc/css/application/diffusion/diffusion.css' => '8d01932f',
+    'rsrc/css/application/diffusion/diffusion.css' => '67bd971b',
     'rsrc/css/application/feed/feed.css' => 'ecd4ec57',
     'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948',
     'rsrc/css/application/flag/flag.css' => 'bba8f811',
@@ -127,7 +127,7 @@ return array(
     'rsrc/css/layout/phabricator-source-code-view.css' => 'aea41829',
     'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494',
     'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68',
-    'rsrc/css/phui/button/phui-button.css' => '022581b4',
+    'rsrc/css/phui/button/phui-button.css' => '3a744520',
     'rsrc/css/phui/calendar/phui-calendar-day.css' => '572b1893',
     'rsrc/css/phui/calendar/phui-calendar-list.css' => '576be600',
     'rsrc/css/phui/calendar/phui-calendar-month.css' => '21154caf',
@@ -571,7 +571,7 @@ return array(
     'differential-revision-history-css' => '0e8eb855',
     'differential-revision-list-css' => 'f3c47d33',
     'differential-table-of-contents-css' => 'ae4b7a55',
-    'diffusion-css' => '8d01932f',
+    'diffusion-css' => '67bd971b',
     'diffusion-icons-css' => '0c15255e',
     'diffusion-readme-css' => '419dd5b6',
     'diffusion-repository-css' => 'ee6f20ec',
@@ -825,7 +825,7 @@ return array(
     'phui-big-info-view-css' => 'd13afcde',
     'phui-box-css' => '745e881d',
     'phui-button-bar-css' => 'f1ff5494',
-    'phui-button-css' => '022581b4',
+    'phui-button-css' => '3a744520',
     'phui-button-simple-css' => '8e1baf68',
     'phui-calendar-css' => 'f1ddf11c',
     'phui-calendar-day-css' => '572b1893',
diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php
index 6def808cff..39877317df 100644
--- a/src/applications/diffusion/controller/DiffusionRepositoryController.php
+++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php
@@ -4,7 +4,7 @@ final class DiffusionRepositoryController extends DiffusionController {
 
   private $historyFuture;
   private $browseFuture;
-  private $tagFuture;
+  private $branchButton = null;
   private $branchFuture;
 
   public function shouldAllowPublic() {
@@ -53,15 +53,25 @@ final class DiffusionRepositoryController extends DiffusionController {
         // This is a valid branch, so we necessarily have some content.
         $page_has_content = true;
       } else {
-        $empty_title = pht('No Such Branch');
-        $empty_message = pht(
-          'There is no branch named "%s" in this repository.',
-          $drequest->getBranch());
+        $default = $repository->getDefaultBranch();
+        if ($default != $drequest->getBranch()) {
+          $empty_title = pht('No Such Branch');
+          $empty_message = pht(
+            'There is no branch named "%s" in this repository.',
+            $drequest->getBranch());
+        } else {
+          $empty_title = pht('No Default Branch');
+          $empty_message = pht(
+            'This repository is configured with default branch "%s"  but '.
+            'there is no branch with that name in this repository.',
+            $default);
+        }
       }
     }
 
     // If we didn't find any branches, check if there are any commits at all.
     // This can tailor the message for empty repositories.
+    $any_commit = null;
     if (!$page_has_content) {
       $any_commit = id(new DiffusionCommitQuery())
         ->setViewer($viewer)
@@ -81,6 +91,9 @@ final class DiffusionRepositoryController extends DiffusionController {
     if ($page_has_content) {
       $content = $this->buildNormalContent($drequest);
     } else {
+      // If we have a commit somewhere, find branches.
+      // TODO: Evan will replace
+      // $this->buildNormalContent($drequest);
       $content = id(new PHUIInfoView())
         ->setTitle($empty_title)
         ->setSeverity(PHUIInfoView::SEVERITY_WARNING)
@@ -110,7 +123,7 @@ final class DiffusionRepositoryController extends DiffusionController {
 
     $bar = id(new PHUILeftRightView())
       ->setLeft($locate_file)
-      ->setRight($clone_button)
+      ->setRight(array($this->branchButton, $clone_button))
       ->addClass('diffusion-action-bar');
 
     $view = id(new PHUITwoColumnView())
@@ -145,6 +158,7 @@ final class DiffusionRepositoryController extends DiffusionController {
     $commit = $drequest->getCommit();
     $path = $drequest->getPath();
 
+    $futures = array();
     $this->historyFuture = $this->callConduitMethod(
       'diffusion.historyquery',
       array(
@@ -153,6 +167,7 @@ final class DiffusionRepositoryController extends DiffusionController {
         'offset' => 0,
         'limit' => 15,
       ));
+    $futures[] = $this->historyFuture;
 
     $browse_pager = id(new PHUIPagerView())
       ->readFromRequest($request);
@@ -164,11 +179,19 @@ final class DiffusionRepositoryController extends DiffusionController {
         'path' => $path,
         'limit' => $browse_pager->getPageSize() + 1,
       ));
+    $futures[] = $this->browseFuture;
+
+    if ($this->needBranchFuture()) {
+      $branch_limit = $this->getBranchLimit();
+      $this->branchFuture = $this->callConduitMethod(
+        'diffusion.branchquery',
+        array(
+          'closed' => false,
+          'limit' => $branch_limit + 1,
+        ));
+      $futures[] = $this->branchFuture;
+    }
 
-    $futures = array(
-      $this->historyFuture,
-      $this->browseFuture,
-    );
     $futures = array_filter($futures);
     $futures = new FutureIterator($futures);
     foreach ($futures as $future) {
@@ -253,6 +276,18 @@ final class DiffusionRepositoryController extends DiffusionController {
       $content[] = $readme;
     }
 
+
+    try {
+      $branch_button = $this->buildBranchList($drequest);
+      $this->branchButton = $branch_button;
+    } catch (Exception $ex) {
+      if (!$repository->isImporting()) {
+        $content[] = $this->renderStatusMessage(
+          pht('Unable to Load Branches'),
+          $ex->getMessage());
+      }
+    }
+
     return $content;
   }
 
@@ -375,6 +410,67 @@ final class DiffusionRepositoryController extends DiffusionController {
     return $panel;
   }
 
+  private function buildBranchList(DiffusionRequest $drequest) {
+    $viewer = $this->getViewer();
+
+    if (!$this->needBranchFuture()) {
+      return null;
+    }
+
+    $branches = $this->branchFuture->resolve();
+    if (!$branches) {
+      return null;
+    }
+
+    $limit = $this->getBranchLimit();
+    $more_branches = (count($branches) > $limit);
+    $branches = array_slice($branches, 0, $limit);
+
+    $branches = DiffusionRepositoryRef::loadAllFromDictionaries($branches);
+
+    $actions = id(new PhabricatorActionListView())
+      ->setViewer($viewer);
+
+    foreach ($branches as $branch) {
+      $branch_uri = $drequest->generateURI(
+        array(
+          'action' => 'browse',
+          'branch' => $branch->getShortname(),
+        ));
+      $actions->addAction(
+        id(new PhabricatorActionView())
+          ->setName($branch->getShortname())
+          ->setIcon('fa-code-fork')
+          ->setHref($branch_uri));
+    }
+
+    if ($more_branches) {
+      $more_uri = $drequest->generateURI(
+        array(
+          'action' => 'branches',
+        ));
+      $actions->addAction(
+        id(new PhabricatorActionView())
+          ->setType(PhabricatorActionView::TYPE_DIVIDER));
+      $actions->addAction(
+        id(new PhabricatorActionView())
+          ->setName(pht('See More Branches'))
+          ->setIcon('fa-external-link')
+          ->setHref($more_uri));
+    }
+
+    $button = id(new PHUIButtonView())
+      ->setText(pht('Branch: %s', $drequest->getBranch()))
+      ->setTag('a')
+      ->addClass('mmr')
+      ->setIcon('fa-code-fork')
+      ->setColor(PHUIButtonView::GREY)
+      ->setDropdown(true)
+      ->setDropdownMenu($actions);
+
+    return $button;
+  }
+
   private function buildLocateFile() {
     $request = $this->getRequest();
     $viewer = $request->getUser();
@@ -457,7 +553,17 @@ final class DiffusionRepositoryController extends DiffusionController {
       ->setPager($pager);
   }
 
-  private function getTagLimit() {
+  private function needBranchFuture() {
+    $drequest = $this->getDiffusionRequest();
+
+    if ($drequest->getBranch() === null) {
+      return false;
+    }
+
+    return true;
+  }
+
+  private function getBranchLimit() {
     return 15;
   }
 
diff --git a/webroot/rsrc/css/application/diffusion/diffusion.css b/webroot/rsrc/css/application/diffusion/diffusion.css
index 405755140b..4d18e3dcb7 100644
--- a/webroot/rsrc/css/application/diffusion/diffusion.css
+++ b/webroot/rsrc/css/application/diffusion/diffusion.css
@@ -16,6 +16,23 @@
   margin-bottom: 16px;
 }
 
+.device-phone .diffusion-action-bar {
+  display: block;
+}
+
+.device-phone .diffusion-action-bar .phui-lr-container {
+  display: block;
+}
+
+.device-phone .diffusion-action-bar .phui-lr-container .phui-left-view {
+  display: block;
+}
+
+.device-phone .diffusion-action-bar .phui-lr-container .phui-right-view {
+  padding-top: 12px;
+  display: block;
+}
+
 .diffusion-profile-locate .phui-form-view {
   margin: 0;
   padding: 0;
diff --git a/webroot/rsrc/css/phui/button/phui-button.css b/webroot/rsrc/css/phui/button/phui-button.css
index 3a356f7deb..ac3ae2176d 100644
--- a/webroot/rsrc/css/phui/button/phui-button.css
+++ b/webroot/rsrc/css/phui/button/phui-button.css
@@ -265,6 +265,14 @@ a.policy-control .phui-button-text {
   right: 10px;
 }
 
+.phui-button-text {
+  display: inline-block;
+}
+
+.dropdown .phui-button-text {
+  margin-right: 16px;
+}
+
 .button.has-icon .phui-button-text {
   margin-left: 16px;
 }

From 8034b9d819eb941c7df08bdade6baa0551ccc177 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Jul 2017 11:37:04 -0700
Subject: [PATCH 02/12] Don't require a device be registered in Almanac to do
 cluster init/resync steps

Summary:
Fixes T12893. See also PHI15. This is complicated but:

  - In the documentation, we say "register your web devices with Almanac". We do this ourselves on `secure` and in the production Phacility cluster.
  - We don't actually require you to do this, don't detect that you didn't, and there's no actual reason you need to.
  - If you don't register your "web" devices, the only bad thing that really happens is that creating repositories skips version initialization, creating the bug in T12893. This process does not actually require the devices be registered, but the code currently just kind of fails silently if they aren't.

Instead, just move forward on these init/resync phases even if the device isn't registered. These steps are safe to run from unregistered hosts since they just wipe the whole table and don't affect specific devices.

If this sticks, I'll probably update the docs to not tell you to register `web` devices, or at least add "Optionally, ...". I don't think there's any future reason we'd need them to be registered.

Test Plan:
This is a bit tough to test without multiple hosts, but I added this piece of code to `AlmanacKeys` so we'd pretend to be a nameless "web" device when creating a repository:

```
if ($_REQUEST['__path__'] == '/diffusion/edit/form/default/') {
  return null;
}
```

Then I created some Git repositories. Before the patch, they came up with `-` versions (no version information). After the patch, they came up with `0` versions (correctly initialized).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12893

Differential Revision: https://secure.phabricator.com/D18273
---
 .../DiffusionRepositoryClusterEngine.php      | 22 ++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
index a1a22670f9..e822bb76b0 100644
--- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
@@ -58,7 +58,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
    * @task sync
    */
   public function synchronizeWorkingCopyAfterCreation() {
-    if (!$this->shouldEnableSynchronization()) {
+    if (!$this->shouldEnableSynchronization(false)) {
       return;
     }
 
@@ -86,7 +86,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
    * @task sync
    */
   public function synchronizeWorkingCopyAfterHostingChange() {
-    if (!$this->shouldEnableSynchronization()) {
+    if (!$this->shouldEnableSynchronization(false)) {
       return;
     }
 
@@ -133,7 +133,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
    * @task sync
    */
   public function synchronizeWorkingCopyBeforeRead() {
-    if (!$this->shouldEnableSynchronization()) {
+    if (!$this->shouldEnableSynchronization(true)) {
       return;
     }
 
@@ -288,7 +288,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
    * @task sync
    */
   public function synchronizeWorkingCopyBeforeWrite() {
-    if (!$this->shouldEnableSynchronization()) {
+    if (!$this->shouldEnableSynchronization(true)) {
       return;
     }
 
@@ -382,7 +382,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
 
 
   public function synchronizeWorkingCopyAfterDiscovery($new_version) {
-    if (!$this->shouldEnableSynchronization()) {
+    if (!$this->shouldEnableSynchronization(true)) {
       return;
     }
 
@@ -426,7 +426,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
    * @task sync
    */
   public function synchronizeWorkingCopyAfterWrite() {
-    if (!$this->shouldEnableSynchronization()) {
+    if (!$this->shouldEnableSynchronization(true)) {
       return;
     }
 
@@ -551,7 +551,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
   /**
    * @task internal
    */
-  private function shouldEnableSynchronization() {
+  private function shouldEnableSynchronization($require_device) {
     $repository = $this->getRepository();
 
     $service_phid = $repository->getAlmanacServicePHID();
@@ -563,9 +563,11 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
       return false;
     }
 
-    $device = AlmanacKeys::getLiveDevice();
-    if (!$device) {
-      return false;
+    if ($require_device) {
+      $device = AlmanacKeys::getLiveDevice();
+      if (!$device) {
+        return false;
+      }
     }
 
     return true;

From c217d7619c5f3fe7e7c32c50c81972851ec236d1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Jul 2017 16:30:44 -0700
Subject: [PATCH 03/12] Make "A" hide or show all inline comments

Summary: Ref T12733. See PHI17.

Test Plan: Pressed "A", then pressed "A".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12733

Differential Revision: https://secure.phabricator.com/D18274
---
 resources/celerity/map.php                      | 14 +++++++-------
 .../view/DifferentialChangesetListView.php      |  3 +++
 .../js/application/diff/DiffChangesetList.js    | 17 +++++++++++++++++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index c724382016..fb20ebfa55 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '5d80e0db',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '45951e9e',
-    'differential.pkg.js' => '414ada25',
+    'differential.pkg.js' => 'b71b8c5d',
     'diffusion.pkg.css' => 'a2d17c7d',
     'diffusion.pkg.js' => '6134c5a1',
     'favicon.ico' => '30672e08',
@@ -398,7 +398,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
     'rsrc/js/application/diff/DiffChangeset.js' => '99abf4cd',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'cb1570cb',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '8f1cd52c',
     'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07',
@@ -778,7 +778,7 @@ return array(
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
     'phabricator-diff-changeset' => '99abf4cd',
-    'phabricator-diff-changeset-list' => 'cb1570cb',
+    'phabricator-diff-changeset-list' => '8f1cd52c',
     'phabricator-diff-inline' => 'e83d28f3',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
@@ -1567,6 +1567,10 @@ return array(
     '8e1baf68' => array(
       'phui-button-css',
     ),
+    '8f1cd52c' => array(
+      'javelin-install',
+      'phuix-button-view',
+    ),
     '8ff5e24c' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1950,10 +1954,6 @@ return array(
     'cae95e89' => array(
       'syntax-default-css',
     ),
-    'cb1570cb' => array(
-      'javelin-install',
-      'phuix-button-view',
-    ),
     'ccf1cbf8' => array(
       'javelin-install',
       'javelin-dom',
diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php
index 2787356bd2..307d424b0e 100644
--- a/src/applications/differential/view/DifferentialChangesetListView.php
+++ b/src/applications/differential/view/DifferentialChangesetListView.php
@@ -298,6 +298,9 @@ final class DifferentialChangesetListView extends AphrontView {
         'Show All Inlines' => pht('Show All Inlines'),
 
         'List Inline Comments' => pht('List Inline Comments'),
+
+        'Hide or show all inline comments.' =>
+          pht('Hide or show all inline comments.'),
       ),
     ));
 
diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js
index 89a18a9a8b..3f47f1ed35 100644
--- a/webroot/rsrc/js/application/diff/DiffChangesetList.js
+++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js
@@ -190,6 +190,10 @@ JX.install('DiffChangesetList', {
 
       label = pht('Collapse or expand inline comment.');
       this._installKey('q', label, this._onkeycollapse);
+
+      label = pht('Hide or show all inline comments.');
+      this._installKey('A', label, this._onkeyhideall);
+
     },
 
     isAsleep: function() {
@@ -448,6 +452,15 @@ JX.install('DiffChangesetList', {
       this._warnUser(pht('You must select a comment to hide.'));
     },
 
+    _onkeyhideall: function() {
+      var inlines = this._getInlinesByType();
+      if (inlines.visible.length) {
+        this._toggleInlines('all');
+      } else {
+        this._toggleInlines('show');
+      }
+    },
+
     _warnUser: function(message) {
       new JX.Notification()
         .setContent(message)
@@ -1701,6 +1714,10 @@ JX.install('DiffChangesetList', {
       this._dropdownMenu.close();
       e.prevent();
 
+      this._toggleInlines(type);
+    },
+
+    _toggleInlines: function(type) {
       var inlines = this._getInlinesByType();
 
       // Clear the selection state since we end up in a weird place if the

From 1588d3e224fe73df60272842e1d8578e72c4970a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 25 Jul 2017 06:50:40 -0700
Subject: [PATCH 04/12] In Differential, render status for any active Drydock
 repository operation, not just "Land" operations

Summary:
See PHI18. Third parties can currently define other types of Drydock operations (like "Merge Check" or "Cherry-Pick") but we won't show them in the UI.

This is a simple change which improves third-party support for now. These kinds of operations generally make sense in the upstream, but the pathways to support are longer.

Test Plan:
  - Verified that there are no other types of repository operation which we'd want to exclude in the upstream today by reviewing the "Repository Operation" subclasses.
  - Will click some buttons in production to make sure this works.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D18276
---
 .../controller/DifferentialRevisionViewController.php     | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index ab03ee6d81..edf65b01ea 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -1051,14 +1051,14 @@ final class DifferentialRevisionViewController extends DifferentialController {
       return null;
     }
 
+    // NOTE: The upstream currently supports only "Land" operations, but
+    // third parties have other operations (see PHI18). For now, we show any
+    // operation that exists. We'll refine this in the future.
+
     $operations = id(new DrydockRepositoryOperationQuery())
       ->setViewer($viewer)
       ->withObjectPHIDs(array($revision->getPHID()))
       ->withIsDismissed(false)
-      ->withOperationTypes(
-        array(
-          DrydockLandRepositoryOperation::OPCONST,
-        ))
       ->execute();
     if (!$operations) {
       return null;

From ca17e2283d92e9f49ed0ef2870895766e82827a3 Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Tue, 25 Jul 2017 13:25:57 -0700
Subject: [PATCH 05/12] Have Maniphest use create transactions when using email

Summary: Fixes T12929. Sets a create transaction if new.

Test Plan: test a new task over email via command line

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12929

Differential Revision: https://secure.phabricator.com/D18279
---
 src/applications/maniphest/mail/ManiphestReplyHandler.php | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php
index bbcfe86551..cc2bdff9d0 100644
--- a/src/applications/maniphest/mail/ManiphestReplyHandler.php
+++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php
@@ -24,6 +24,10 @@ final class ManiphestReplyHandler
     $xactions = array();
 
     if ($is_new) {
+      $xactions[] = $this->newTransaction()
+        ->setTransactionType(PhabricatorTransactions::TYPE_CREATE)
+        ->setNewValue(true);
+
       $xactions[] = $this->newTransaction()
         ->setTransactionType(ManiphestTaskTitleTransaction::TRANSACTIONTYPE)
         ->setNewValue(nonempty($mail->getSubject(), pht('Untitled Task')));

From e7f94d7528eb885d8de8fc4f7ee6c942e96eadc0 Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Wed, 26 Jul 2017 09:29:47 -0700
Subject: [PATCH 06/12] Properly version Legalpad documents

Summary: Fixes T12933. This now creates a new DocumentBody when creating or editing a legalpad document.

Test Plan:
Create a new document, edit document. Check database that version is saved as new row, and timestamps are correct.

```mysql> select * from legalpad_documentbody;
+----+--------------------------------+--------------------------------+--------------------------------+---------+---------------+--------+-------------+--------------+
| id | phid                           | creatorPHID                    | documentPHID                   | version | title         | text   | dateCreated | dateModified |
+----+--------------------------------+--------------------------------+--------------------------------+---------+---------------+--------+-------------+--------------+
|  1 | PHID-LEGB-nsgzqklzfmjahlcgobm7 | PHID-USER-72xwu7eurrpsu2kxgrvw | PHID-LEGD-v7mc3xyithjvbiqeksbj |       2 | Legal Title 1 | Body 2 |  1501037011 |   1501037081 |
|  2 | PHID-LEGB-2kaytwmjusljib6pjycc | PHID-USER-72xwu7eurrpsu2kxgrvw | PHID-LEGD-v7mc3xyithjvbiqeksbj |       3 | Legal Title 1 | Body 3 |  1501037521 |   1501037521 |
|  3 | PHID-LEGB-h6q6bi42w4rgxrhk3qdb | PHID-USER-72xwu7eurrpsu2kxgrvw | PHID-LEGD-7gxuhafvkoy2izkv4gdd |       1 | New 2         | asdf   |  1501037553 |   1501037553 |
+----+--------------------------------+--------------------------------+--------------------------------+---------+---------------+--------+-------------+--------------+
3 rows in set (0.00 sec)```

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: tmakarios, Korvin

Maniphest Tasks: T12933

Differential Revision: https://secure.phabricator.com/D18280
---
 .../sql/autopatches/20170725.legalpad.date.01.sql     |  2 ++
 .../legalpad/editor/LegalpadDocumentEditor.php        | 11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)
 create mode 100644 resources/sql/autopatches/20170725.legalpad.date.01.sql

diff --git a/resources/sql/autopatches/20170725.legalpad.date.01.sql b/resources/sql/autopatches/20170725.legalpad.date.01.sql
new file mode 100644
index 0000000000..a091220894
--- /dev/null
+++ b/resources/sql/autopatches/20170725.legalpad.date.01.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_legalpad.legalpad_documentbody
+  SET dateCreated = dateModified;
diff --git a/src/applications/legalpad/editor/LegalpadDocumentEditor.php b/src/applications/legalpad/editor/LegalpadDocumentEditor.php
index 7882635285..35f2487a81 100644
--- a/src/applications/legalpad/editor/LegalpadDocumentEditor.php
+++ b/src/applications/legalpad/editor/LegalpadDocumentEditor.php
@@ -45,18 +45,23 @@ final class LegalpadDocumentEditor
     }
 
     if ($is_contribution) {
+      $text = $object->getDocumentBody()->getText();
+      $title = $object->getDocumentBody()->getTitle();
       $object->setVersions($object->getVersions() + 1);
-      $body = $object->getDocumentBody();
+
+      $body = new LegalpadDocumentBody();
+      $body->setCreatorPHID($this->getActingAsPHID());
+      $body->setText($text);
+      $body->setTitle($title);
       $body->setVersion($object->getVersions());
       $body->setDocumentPHID($object->getPHID());
       $body->save();
 
       $object->setDocumentBodyPHID($body->getPHID());
 
-      $actor = $this->getActor();
       $type = PhabricatorContributedToObjectEdgeType::EDGECONST;
       id(new PhabricatorEdgeEditor())
-        ->addEdge($actor->getPHID(), $type, $object->getPHID())
+        ->addEdge($this->getActingAsPHID(), $type, $object->getPHID())
         ->save();
 
       $type = PhabricatorObjectHasContributorEdgeType::EDGECONST;

From e47f85cd98b869fe97b3e6143f5de0670039d6da Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 26 Jul 2017 10:09:36 -0700
Subject: [PATCH 07/12] In Differential, filter repository operations to just
 "Land" operations again

Summary:
Reverts D18276. See PHI18 for discussion. The additional rules here (roughly, "only show the first successful operation") didn't actually work out for the other types of operations.

This is all just figuring out a stopgap, T12935 and other changes should eventually provide real pathways here.

Test Plan: Straight revert.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D18281
---
 .../controller/DifferentialRevisionViewController.php     | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index edf65b01ea..ab03ee6d81 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -1051,14 +1051,14 @@ final class DifferentialRevisionViewController extends DifferentialController {
       return null;
     }
 
-    // NOTE: The upstream currently supports only "Land" operations, but
-    // third parties have other operations (see PHI18). For now, we show any
-    // operation that exists. We'll refine this in the future.
-
     $operations = id(new DrydockRepositoryOperationQuery())
       ->setViewer($viewer)
       ->withObjectPHIDs(array($revision->getPHID()))
       ->withIsDismissed(false)
+      ->withOperationTypes(
+        array(
+          DrydockLandRepositoryOperation::OPCONST,
+        ))
       ->execute();
     if (!$operations) {
       return null;

From a7121f022f77dc4e30ba937211c3901ee00ac7ff Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Thu, 27 Jul 2017 15:12:48 -0700
Subject: [PATCH 08/12] Fix Firefox select dropdowns, maybe

Summary: Fixes T12930. I can't verify this fix, but the colors here work in light/dark mode correctly.

Test Plan: Wait for @cspeckmim to verify

Reviewers: epriestley, cspeckmim

Reviewed By: cspeckmim

Subscribers: cspeckmim, Korvin

Maniphest Tasks: T12930

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

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index fb20ebfa55..23e9733c10 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => 'b5b51108',
-    'core.pkg.css' => 'c0a7ecfd',
+    'core.pkg.css' => 'd6046dd9',
     'core.pkg.js' => '5d80e0db',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '45951e9e',
@@ -156,7 +156,7 @@ return array(
     'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9',
     'rsrc/css/phui/phui-fontkit.css' => '1320ed01',
     'rsrc/css/phui/phui-form-view.css' => '6175808d',
-    'rsrc/css/phui/phui-form.css' => 'efa86a27',
+    'rsrc/css/phui/phui-form.css' => '7aaa04e3',
     'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f',
     'rsrc/css/phui/phui-header-view.css' => 'e7de7ee2',
     'rsrc/css/phui/phui-hovercard.css' => 'f0592bcf',
@@ -843,7 +843,7 @@ return array(
     'phui-feed-story-css' => '44a9c8e9',
     'phui-font-icon-base-css' => '870a7360',
     'phui-fontkit-css' => '1320ed01',
-    'phui-form-css' => 'efa86a27',
+    'phui-form-css' => '7aaa04e3',
     'phui-form-view-css' => '6175808d',
     'phui-head-thing-view-css' => 'fd311e5f',
     'phui-header-view-css' => 'e7de7ee2',
diff --git a/webroot/rsrc/css/phui/phui-form.css b/webroot/rsrc/css/phui/phui-form.css
index 98e6e5b28f..1c587b5d3d 100644
--- a/webroot/rsrc/css/phui/phui-form.css
+++ b/webroot/rsrc/css/phui/phui-form.css
@@ -22,7 +22,7 @@ div.jx-tokenizer-container {
   display: inline-block;
   height: 30px;
   line-height: 18px;
-  color: {$darkbluetext};
+  color: inherit;
   vertical-align: middle;
   font: {$basefont};
   -webkit-font-smoothing: antialiased;
@@ -108,7 +108,6 @@ select {
   background: {$page.content} url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAUCAMAAACzvE1FAAAADFBMVEUzMzMzMzMzMzMzMzMKAG/3AAAAA3RSTlMAf4C/aSLHAAAAPElEQVR42q3NMQ4AIAgEQTn//2cLdRKppSGzBYwzVXvznNWs8C58CiussPJj8h6NwgorrKRdTvuV9v16Afn0AYFOB7aYAAAAAElFTkSuQmCC") no-repeat right 8px center;
   background-size: 8px 10px;
   border-radius: 3px;
-  color: {$darkbluetext};
   border: 1px solid {$greyborder};
   height: 30px;
   padding: 0 24px 0 8px;

From 9cf5bc30cdc057d39fa318583defeb2f285de65e Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Thu, 27 Jul 2017 14:30:35 -0700
Subject: [PATCH 09/12] Fix some copy and bugs in Ponder

Summary: Fixes T12939. Fixes some feed stories, mailtags, and headers.

Test Plan: Review changes locally by asking how babby is formed.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12939

Differential Revision: https://secure.phabricator.com/D18285
---
 .../controller/PonderQuestionViewController.php |  2 +-
 .../ponder/editor/PonderQuestionEditor.php      |  4 ++--
 .../ponder/storage/PonderAnswerTransaction.php  |  7 +++++++
 .../xaction/PonderAnswerContentTransaction.php  | 17 +++++++++++++++++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/applications/ponder/controller/PonderQuestionViewController.php b/src/applications/ponder/controller/PonderQuestionViewController.php
index 59da519834..2cd555204c 100644
--- a/src/applications/ponder/controller/PonderQuestionViewController.php
+++ b/src/applications/ponder/controller/PonderQuestionViewController.php
@@ -94,7 +94,7 @@ final class PonderQuestionViewController extends PonderController {
       $wiki = new PHUIRemarkupView($viewer, $question->getAnswerWiki());
       $answer_wiki = id(new PHUIObjectBoxView())
         ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
-        ->setHeaderText(pht('ANSWER SUMMARY'))
+        ->setHeaderText(pht('Answer Summary'))
         ->appendChild($wiki)
         ->addClass('ponder-answer-wiki');
     }
diff --git a/src/applications/ponder/editor/PonderQuestionEditor.php b/src/applications/ponder/editor/PonderQuestionEditor.php
index a17aebecf8..0720f436b9 100644
--- a/src/applications/ponder/editor/PonderQuestionEditor.php
+++ b/src/applications/ponder/editor/PonderQuestionEditor.php
@@ -10,11 +10,11 @@ final class PonderQuestionEditor
   }
 
   public function getCreateObjectTitle($author, $object) {
-    return pht('%s created this question.', $author);
+    return pht('%s asked this question.', $author);
   }
 
   public function getCreateObjectTitleForFeed($author, $object) {
-    return pht('%s created %s.', $author, $object);
+    return pht('%s asked %s.', $author, $object);
   }
 
   /**
diff --git a/src/applications/ponder/storage/PonderAnswerTransaction.php b/src/applications/ponder/storage/PonderAnswerTransaction.php
index 784b8a36e7..efbe020106 100644
--- a/src/applications/ponder/storage/PonderAnswerTransaction.php
+++ b/src/applications/ponder/storage/PonderAnswerTransaction.php
@@ -23,4 +23,11 @@ final class PonderAnswerTransaction
     return 'PonderAnswerTransactionType';
   }
 
+  public function getMailTags() {
+    $tags = parent::getMailTags();
+    $tags[] = PonderQuestionTransaction::MAILTAG_OTHER;
+
+    return $tags;
+  }
+
 }
diff --git a/src/applications/ponder/xaction/PonderAnswerContentTransaction.php b/src/applications/ponder/xaction/PonderAnswerContentTransaction.php
index 5d5c6ad157..2e55ef5fea 100644
--- a/src/applications/ponder/xaction/PonderAnswerContentTransaction.php
+++ b/src/applications/ponder/xaction/PonderAnswerContentTransaction.php
@@ -14,12 +14,29 @@ final class PonderAnswerContentTransaction
   }
 
   public function getTitle() {
+    $old = $this->getOldValue();
+
+    if (!strlen($old)) {
+      return pht(
+        '%s added an answer.',
+        $this->renderAuthor());
+    }
+
     return pht(
       '%s updated the answer details.',
       $this->renderAuthor());
   }
 
   public function getTitleForFeed() {
+    $old = $this->getOldValue();
+
+    if (!strlen($old)) {
+      return pht(
+        '%s added %s.',
+        $this->renderAuthor(),
+        $this->renderObject());
+    }
+
     return pht(
       '%s updated the answer details for %s.',
       $this->renderAuthor(),

From d114bc4a19bd31f746dcfd8d6bc0e840621ebd27 Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Thu, 27 Jul 2017 15:17:20 -0700
Subject: [PATCH 10/12] Fix dark mode highlight in Timeline

Summary: Moves these to `gentle.highlight`

Test Plan: View a timeline that is collapsed, see correct color in Dark Mode.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18287
---
 webroot/rsrc/css/phui/phui-timeline-view.css | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/webroot/rsrc/css/phui/phui-timeline-view.css b/webroot/rsrc/css/phui/phui-timeline-view.css
index b80a4e8d75..33e0f8ed0c 100644
--- a/webroot/rsrc/css/phui/phui-timeline-view.css
+++ b/webroot/rsrc/css/phui/phui-timeline-view.css
@@ -344,8 +344,8 @@
 }
 
 .phui-timeline-older-transactions-are-hidden {
-  background: {$sh-yellowbackground};
-  border: 1px solid {$sh-yellowborder};
+  background: {$gentle.highlight};
+  border: 1px solid {$gentle.highlight.border};
   text-align: center;
   padding: 12px;
   color: {$darkgreytext};

From ee884db1f9358d263fb3f6bbaae5fda285ea38b3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 28 Jul 2017 10:37:53 -0700
Subject: [PATCH 11/12] Don't fatal when viewing tags pointing at commits we
 haven't imported/parsed yet

Summary:
In Diffusion, the "Tags" view may read commits which haven't imported or parsed yet, and thus don't have loadable objects.

Most of this logic tests for `if ($commit)`, but the author part did not. Instead, don't render author information if `$commit` is not present.

Test Plan:
  - Loaded tags view with commits present.
  - Faked `$commit = null;`, loaded tag view, got this instead of a fatal:

{F5068432}

Reviewers: chad, amckinley

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D18290
---
 .../diffusion/view/DiffusionTagListView.php   | 47 ++++++++++++-------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/src/applications/diffusion/view/DiffusionTagListView.php b/src/applications/diffusion/view/DiffusionTagListView.php
index abab9003c3..0f8cbeae71 100644
--- a/src/applications/diffusion/view/DiffusionTagListView.php
+++ b/src/applications/diffusion/view/DiffusionTagListView.php
@@ -52,24 +52,12 @@ final class DiffusionTagListView extends DiffusionView {
           'commit' => $tag->getCommitIdentifier(),
         ));
 
-      $author = null;
-      if ($commit && $commit->getAuthorPHID()) {
-        $author = $this->handles[$commit->getAuthorPHID()]->renderLink();
-      } else if ($commit && $commit->getCommitData()) {
-        $author = self::renderName($commit->getCommitData()->getAuthorName());
+      if ($commit) {
+        $author = $this->renderAuthor($tag, $commit);
       } else {
-        $author = self::renderName($tag->getAuthor());
+        $author = null;
       }
 
-      $committed = phabricator_datetime($commit->getEpoch(), $viewer);
-      $author_name = phutil_tag(
-        'strong',
-        array(
-          'class' => 'diffusion-history-author-name',
-        ),
-        $author);
-      $authored = pht('%s on %s.', $author_name, $committed);
-
       $description = null;
       if ($tag->getType() == 'git/tag') {
         // In Git, a tag may be a "real" tag, or just a reference to a commit.
@@ -139,16 +127,43 @@ final class DiffusionTagListView extends DiffusionView {
         ->setHref($tag_href)
         ->addAttribute(array($commit_tag))
         ->addAttribute($description)
-        ->addAttribute($authored)
         ->setSideColumn(array(
           $build_view,
           $button_bar,
         ));
 
+      if ($author) {
+        $item->addAttribute($author);
+      }
+
       $list->addItem($item);
     }
 
     return $list;
   }
 
+  private function renderAuthor(
+    DiffusionRepositoryTag $tag,
+    PhabricatorRepositoryCommit $commit) {
+    $viewer = $this->getViewer();
+
+    if ($commit->getAuthorPHID()) {
+      $author = $this->handles[$commit->getAuthorPHID()]->renderLink();
+    } else if ($commit->getCommitData()) {
+      $author = self::renderName($commit->getCommitData()->getAuthorName());
+    } else {
+      $author = self::renderName($tag->getAuthor());
+    }
+
+    $committed = phabricator_datetime($commit->getEpoch(), $viewer);
+    $author_name = phutil_tag(
+      'strong',
+      array(
+        'class' => 'diffusion-history-author-name',
+      ),
+      $author);
+
+    return pht('%s on %s.', $author_name, $committed);
+  }
+
 }

From ea0db5aa9d574d65ecec7701e18938597f44ecea Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Fri, 28 Jul 2017 15:11:08 -0700
Subject: [PATCH 12/12] Clean up dropdown carets

Summary: Adds dropdown carets to buttons more universally that are actually dropdowns.

Test Plan: Differential, Application Search, Diffusion. Mobile and Desktop.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18292
---
 resources/celerity/map.php                    |  6 +--
 ...PhabricatorApplicationSearchController.php |  2 +-
 src/view/phui/PHUIButtonView.php              | 48 +++++++++----------
 webroot/rsrc/css/phui/button/phui-button.css  |  2 +-
 4 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 23e9733c10..86c6567f0e 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => 'b5b51108',
-    'core.pkg.css' => 'd6046dd9',
+    'core.pkg.css' => 'd9c9cfd0',
     'core.pkg.js' => '5d80e0db',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '45951e9e',
@@ -127,7 +127,7 @@ return array(
     'rsrc/css/layout/phabricator-source-code-view.css' => 'aea41829',
     'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494',
     'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68',
-    'rsrc/css/phui/button/phui-button.css' => '3a744520',
+    'rsrc/css/phui/button/phui-button.css' => '3ca51caa',
     'rsrc/css/phui/calendar/phui-calendar-day.css' => '572b1893',
     'rsrc/css/phui/calendar/phui-calendar-list.css' => '576be600',
     'rsrc/css/phui/calendar/phui-calendar-month.css' => '21154caf',
@@ -825,7 +825,7 @@ return array(
     'phui-big-info-view-css' => 'd13afcde',
     'phui-box-css' => '745e881d',
     'phui-button-bar-css' => 'f1ff5494',
-    'phui-button-css' => '3a744520',
+    'phui-button-css' => '3ca51caa',
     'phui-button-simple-css' => '8e1baf68',
     'phui-calendar-css' => 'f1ddf11c',
     'phui-calendar-day-css' => '572b1893',
diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php
index b0cab13e07..dbf964f9e4 100644
--- a/src/applications/search/controller/PhabricatorApplicationSearchController.php
+++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php
@@ -554,7 +554,7 @@ final class PhabricatorApplicationSearchController
     return id(new PHUIButtonView())
       ->setTag('a')
       ->setHref('#')
-      ->setText(pht('Use Results...'))
+      ->setText(pht('Use Results'))
       ->setIcon('fa-bars')
       ->setDropdownMenu($action_list)
       ->addClass('dropdown');
diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php
index 10ce5b5876..1aaa1cbaad 100644
--- a/src/view/phui/PHUIButtonView.php
+++ b/src/view/phui/PHUIButtonView.php
@@ -134,6 +134,7 @@ final class PHUIButtonView extends AphrontTagView {
     Javelin::initBehavior('phui-dropdown-menu');
 
     $this->addSigil('phui-dropdown-menu');
+    $this->setDropdown(true);
     $this->setMetadata($actions->getDropdownMenuMetadata());
 
     return $this;
@@ -223,32 +224,29 @@ final class PHUIButtonView extends AphrontTagView {
 
   protected function getTagContent() {
 
-    $icon = null;
-    $text = $this->text;
-    if ($this->icon) {
-      $icon = $this->icon;
+    $icon = $this->icon;
+    $text = null;
+    $subtext = null;
 
-      $subtext = null;
-      if ($this->subtext) {
-        $subtext = phutil_tag(
-          'div',
-          array(
-            'class' => 'phui-button-subtext',
-          ),
-        $this->subtext);
-      }
+    if ($this->subtext) {
+      $subtext = phutil_tag(
+        'div',
+        array(
+          'class' => 'phui-button-subtext',
+        ),
+      $this->subtext);
+    }
 
-      if ($this->text !== null) {
-        $text = phutil_tag(
-          'div',
-          array(
-            'class' => 'phui-button-text',
-          ),
-          array(
-            $text,
-            $subtext,
-          ));
-      }
+    if ($this->text !== null) {
+      $text = phutil_tag(
+        'div',
+        array(
+          'class' => 'phui-button-text',
+        ),
+        array(
+          $this->text,
+          $subtext,
+        ));
     }
 
     $caret = null;
@@ -259,7 +257,7 @@ final class PHUIButtonView extends AphrontTagView {
     if ($this->iconFirst == true) {
       return array($icon, $text, $caret);
     } else {
-      return array($text, $icon);
+      return array($text, $icon, $caret);
     }
   }
 }
diff --git a/webroot/rsrc/css/phui/button/phui-button.css b/webroot/rsrc/css/phui/button/phui-button.css
index ac3ae2176d..8cdd818f12 100644
--- a/webroot/rsrc/css/phui/button/phui-button.css
+++ b/webroot/rsrc/css/phui/button/phui-button.css
@@ -270,7 +270,7 @@ a.policy-control .phui-button-text {
 }
 
 .dropdown .phui-button-text {
-  margin-right: 16px;
+  margin-right: 8px;
 }
 
 .button.has-icon .phui-button-text {