From 93b08f0e83e684d3bb0c1c1b0ed414618a9987a6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 15 May 2020 13:25:52 -0700
Subject: [PATCH 01/23] Fix an out-of-order access issue with inlines

Summary: Ref T13513. On `secure`, I caught an issue where inlines may be accessed directly before they're constructed. Instead, access them through the relevant accessor.

Test Plan: Will deploy.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21263
---
 resources/celerity/map.php                    | 32 +++++++++----------
 .../rsrc/js/application/diff/DiffChangeset.js |  8 +++--
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index bc578201f4..68fcf9e951 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '42a2334f',
-    'differential.pkg.js' => '623b4801',
+    'differential.pkg.js' => 'b57da3e7',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -379,7 +379,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '68d963eb',
+    'rsrc/js/application/diff/DiffChangeset.js' => '0c083409',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'ac403c32',
     'rsrc/js/application/diff/DiffInline.js' => 'b00168c1',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
@@ -774,7 +774,7 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '68d963eb',
+    'phabricator-diff-changeset' => '0c083409',
     'phabricator-diff-changeset-list' => 'ac403c32',
     'phabricator-diff-inline' => 'b00168c1',
     'phabricator-diff-path-view' => '8207abf9',
@@ -1000,6 +1000,19 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
+    '0c083409' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+    ),
     '0cf79f45' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1512,19 +1525,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    '68d963eb' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-      'phuix-button-view',
-    ),
     '6a1583a8' => array(
       'javelin-behavior',
       'javelin-history',
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
index ebb50eb379..70bd92e6bc 100644
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -795,8 +795,10 @@ JX.install('DiffChangeset', {
     },
 
     _findInline: function(field, value) {
-      for (var ii = 0; ii < this._inlines.length; ii++) {
-        var inline = this._inlines[ii];
+      var inlines = this.getInlines();
+
+      for (var ii = 0; ii < inlines.length; ii++) {
+        var inline = inlines[ii];
 
         var target;
         switch (field) {
@@ -844,7 +846,7 @@ JX.install('DiffChangeset', {
     },
 
     redrawFileTree: function() {
-      var inlines = this._inlines;
+      var inlines = this.getInlines();
       var done = [];
       var undone = [];
       var inline;

From 7b0db3eb54b9ba948f836d400b73fd625ad6158b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 18 May 2020 07:32:05 -0700
Subject: [PATCH 02/23] Fix an email address validation UI feedback issue when
 creating new users

Summary: On the "New User" web workflow, if you use an invalid email address, you get a failure with an empty message.

Test Plan:
  - Before: Tried to create a new user with address "asdf". Got no specific guidance.
  - After: Got specific guidance about email address formatting and length.

Differential Revision: https://secure.phabricator.com/D21264
---
 .../people/controller/PhabricatorPeopleNewController.php     | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/applications/people/controller/PhabricatorPeopleNewController.php b/src/applications/people/controller/PhabricatorPeopleNewController.php
index 44dfe0e8a6..858ebef234 100644
--- a/src/applications/people/controller/PhabricatorPeopleNewController.php
+++ b/src/applications/people/controller/PhabricatorPeopleNewController.php
@@ -50,9 +50,12 @@ final class PhabricatorPeopleNewController
       if (!strlen($new_email)) {
         $errors[] = pht('Email is required.');
         $e_email = pht('Required');
-      } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) {
+      } else if (!PhabricatorUserEmail::isValidAddress($new_email)) {
+        $errors[] = PhabricatorUserEmail::describeValidAddresses();
         $e_email = pht('Invalid');
+      } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) {
         $errors[] = PhabricatorUserEmail::describeAllowedAddresses();
+        $e_email = pht('Not Allowed');
       } else {
         $e_email = null;
       }

From f86d822a37ea2c3cb174d03be538c3752f304021 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 18 May 2020 11:59:26 -0700
Subject: [PATCH 03/23] Update MySQL schema inspection code for deprecation of
 integer display widths

Summary:
Fixes T13536. See that task for discussion.

Older versions of MySQL (roughly, prior to 8.0.19) emit "int(10)" types. Newer versions emit "int" types. Accept these as equivalent.

Test Plan: Ran `bin/storage upgrade --force` against MySQL 8.0.11 and 8.0.20. Got clean adjustment lists on both versions.

Maniphest Tasks: T13536

Differential Revision: https://secure.phabricator.com/D21265
---
 .../schema/PhabricatorConfigColumnSchema.php  | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/src/applications/config/schema/PhabricatorConfigColumnSchema.php b/src/applications/config/schema/PhabricatorConfigColumnSchema.php
index 091b5caef9..e3e2b6f23b 100644
--- a/src/applications/config/schema/PhabricatorConfigColumnSchema.php
+++ b/src/applications/config/schema/PhabricatorConfigColumnSchema.php
@@ -68,6 +68,37 @@ final class PhabricatorConfigColumnSchema
     return $this->characterSet;
   }
 
+  public function hasSameColumnTypeAs(PhabricatorConfigColumnSchema $other) {
+    $u_type = $this->getColumnType();
+    $v_type = $other->getColumnType();
+
+    if ($u_type === $v_type) {
+      return true;
+    }
+
+    // See T13536. Display widths for integers were deprecated in MySQL 8.0.17
+    // and removed from some display contexts in or around 8.0.19. Older
+    // MySQL versions will report "int(10)"; newer versions will report "int".
+    // Accept these as equivalent.
+
+    static $map = array(
+      'int(10) unsigned' => 'int unsigned',
+      'int(10)' => 'int',
+      'bigint(20) unsigned' => 'bigint unsigned',
+      'bigint(20)' => 'bigint',
+    );
+
+    if (isset($map[$u_type])) {
+      $u_type = $map[$u_type];
+    }
+
+    if (isset($map[$v_type])) {
+      $v_type = $map[$v_type];
+    }
+
+    return ($u_type === $v_type);
+  }
+
   public function getKeyByteLength($prefix = null) {
     $type = $this->getColumnType();
 
@@ -138,7 +169,7 @@ final class PhabricatorConfigColumnSchema
         $issues[] = self::ISSUE_COLLATION;
       }
 
-      if ($this->getColumnType() != $expect->getColumnType()) {
+      if (!$this->hasSameColumnTypeAs($expect)) {
         $issues[] = self::ISSUE_COLUMNTYPE;
       }
 

From 0c51885cf7b2a94824b3fbf5de60623555cb9004 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 18 May 2020 19:55:48 -0700
Subject: [PATCH 04/23] Survive importing Git commits with no commit message
 and/or no author

Summary:
Ref T13538. See PHI1739. Synthetic Git commits with no author and/or no commit message currently extract `null` and then fail to parse.

Ideally, we would carefully distinguish between `null` and empty string. In practice, that requires significant schema changes (these columns are non-nullable and have indexing requirements) and these cases are degenerate. These commits are challenging to build and can not normally be constructed with `git commit`.

At least for now, merge the `null` cases into the empty string cases so we can survive import.

Test Plan:
  - Constructed a commit with no author and no commit message using the approach described in T13538; pushed and parsed it.
  - Before: fatals during identity selection and storing the commit message (both roughly NULL inserts into non-null columns).
  - After: clean import.

This produces a less-than-ideal UI in Diffusion, but it doesn't break anything:

{F7492094}

Maniphest Tasks: T13538

Differential Revision: https://secure.phabricator.com/D21266
---
 ...bricatorRepositoryCommitMessageParserWorker.php | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
index fef1c2eeeb..e589b52142 100644
--- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
+++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
@@ -62,7 +62,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
   final protected function updateCommitData(DiffusionCommitRef $ref) {
     $commit = $this->commit;
     $author = $ref->getAuthor();
-    $message = $ref->getMessage();
     $committer = $ref->getCommitter();
     $hashes = $ref->getHashes();
     $has_committer = (bool)strlen($committer);
@@ -73,6 +72,14 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       ->setViewer($viewer)
       ->setSourcePHID($commit->getPHID());
 
+    // See T13538. It is possible to synthetically construct a Git commit with
+    // no author and arrive here with NULL for the author value.
+
+    // This is distinct from a commit with an empty author. Because both these
+    // cases are degenerate and we can't resolve NULL into an identity, cast
+    // NULL to the empty string and merge the flows.
+    $author = phutil_string_cast($author);
+
     $author_identity = $identity_engine->newResolvedIdentity($author);
 
     if ($has_committer) {
@@ -102,6 +109,11 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       'authorPHID',
       $author_identity->getCurrentEffectiveUserPHID());
 
+    // See T13538. It is possible to synthetically construct a Git commit with
+    // no message. As above, treat this as though it is the same as the empty
+    // message.
+    $message = $ref->getMessage();
+    $message = phutil_string_cast($message);
     $data->setCommitMessage($message);
 
     if ($has_committer) {

From 6cf017d68003def9dcdd02be58384b89e61435d5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 08:57:42 -0700
Subject: [PATCH 05/23] Fix an unusual issue with intradiff highlighting of
 files with uncommon end-of-file modifications

Summary:
Fixes T13539. See that task for discussion and a reproduction case.

This algorithm currently counts "\ No newline at end of file" lines as though they were normal source lines. This can cause offset issues in the rare case that a diff contains two of these lines (for each side of the file) and has changes between them (because the last line of the file was modified between the diffs).

Instead, don't count "\" as a display line.

Test Plan:
  - See T13539 and PHI1740.
  - Before: got fatals on the "wild" diff and the synthetic simplified version.
  - After: clean intradiff rendering in both cases.

Maniphest Tasks: T13539

Differential Revision: https://secure.phabricator.com/D21267
---
 .../parser/DifferentialHunkParser.php         | 38 +++++++++++++++----
 .../DifferentialChangesetParserTestCase.php   | 14 +++----
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php
index 52f5bb90dc..2902022184 100644
--- a/src/applications/differential/parser/DifferentialHunkParser.php
+++ b/src/applications/differential/parser/DifferentialHunkParser.php
@@ -582,8 +582,13 @@ final class DifferentialHunkParser extends Phobject {
       $changes = $hunk->getSplitLines();
       foreach ($changes as $line) {
         $diff_type = $line[0]; // Change type in diff of diffs.
+        $is_same = ($diff_type === ' ');
+        $is_add = ($diff_type === '+');
+        $is_rem = ($diff_type === '-');
+
         $orig_type = $line[1]; // Change type in the original diff.
-        if ($diff_type == ' ') {
+
+        if ($is_same) {
           // Use the same key for lines that are next to each other.
           if ($olds_cursor > $news_cursor) {
             $key = $olds_cursor + 1;
@@ -594,17 +599,32 @@ final class DifferentialHunkParser extends Phobject {
           $news[$key] = null;
           $olds_cursor = $key;
           $news_cursor = $key;
-        } else if ($diff_type == '-') {
+        } else if ($is_rem) {
           $olds[] = array($n_old, $orig_type);
           $olds_cursor++;
-        } else if ($diff_type == '+') {
+        } else if ($is_add) {
           $news[] = array($n_new, $orig_type);
           $news_cursor++;
+        } else {
+          throw new Exception(
+            pht(
+              'Found unknown intradiff source line, expected a line '.
+              'beginning with "+", "-", or " " (space): %s.',
+              $line));
         }
-        if (($diff_type == '-' || $diff_type == ' ') && $orig_type != '-') {
+
+        // See T13539. Don't increment the line count if this line was removed,
+        // or if the line is a "No newline at end of file" marker.
+        $not_a_line = ($orig_type === '-' || $orig_type === '\\');
+        if ($not_a_line) {
+          continue;
+        }
+
+        if ($is_same || $is_rem) {
           $n_old++;
         }
-        if (($diff_type == '+' || $diff_type == ' ') && $orig_type != '-') {
+
+        if ($is_same || $is_add) {
           $n_new++;
         }
       }
@@ -623,14 +643,18 @@ final class DifferentialHunkParser extends Phobject {
         list($n, $type) = $olds[$i];
         if ($type == '+' ||
             ($type == ' ' && isset($news[$i]) && $news[$i][1] != ' ')) {
-          $highlight_old[] = $offsets_old[$n];
+          if (isset($offsets_old[$n])) {
+            $highlight_old[] = $offsets_old[$n];
+          }
         }
       }
       if (isset($news[$i])) {
         list($n, $type) = $news[$i];
         if ($type == '+' ||
             ($type == ' ' && isset($olds[$i]) && $olds[$i][1] != ' ')) {
-          $highlight_new[] = $offsets_new[$n];
+          if (isset($offsets_new[$n])) {
+            $highlight_new[] = $offsets_new[$n];
+          }
         }
       }
     }
diff --git a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php
index d8d10169b5..f84f166bf6 100644
--- a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php
+++ b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php
@@ -4,19 +4,19 @@ final class DifferentialChangesetParserTestCase extends PhabricatorTestCase {
 
   public function testDiffChangesets() {
     $hunk = new DifferentialHunk();
-    $hunk->setChanges("+a\n b\n-c");
+    $hunk->setChanges("+a\n b\n-c\n");
     $hunk->setNewOffset(1);
     $hunk->setNewLen(2);
     $left = new DifferentialChangeset();
     $left->attachHunks(array($hunk));
 
     $tests = array(
-      "+a\n b\n-c" => array(array(), array()),
-      "+a\n x\n-c" => array(array(), array()),
-      "+aa\n b\n-c" => array(array(1), array(11)),
-      " b\n-c" => array(array(1), array()),
-      "+a\n b\n c" => array(array(), array(13)),
-      "+a\n x\n c" => array(array(), array(13)),
+      "+a\n b\n-c\n" => array(array(), array()),
+      "+a\n x\n-c\n" => array(array(), array()),
+      "+aa\n b\n-c\n" => array(array(1), array(11)),
+      " b\n-c\n" => array(array(1), array()),
+      "+a\n b\n c\n" => array(array(), array(13)),
+      "+a\n x\n c\n" => array(array(), array(13)),
     );
 
     foreach ($tests as $changes => $expected) {

From 770a5c8412d1d8d1d33a1f8e36c81a64d594d1c9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 09:25:09 -0700
Subject: [PATCH 06/23] Fix "r" and "R" both replying with quote on inline
 comments

Summary: Ref T13513. The code which added "r" and "R" to the inline menu accidentally discarded the difference between the keystrokes.

Test Plan:
  - Clicked an inline, pressed "r", got new empty inline (previously: inline with quote).
  - Clicked an inline, pressed "R", got a new quoted inline.
  - Repeated steps with the menu items, got the expected behaviors.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21268
---
 resources/celerity/map.php                       | 16 ++++++++--------
 .../js/application/diff/DiffChangesetList.js     |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 68fcf9e951..53cf215ae8 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '42a2334f',
-    'differential.pkg.js' => 'b57da3e7',
+    'differential.pkg.js' => '8f59bce2',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -380,7 +380,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => '0c083409',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'ac403c32',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'db615898',
     'rsrc/js/application/diff/DiffInline.js' => 'b00168c1',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -775,7 +775,7 @@ return array(
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '0c083409',
-    'phabricator-diff-changeset-list' => 'ac403c32',
+    'phabricator-diff-changeset-list' => 'db615898',
     'phabricator-diff-inline' => 'b00168c1',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1911,11 +1911,6 @@ return array(
       'javelin-dom',
       'phabricator-notification',
     ),
-    'ac403c32' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     'ad258e28' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2124,6 +2119,11 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    'db615898' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'e150bd50' => array(
       'javelin-behavior',
       'javelin-stratcom',
diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js
index e5a6095da2..0397354859 100644
--- a/webroot/rsrc/js/application/diff/DiffChangesetList.js
+++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js
@@ -342,7 +342,7 @@ JX.install('DiffChangesetList', {
           var inline = cursor.target;
           if (inline.canReply()) {
             this.setFocus(null);
-            inline.reply(true);
+            inline.reply(is_quote);
             return;
           }
         }

From 86d6abe9db2632708043db7caa30bbdcd19f8f64 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 09:43:01 -0700
Subject: [PATCH 07/23] Fix an issue where builds with no initiator failed to
 render in build plans

Summary: See PHI1743. If a build has no initiator PHID, the rendering pathway incorrectly tries to access a handle for it anyway.

Test Plan:
  - Set a build to have no initiator PHID.
  - Viewed the build plan for the build.
  - Before: fatal when trying to access the `null` handle.
  - After: clean build plan rendering.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21269
---
 .../harbormaster/view/HarbormasterBuildView.php             | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/applications/harbormaster/view/HarbormasterBuildView.php b/src/applications/harbormaster/view/HarbormasterBuildView.php
index 54f5abe093..f54b1d3ba6 100644
--- a/src/applications/harbormaster/view/HarbormasterBuildView.php
+++ b/src/applications/harbormaster/view/HarbormasterBuildView.php
@@ -34,7 +34,7 @@ final class HarbormasterBuildView
     $list = new PHUIObjectItemListView();
     foreach ($builds as $build) {
       $id = $build->getID();
-      $initiator = $handles[$build->getInitiatorPHID()];
+
       $buildable_object = $handles[$build->getBuildable()->getBuildablePHID()];
 
       $item = id(new PHUIObjectItemView())
@@ -46,7 +46,9 @@ final class HarbormasterBuildView
         ->setEpoch($build->getDateCreated())
         ->addAttribute($buildable_object->getName());
 
-      if ($initiator) {
+      $initiator_phid = $build->getInitiatorPHID();
+      if ($initiator_phid) {
+        $initiator = $handles[$initiator_phid];
         $item->addByline($initiator->renderLink());
       }
 

From 43a8d8763df3140033874676a4714280144f0445 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 10:29:43 -0700
Subject: [PATCH 08/23] Update out-of-date API calls when rendering diffs
 inline in email

Summary: See PHI1745. This callsite for "ChangesetParser" was not properly updated for recent changes.

Test Plan:
  - Set `metamta.differential.inline-patches` to 100.
  - Created a new revision with a small (<100 line) diff, with at least one reviewer.
  - Ran `bin/phd debug` and observed outbound mail queue with `bin/mail list-outbound`.
  - Before: fatal when trying to generate the inline changes for mail.
  - After: clean mail generation.

Differential Revision: https://secure.phabricator.com/D21270
---
 .../differential/mail/DifferentialChangeDetailMailView.php    | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/applications/differential/mail/DifferentialChangeDetailMailView.php b/src/applications/differential/mail/DifferentialChangeDetailMailView.php
index d0f0be71b1..1f0eac537e 100644
--- a/src/applications/differential/mail/DifferentialChangeDetailMailView.php
+++ b/src/applications/differential/mail/DifferentialChangeDetailMailView.php
@@ -40,11 +40,13 @@ final class DifferentialChangeDetailMailView
     $diff = $this->getDiff();
 
     $engine = new PhabricatorMarkupEngine();
+    $viewstate = new PhabricatorChangesetViewState();
 
     $out = array();
     foreach ($diff->getChangesets() as $changeset) {
       $parser = id(new DifferentialChangesetParser())
-        ->setUser($viewer)
+        ->setViewer($viewer)
+        ->setViewState($viewstate)
         ->setChangeset($changeset)
         ->setLinesOfContext(2)
         ->setMarkupEngine($engine);

From 4257b26abc61ad24b3ffd3cdd2e2f798499d0742 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 10:36:43 -0700
Subject: [PATCH 09/23] Treat PHP7 "Throwable" exceptions like other unhandled
 "Exception" cases in the worker queue

Summary: See PHI1745. Under PHP7, errors raised as Throwable miss this "generic exception" logic and don't increment their failure count. Instead, treat any "Throwable" we don't recognize like any "Exception" we don't recognize.

Test Plan:
  - Under PHP7, caused a worker task to raise a Throwable (e.g., call to undefined method, see D21270).
  - Ran `bin/worker execute --id ...`.
  - Before: worker failed, but did not increment failure count.
  - After: worker fails and increments failure count as it would for other types of unknown error.

Differential Revision: https://secure.phabricator.com/D21271
---
 .../daemon/workers/storage/PhabricatorWorkerActiveTask.php | 7 +++++++
 .../daemon/workers/storage/PhabricatorWorkerTask.php       | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
index 7139e39ac3..dfb2bf159d 100644
--- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
+++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
@@ -134,6 +134,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask {
 
     $did_succeed = false;
     $worker = null;
+    $caught = null;
     try {
       $worker = $this->getWorkerInstance();
       $worker->setCurrentWorkerTask($this);
@@ -180,6 +181,12 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask {
 
       $result = $this;
     } catch (Exception $ex) {
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $caught = $ex;
+    }
+
+    if ($caught) {
       $this->setExecutionException($ex);
       $this->setFailureCount($this->getFailureCount() + 1);
       $this->setFailureTime(time());
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php
index b0937c4e6d..2fc26503ea 100644
--- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php
+++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php
@@ -34,7 +34,7 @@ abstract class PhabricatorWorkerTask extends PhabricatorWorkerDAO {
     ) + parent::getConfiguration();
   }
 
-  final public function setExecutionException(Exception $execution_exception) {
+  final public function setExecutionException($execution_exception) {
     $this->executionException = $execution_exception;
     return $this;
   }

From 9d5b8bd14a973bd987d2edc1ee49621241ba2f62 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 15 May 2020 06:20:28 -0700
Subject: [PATCH 10/23] Remove PHPMailer code which generates bogus
 "Message-ID" email headers

Summary:
See <https://discourse.phabricator-community.org/t/how-to-override-localhost-localdomain-in-email-message-id/3876/>.

Currently, Phabricator generates a "Message-ID" only in a subset of cases (roughly: when the message is first-in-thread and we expect the thread may have more than one message).

In cases where it does not generate a message ID, it expects the SMTP server to generate one for it. Servers will generally do this, and some ONLY do this (that is, they ignore IDs from Phabricator and replace them). Thus, several pieces of configuration control whether Phabricator attempts to generate a "Message-ID" at all.

The PHPMailer code has fallback behavior which generates a "<random>@localhost.localdomain" message ID. This is never desirable and ignores Phabricator-level configuration that Message IDs should not be generated.

For now, remove this code: it is never the desired behavior and sometimes explicitly contradicts the intent of configuration.

Possibly, a better change may be to make Phabricator always generate a message ID in cases where it isn't forbidden from doing so by configuration. However, that's a more complicated change and it's not clear if/when it would produce better behavior, so start here for now.

Test Plan: Confirmed by affected user (see linked thread).

Differential Revision: https://secure.phabricator.com/D21272
---
 externals/phpmailer/class.phpmailer.php | 2 --
 1 file changed, 2 deletions(-)

diff --git a/externals/phpmailer/class.phpmailer.php b/externals/phpmailer/class.phpmailer.php
index 5ddbddc144..69f9c45ba5 100644
--- a/externals/phpmailer/class.phpmailer.php
+++ b/externals/phpmailer/class.phpmailer.php
@@ -1110,8 +1110,6 @@ class PHPMailer {
 
     if($this->MessageID != '') {
       $result .= $this->HeaderLine('Message-ID',$this->MessageID);
-    } else {
-      $result .= sprintf("Message-ID: <%s@%s>%s", $uniq_id, $this->ServerHostname(), $this->LE);
     }
     $result .= $this->HeaderLine('X-Priority', $this->Priority);
     $result .= $this->HeaderLine('X-Mailer', 'PHPMailer '.$this->Version.' (phpmailer.sourceforge.net)');

From 87bc30526b7a7a13b5ef6ae697549b7ad459f4f6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 13:46:58 -0700
Subject: [PATCH 11/23] Make inline content "state-oriented", not
 "string-oriented"

Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.

This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.

Test Plan: Created, edited, submitted, cancelled, etc., comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21273
---
 resources/celerity/map.php                    |  56 +++----
 .../PhabricatorInlineCommentController.php    |  90 +++++++-----
 .../interface/PhabricatorInlineComment.php    |   5 +
 .../view/PHUIDiffInlineCommentEditView.php    |  27 ----
 .../diff/view/PHUIDiffInlineCommentView.php   |   3 +-
 .../rsrc/js/application/diff/DiffChangeset.js |   4 +-
 .../js/application/diff/DiffChangesetList.js  |   2 +-
 .../rsrc/js/application/diff/DiffInline.js    | 138 +++++++++++-------
 8 files changed, 176 insertions(+), 149 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 53cf215ae8..22d8d56457 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '42a2334f',
-    'differential.pkg.js' => '8f59bce2',
+    'differential.pkg.js' => 'ff8ca085',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -379,9 +379,9 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '0c083409',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'db615898',
-    'rsrc/js/application/diff/DiffInline.js' => 'b00168c1',
+    'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
+    'rsrc/js/application/diff/DiffInline.js' => '28e53a2c',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -774,9 +774,9 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '0c083409',
-    'phabricator-diff-changeset-list' => 'db615898',
-    'phabricator-diff-inline' => 'b00168c1',
+    'phabricator-diff-changeset' => '6e5e03d2',
+    'phabricator-diff-changeset-list' => 'b51ba93a',
+    'phabricator-diff-inline' => '28e53a2c',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1000,19 +1000,6 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
-    '0c083409' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-      'phuix-button-view',
-    ),
     '0cf79f45' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1150,6 +1137,9 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
+    '28e53a2c' => array(
+      'javelin-dom',
+    ),
     '29819b75' => array(
       'phabricator-notification',
       'javelin-stratcom',
@@ -1561,6 +1551,19 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
+    '6e5e03d2' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+    ),
     70245195 => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1935,9 +1938,6 @@ return array(
       'javelin-behavior-device',
       'javelin-vector',
     ),
-    'b00168c1' => array(
-      'javelin-dom',
-    ),
     'b105a3a6' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1970,6 +1970,11 @@ return array(
     'b517bfa0' => array(
       'phui-oi-list-view-css',
     ),
+    'b51ba93a' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'b557770a' => array(
       'javelin-install',
       'javelin-util',
@@ -2119,11 +2124,6 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
-    'db615898' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     'e150bd50' => array(
       'javelin-behavior',
       'javelin-stratcom',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 4d4b1c0d2c..4fa465d8ee 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -39,7 +39,6 @@ abstract class PhabricatorInlineCommentController
   private $isOnRight;
   private $lineNumber;
   private $lineLength;
-  private $commentText;
   private $operation;
   private $commentID;
   private $renderer;
@@ -99,16 +98,16 @@ abstract class PhabricatorInlineCommentController
     $request = $this->getRequest();
     $viewer = $this->getViewer();
 
+    if (!$request->validateCSRF()) {
+      return new Aphront404Response();
+    }
+
     $this->readRequestParameters();
 
     $op = $this->getOperation();
     switch ($op) {
       case 'hide':
       case 'show':
-        if (!$request->validateCSRF()) {
-          return new Aphront404Response();
-        }
-
         $ids = $request->getStrList('ids');
         if ($ids) {
           if ($op == 'hide') {
@@ -120,9 +119,6 @@ abstract class PhabricatorInlineCommentController
 
         return id(new AphrontAjaxResponse())->setContent(array());
       case 'done':
-        if (!$request->validateCSRF()) {
-          return new Aphront404Response();
-        }
         $inline = $this->loadCommentForDone($this->getCommentID());
 
         $is_draft_state = false;
@@ -158,10 +154,6 @@ abstract class PhabricatorInlineCommentController
       case 'delete':
       case 'undelete':
       case 'refdelete':
-        if (!$request->validateCSRF()) {
-          return new Aphront404Response();
-        }
-
         // NOTE: For normal deletes, we just process the delete immediately
         // and show an "Undo" action. For deletes by reference from the
         // preview ("refdelete"), we prompt first (because the "Undo" may
@@ -193,15 +185,14 @@ abstract class PhabricatorInlineCommentController
 
         return $this->buildEmptyResponse();
       case 'edit':
+      case 'save':
         $inline = $this->loadCommentByIDForEdit($this->getCommentID());
-        $text = $this->getCommentText();
+        if ($op === 'save') {
+          $this->updateCommentContentState($inline);
 
-        if ($request->isFormPost()) {
-          if (strlen($text)) {
-            $inline
-              ->setContent($text)
-              ->setIsEditing(false);
+          $inline->setIsEditing(false);
 
+          if (!$inline->isVoidComment($viewer)) {
             $this->saveComment($inline);
 
             return $this->buildRenderedCommentResponse(
@@ -216,7 +207,7 @@ abstract class PhabricatorInlineCommentController
           }
         } else {
           // NOTE: At time of writing, the "editing" state of inlines is
-          // preserved by simluating a click on "Edit" when the inline loads.
+          // preserved by simulating a click on "Edit" when the inline loads.
 
           // In this case, we don't want to "saveComment()", because it
           // recalculates object drafts and purges versioned drafts.
@@ -234,8 +225,8 @@ abstract class PhabricatorInlineCommentController
             $is_dirty = true;
           }
 
-          if (strlen($text)) {
-            $inline->setContent($text);
+          if ($this->hasContentState()) {
+            $this->updateCommentContentState($inline);
             $is_dirty = true;
           } else {
             PhabricatorInlineComment::loadAndAttachVersionedDrafts(
@@ -262,13 +253,9 @@ abstract class PhabricatorInlineCommentController
         // If the user uses "Undo" to get into an edited state ("AB"), then
         // clicks cancel to return to the previous state ("A"), we also want
         // to set the stored state back to "A".
-        $text = $this->getCommentText();
-        if (strlen($text)) {
-          $inline->setContent($text);
-        }
+        $this->updateCommentContentState($inline);
 
-        $content = $inline->getContent();
-        if (!strlen($content)) {
+        if ($inline->isVoidComment($viewer)) {
           $inline->setIsDeleted(1);
         }
 
@@ -283,11 +270,11 @@ abstract class PhabricatorInlineCommentController
           $viewer->getPHID(),
           $inline->getID());
 
-        $text = $this->getCommentText();
-
-        $versioned_draft
-          ->setProperty('inline.text', $text)
-          ->save();
+        $map = $this->getContentState();
+        foreach ($map as $key => $value) {
+          $versioned_draft->setProperty($key, $value);
+        }
+        $versioned_draft->save();
 
         // We have to synchronize the draft engine after saving a versioned
         // draft, because taking an inline comment from "no text, no draft"
@@ -318,11 +305,11 @@ abstract class PhabricatorInlineCommentController
           ->setIsNewFile($is_new)
           ->setLineNumber($number)
           ->setLineLength($length)
-          ->setContent((string)$this->getCommentText())
           ->setReplyToCommentPHID($this->getReplyToCommentPHID())
           ->setIsEditing(true)
           ->setStartOffset($request->getInt('startOffset'))
-          ->setEndOffset($request->getInt('endOffset'));
+          ->setEndOffset($request->getInt('endOffset'))
+          ->setContent('');
 
         $document_engine_key = $request->getStr('documentEngineKey');
         if ($document_engine_key !== null) {
@@ -338,6 +325,10 @@ abstract class PhabricatorInlineCommentController
           }
         }
 
+        if ($this->hasContentState()) {
+          $this->updateCommentContentState($inline);
+        }
+
         $this->saveComment($inline);
 
         $edit_dialog = $this->buildEditDialog($inline);
@@ -365,7 +356,6 @@ abstract class PhabricatorInlineCommentController
     $this->isOnRight = $request->getBool('on_right');
     $this->lineNumber = $request->getInt('number');
     $this->lineLength = $request->getInt('length');
-    $this->commentText = $request->getStr('text');
     $this->commentID = $request->getInt('id');
     $this->operation = $request->getStr('op');
     $this->renderer = $request->getStr('renderer');
@@ -529,6 +519,36 @@ abstract class PhabricatorInlineCommentController
     return $inline;
   }
 
+  private function hasContentState() {
+    $request = $this->getRequest();
+    return (bool)$request->getBool('hasContentState');
+  }
+
+  private function getContentState() {
+    $request = $this->getRequest();
+
+    $comment_text = $request->getStr('text');
+
+    return array(
+      'inline.text' => (string)$comment_text,
+    );
+  }
+
+  private function updateCommentContentState(PhabricatorInlineComment $inline) {
+    if (!$this->hasContentState()) {
+      throw new Exception(
+        pht(
+          'Attempting to update comment content state, but request has no '.
+          'content state.'));
+    }
+
+    $state = $this->getContentState();
+
+    $text = $state['inline.text'];
+
+    $inline->setContent($text);
+  }
+
   private function saveComment(PhabricatorInlineComment $inline) {
     $viewer = $this->getViewer();
     $draft_engine = $this->newDraftEngine();
diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
index ae0b950544..e5ac83139c 100644
--- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php
+++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
@@ -322,6 +322,11 @@ abstract class PhabricatorInlineComment
     return $draft_text;
   }
 
+  public function getContentState() {
+    return array(
+      'text' => $this->getContent(),
+    );
+  }
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
index b70b0bcd1f..fa61ef89a3 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
@@ -22,39 +22,12 @@ final class PHUIDiffInlineCommentEditView
         'sigil' => 'inline-edit-form',
       ),
       array(
-        $this->renderInputs(),
         $this->renderBody(),
       ));
 
     return $content;
   }
 
-  private function renderInputs() {
-    $inputs = array();
-    $inline = $this->getInlineComment();
-
-    $inputs[] = array('op', 'edit');
-    $inputs[] = array('id', $inline->getID());
-
-    $inputs[] = array('on_right', $this->getIsOnRight());
-    $inputs[] = array('renderer', $this->getRenderer());
-
-    $out = array();
-
-    foreach ($inputs as $input) {
-      list($name, $value) = $input;
-      $out[] = phutil_tag(
-        'input',
-        array(
-          'type'  => 'hidden',
-          'name'  => $name,
-          'value' => $value,
-        ));
-    }
-
-    return $out;
-  }
-
   private function renderBody() {
     $buttons = array();
 
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
index 2481442c1d..97fc9ee6e5 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
@@ -82,7 +82,6 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
       'number' => $inline->getLineNumber(),
       'length' => $inline->getLineLength(),
       'isNewFile' => (bool)$inline->getIsNewFile(),
-      'original' => $inline->getContent(),
       'replyToCommentPHID' => $inline->getReplyToCommentPHID(),
       'isDraft' => $inline->isDraft(),
       'isFixed' => $is_fixed,
@@ -93,8 +92,8 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
       'documentEngineKey' => $inline->getDocumentEngineKey(),
       'startOffset' => $inline->getStartOffset(),
       'endOffset' => $inline->getEndOffset(),
-
       'on_right' => $this->getIsOnRight(),
+      'contentState' => $inline->getContentState(),
     );
   }
 
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
index 70bd92e6bc..79328a52b4 100644
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -761,14 +761,14 @@ JX.install('DiffChangeset', {
       return inline;
     },
 
-    newInlineReply: function(original, text) {
+    newInlineReply: function(original, state) {
       var inline = new JX.DiffInline()
         .setChangeset(this)
         .bindToReply(original);
 
       this._inlines.push(inline);
 
-      inline.create(text);
+      inline.create(state);
 
       return inline;
     },
diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js
index 0397354859..e7cc8b52fe 100644
--- a/webroot/rsrc/js/application/diff/DiffChangesetList.js
+++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js
@@ -2410,7 +2410,7 @@ JX.install('DiffChangesetList', {
 
       switch (action) {
         case 'save':
-          inline.save(e.getTarget());
+          inline.save();
           break;
         case 'cancel':
           inline.cancel();
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index cfec020544..0e77ec59a2 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -19,7 +19,7 @@ JX.install('DiffInline', {
     _displaySide: null,
     _isNewFile: null,
     _replyToCommentPHID: null,
-    _originalText: null,
+    _originalState: null,
     _snippet: null,
     _menuItems: null,
     _documentEngineKey: null,
@@ -42,7 +42,7 @@ JX.install('DiffInline', {
     _editRow: null,
     _undoRow: null,
     _undoType: null,
-    _undoText: null,
+    _undoState: null,
 
     _draftRequest: null,
     _skipFocus: false,
@@ -76,12 +76,7 @@ JX.install('DiffInline', {
       this._number = parseInt(data.number, 10);
       this._length = parseInt(data.length, 10);
 
-      var original = '' + data.original;
-      if (original.length) {
-        this._originalText = original;
-      } else {
-        this._originalText = null;
-      }
+      this._originalState = data.contentState;
       this._isNewFile = data.isNewFile;
 
       this._replyToCommentPHID = data.replyToCommentPHID;
@@ -314,10 +309,6 @@ JX.install('DiffInline', {
       return this._hasMenuAction('collapse');
     },
 
-    getRawText: function() {
-      return this._originalText;
-    },
-
     _newRow: function() {
       var attributes = {
         sigil: 'inline-row'
@@ -332,7 +323,7 @@ JX.install('DiffInline', {
       this._phid = null;
       this._isCollapsed = false;
 
-      this._originalText = null;
+      this._originalState = null;
 
       return row;
     },
@@ -404,7 +395,7 @@ JX.install('DiffInline', {
       this._didUpdate();
     },
 
-    create: function(text) {
+    create: function(content_state) {
       var changeset = this.getChangeset();
       if (!this._documentEngineKey) {
         this._documentEngineKey = changeset.getResponseDocumentEngineKey();
@@ -412,7 +403,7 @@ JX.install('DiffInline', {
 
       var uri = this._getInlineURI();
       var handler = JX.bind(this, this._oncreateresponse);
-      var data = this._newRequestData('new', text);
+      var data = this._newRequestData('new', content_state);
 
       this.setLoading(true);
 
@@ -421,22 +412,33 @@ JX.install('DiffInline', {
         .send();
     },
 
+    _getContentState: function() {
+      var state;
+
+      if (this._editRow) {
+        state = this._readFormState(this._editRow);
+      } else {
+        state = this._originalState;
+      }
+
+      return state;
+    },
+
     reply: function(with_quote) {
       this._closeMenu();
 
-      var text;
+      var content_state = this._newContentState();
       if (with_quote) {
-        text = this.getRawText();
+        var text = this._getContentState().text;
         text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n';
-      } else {
-        text = '';
+        content_state.text = text;
       }
 
       var changeset = this.getChangeset();
-      return changeset.newInlineReply(this, text);
+      return changeset.newInlineReply(this, content_state);
     },
 
-    edit: function(text, skip_focus) {
+    edit: function(content_state, skip_focus) {
       this._closeMenu();
 
       this._skipFocus = !!skip_focus;
@@ -456,7 +458,7 @@ JX.install('DiffInline', {
       var uri = this._getInlineURI();
       var handler = JX.bind(this, this._oneditresponse);
 
-      var data = this._newRequestData('edit', text || null);
+      var data = this._newRequestData('edit', content_state);
 
       this.setLoading(true);
 
@@ -545,13 +547,12 @@ JX.install('DiffInline', {
       return this;
     },
 
-    _newRequestData: function(operation, text) {
+    _newRequestData: function(operation, content_state) {
       var data = {
         op: operation,
         is_new: this.isNewFile(),
         on_right: ((this.getDisplaySide() == 'right') ? 1 : 0),
-        renderer: this.getChangeset().getRendererKey(),
-        text: text || null
+        renderer: this.getChangeset().getRendererKey()
       };
 
       if (operation === 'new') {
@@ -574,6 +575,11 @@ JX.install('DiffInline', {
         JX.copy(data, edit_data);
       }
 
+      if (content_state) {
+        data.hasContentState = 1;
+        JX.copy(data, content_state);
+      }
+
       return data;
     },
 
@@ -608,14 +614,14 @@ JX.install('DiffInline', {
       // If there's an existing editor, remove it. This happens when you
       // delete a comment from the comment preview area. In this case, we
       // read and preserve the text so "Undo" restores it.
-      var text;
+      var state = null;
       if (this._editRow) {
-        text = this._readText(this._editRow);
+        state = this._readFormState(this._editRow);
         JX.DOM.remove(this._editRow);
         this._editRow = null;
       }
 
-      this._drawUndeleteRows(text);
+      this._drawUndeleteRows(state);
 
       this.setLoading(false);
       this.setDeleted(true);
@@ -623,21 +629,21 @@ JX.install('DiffInline', {
       this._didUpdate();
     },
 
-    _drawUndeleteRows: function(text) {
+    _drawUndeleteRows: function(content_state) {
       this._undoType = 'undelete';
-      this._undoText = text || null;
+      this._undoState = content_state || null;
 
       return this._drawUndoRows('undelete', this._row);
     },
 
-    _drawUneditRows: function(text) {
+    _drawUneditRows: function(content_state) {
       this._undoType = 'unedit';
-      this._undoText = text;
+      this._undoState = content_state;
 
-      return this._drawUndoRows('unedit', null, text);
+      return this._drawUndoRows('unedit', null);
     },
 
-    _drawUndoRows: function(mode, cursor, text) {
+    _drawUndoRows: function(mode, cursor) {
       var templates = this.getChangeset().getUndoTemplates();
 
       var template;
@@ -648,7 +654,7 @@ JX.install('DiffInline', {
       }
       template = JX.$H(template).getNode();
 
-      this._undoRow = this._drawRows(template, cursor, mode, text);
+      this._undoRow = this._drawRows(template, cursor, mode);
     },
 
     _drawContentRows: function(rows) {
@@ -660,7 +666,7 @@ JX.install('DiffInline', {
       this._editRow = this._drawRows(rows, null, 'edit');
     },
 
-    _drawRows: function(rows, cursor, type, text) {
+    _drawRows: function(rows, cursor, type) {
       var first_row = JX.DOM.scry(rows, 'tr')[0];
       var row = first_row;
       var anchor = cursor || this._row;
@@ -713,14 +719,17 @@ JX.install('DiffInline', {
       return result_row;
     },
 
-    save: function(form) {
+    save: function() {
       var handler = JX.bind(this, this._onsubmitresponse);
 
       this.setLoading(true);
 
-      JX.Workflow.newFromForm(form)
-        .setHandler(handler)
-        .start();
+      var uri = this._getInlineURI();
+      var data = this._newRequestData('save', this._getContentState());
+
+      new JX.Request(uri, handler)
+        .setData(data)
+        .send();
     },
 
     undo: function() {
@@ -740,8 +749,8 @@ JX.install('DiffInline', {
           .send();
       }
 
-      if (this._undoText !== null) {
-        this.edit(this._undoText);
+      if (this._undoState !== null) {
+        this.edit(this._undoState);
       }
     },
 
@@ -751,18 +760,20 @@ JX.install('DiffInline', {
     },
 
     cancel: function() {
-      var text = this._readText(this._editRow);
+      var state = this._readFormState(this._editRow);
 
       JX.DOM.remove(this._editRow);
       this._editRow = null;
 
-      if (text && text.length && (text != this._originalText)) {
-        this._drawUneditRows(text);
+      var is_empty = this._isVoidContentState(state);
+      var is_same = this._isSameContentState(state, this._originalState);
+      if (!is_empty && !is_same) {
+        this._drawUneditRows(state);
       }
 
       // If this was an empty box and we typed some text and then hit cancel,
       // don't show the empty concrete inline.
-      if (!this._originalText) {
+      if (!this._isVoidContentState(this._originalState)) {
         this.setInvisible(true);
       } else {
         this.setInvisible(false);
@@ -773,7 +784,7 @@ JX.install('DiffInline', {
       // text ("A") to the server as the current persistent state.
 
       var uri = this._getInlineURI();
-      var data = this._newRequestData('cancel', this._originalText);
+      var data = this._newRequestData('cancel', this._originalState);
       var handler = JX.bind(this, this._onCancelResponse);
 
       this.setLoading(true);
@@ -792,7 +803,7 @@ JX.install('DiffInline', {
       // If the comment was empty when we started editing it (there's no
       // original text) and empty when we finished editing it (there's no
       // undo row), just delete the comment.
-      if (!this._originalText && !this.isUndo()) {
+      if (this._isVoidContentState(this._originalState) && !this.isUndo()) {
         this.setDeleted(true);
 
         JX.DOM.remove(this._row);
@@ -802,7 +813,7 @@ JX.install('DiffInline', {
       }
     },
 
-    _readText: function(row) {
+    _readFormState: function(row) {
       var textarea;
       try {
         textarea = JX.DOM.find(
@@ -813,7 +824,9 @@ JX.install('DiffInline', {
         return null;
       }
 
-      return textarea.value;
+      return {
+        text: textarea.value
+      };
     },
 
     _onsubmitresponse: function(response) {
@@ -920,16 +933,19 @@ JX.install('DiffInline', {
         return null;
       }
 
-      var text = this._readText(this._editRow);
-      if (text === null) {
+      var state = this._readFormState(this._editRow);
+      if (this._isVoidContentState(state)) {
         return null;
       }
 
-      return {
+      var draft_data = {
         op: 'draft',
         id: this.getID(),
-        text: text
       };
+
+      JX.copy(draft_data, state);
+
+      return draft_data;
     },
 
     triggerDraft: function() {
@@ -1035,6 +1051,20 @@ JX.install('DiffInline', {
       if (this._menu) {
         this._menu.close();
       }
+    },
+
+    _isVoidContentState: function(state) {
+      return !state.text.length;
+    },
+
+    _isSameContentState: function(u, v) {
+      return (u.text === v.text);
+    },
+
+    _newContentState: function() {
+      return {
+        text: ''
+      };
     }
 
   }

From 4b2a447003be5582932a509f228c14f91cf30f55 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 13:52:14 -0700
Subject: [PATCH 12/23] Allow "has draft inlines?" queries to overheat

Summary:
Ref T13513. If your 10 most recently authored inlines have all been deleted, these queries can fail by overheating. This is silly and probably rarely happens outside of development.

For now, just let them overheat. This may create a false negative (incorrect "no draft" signal when the real condition is "drafts, but 10 most recent comments were deleted"). This could be sorted out later with a query mode like "executeAny()", perhaps.

Test Plan:
  - Created and deleted 10 inlines.
  - Submitted comments.
  - Before: overheating fatal during draft flag generation.
  - After: clean submission.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21274
---
 .../differential/engine/DifferentialRevisionDraftEngine.php      | 1 +
 src/applications/diffusion/engine/DiffusionCommitDraftEngine.php | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php
index 22b103c525..da45f5bc82 100644
--- a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php
+++ b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php
@@ -11,6 +11,7 @@ final class DifferentialRevisionDraftEngine
       ->setViewer($viewer)
       ->withRevisionPHIDs(array($revision->getPHID()))
       ->withPublishableComments(true)
+      ->setReturnPartialResultsOnOverheat(true)
       ->setLimit(1)
       ->execute();
 
diff --git a/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php b/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php
index 8837dbe63f..e5d45f315b 100644
--- a/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php
+++ b/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php
@@ -11,6 +11,7 @@ final class DiffusionCommitDraftEngine
       ->setViewer($viewer)
       ->withCommitPHIDs(array($commit->getPHID()))
       ->withPublishableComments(true)
+      ->setReturnPartialResultsOnOverheat(true)
       ->setLimit(1)
       ->execute();
 

From 00430fdbe117ffb1de209a8987e90efe10eab4c2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 14:50:32 -0700
Subject: [PATCH 13/23] Make server components of inline comment content
 handling state-oriented

Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.

Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21275
---
 resources/celerity/map.php                    | 12 ++--
 src/__phutil_library_map__.php                |  4 ++
 .../PhabricatorInlineCommentController.php    | 26 ++-----
 ...abricatorDiffInlineCommentContentState.php | 69 +++++++++++++++++++
 .../PhabricatorInlineCommentContentState.php  | 47 +++++++++++++
 .../interface/PhabricatorInlineComment.php    | 57 ++++++++++-----
 .../PhabricatorDiffInlineCommentQuery.php     |  6 +-
 .../view/PHUIDiffInlineCommentDetailView.php  |  1 -
 .../view/PHUIDiffInlineCommentEditView.php    |  7 +-
 .../rsrc/js/application/diff/DiffInline.js    | 48 ++++++++-----
 10 files changed, 207 insertions(+), 70 deletions(-)
 create mode 100644 src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php
 create mode 100644 src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 22d8d56457..e57a9bfc0f 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '42a2334f',
-    'differential.pkg.js' => 'ff8ca085',
+    'differential.pkg.js' => 'd0ddfb19',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -381,7 +381,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
-    'rsrc/js/application/diff/DiffInline.js' => '28e53a2c',
+    'rsrc/js/application/diff/DiffInline.js' => '6fa445ef',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -776,7 +776,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '6e5e03d2',
     'phabricator-diff-changeset-list' => 'b51ba93a',
-    'phabricator-diff-inline' => '28e53a2c',
+    'phabricator-diff-inline' => '6fa445ef',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1137,9 +1137,6 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
-    '28e53a2c' => array(
-      'javelin-dom',
-    ),
     '29819b75' => array(
       'phabricator-notification',
       'javelin-stratcom',
@@ -1564,6 +1561,9 @@ return array(
       'phabricator-diff-path-view',
       'phuix-button-view',
     ),
+    '6fa445ef' => array(
+      'javelin-dom',
+    ),
     70245195 => array(
       'javelin-behavior',
       'javelin-stratcom',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 37d7435eaf..114dcc98d0 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3160,6 +3160,7 @@ phutil_register_library_map(array(
     'PhabricatorDestructionEngineExtensionModule' => 'applications/system/engine/PhabricatorDestructionEngineExtensionModule.php',
     'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php',
     'PhabricatorDeveloperPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDeveloperPreferencesSettingsPanel.php',
+    'PhabricatorDiffInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php',
     'PhabricatorDiffInlineCommentQuery' => 'infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php',
     'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php',
     'PhabricatorDiffScopeEngine' => 'infrastructure/diff/PhabricatorDiffScopeEngine.php',
@@ -3592,6 +3593,7 @@ phutil_register_library_map(array(
     'PhabricatorInfrastructureTestCase' => '__tests__/PhabricatorInfrastructureTestCase.php',
     'PhabricatorInlineComment' => 'infrastructure/diff/interface/PhabricatorInlineComment.php',
     'PhabricatorInlineCommentAdjustmentEngine' => 'infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php',
+    'PhabricatorInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorInlineCommentContentState.php',
     'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php',
     'PhabricatorInlineCommentInterface' => 'applications/transactions/interface/PhabricatorInlineCommentInterface.php',
     'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php',
@@ -9627,6 +9629,7 @@ phutil_register_library_map(array(
     'PhabricatorDestructionEngineExtensionModule' => 'PhabricatorConfigModule',
     'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel',
+    'PhabricatorDiffInlineCommentContentState' => 'PhabricatorInlineCommentContentState',
     'PhabricatorDiffInlineCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery',
     'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel',
     'PhabricatorDiffScopeEngine' => 'Phobject',
@@ -10118,6 +10121,7 @@ phutil_register_library_map(array(
       'PhabricatorMarkupInterface',
     ),
     'PhabricatorInlineCommentAdjustmentEngine' => 'Phobject',
+    'PhabricatorInlineCommentContentState' => 'Phobject',
     'PhabricatorInlineCommentController' => 'PhabricatorController',
     'PhabricatorInlineSummaryView' => 'AphrontView',
     'PhabricatorInstructionsEditField' => 'PhabricatorEditField',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 4fa465d8ee..83838e8385 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -52,10 +52,6 @@ abstract class PhabricatorInlineCommentController
     return $this->operation;
   }
 
-  public function getCommentText() {
-    return $this->commentText;
-  }
-
   public function getLineLength() {
     return $this->lineLength;
   }
@@ -270,10 +266,8 @@ abstract class PhabricatorInlineCommentController
           $viewer->getPHID(),
           $inline->getID());
 
-        $map = $this->getContentState();
-        foreach ($map as $key => $value) {
-          $versioned_draft->setProperty($key, $value);
-        }
+        $map = $this->newRequestContentState($inline)->newStorageMap();
+        $versioned_draft->setProperty('inline.state', $map);
         $versioned_draft->save();
 
         // We have to synchronize the draft engine after saving a versioned
@@ -524,14 +518,9 @@ abstract class PhabricatorInlineCommentController
     return (bool)$request->getBool('hasContentState');
   }
 
-  private function getContentState() {
+  private function newRequestContentState($inline) {
     $request = $this->getRequest();
-
-    $comment_text = $request->getStr('text');
-
-    return array(
-      'inline.text' => (string)$comment_text,
-    );
+    return $inline->newContentStateFromRequest($request);
   }
 
   private function updateCommentContentState(PhabricatorInlineComment $inline) {
@@ -542,11 +531,8 @@ abstract class PhabricatorInlineCommentController
           'content state.'));
     }
 
-    $state = $this->getContentState();
-
-    $text = $state['inline.text'];
-
-    $inline->setContent($text);
+    $state = $this->newRequestContentState($inline);
+    $inline->setContentState($state);
   }
 
   private function saveComment(PhabricatorInlineComment $inline) {
diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php
new file mode 100644
index 0000000000..81a520b0cf
--- /dev/null
+++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php
@@ -0,0 +1,69 @@
+<?php
+
+final class PhabricatorDiffInlineCommentContentState
+  extends PhabricatorInlineCommentContentState {
+
+  private $hasSuggestion = false;
+  private $suggestionText = '';
+
+  public function isEmptyContentState() {
+    if (!parent::isEmptyContentState()) {
+      return false;
+    }
+
+    if ($this->getContentHasSuggestion()) {
+      if (strlen($this->getSuggestionText())) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  public function setContentSuggestionText($suggestion_text) {
+    $this->suggestionText = $suggestion_text;
+    return $this;
+  }
+
+  public function getContentSuggestionText() {
+    return $this->suggestionText;
+  }
+
+  public function setContentHasSuggestion($has_suggestion) {
+    $this->hasSuggestion = $has_suggestion;
+    return $this;
+  }
+
+  public function getContentHasSuggestion() {
+    return $this->hasSuggestion;
+  }
+
+  public function newStorageMap() {
+    return parent::writeStorageMap() + array(
+      'hasSuggestion' => $this->getContentHasSuggestion(),
+      'suggestionText' => $this->getContentSuggestionText(),
+    );
+  }
+
+  public function readStorageMap(array $map) {
+    $result = parent::readStorageMap($map);
+
+    $has_suggestion = (bool)idx($map, 'hasSuggestion');
+    $this->setContentHasSuggestion($has_suggestion);
+
+    $suggestion_text = (string)idx($map, 'suggestionText');
+    $this->setContentSuggestionText($suggestion_text);
+
+    return $result;
+  }
+
+  protected function newStorageMapFromRequest(AphrontRequest $request) {
+    $map = parent::newStorageMapFromRequest($request);
+
+    $map['hasSuggestion'] = (bool)$request->getBool('hasSuggestion');
+    $map['suggestionText'] = (string)$request->getStr('suggestionText');
+
+    return $map;
+  }
+
+}
diff --git a/src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php b/src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php
new file mode 100644
index 0000000000..97d444b711
--- /dev/null
+++ b/src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php
@@ -0,0 +1,47 @@
+<?php
+
+abstract class PhabricatorInlineCommentContentState
+  extends Phobject {
+
+  private $contentText = '';
+
+  public function setContentText($content_text) {
+    $this->contentText = $content_text;
+    return $this;
+  }
+
+  public function getContentText() {
+    return $this->contentText;
+  }
+
+  public function isEmptyContentState() {
+    return !strlen($this->getContentText());
+  }
+
+  public function writeStorageMap() {
+    return array(
+      'text' => $this->getContentText(),
+    );
+  }
+
+  public function readStorageMap(array $map) {
+    $text = (string)idx($map, 'text');
+    $this->setContentText($text);
+
+    return $this;
+  }
+
+  final public function readFromRequest(AphrontRequest $request) {
+    $map = $this->newStorageMapFromRequest($request);
+    return $this->readStorageMap($map);
+  }
+
+  protected function newStorageMapFromRequest(AphrontRequest $request) {
+    $map = array();
+
+    $map['text'] = (string)$request->getStr('text');
+
+    return $map;
+  }
+
+}
diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
index e5ac83139c..02ea4e6dd3 100644
--- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php
+++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
@@ -299,35 +299,58 @@ abstract class PhabricatorInlineComment
   }
 
   public function isVoidComment(PhabricatorUser $viewer) {
-    return !strlen($this->getContentForEdit($viewer));
+    return $this->getContentStateForEdit($viewer)->isEmptyContentState();
   }
 
-  public function getContentForEdit(PhabricatorUser $viewer) {
-    $content = $this->getContent();
+  public function getContentStateForEdit(PhabricatorUser $viewer) {
+    $state = $this->getContentState();
 
-    if (!$this->hasVersionedDraftForViewer($viewer)) {
-      return $content;
+    if ($this->hasVersionedDraftForViewer($viewer)) {
+      $versioned_draft = $this->getVersionedDraftForViewer($viewer);
+      if ($versioned_draft) {
+        $storage_map = $versioned_draft->getProperty('inline.state');
+        if (is_array($storage_map)) {
+          $state->readStorageMap($storage_map);
+        }
+      }
     }
 
-    $versioned_draft = $this->getVersionedDraftForViewer($viewer);
-    if (!$versioned_draft) {
-      return $content;
-    }
+    return $state;
+  }
 
-    $draft_text = $versioned_draft->getProperty('inline.text');
-    if ($draft_text === null) {
-      return $content;
-    }
+  protected function newContentState() {
+    return new PhabricatorDiffInlineCommentContentState();
+  }
 
-    return $draft_text;
+  public function newContentStateFromRequest(AphrontRequest $request) {
+    return $this->newContentState()->readFromRequest($request);
   }
 
   public function getContentState() {
-    return array(
-      'text' => $this->getContent(),
-    );
+    $state = $this->newContentState();
+
+    $storage = $this->getStorageObject();
+    $storage_map = $storage->getAttribute('inline.state');
+    if (is_array($storage_map)) {
+      $state->readStorageMap($storage_map);
+    }
+
+    $state->setContentText($this->getContent());
+
+    return $state;
   }
 
+  public function setContentState(PhabricatorInlineCommentContentState $state) {
+    $storage = $this->getStorageObject();
+    $storage_map = $state->newStorageMap();
+    $storage->setAttribute('inline.state', $storage_map);
+
+    $this->setContent($state->getContentText());
+
+    return $this;
+  }
+
+
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 
 
diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
index 88b6abddec..4e846a0495 100644
--- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
+++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
@@ -218,9 +218,9 @@ abstract class PhabricatorDiffInlineCommentQuery
           // as it is currently shown to the user, not as it was stored the last
           // time they clicked "Save".
 
-          $draft_content = $inline->getContentForEdit($viewer);
-          if (strlen($draft_content)) {
-            $inline->setContent($draft_content);
+          $draft_state = $inline->getContentStateForEdit($viewer);
+          if (!$draft_state->isEmptyContentState()) {
+            $inline->setContentState($draft_state);
           }
         }
       }
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
index 900491e00b..db2952faa8 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
@@ -101,7 +101,6 @@ final class PHUIDiffInlineCommentDetailView
       $classes[] = 'inline-comment-element';
     }
 
-    $content = $inline->getContent();
     $handles = $this->handles;
 
     $links = array();
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
index fa61ef89a3..ce43653119 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
@@ -82,13 +82,12 @@ final class PHUIDiffInlineCommentEditView
     $viewer = $this->getViewer();
     $inline = $this->getInlineComment();
 
-    $text = $inline->getContentForEdit($viewer);
+    $state = $inline->getContentStateForEdit($viewer);
 
     return id(new PhabricatorRemarkupControl())
       ->setViewer($viewer)
-      ->setSigil('differential-inline-comment-edit-textarea')
-      ->setName('text')
-      ->setValue($text)
+      ->setSigil('inline-content-text')
+      ->setValue($state->getContentText())
       ->setDisableFullScreen(true);
   }
 
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 0e77ec59a2..49f72101f4 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -701,7 +701,7 @@ JX.install('DiffInline', {
           var textareas = JX.DOM.scry(
             row,
             'textarea',
-            'differential-inline-comment-edit-textarea');
+            'inline-content-text');
           if (textareas.length) {
             var area = textareas[0];
             area.focus();
@@ -814,19 +814,25 @@ JX.install('DiffInline', {
     },
 
     _readFormState: function(row) {
-      var textarea;
+      var state = this._newContentState();
+
+      var node;
+
       try {
-        textarea = JX.DOM.find(
-          row,
-          'textarea',
-          'differential-inline-comment-edit-textarea');
+        node = JX.DOM.find(row, 'textarea', 'inline-content-text');
+        state.text = node.value;
       } catch (ex) {
-        return null;
+        // Ignore.
       }
 
-      return {
-        text: textarea.value
-      };
+      try {
+        node = JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
+        state.suggestionText = node.value;
+      } catch (ex) {
+        // Ignore.
+      }
+
+      return state;
     },
 
     _onsubmitresponse: function(response) {
@@ -1053,20 +1059,24 @@ JX.install('DiffInline', {
       }
     },
 
+    _newContentState: function() {
+      return {
+        text: '',
+        suggestionText: '',
+        hasSuggestion: true
+      };
+    },
+
     _isVoidContentState: function(state) {
-      return !state.text.length;
+      return (!state.text.length && !state.suggestionText.length);
     },
 
     _isSameContentState: function(u, v) {
-      return (u.text === v.text);
-    },
-
-    _newContentState: function() {
-      return {
-        text: ''
-      };
+      return (
+        (u.text === v.text) &&
+        (u.suggestionText === v.suggestionText) &&
+        (u.hasSuggestion === v.hasSuggestion));
     }
-
   }
 
 });

From 846562158aae026a6207975c4c2681ae4856603b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 14:50:47 -0700
Subject: [PATCH 14/23] Roughly support inline comment suggestions

Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.

Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.

Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21276
---
 resources/celerity/map.php                    |  18 +-
 src/__phutil_library_map__.php                |   4 +
 .../PhabricatorAuditTransactionComment.php    |  15 +-
 .../DifferentialChangesetViewController.php   |   1 +
 .../editor/DifferentialRevisionEditEngine.php |   7 +
 .../DifferentialTransactionComment.php        |  15 +-
 .../view/DifferentialChangesetListView.php    |   3 +
 .../PhabricatorInlineCommentController.php    |  32 +++-
 ...abricatorDiffInlineCommentContentState.php |   2 +-
 .../PhabricatorDiffInlineCommentContext.php   |  37 +++++
 .../PhabricatorInlineCommentContext.php       |   4 +
 .../interface/PhabricatorInlineComment.php    |   4 +
 .../PhabricatorDiffInlineCommentQuery.php     | 155 +++++++++++++++---
 .../view/PHUIDiffInlineCommentDetailView.php  |  74 ++++++++-
 .../view/PHUIDiffInlineCommentEditView.php    | 127 +++++++++++++-
 .../diff/view/PHUIDiffInlineCommentView.php   |   2 +-
 .../differential/phui-inline-comment.css      |  57 +++++++
 .../rsrc/js/application/diff/DiffInline.js    | 113 ++++++++++++-
 18 files changed, 615 insertions(+), 55 deletions(-)
 create mode 100644 src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
 create mode 100644 src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index e57a9bfc0f..434ca6e869 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -12,8 +12,8 @@ return array(
     'core.pkg.css' => 'ba768cdb',
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
-    'differential.pkg.css' => '42a2334f',
-    'differential.pkg.js' => 'd0ddfb19',
+    'differential.pkg.css' => 'f924dbcf',
+    'differential.pkg.js' => '256a327a',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -65,7 +65,7 @@ return array(
     'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
     'rsrc/css/application/differential/changeset-view.css' => '60c3d405',
     'rsrc/css/application/differential/core.css' => '7300a73e',
-    'rsrc/css/application/differential/phui-inline-comment.css' => 'd5749acc',
+    'rsrc/css/application/differential/phui-inline-comment.css' => '4107254a',
     'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
     'rsrc/css/application/differential/revision-history.css' => '8aa3eac5',
     'rsrc/css/application/differential/revision-list.css' => '93d2df7d',
@@ -381,7 +381,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
-    'rsrc/js/application/diff/DiffInline.js' => '6fa445ef',
+    'rsrc/js/application/diff/DiffInline.js' => '829b88bf',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -776,7 +776,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '6e5e03d2',
     'phabricator-diff-changeset-list' => 'b51ba93a',
-    'phabricator-diff-inline' => '6fa445ef',
+    'phabricator-diff-inline' => '829b88bf',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -854,7 +854,7 @@ return array(
     'phui-icon-view-css' => '4cbc684a',
     'phui-image-mask-css' => '62c7f4d2',
     'phui-info-view-css' => 'a10a909b',
-    'phui-inline-comment-view-css' => 'd5749acc',
+    'phui-inline-comment-view-css' => '4107254a',
     'phui-invisible-character-view-css' => 'c694c4a4',
     'phui-left-right-css' => '68513c34',
     'phui-lightbox-css' => '4ebf22da',
@@ -1561,9 +1561,6 @@ return array(
       'phabricator-diff-path-view',
       'phuix-button-view',
     ),
-    '6fa445ef' => array(
-      'javelin-dom',
-    ),
     70245195 => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1642,6 +1639,9 @@ return array(
     '8207abf9' => array(
       'javelin-dom',
     ),
+    '829b88bf' => array(
+      'javelin-dom',
+    ),
     83754533 => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 114dcc98d0..fa3eaabe7a 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3161,6 +3161,7 @@ phutil_register_library_map(array(
     'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php',
     'PhabricatorDeveloperPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDeveloperPreferencesSettingsPanel.php',
     'PhabricatorDiffInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php',
+    'PhabricatorDiffInlineCommentContext' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php',
     'PhabricatorDiffInlineCommentQuery' => 'infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php',
     'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php',
     'PhabricatorDiffScopeEngine' => 'infrastructure/diff/PhabricatorDiffScopeEngine.php',
@@ -3594,6 +3595,7 @@ phutil_register_library_map(array(
     'PhabricatorInlineComment' => 'infrastructure/diff/interface/PhabricatorInlineComment.php',
     'PhabricatorInlineCommentAdjustmentEngine' => 'infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php',
     'PhabricatorInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorInlineCommentContentState.php',
+    'PhabricatorInlineCommentContext' => 'infrastructure/diff/inline/PhabricatorInlineCommentContext.php',
     'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php',
     'PhabricatorInlineCommentInterface' => 'applications/transactions/interface/PhabricatorInlineCommentInterface.php',
     'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php',
@@ -9630,6 +9632,7 @@ phutil_register_library_map(array(
     'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel',
     'PhabricatorDiffInlineCommentContentState' => 'PhabricatorInlineCommentContentState',
+    'PhabricatorDiffInlineCommentContext' => 'PhabricatorInlineCommentContext',
     'PhabricatorDiffInlineCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery',
     'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel',
     'PhabricatorDiffScopeEngine' => 'Phobject',
@@ -10122,6 +10125,7 @@ phutil_register_library_map(array(
     ),
     'PhabricatorInlineCommentAdjustmentEngine' => 'Phobject',
     'PhabricatorInlineCommentContentState' => 'Phobject',
+    'PhabricatorInlineCommentContext' => 'Phobject',
     'PhabricatorInlineCommentController' => 'PhabricatorController',
     'PhabricatorInlineSummaryView' => 'AphrontView',
     'PhabricatorInstructionsEditField' => 'PhabricatorEditField',
diff --git a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php
index 01f173bd9a..cb9d5e20eb 100644
--- a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php
+++ b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php
@@ -17,6 +17,7 @@ final class PhabricatorAuditTransactionComment
   protected $attributes = array();
 
   private $replyToComment = self::ATTACHABLE;
+  private $inlineContext = self::ATTACHABLE;
 
   public function getApplicationTransactionObject() {
     return new PhabricatorAuditTransaction();
@@ -83,12 +84,18 @@ final class PhabricatorAuditTransactionComment
     return $this;
   }
 
-  public function isEmptyInlineComment() {
-    return !strlen($this->getContent());
-  }
-
   public function newInlineCommentObject() {
     return PhabricatorAuditInlineComment::newFromModernComment($this);
   }
 
+  public function getInlineContext() {
+    return $this->assertAttached($this->inlineContext);
+  }
+
+  public function attachInlineContext(
+    PhabricatorInlineCommentContext $context = null) {
+    $this->inlineContext = $context;
+    return $this;
+  }
+
 }
diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php
index df1ffa1d05..0e263d013c 100644
--- a/src/applications/differential/controller/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/DifferentialChangesetViewController.php
@@ -200,6 +200,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
         ->withPublishableComments(true)
         ->withPublishedComments(true)
         ->needHidden(true)
+        ->needInlineContext(true)
         ->execute();
 
       $inlines = mpull($inlines, 'newInlineCommentObject');
diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
index c5b8311630..cc90e36a61 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
@@ -328,6 +328,13 @@ final class DifferentialRevisionEditEngine
     $content = array();
 
     if ($inlines) {
+      // Reload inlines to get inline context.
+      $inlines = id(new DifferentialDiffInlineCommentQuery())
+        ->setViewer($viewer)
+        ->withIDs(mpull($inlines, 'getID'))
+        ->needInlineContext(true)
+        ->execute();
+
       $inline_preview = id(new PHUIDiffInlineCommentPreviewListView())
         ->setViewer($viewer)
         ->setInlineComments($inlines);
diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php
index f5cb772d48..491d89a968 100644
--- a/src/applications/differential/storage/DifferentialTransactionComment.php
+++ b/src/applications/differential/storage/DifferentialTransactionComment.php
@@ -18,6 +18,7 @@ final class DifferentialTransactionComment
   private $replyToComment = self::ATTACHABLE;
   private $isHidden = self::ATTACHABLE;
   private $changeset = self::ATTACHABLE;
+  private $inlineContext = self::ATTACHABLE;
 
   public function getApplicationTransactionObject() {
     return new DifferentialTransaction();
@@ -129,12 +130,18 @@ final class DifferentialTransactionComment
     return $this;
   }
 
-  public function isEmptyInlineComment() {
-    return !strlen($this->getContent());
-  }
-
   public function newInlineCommentObject() {
     return DifferentialInlineComment::newFromModernComment($this);
   }
 
+  public function getInlineContext() {
+    return $this->assertAttached($this->inlineContext);
+  }
+
+  public function attachInlineContext(
+    PhabricatorInlineCommentContext $context = null) {
+    $this->inlineContext = $context;
+    return $this;
+  }
+
 }
diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php
index bf8aa8fae5..cab63eb998 100644
--- a/src/applications/differential/view/DifferentialChangesetListView.php
+++ b/src/applications/differential/view/DifferentialChangesetListView.php
@@ -373,6 +373,9 @@ final class DifferentialChangesetListView extends AphrontView {
 
         'Add new inline comment on selected source text.' =>
           pht('Add new inline comment on selected source text.'),
+
+        'Suggest Edit' => pht('Suggest Edit'),
+        'Discard Edit' => pht('Discard Edit'),
       ),
     ));
 
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 83838e8385..e1b8dc7264 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -240,7 +240,7 @@ abstract class PhabricatorInlineCommentController
 
         $view = $this->buildScaffoldForView($edit_dialog);
 
-        return $this->newInlineResponse($inline, $view);
+        return $this->newInlineResponse($inline, $view, true);
       case 'cancel':
         $inline = $this->loadCommentByIDForEdit($this->getCommentID());
 
@@ -325,6 +325,9 @@ abstract class PhabricatorInlineCommentController
 
         $this->saveComment($inline);
 
+        // Reload the inline to attach context.
+        $inline = $this->loadCommentByIDForEdit($inline->getID());
+
         $edit_dialog = $this->buildEditDialog($inline);
 
         if ($this->getOperation() == 'reply') {
@@ -335,7 +338,7 @@ abstract class PhabricatorInlineCommentController
 
         $view = $this->buildScaffoldForView($edit_dialog);
 
-        return $this->newInlineResponse($inline, $view);
+        return $this->newInlineResponse($inline, $view, true);
     }
   }
 
@@ -431,7 +434,7 @@ abstract class PhabricatorInlineCommentController
 
     $view = $this->buildScaffoldForView($view);
 
-    return $this->newInlineResponse($inline, $view);
+    return $this->newInlineResponse($inline, $view, false);
   }
 
   private function buildScaffoldForView(PHUIDiffInlineCommentView $view) {
@@ -446,11 +449,29 @@ abstract class PhabricatorInlineCommentController
 
   private function newInlineResponse(
     PhabricatorInlineComment $inline,
-    $view) {
+    $view,
+    $is_edit) {
+
+    if ($inline->getReplyToCommentPHID()) {
+      $can_suggest = false;
+    } else {
+      $can_suggest = (bool)$inline->getInlineContext();
+    }
+
+    if ($is_edit) {
+      $viewer = $this->getViewer();
+      $content_state = $inline->getContentStateForEdit($viewer);
+    } else {
+      $content_state = $inline->getContentState();
+    }
+
+    $state_map = $content_state->newStorageMap();
 
     $response = array(
       'inline' => array(
         'id' => $inline->getID(),
+        'contentState' => $state_map,
+        'canSuggestEdit' => $can_suggest,
       ),
       'view' => hsprintf('%s', $view),
     );
@@ -477,7 +498,8 @@ abstract class PhabricatorInlineCommentController
     $viewer = $this->getViewer();
 
     $query = $this->newInlineCommentQuery()
-      ->withIDs(array($id));
+      ->withIDs(array($id))
+      ->needInlineContext(true);
 
     $inline = $this->loadCommentByQuery($query);
 
diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php
index 81a520b0cf..0f379acc3e 100644
--- a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php
+++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php
@@ -12,7 +12,7 @@ final class PhabricatorDiffInlineCommentContentState
     }
 
     if ($this->getContentHasSuggestion()) {
-      if (strlen($this->getSuggestionText())) {
+      if (strlen($this->getContentSuggestionText())) {
         return false;
       }
     }
diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
new file mode 100644
index 0000000000..e46d934b5a
--- /dev/null
+++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
@@ -0,0 +1,37 @@
+<?php
+
+final class PhabricatorDiffInlineCommentContext
+  extends PhabricatorInlineCommentContext {
+
+  private $headLines;
+  private $bodyLines;
+  private $tailLines;
+
+  public function setHeadLines(array $head_lines) {
+    $this->headLines = $head_lines;
+    return $this;
+  }
+
+  public function getHeadLines() {
+    return $this->headLines;
+  }
+
+  public function setBodyLines(array $body_lines) {
+    $this->bodyLines = $body_lines;
+    return $this;
+  }
+
+  public function getBodyLines() {
+    return $this->bodyLines;
+  }
+
+  public function setTailLines(array $tail_lines) {
+    $this->tailLines = $tail_lines;
+    return $this;
+  }
+
+  public function getTailLines() {
+    return $this->tailLines;
+  }
+
+}
diff --git a/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php
new file mode 100644
index 0000000000..a292dc72ee
--- /dev/null
+++ b/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php
@@ -0,0 +1,4 @@
+<?php
+
+abstract class PhabricatorInlineCommentContext
+  extends Phobject {}
diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
index 02ea4e6dd3..3d3bb6e106 100644
--- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php
+++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
@@ -350,6 +350,10 @@ abstract class PhabricatorInlineComment
     return $this;
   }
 
+  public function getInlineContext() {
+    return $this->getStorageObject()->getInlineContext();
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 
diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
index 4e846a0495..b5f39b1d2f 100644
--- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
+++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
@@ -9,6 +9,7 @@ abstract class PhabricatorDiffInlineCommentQuery
   private $publishableComments;
   private $needHidden;
   private $needAppliedDrafts;
+  private $needInlineContext;
 
   abstract protected function buildInlineCommentWhereClauseParts(
     AphrontDatabaseConnection $conn);
@@ -42,6 +43,11 @@ abstract class PhabricatorDiffInlineCommentQuery
     return $this;
   }
 
+  final public function needInlineContext($need_context) {
+    $this->needInlineContext = $need_context;
+    return $this;
+  }
+
   final public function needAppliedDrafts($need_applied) {
     $this->needAppliedDrafts = $need_applied;
     return $this;
@@ -173,26 +179,6 @@ abstract class PhabricatorDiffInlineCommentQuery
       return $inlines;
     }
 
-    if ($this->needHidden) {
-      $viewer_phid = $viewer->getPHID();
-
-      if ($viewer_phid) {
-        $hidden = $this->loadHiddenCommentIDs(
-          $viewer_phid,
-          $inlines);
-      } else {
-        $hidden = array();
-      }
-
-      foreach ($inlines as $inline) {
-        $inline->attachIsHidden(isset($hidden[$inline->getID()]));
-      }
-    }
-
-    if (!$inlines) {
-      return $inlines;
-    }
-
     $need_drafts = $this->needAppliedDrafts;
     $drop_void = $this->publishableComments;
     $convert_objects = ($need_drafts || $drop_void);
@@ -247,4 +233,133 @@ abstract class PhabricatorDiffInlineCommentQuery
     return $inlines;
   }
 
+  protected function didFilterPage(array $inlines) {
+    $viewer = $this->getViewer();
+
+    if ($this->needHidden) {
+      $viewer_phid = $viewer->getPHID();
+
+      if ($viewer_phid) {
+        $hidden = $this->loadHiddenCommentIDs(
+          $viewer_phid,
+          $inlines);
+      } else {
+        $hidden = array();
+      }
+
+      foreach ($inlines as $inline) {
+        $inline->attachIsHidden(isset($hidden[$inline->getID()]));
+      }
+    }
+
+    if ($this->needInlineContext) {
+      $need_context = array();
+      foreach ($inlines as $inline) {
+        $object = $inline->newInlineCommentObject();
+
+        if ($object->getDocumentEngineKey() !== null) {
+          $inline->attachInlineContext(null);
+          continue;
+        }
+
+        $need_context[] = $inline;
+      }
+
+      foreach ($need_context as $inline) {
+        $changeset = id(new DifferentialChangesetQuery())
+          ->setViewer($viewer)
+          ->withIDs(array($inline->getChangesetID()))
+          ->needHunks(true)
+          ->executeOne();
+        if (!$changeset) {
+          $inline->attachInlineContext(null);
+          continue;
+        }
+
+        $hunks = $changeset->getHunks();
+
+        $is_simple =
+          (count($hunks) === 1) &&
+          ((int)head($hunks)->getOldOffset() <= 1) &&
+          ((int)head($hunks)->getNewOffset() <= 1);
+
+        if (!$is_simple) {
+          $inline->attachInlineContext(null);
+          continue;
+        }
+
+        if ($inline->getIsNewFile()) {
+          $corpus = $changeset->makeNewFile();
+        } else {
+          $corpus = $changeset->makeOldFile();
+        }
+
+        $corpus = phutil_split_lines($corpus);
+
+        // Adjust the line number into a 0-based offset.
+        $offset = $inline->getLineNumber();
+        $offset = $offset - 1;
+
+        // Adjust the inclusive range length into a row count.
+        $length = $inline->getLineLength();
+        $length = $length + 1;
+
+        $head_min = max(0, $offset - 3);
+        $head_max = $offset;
+        $head_len = $head_max - $head_min;
+
+        if ($head_len) {
+          $head = array_slice($corpus, $head_min, $head_len, true);
+          $head = $this->simplifyContext($head, true);
+        } else {
+          $head = array();
+        }
+
+        $body = array_slice($corpus, $offset, $length, true);
+
+        $tail = array_slice($corpus, $offset + $length, 3, true);
+        $tail = $this->simplifyContext($tail, false);
+
+        $context = id(new PhabricatorDiffInlineCommentContext())
+          ->setHeadLines($head)
+          ->setBodyLines($body)
+          ->setTailLines($tail);
+
+        $inline->attachInlineContext($context);
+      }
+
+    }
+
+    return $inlines;
+  }
+
+  private function simplifyContext(array $lines, $is_head) {
+    // We want to provide the smallest amount of context we can while still
+    // being useful, since the actual code is visible nearby and showing a
+    // ton of context is silly.
+
+    // Examine each line until we find one that looks "useful" (not just
+    // whitespace or a single bracket). Once we find a useful piece of context
+    // to anchor the text, discard the rest of the lines beyond it.
+
+    if ($is_head) {
+      $lines = array_reverse($lines, true);
+    }
+
+    $saw_context = false;
+    foreach ($lines as $key => $line) {
+      if ($saw_context) {
+        unset($lines[$key]);
+        continue;
+      }
+
+      $saw_context = (strlen(trim($line)) > 3);
+    }
+
+    if ($is_head) {
+      $lines = array_reverse($lines, true);
+    }
+
+    return $lines;
+  }
 }
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
index db2952faa8..b2a49f0624 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
@@ -427,6 +427,15 @@ final class PHUIDiffInlineCommentDetailView
 
     $metadata['menuItems'] = $menu_items;
 
+    $suggestion_content = $this->newSuggestionView($inline);
+
+    $inline_content = phutil_tag(
+      'div',
+      array(
+        'class' => 'phabricator-remarkup',
+      ),
+      $content);
+
     $markup = javelin_tag(
       'div',
       array(
@@ -445,9 +454,15 @@ final class PHUIDiffInlineCommentDetailView
             $group_left,
             $group_right,
           )),
-        phutil_tag_div(
-          'differential-inline-comment-content',
-          phutil_tag_div('phabricator-remarkup', $content)),
+        phutil_tag(
+          'div',
+          array(
+            'class' => 'differential-inline-comment-content',
+          ),
+          array(
+            $suggestion_content,
+            $inline_content,
+          )),
       ));
 
     $summary = phutil_tag(
@@ -491,4 +506,57 @@ final class PHUIDiffInlineCommentDetailView
     return true;
   }
 
+  private function newSuggestionView(PhabricatorInlineComment $inline) {
+    $content_state = $inline->getContentState();
+    if (!$content_state->getContentHasSuggestion()) {
+      return null;
+    }
+
+    $context = $inline->getInlineContext();
+    if (!$context) {
+      return null;
+    }
+
+    $head_lines = $context->getHeadLines();
+    $head_lines = implode('', $head_lines);
+
+    $tail_lines = $context->getTailLines();
+    $tail_lines = implode('', $tail_lines);
+
+    $old_lines = $context->getBodyLines();
+    $old_lines = implode('', $old_lines);
+    $old_lines = $head_lines.$old_lines.$tail_lines;
+    if (strlen($old_lines) && !preg_match('/\n\z/', $old_lines)) {
+      $old_lines .= "\n";
+    }
+
+    $new_lines = $content_state->getContentSuggestionText();
+    $new_lines = $head_lines.$new_lines.$tail_lines;
+    if (strlen($new_lines) && !preg_match('/\n\z/', $new_lines)) {
+      $new_lines .= "\n";
+    }
+
+    if ($old_lines === $new_lines) {
+      return null;
+    }
+
+
+    $raw_diff = id(new PhabricatorDifferenceEngine())
+      ->generateRawDiffFromFileContent($old_lines, $new_lines);
+
+    $raw_diff = phutil_split_lines($raw_diff);
+    $raw_diff = array_slice($raw_diff, 3);
+    $raw_diff = implode('', $raw_diff);
+
+    $view = phutil_tag(
+      'div',
+      array(
+        'class' => 'inline-suggestion-view PhabricatorMonospaced',
+      ),
+      $raw_diff);
+
+    return $view;
+  }
+
+
 }
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
index ce43653119..5c29c1666c 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
@@ -46,17 +46,23 @@ final class PHUIDiffInlineCommentEditView
       ),
       $this->title);
 
+    $corpus_view = $this->newCorpusView();
+
     $body = phutil_tag(
       'div',
       array(
         'class' => 'differential-inline-comment-edit-body',
       ),
-      $this->newTextarea());
+      array(
+        $corpus_view,
+        $this->newTextarea(),
+      ));
 
-    $edit = phutil_tag(
+    $edit = javelin_tag(
       'div',
       array(
         'class' => 'differential-inline-comment-edit-buttons grouped',
+        'sigil' => 'inline-edit-buttons',
       ),
       array(
         $buttons,
@@ -91,4 +97,121 @@ final class PHUIDiffInlineCommentEditView
       ->setDisableFullScreen(true);
   }
 
+  private function newCorpusView() {
+    $viewer = $this->getViewer();
+    $inline = $this->getInlineComment();
+
+    $context = $inline->getInlineContext();
+    if ($context === null) {
+      return null;
+    }
+
+    $head = $context->getHeadLines();
+    $head = $this->newContextView($head);
+
+    $state = $inline->getContentStateForEdit($viewer);
+
+    $main = $state->getContentSuggestionText();
+    $main_count = count(phutil_split_lines($main));
+
+    $default = $context->getBodyLines();
+    $default = implode('', $default);
+
+    // Browsers ignore one leading newline in text areas. Add one so that
+    // any actual leading newlines in the content are preserved.
+    $main = "\n".$main;
+
+    $textarea = javelin_tag(
+      'textarea',
+      array(
+        'class' => 'inline-suggestion-input PhabricatorMonospaced',
+        'rows' => max(3, $main_count + 1),
+        'sigil' => 'inline-content-suggestion',
+        'meta' => array(
+          'defaultText' => $default,
+        ),
+      ),
+      $main);
+
+    $main = phutil_tag(
+      'tr',
+      array(
+        'class' => 'inline-suggestion-input-row',
+      ),
+      array(
+        phutil_tag(
+          'td',
+          array(
+            'class' => 'inline-suggestion-line-cell',
+          ),
+          null),
+        phutil_tag(
+          'td',
+          array(
+            'class' => 'inline-suggestion-input-cell',
+          ),
+          $textarea),
+      ));
+
+    $tail = $context->getTailLines();
+    $tail = $this->newContextView($tail);
+
+    $body = phutil_tag(
+      'tbody',
+      array(),
+      array(
+        $head,
+        $main,
+        $tail,
+      ));
+
+    $table = phutil_tag(
+      'table',
+      array(
+        'class' => 'inline-suggestion-table',
+      ),
+      $body);
+
+    $container = phutil_tag(
+      'div',
+      array(
+        'class' => 'inline-suggestion',
+      ),
+      $table);
+
+    return $container;
+  }
+
+  private function newContextView(array $lines) {
+    if (!$lines) {
+      return array();
+    }
+
+    $rows = array();
+    foreach ($lines as $index => $line) {
+      $line_cell = phutil_tag(
+        'td',
+        array(
+          'class' => 'inline-suggestion-line-cell PhabricatorMonospaced',
+        ),
+        $index + 1);
+
+      $text_cell = phutil_tag(
+        'td',
+        array(
+          'class' => 'inline-suggestion-text-cell PhabricatorMonospaced',
+        ),
+        $line);
+
+      $cells = array(
+        $line_cell,
+        $text_cell,
+      );
+
+      $rows[] = phutil_tag('tr', array(), $cells);
+    }
+
+    return $rows;
+  }
+
 }
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
index 97fc9ee6e5..11be8d0ec1 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
@@ -93,7 +93,7 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
       'startOffset' => $inline->getStartOffset(),
       'endOffset' => $inline->getEndOffset(),
       'on_right' => $this->getIsOnRight(),
-      'contentState' => $inline->getContentState(),
+      'contentState' => $inline->getContentState()->newStorageMap(),
     );
   }
 
diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css
index ce5b87effd..9fefc65353 100644
--- a/webroot/rsrc/css/application/differential/phui-inline-comment.css
+++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css
@@ -436,3 +436,60 @@
   background: {$lightyellow};
   border-color: {$yellow};
 }
+
+.inline-suggestion {
+  display: none;
+  margin: 0 -8px;
+}
+
+.has-suggestion .inline-suggestion {
+  display: block;
+}
+
+.differential-inline-comment-edit-buttons button.inline-button-left {
+  float: left;
+  margin: 0 6px 0 0;
+}
+
+.inline-suggestion-table {
+  table-layout: fixed;
+  width: 100%;
+  margin-bottom: 8px;
+  white-space: pre-wrap;
+  background: {$greybackground};
+  border-width: 1px 0;
+  border-style: solid;
+  border-color: {$lightgreyborder};
+}
+
+textarea.inline-suggestion-input {
+  width: 100%;
+  height: auto;
+  max-width: 100%;
+}
+
+.inline-suggestion-line-cell {
+  text-align: right;
+  background: {$darkgreybackground};
+  width: 36px;
+  color: {$greytext};
+  border-right: 1px solid {$lightgreyborder};
+}
+
+.inline-suggestion-input-cell {
+  padding: 8px;
+}
+
+.inline-suggestion-text-cell {
+  padding: 0 8px;
+}
+
+.inline-suggestion-view {
+  padding: 8px 12px;
+  white-space: pre-wrap;
+  background: {$greybackground};
+  margin: 0 -12px 8px;
+  border-width: 1px 0;
+  border-style: solid;
+  border-color: {$lightgreyborder};
+}
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 49f72101f4..e03138abfa 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -51,6 +51,7 @@ JX.install('DiffInline', {
     _startOffset: null,
     _endOffset: null,
     _isSelected: false,
+    _canSuggestEdit: false,
 
     bindToRow: function(row) {
       this._row = row;
@@ -76,7 +77,6 @@ JX.install('DiffInline', {
       this._number = parseInt(data.number, 10);
       this._length = parseInt(data.length, 10);
 
-      this._originalState = data.contentState;
       this._isNewFile = data.isNewFile;
 
       this._replyToCommentPHID = data.replyToCommentPHID;
@@ -602,6 +602,8 @@ JX.install('DiffInline', {
 
     _readInlineState: function(state) {
       this._id = state.id;
+      this._originalState = state.contentState;
+      this._canSuggestEdit = state.canSuggestEdit;
     },
 
     _ondeleteresponse: function() {
@@ -664,6 +666,11 @@ JX.install('DiffInline', {
     _drawEditRows: function(rows) {
       this.setEditing(true);
       this._editRow = this._drawRows(rows, null, 'edit');
+
+      this._drawSuggestionState(this._editRow);
+      JX.log(this._originalState);
+
+      this.setHasSuggestion(this._originalState.hasSuggestion);
     },
 
     _drawRows: function(rows, cursor, type) {
@@ -719,6 +726,91 @@ JX.install('DiffInline', {
       return result_row;
     },
 
+    _drawSuggestionState: function(row) {
+      if (this._canSuggestEdit) {
+        var button = this._getSuggestionButton();
+        var node = button.getNode();
+
+        // As a side effect of form submission, the button may become
+        // visually disabled. Re-enable it. This is a bit hacky.
+        JX.DOM.alterClass(node, 'disabled', false);
+        node.disabled = false;
+
+        var container = JX.DOM.find(row, 'div', 'inline-edit-buttons');
+        container.appendChild(node);
+      }
+    },
+
+    _getSuggestionButton: function() {
+      if (!this._suggestionButton) {
+        var button = new JX.PHUIXButtonView()
+          .setIcon('fa-pencil-square-o')
+          .setColor('grey');
+
+        var node = button.getNode();
+        JX.DOM.alterClass(node, 'inline-button-left', true);
+
+        var onclick = JX.bind(this, this._onSuggestEdit);
+        JX.DOM.listen(node, 'click', null, onclick);
+
+        this._suggestionButton = button;
+      }
+
+      return this._suggestionButton;
+    },
+
+    _onSuggestEdit: function(e) {
+      e.kill();
+
+      this.setHasSuggestion(!this.getHasSuggestion());
+
+      // The first time the user actually clicks the button and enables
+      // suggestions for a given editor state, fill the input with the
+      // underlying text if there isn't any text yet.
+      if (this.getHasSuggestion()) {
+        if (this._editRow) {
+          var node = this._getSuggestionNode(this._editRow);
+          if (node) {
+            if (!node.value.length) {
+              var data = JX.Stratcom.getData(node);
+              if (!data.hasSetDefault) {
+                data.hasSetDefault = true;
+                node.value = data.defaultText;
+                node.rows = Math.max(3, node.value.split('\n').length);
+              }
+            }
+          }
+        }
+      }
+
+      // Save the "hasSuggestion" part of the content state.
+      this.triggerDraft();
+    },
+
+    setHasSuggestion: function(has_suggestion) {
+      this._hasSuggestion = has_suggestion;
+
+      var button = this._getSuggestionButton();
+      var pht = this.getChangeset().getChangesetList().getTranslations();
+      if (has_suggestion) {
+        button
+          .setIcon('fa-times')
+          .setText(pht('Discard Edit'));
+      } else {
+        button
+          .setIcon('fa-plus')
+          .setText(pht('Suggest Edit'));
+      }
+
+      if (this._editRow) {
+        JX.DOM.alterClass(this._editRow, 'has-suggestion', has_suggestion);
+      }
+    },
+
+    getHasSuggestion: function() {
+      return this._hasSuggestion;
+    },
+
     save: function() {
       var handler = JX.bind(this, this._onsubmitresponse);
 
@@ -825,16 +917,24 @@ JX.install('DiffInline', {
         // Ignore.
       }
 
-      try {
-        node = JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
+      node = this._getSuggestionNode(row);
+      if (node) {
         state.suggestionText = node.value;
-      } catch (ex) {
-        // Ignore.
       }
 
+      state.hasSuggestion = this.getHasSuggestion();
+
       return state;
     },
 
+    _getSuggestionNode: function(row) {
+      try {
+        return JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
+      } catch (ex) {
+        return null;
+      }
+    },
+
     _onsubmitresponse: function(response) {
       if (this._editRow) {
         JX.DOM.remove(this._editRow);
@@ -1063,7 +1163,7 @@ JX.install('DiffInline', {
       return {
         text: '',
         suggestionText: '',
-        hasSuggestion: true
+        hasSuggestion: false
       };
     },
 
@@ -1073,6 +1173,7 @@ JX.install('DiffInline', {
 
     _isSameContentState: function(u, v) {
       return (
+        ((u === null) === (v === null)) &&
         (u.text === v.text) &&
         (u.suggestionText === v.suggestionText) &&
         (u.hasSuggestion === v.hasSuggestion));

From 10f241352dcef12fd599a8b6c0f32c704d9f1f9a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 May 2020 09:01:23 -0700
Subject: [PATCH 15/23] Render inline comment suggestions as real diffs

Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.

Test Plan: {F7495053}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21277
---
 resources/celerity/map.php                    |  6 +--
 .../parser/DifferentialChangesetParser.php    |  5 ++-
 .../DifferentialChangesetOneUpRenderer.php    | 39 ++++++++++++++++++-
 .../PhabricatorDiffInlineCommentContext.php   | 10 +++++
 .../PhabricatorDiffInlineCommentQuery.php     |  5 +++
 .../view/PHUIDiffInlineCommentDetailView.php  | 27 ++++++++++---
 .../differential/phui-inline-comment.css      | 24 +++++++++---
 7 files changed, 98 insertions(+), 18 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 434ca6e869..c1b05eae86 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -12,7 +12,7 @@ return array(
     'core.pkg.css' => 'ba768cdb',
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
-    'differential.pkg.css' => 'f924dbcf',
+    'differential.pkg.css' => '5c459f92',
     'differential.pkg.js' => '256a327a',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
@@ -65,7 +65,7 @@ return array(
     'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
     'rsrc/css/application/differential/changeset-view.css' => '60c3d405',
     'rsrc/css/application/differential/core.css' => '7300a73e',
-    'rsrc/css/application/differential/phui-inline-comment.css' => '4107254a',
+    'rsrc/css/application/differential/phui-inline-comment.css' => '9863a85e',
     'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
     'rsrc/css/application/differential/revision-history.css' => '8aa3eac5',
     'rsrc/css/application/differential/revision-list.css' => '93d2df7d',
@@ -854,7 +854,7 @@ return array(
     'phui-icon-view-css' => '4cbc684a',
     'phui-image-mask-css' => '62c7f4d2',
     'phui-info-view-css' => 'a10a909b',
-    'phui-inline-comment-view-css' => '4107254a',
+    'phui-inline-comment-view-css' => '9863a85e',
     'phui-invisible-character-view-css' => 'c694c4a4',
     'phui-left-right-css' => '68513c34',
     'phui-lightbox-css' => '4ebf22da',
diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
index d9ebad0663..35cdedbcc6 100644
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -834,7 +834,6 @@ final class DifferentialChangesetParser extends Phobject {
       ->setNewAttachesToNewFile($this->rightSideAttachesToNewFile)
       ->setCodeCoverage($this->getCoverage())
       ->setRenderingReference($this->getRenderingReference())
-      ->setMarkupEngine($this->markupEngine)
       ->setHandles($this->handles)
       ->setOldLines($this->old)
       ->setNewLines($this->new)
@@ -845,6 +844,10 @@ final class DifferentialChangesetParser extends Phobject {
       ->setHighlightingDisabled($this->highlightingDisabled)
       ->setDepthOnlyLines($this->getDepthOnlyLines());
 
+    if ($this->markupEngine) {
+      $renderer->setMarkupEngine($this->markupEngine);
+    }
+
     list($engine, $old_ref, $new_ref) = $this->newDocumentEngine();
     if ($engine) {
       $engine_blocks = $engine->newEngineBlocks(
diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php
index f4ed05545b..186924e359 100644
--- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php
+++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php
@@ -3,6 +3,17 @@
 final class DifferentialChangesetOneUpRenderer
   extends DifferentialChangesetHTMLRenderer {
 
+  private $simpleMode;
+
+  public function setSimpleMode($simple_mode) {
+    $this->simpleMode = $simple_mode;
+    return $this;
+  }
+
+  public function getSimpleMode() {
+    return $this->simpleMode;
+  }
+
   public function isOneUpRenderer() {
     return true;
   }
@@ -36,6 +47,8 @@ final class DifferentialChangesetOneUpRenderer
   protected function renderPrimitives(array $primitives, $rows) {
     list($left_prefix, $right_prefix) = $this->getLineIDPrefixes();
 
+    $is_simple = $this->getSimpleMode();
+
     $no_copy = phutil_tag('td', array('class' => 'copy'));
     $no_coverage = null;
 
@@ -185,6 +198,12 @@ final class DifferentialChangesetOneUpRenderer
             $cells[] = $no_coverage;
           }
 
+          // In simple mode, only render the text. This is used to render
+          // "Edit Suggestions" in inline comments.
+          if ($is_simple) {
+            $cells = array($cells[3]);
+          }
+
           $out[] = phutil_tag('tr', array(), $cells);
 
           break;
@@ -231,11 +250,17 @@ final class DifferentialChangesetOneUpRenderer
       }
     }
 
+    $result = null;
+
     if ($out) {
-      return $this->wrapChangeInTable(phutil_implode_html('', $out));
+      if ($is_simple) {
+        $result = $this->newSimpleTable($out);
+      } else {
+        $result = $this->wrapChangeInTable(phutil_implode_html('', $out));
+      }
     }
 
-    return null;
+    return $result;
   }
 
   public function renderDocumentEngineBlocks(
@@ -488,4 +513,14 @@ final class DifferentialChangesetOneUpRenderer
       ->addInlineView($view);
   }
 
+
+  private function newSimpleTable($content) {
+    return phutil_tag(
+      'table',
+      array(
+        'class' => 'diff-1up-simple-table',
+      ),
+      $content);
+  }
+
 }
diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
index e46d934b5a..95f8473caf 100644
--- a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
+++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
@@ -3,10 +3,20 @@
 final class PhabricatorDiffInlineCommentContext
   extends PhabricatorInlineCommentContext {
 
+  private $filename;
   private $headLines;
   private $bodyLines;
   private $tailLines;
 
+  public function setFilename($filename) {
+    $this->filename = $filename;
+    return $this;
+  }
+
+  public function getFilename() {
+    return $this->filename;
+  }
+
   public function setHeadLines(array $head_lines) {
     $this->headLines = $head_lines;
     return $this;
diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
index b5f39b1d2f..48a100f607 100644
--- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
+++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
@@ -289,8 +289,12 @@ abstract class PhabricatorDiffInlineCommentQuery
         }
 
         if ($inline->getIsNewFile()) {
+          $vector = $changeset->getNewStatePathVector();
+          $filename = last($vector);
           $corpus = $changeset->makeNewFile();
         } else {
+          $vector = $changeset->getOldStatePathVector();
+          $filename = last($vector);
           $corpus = $changeset->makeOldFile();
         }
 
@@ -321,6 +325,7 @@ abstract class PhabricatorDiffInlineCommentQuery
         $tail = $this->simplifyContext($tail, false);
 
         $context = id(new PhabricatorDiffInlineCommentContext())
+          ->setFilename($filename)
           ->setHeadLines($head)
           ->setBodyLines($body)
           ->setTailLines($tail);
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
index b2a49f0624..ddf0dddf15 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
@@ -540,20 +540,35 @@ final class PHUIDiffInlineCommentDetailView
       return null;
     }
 
+    $viewer = $this->getViewer();
 
-    $raw_diff = id(new PhabricatorDifferenceEngine())
-      ->generateRawDiffFromFileContent($old_lines, $new_lines);
+    $changeset = id(new PhabricatorDifferenceEngine())
+      ->generateChangesetFromFileContent($old_lines, $new_lines);
 
-    $raw_diff = phutil_split_lines($raw_diff);
-    $raw_diff = array_slice($raw_diff, 3);
-    $raw_diff = implode('', $raw_diff);
+    $changeset->setFilename($context->getFilename());
+
+    // TODO: This isn't cached!
+
+    $viewstate = new PhabricatorChangesetViewState();
+
+    $parser = id(new DifferentialChangesetParser())
+      ->setViewer($viewer)
+      ->setViewstate($viewstate)
+      ->setChangeset($changeset);
+
+    $renderer = new DifferentialChangesetOneUpRenderer();
+    $renderer->setSimpleMode(true);
+
+    $parser->setRenderer($renderer);
+
+    $diff_view = $parser->render(0, 0xFFFF, array());
 
     $view = phutil_tag(
       'div',
       array(
         'class' => 'inline-suggestion-view PhabricatorMonospaced',
       ),
-      $raw_diff);
+      $diff_view);
 
     return $view;
   }
diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css
index 9fefc65353..246c81013c 100644
--- a/webroot/rsrc/css/application/differential/phui-inline-comment.css
+++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css
@@ -476,20 +476,32 @@ textarea.inline-suggestion-input {
   border-right: 1px solid {$lightgreyborder};
 }
 
-.inline-suggestion-input-cell {
-  padding: 8px;
+.inline-suggestion-table td.inline-suggestion-input-cell {
+  padding: 8px 4px;
 }
 
-.inline-suggestion-text-cell {
-  padding: 0 8px;
+.inline-suggestion-table td.inline-suggestion-text-cell {
+  /* This is attempting to align the text in the textarea with the text on
+     the surrounding context lines. */
+  padding: 0 8px 0 11px;
 }
 
 .inline-suggestion-view {
-  padding: 8px 12px;
+  padding: 4px 0;
   white-space: pre-wrap;
-  background: {$greybackground};
+  background: {$lightgreybackground};
   margin: 0 -12px 8px;
   border-width: 1px 0;
   border-style: solid;
   border-color: {$lightgreyborder};
 }
+
+.diff-1up-simple-table {
+  width: 100%;
+  table-layout: fixed;
+}
+
+.diff-1up-simple-table > tbody > tr > td {
+  padding-left: 12px;
+  padding-right: 12px;
+}

From d2d7e7f5ff1b473f92137c9954dd14dcbd44f25a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 May 2020 09:18:18 -0700
Subject: [PATCH 16/23] Clean up Diffusion behaviors for inline edit
 suggestions

Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

Test Plan:
  - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
  - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278
---
 resources/celerity/map.php                    | 12 +--
 .../DifferentialDiffInlineCommentQuery.php    | 74 +++++++++++++++++++
 .../query/DiffusionDiffInlineCommentQuery.php |  4 +
 .../PhabricatorDiffInlineCommentQuery.php     | 74 ++-----------------
 .../rsrc/js/application/diff/DiffInline.js    |  2 -
 5 files changed, 92 insertions(+), 74 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index c1b05eae86..6dd33c64bf 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '5c459f92',
-    'differential.pkg.js' => '256a327a',
+    'differential.pkg.js' => '24616785',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -381,7 +381,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
-    'rsrc/js/application/diff/DiffInline.js' => '829b88bf',
+    'rsrc/js/application/diff/DiffInline.js' => '008b6a15',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -776,7 +776,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '6e5e03d2',
     'phabricator-diff-changeset-list' => 'b51ba93a',
-    'phabricator-diff-inline' => '829b88bf',
+    'phabricator-diff-inline' => '008b6a15',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -917,6 +917,9 @@ return array(
     'unhandled-exception-css' => '9ecfc00d',
   ),
   'requires' => array(
+    '008b6a15' => array(
+      'javelin-dom',
+    ),
     '0116d3e8' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1639,9 +1642,6 @@ return array(
     '8207abf9' => array(
       'javelin-dom',
     ),
-    '829b88bf' => array(
-      'javelin-dom',
-    ),
     83754533 => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php
index b40589bb36..b8392c474d 100644
--- a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php
+++ b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php
@@ -67,4 +67,78 @@ final class DifferentialDiffInlineCommentQuery
     return $id_map;
   }
 
+  protected function newInlineContextMap(array $inlines) {
+    $viewer = $this->getViewer();
+
+    $map = array();
+
+    foreach ($inlines as $key => $inline) {
+      $changeset = id(new DifferentialChangesetQuery())
+        ->setViewer($viewer)
+        ->withIDs(array($inline->getChangesetID()))
+        ->needHunks(true)
+        ->executeOne();
+      if (!$changeset) {
+        continue;
+      }
+
+      $hunks = $changeset->getHunks();
+
+      $is_simple =
+        (count($hunks) === 1) &&
+        ((int)head($hunks)->getOldOffset() <= 1) &&
+        ((int)head($hunks)->getNewOffset() <= 1);
+
+      if (!$is_simple) {
+        continue;
+      }
+
+      if ($inline->getIsNewFile()) {
+        $vector = $changeset->getNewStatePathVector();
+        $filename = last($vector);
+        $corpus = $changeset->makeNewFile();
+      } else {
+        $vector = $changeset->getOldStatePathVector();
+        $filename = last($vector);
+        $corpus = $changeset->makeOldFile();
+      }
+
+      $corpus = phutil_split_lines($corpus);
+
+      // Adjust the line number into a 0-based offset.
+      $offset = $inline->getLineNumber();
+      $offset = $offset - 1;
+
+      // Adjust the inclusive range length into a row count.
+      $length = $inline->getLineLength();
+      $length = $length + 1;
+
+      $head_min = max(0, $offset - 3);
+      $head_max = $offset;
+      $head_len = $head_max - $head_min;
+
+      if ($head_len) {
+        $head = array_slice($corpus, $head_min, $head_len, true);
+        $head = $this->simplifyContext($head, true);
+      } else {
+        $head = array();
+      }
+
+      $body = array_slice($corpus, $offset, $length, true);
+
+      $tail = array_slice($corpus, $offset + $length, 3, true);
+      $tail = $this->simplifyContext($tail, false);
+
+      $context = id(new PhabricatorDiffInlineCommentContext())
+        ->setFilename($filename)
+        ->setHeadLines($head)
+        ->setBodyLines($body)
+        ->setTailLines($tail);
+
+      $map[$key] = $context;
+    }
+
+    return $map;
+  }
+
 }
diff --git a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php
index af116a4097..c395231808 100644
--- a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php
+++ b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php
@@ -66,4 +66,8 @@ final class DiffusionDiffInlineCommentQuery
     return array();
   }
 
+  protected function newInlineContextMap(array $inlines) {
+    return array();
+  }
+
 }
diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
index 48a100f607..85987c89a5 100644
--- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
+++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
@@ -18,6 +18,8 @@ abstract class PhabricatorDiffInlineCommentQuery
     $viewer_phid,
     array $comments);
 
+  abstract protected function newInlineContextMap(array $inlines);
+
   final public function withFixedStates(array $states) {
     $this->fixedStates = $states;
     return $this;
@@ -265,72 +267,12 @@ abstract class PhabricatorDiffInlineCommentQuery
         $need_context[] = $inline;
       }
 
-      foreach ($need_context as $inline) {
-        $changeset = id(new DifferentialChangesetQuery())
-          ->setViewer($viewer)
-          ->withIDs(array($inline->getChangesetID()))
-          ->needHunks(true)
-          ->executeOne();
-        if (!$changeset) {
-          $inline->attachInlineContext(null);
-          continue;
+      if ($need_context) {
+        $context_map = $this->newInlineContextMap($need_context);
+
+        foreach ($need_context as $key => $inline) {
+          $inline->attachInlineContext(idx($context_map, $key));
         }
-
-        $hunks = $changeset->getHunks();
-
-        $is_simple =
-          (count($hunks) === 1) &&
-          ((int)head($hunks)->getOldOffset() <= 1) &&
-          ((int)head($hunks)->getNewOffset() <= 1);
-
-        if (!$is_simple) {
-          $inline->attachInlineContext(null);
-          continue;
-        }
-
-        if ($inline->getIsNewFile()) {
-          $vector = $changeset->getNewStatePathVector();
-          $filename = last($vector);
-          $corpus = $changeset->makeNewFile();
-        } else {
-          $vector = $changeset->getOldStatePathVector();
-          $filename = last($vector);
-          $corpus = $changeset->makeOldFile();
-        }
-
-        $corpus = phutil_split_lines($corpus);
-
-        // Adjust the line number into a 0-based offset.
-        $offset = $inline->getLineNumber();
-        $offset = $offset - 1;
-
-        // Adjust the inclusive range length into a row count.
-        $length = $inline->getLineLength();
-        $length = $length + 1;
-
-        $head_min = max(0, $offset - 3);
-        $head_max = $offset;
-        $head_len = $head_max - $head_min;
-
-        if ($head_len) {
-          $head = array_slice($corpus, $head_min, $head_len, true);
-          $head = $this->simplifyContext($head, true);
-        } else {
-          $head = array();
-        }
-
-        $body = array_slice($corpus, $offset, $length, true);
-
-        $tail = array_slice($corpus, $offset + $length, 3, true);
-        $tail = $this->simplifyContext($tail, false);
-
-        $context = id(new PhabricatorDiffInlineCommentContext())
-          ->setFilename($filename)
-          ->setHeadLines($head)
-          ->setBodyLines($body)
-          ->setTailLines($tail);
-
-        $inline->attachInlineContext($context);
       }
 
     }
@@ -338,7 +280,7 @@ abstract class PhabricatorDiffInlineCommentQuery
     return $inlines;
   }
 
-  private function simplifyContext(array $lines, $is_head) {
+  final protected function simplifyContext(array $lines, $is_head) {
     // We want to provide the smallest amount of context we can while still
     // being useful, since the actual code is visible nearby and showing a
     // ton of context is silly.
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index e03138abfa..1e700855f1 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -668,8 +668,6 @@ JX.install('DiffInline', {
       this._editRow = this._drawRows(rows, null, 'edit');
 
       this._drawSuggestionState(this._editRow);
-      JX.log(this._originalState);
-
       this.setHasSuggestion(this._originalState.hasSuggestion);
     },
 

From 5d0ae283a9ab44e2ec3986d9a409abbb3643730b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 May 2020 10:32:52 -0700
Subject: [PATCH 17/23] Put a readthrough cache in front of inline context
 construction

Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.

Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21279
---
 .../DifferentialDiffInlineCommentQuery.php    | 21 +++--
 .../query/DiffusionDiffInlineCommentQuery.php |  4 +
 .../PhabricatorDiffInlineCommentContext.php   | 20 +++++
 .../interface/PhabricatorInlineComment.php    | 10 +++
 .../PhabricatorDiffInlineCommentQuery.php     | 82 +++++++++++++++++--
 5 files changed, 125 insertions(+), 12 deletions(-)

diff --git a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php
index b8392c474d..7e2f7127b1 100644
--- a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php
+++ b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php
@@ -67,17 +67,26 @@ final class DifferentialDiffInlineCommentQuery
     return $id_map;
   }
 
+  protected function newInlineContextFromCacheData(array $map) {
+    return PhabricatorDiffInlineCommentContext::newFromCacheData($map);
+  }
+
   protected function newInlineContextMap(array $inlines) {
     $viewer = $this->getViewer();
-
     $map = array();
 
+    $changeset_ids = mpull($inlines, 'getChangesetID');
+
+    $changesets = id(new DifferentialChangesetQuery())
+      ->setViewer($viewer)
+      ->withIDs($changeset_ids)
+      ->needHunks(true)
+      ->execute();
+    $changesets = mpull($changesets, null, 'getID');
+
     foreach ($inlines as $key => $inline) {
-      $changeset = id(new DifferentialChangesetQuery())
-        ->setViewer($viewer)
-        ->withIDs(array($inline->getChangesetID()))
-        ->needHunks(true)
-        ->executeOne();
+      $changeset = idx($changesets, $inline->getChangesetID());
+
       if (!$changeset) {
         continue;
       }
diff --git a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php
index c395231808..01dc49a91e 100644
--- a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php
+++ b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php
@@ -70,4 +70,8 @@ final class DiffusionDiffInlineCommentQuery
     return array();
   }
 
+  protected function newInlineContextFromCacheData(array $map) {
+    return PhabricatorDiffInlineCommentContext::newFromCacheData($map);
+  }
+
 }
diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
index 95f8473caf..f09d53092d 100644
--- a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
+++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php
@@ -8,6 +8,26 @@ final class PhabricatorDiffInlineCommentContext
   private $bodyLines;
   private $tailLines;
 
+  public static function newFromCacheData(array $map) {
+    $context = new self();
+
+    $context->setFilename(idx($map, 'filename'));
+    $context->setHeadLines(idx($map, 'headLines'));
+    $context->setBodyLines(idx($map, 'bodyLines'));
+    $context->setTailLines(idx($map, 'tailLines'));
+
+    return $context;
+  }
+
+  public function newCacheDataMap() {
+    return array(
+      'filename' => $this->getFilename(),
+      'headLines' => $this->getHeadLines(),
+      'bodyLines' => $this->getBodyLines(),
+      'tailLines' => $this->getTailLines(),
+    );
+  }
+
   public function setFilename($filename) {
     $this->filename = $filename;
     return $this;
diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
index 3d3bb6e106..76976b5929 100644
--- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php
+++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
@@ -82,6 +82,16 @@ abstract class PhabricatorInlineComment
     return $this->storageObject;
   }
 
+  public function getInlineCommentCacheFragment() {
+    $phid = $this->getPHID();
+
+    if ($phid === null) {
+      return null;
+    }
+
+    return sprintf('inline(%s)', $phid);
+  }
+
   abstract protected function newStorageObject();
   abstract public function getControllerURI();
 
diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
index 85987c89a5..275a9a5aea 100644
--- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
+++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php
@@ -3,6 +3,8 @@
 abstract class PhabricatorDiffInlineCommentQuery
   extends PhabricatorApplicationTransactionCommentQuery {
 
+  const INLINE_CONTEXT_CACHE_VERSION = 1;
+
   private $fixedStates;
   private $needReplyToComments;
   private $publishedComments;
@@ -19,6 +21,7 @@ abstract class PhabricatorDiffInlineCommentQuery
     array $comments);
 
   abstract protected function newInlineContextMap(array $inlines);
+  abstract protected function newInlineContextFromCacheData(array $map);
 
   final public function withFixedStates(array $states) {
     $this->fixedStates = $states;
@@ -268,18 +271,85 @@ abstract class PhabricatorDiffInlineCommentQuery
       }
 
       if ($need_context) {
-        $context_map = $this->newInlineContextMap($need_context);
-
-        foreach ($need_context as $key => $inline) {
-          $inline->attachInlineContext(idx($context_map, $key));
-        }
+        $this->loadInlineCommentContext($need_context);
       }
-
     }
 
     return $inlines;
   }
 
+  private function loadInlineCommentContext(array $inlines) {
+    $cache_keys = array();
+    foreach ($inlines as $key => $inline) {
+      $object = $inline->newInlineCommentObject();
+      $fragment = $object->getInlineCommentCacheFragment();
+
+      if ($fragment === null) {
+        continue;
+      }
+
+      $cache_keys[$key] = sprintf(
+        '%s.context(v%d)',
+        $fragment,
+        self::INLINE_CONTEXT_CACHE_VERSION);
+    }
+
+    $cache = PhabricatorCaches::getMutableStructureCache();
+
+    $cache_map = $cache->getKeys($cache_keys);
+
+    $context_map = array();
+    $need_construct = array();
+
+    foreach ($inlines as $key => $inline) {
+      $cache_key = idx($cache_keys, $key);
+
+      if ($cache_key !== null) {
+        if (array_key_exists($cache_key, $cache_map)) {
+          $cache_data = $cache_map[$cache_key];
+          $context_map[$key] = $this->newInlineContextFromCacheData(
+            $cache_data);
+          continue;
+        }
+      }
+
+      $need_construct[$key] = $inline;
+    }
+
+    if ($need_construct) {
+      $construct_map = $this->newInlineContextMap($need_construct);
+
+      $write_map = array();
+      foreach ($construct_map as $key => $context) {
+        if ($context === null) {
+          $cache_data = $context;
+        } else {
+          $cache_data = $this->newCacheDataFromInlineContext($context);
+        }
+
+        $cache_key = idx($cache_keys, $key);
+        if ($cache_key !== null) {
+          $write_map[$cache_key] = $cache_data;
+        }
+      }
+
+      if ($write_map) {
+        $cache->setKeys($write_map);
+      }
+
+      $context_map += $construct_map;
+    }
+
+    foreach ($inlines as $key => $inline) {
+      $inline->attachInlineContext(idx($context_map, $key));
+    }
+  }
+
+  protected function newCacheDataFromInlineContext(
+    PhabricatorInlineCommentContext $context) {
+    return $context->newCacheDataMap();
+  }
+
   final protected function simplifyContext(array $lines, $is_head) {
     // We want to provide the smallest amount of context we can while still
     // being useful, since the actual code is visible nearby and showing a

From 6d0dbeb77f772d096aadd49f435b7a874a179454 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 May 2020 10:59:33 -0700
Subject: [PATCH 18/23] Use the changeset parse cache to cache suggestion
 changesets

Summary:
Ref T13513. Syntax highlighting is potentially expensive, and the changeset rendering pipeline can cache it. However, the cache is currently keyed ONLY by Differential changeset ID.

Destroy the existing cache and rebuild it with a more standard cache key so it can be used in a more ad-hoc way by inline suggestion snippets.

Test Plan: Used Darkconsole, saw cache hits and no more inline syntax highlighting for changesets with many inlines.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21280
---
 .../autopatches/20200520.inline.01.remcache.sql    |  1 +
 .../autopatches/20200520.inline.02.addcache.sql    |  7 +++++++
 .../parser/DifferentialChangesetParser.php         | 14 ++++++--------
 .../storage/DifferentialSchemaSpec.php             |  9 +++++++--
 .../diff/view/PHUIDiffInlineCommentDetailView.php  | 13 +++++++++----
 5 files changed, 30 insertions(+), 14 deletions(-)
 create mode 100644 resources/sql/autopatches/20200520.inline.01.remcache.sql
 create mode 100644 resources/sql/autopatches/20200520.inline.02.addcache.sql

diff --git a/resources/sql/autopatches/20200520.inline.01.remcache.sql b/resources/sql/autopatches/20200520.inline.01.remcache.sql
new file mode 100644
index 0000000000..356bf5a5a5
--- /dev/null
+++ b/resources/sql/autopatches/20200520.inline.01.remcache.sql
@@ -0,0 +1 @@
+DROP TABLE {$NAMESPACE}_differential.differential_changeset_parse_cache;
diff --git a/resources/sql/autopatches/20200520.inline.02.addcache.sql b/resources/sql/autopatches/20200520.inline.02.addcache.sql
new file mode 100644
index 0000000000..7b9ed64aac
--- /dev/null
+++ b/resources/sql/autopatches/20200520.inline.02.addcache.sql
@@ -0,0 +1,7 @@
+CREATE TABLE {$NAMESPACE}_differential.differential_changeset_parse_cache (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  cacheIndex BINARY(12) NOT NULL,
+  cache LONGBLOB NOT NULL,
+  dateCreated INT UNSIGNED NOT NULL,
+  UNIQUE KEY `key_cacheIndex` (cacheIndex)
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
index 35cdedbcc6..b7ea61b7a9 100644
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -295,8 +295,6 @@ final class DifferentialChangesetParser extends Phobject {
    * By default, there is no render cache key and parsers do not use the cache.
    * This is appropriate for rarely-viewed changesets.
    *
-   * NOTE: Currently, this key must be a valid Differential Changeset ID.
-   *
    * @param   string  Key for identifying this changeset in the render cache.
    * @return  this
    */
@@ -376,9 +374,9 @@ final class DifferentialChangesetParser extends Phobject {
     $conn_r = $changeset->establishConnection('r');
     $data = queryfx_one(
       $conn_r,
-      'SELECT * FROM %T WHERE id = %d',
-      $changeset->getTableName().'_parse_cache',
-      $render_cache_key);
+      'SELECT * FROM %T WHERE cacheIndex = %s',
+      DifferentialChangeset::TABLE_CACHE,
+      PhabricatorHash::digestForIndex($render_cache_key));
 
     if (!$data) {
       return false;
@@ -480,12 +478,12 @@ final class DifferentialChangesetParser extends Phobject {
       try {
         queryfx(
           $conn_w,
-          'INSERT INTO %T (id, cache, dateCreated) VALUES (%d, %B, %d)
+          'INSERT INTO %T (cacheIndex, cache, dateCreated) VALUES (%s, %B, %d)
             ON DUPLICATE KEY UPDATE cache = VALUES(cache)',
           DifferentialChangeset::TABLE_CACHE,
-          $render_cache_key,
+          PhabricatorHash::digestForIndex($render_cache_key),
           $cache,
-          time());
+          PhabricatorTime::getNow());
       } catch (AphrontQueryException $ex) {
         // Ignore these exceptions. A common cause is that the cache is
         // larger than 'max_allowed_packet', in which case we're better off
diff --git a/src/applications/differential/storage/DifferentialSchemaSpec.php b/src/applications/differential/storage/DifferentialSchemaSpec.php
index 4f747af06c..d08a0b1e29 100644
--- a/src/applications/differential/storage/DifferentialSchemaSpec.php
+++ b/src/applications/differential/storage/DifferentialSchemaSpec.php
@@ -9,7 +9,8 @@ final class DifferentialSchemaSpec extends PhabricatorConfigSchemaSpec {
       id(new DifferentialRevision())->getApplicationName(),
       DifferentialChangeset::TABLE_CACHE,
       array(
-        'id' => 'id',
+        'id' => 'auto',
+        'cacheIndex' => 'bytes12',
         'cache' => 'bytes',
         'dateCreated' => 'epoch',
       ),
@@ -18,7 +19,11 @@ final class DifferentialSchemaSpec extends PhabricatorConfigSchemaSpec {
           'columns' => array('id'),
           'unique' => true,
         ),
-        'dateCreated' => array(
+        'key_cacheIndex' => array(
+          'columns' => array('cacheIndex'),
+          'unique' => true,
+        ),
+        'key_created' => array(
           'columns' => array('dateCreated'),
         ),
       ),
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
index ddf0dddf15..fdaa99ac7e 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
@@ -547,8 +547,6 @@ final class PHUIDiffInlineCommentDetailView
 
     $changeset->setFilename($context->getFilename());
 
-    // TODO: This isn't cached!
-
     $viewstate = new PhabricatorChangesetViewState();
 
     $parser = id(new DifferentialChangesetParser())
@@ -556,6 +554,15 @@ final class PHUIDiffInlineCommentDetailView
       ->setViewstate($viewstate)
       ->setChangeset($changeset);
 
+    $fragment = $inline->getInlineCommentCacheFragment();
+    if ($fragment !== null) {
+      $cache_key = sprintf(
+        '%s.suggestion-view(v1, %s)',
+        $fragment,
+        PhabricatorHash::digestForIndex($new_lines));
+      $parser->setRenderCacheKey($cache_key);
+    }
+
     $renderer = new DifferentialChangesetOneUpRenderer();
     $renderer->setSimpleMode(true);
 
@@ -572,6 +579,4 @@ final class PHUIDiffInlineCommentDetailView
 
     return $view;
   }
-
-
 }

From d3d41324be9fd5c0dce02478edd2840c70988f60 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 May 2020 11:53:54 -0700
Subject: [PATCH 19/23] Drop old "differential_commit" table

Summary: Ref T13276. Ref T13513. All readers and writers were removed more than a year ago; clean up the last remnants of this table.

Test Plan: Grepped for table references, found none.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513, T13276

Differential Revision: https://secure.phabricator.com/D21281
---
 .../20200520.inline.03.dropcommit.sql         |  1 +
 .../storage/DifferentialSchemaSpec.php        | 21 -------------------
 2 files changed, 1 insertion(+), 21 deletions(-)
 create mode 100644 resources/sql/autopatches/20200520.inline.03.dropcommit.sql

diff --git a/resources/sql/autopatches/20200520.inline.03.dropcommit.sql b/resources/sql/autopatches/20200520.inline.03.dropcommit.sql
new file mode 100644
index 0000000000..a757a8a0a7
--- /dev/null
+++ b/resources/sql/autopatches/20200520.inline.03.dropcommit.sql
@@ -0,0 +1 @@
+DROP TABLE IF EXISTS {$NAMESPACE}_differential.differential_commit;
diff --git a/src/applications/differential/storage/DifferentialSchemaSpec.php b/src/applications/differential/storage/DifferentialSchemaSpec.php
index d08a0b1e29..cf8b996e97 100644
--- a/src/applications/differential/storage/DifferentialSchemaSpec.php
+++ b/src/applications/differential/storage/DifferentialSchemaSpec.php
@@ -31,27 +31,6 @@ final class DifferentialSchemaSpec extends PhabricatorConfigSchemaSpec {
         'persistence' => PhabricatorConfigTableSchema::PERSISTENCE_CACHE,
       ));
 
-    // TODO: All readers and writers for this table were removed in April
-    // 2019. Destroy this table once we're sure we won't miss it.
-
-    $this->buildRawSchema(
-      id(new DifferentialRevision())->getApplicationName(),
-      'differential_commit',
-      array(
-        'revisionID' => 'id',
-        'commitPHID' => 'phid',
-      ),
-      array(
-        'PRIMARY' => array(
-          'columns' => array('revisionID', 'commitPHID'),
-          'unique' => true,
-        ),
-        'commitPHID' => array(
-          'columns' => array('commitPHID'),
-          'unique' => true,
-        ),
-      ));
-
     $this->buildRawSchema(
       id(new DifferentialRevision())->getApplicationName(),
       ArcanistDifferentialRevisionHash::TABLE_NAME,

From 66566f878d82058f36a438b99f2bfe0479a3c9f8 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 21 May 2020 14:03:21 -0700
Subject: [PATCH 20/23] Make "Open in Editor" use the simple line number of the
 current selected block

Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.

For inlines, this is the inline line number.

For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.

This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.

Test Plan:
  - In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
  - Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
  - Used "\" and "Open in Editor" menu item to open a file with:
    - Nothing selected or changeset selected (line: 1).
    - An inline selected (line: inline line).
    - A block selected (line: first line in block, per above).

Differential Revision: https://secure.phabricator.com/D21282
---
 resources/celerity/map.php                    |  70 ++++----
 resources/celerity/packages.php               |   2 +
 .../view/DifferentialChangesetDetailView.php  |   8 +-
 .../rsrc/js/application/diff/DiffChangeset.js |   9 +-
 .../js/application/diff/DiffChangesetList.js  | 163 ++++++++++++------
 .../diffusion/ExternalEditorLinkEngine.js     |  41 +++++
 webroot/rsrc/js/core/behavior-line-linker.js  |  26 +--
 7 files changed, 212 insertions(+), 107 deletions(-)
 create mode 100644 webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 6dd33c64bf..ea44247b95 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '5c459f92',
-    'differential.pkg.js' => '24616785',
+    'differential.pkg.js' => 'a7171fb6',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -379,14 +379,15 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
+    'rsrc/js/application/diff/DiffChangeset.js' => '3a1ca35b',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
     'rsrc/js/application/diff/DiffInline.js' => '008b6a15',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
     'rsrc/js/application/differential/behavior-populate.js' => 'b86ef6c2',
     'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '94243d89',
+    'rsrc/js/application/diffusion/ExternalEditorLinkEngine.js' => '48a8641f',
     'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'b7b73831',
     'rsrc/js/application/diffusion/behavior-commit-branches.js' => '4b671572',
     'rsrc/js/application/diffusion/behavior-commit-graph.js' => 'ef836bf2',
@@ -484,7 +485,7 @@ return array(
     'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731',
     'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b',
     'rsrc/js/core/behavior-lightbox-attachments.js' => 'c7e748bf',
-    'rsrc/js/core/behavior-line-linker.js' => '590e6527',
+    'rsrc/js/core/behavior-line-linker.js' => '0d915ff5',
     'rsrc/js/core/behavior-linked-container.js' => '74446546',
     'rsrc/js/core/behavior-more.js' => '506aa3f4',
     'rsrc/js/core/behavior-object-selector.js' => '98ef467f',
@@ -645,7 +646,7 @@ return array(
     'javelin-behavior-phabricator-gesture-example' => '242dedd0',
     'javelin-behavior-phabricator-keyboard-pager' => '1325b731',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '42c44e8b',
-    'javelin-behavior-phabricator-line-linker' => '590e6527',
+    'javelin-behavior-phabricator-line-linker' => '0d915ff5',
     'javelin-behavior-phabricator-notification-example' => '29819b75',
     'javelin-behavior-phabricator-object-selector' => '98ef467f',
     'javelin-behavior-phabricator-oncopy' => 'da8f5259',
@@ -709,6 +710,7 @@ return array(
     'javelin-dom' => '94681e22',
     'javelin-dynval' => '202a2e85',
     'javelin-event' => 'c03f2fb4',
+    'javelin-external-editor-link-engine' => '48a8641f',
     'javelin-fx' => '34450586',
     'javelin-history' => '030b4f7a',
     'javelin-install' => '5902260c',
@@ -774,8 +776,8 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '6e5e03d2',
-    'phabricator-diff-changeset-list' => 'b51ba93a',
+    'phabricator-diff-changeset' => '3a1ca35b',
+    'phabricator-diff-changeset-list' => 'cc2c5de5',
     'phabricator-diff-inline' => '008b6a15',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1013,6 +1015,13 @@ return array(
     '0d2490ce' => array(
       'javelin-install',
     ),
+    '0d915ff5' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-history',
+      'javelin-external-editor-link-engine',
+    ),
     '0eaa33a9' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1222,6 +1231,20 @@ return array(
       'trigger-rule',
       'trigger-rule-type',
     ),
+    '3a1ca35b' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+      'javelin-external-editor-link-engine',
+    ),
     '3ae89b20' => array(
       'phui-workcard-view-css',
     ),
@@ -1306,6 +1329,9 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
+    '48a8641f' => array(
+      'javelin-install',
+    ),
     '48fe33d0' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1442,12 +1468,6 @@ return array(
       'javelin-util',
       'javelin-magical-init',
     ),
-    '590e6527' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-history',
-    ),
     '5a6f6a06' => array(
       'javelin-behavior',
       'javelin-quicksand',
@@ -1551,19 +1571,6 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
-    '6e5e03d2' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-      'phuix-button-view',
-    ),
     70245195 => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1970,11 +1977,6 @@ return array(
     'b517bfa0' => array(
       'phui-oi-list-view-css',
     ),
-    'b51ba93a' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     'b557770a' => array(
       'javelin-install',
       'javelin-util',
@@ -2082,6 +2084,11 @@ return array(
       'phuix-icon-view',
       'phabricator-busy',
     ),
+    'cc2c5de5' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'cef53b3e' => array(
       'javelin-install',
       'javelin-dom',
@@ -2422,6 +2429,7 @@ return array(
       'phuix-formation-view',
       'phuix-formation-column-view',
       'phuix-formation-flank-view',
+      'javelin-external-editor-link-engine',
     ),
     'diffusion.pkg.css' => array(
       'diffusion-icons-css',
diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php
index a3df0dd154..a5dabb6014 100644
--- a/resources/celerity/packages.php
+++ b/resources/celerity/packages.php
@@ -220,6 +220,8 @@ return array(
     'phuix-formation-view',
     'phuix-formation-column-view',
     'phuix-formation-flank-view',
+
+    'javelin-external-editor-link-engine',
   ),
   'diffusion.pkg.css' => array(
     'diffusion-icons-css',
diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php
index 357dc375cf..d23ddc62a4 100644
--- a/src/applications/differential/view/DifferentialChangesetDetailView.php
+++ b/src/applications/differential/view/DifferentialChangesetDetailView.php
@@ -252,7 +252,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
           'isLowImportance' => $changeset->getIsLowImportanceChangeset(),
           'isOwned' => $changeset->getIsOwnedChangeset(),
 
-          'editorURI' => $this->getEditorURI(),
+          'editorURITemplate' => $this->getEditorURITemplate(),
           'editorConfigureURI' => $this->getEditorConfigureURI(),
 
           'loaded' => $is_loaded,
@@ -321,7 +321,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
     return $this->diff;
   }
 
-  private function getEditorURI() {
+  private function getEditorURITemplate() {
     $repository = $this->getRepository();
     if (!$repository) {
       return null;
@@ -342,9 +342,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
     $path = $changeset->getAbsoluteRepositoryPath($repository, $diff);
     $path = ltrim($path, '/');
 
-    $line = idx($changeset->getMetadata(), 'line:first', 1);
-
-    return $link_engine->getURIForPath($path, $line);
+    return $link_engine->getURITokensForPath($path);
   }
 
   private function getEditorConfigureURI() {
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
index 79328a52b4..a1d8cba267 100644
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -11,6 +11,7 @@
  *           phabricator-diff-inline
  *           phabricator-diff-path-view
  *           phuix-button-view
+ *           javelin-external-editor-link-engine
  * @javelin
  */
 
@@ -33,7 +34,7 @@ JX.install('DiffChangeset', {
     this._pathParts = data.pathParts;
     this._icon = data.icon;
 
-    this._editorURI = data.editorURI;
+    this._editorURITemplate = data.editorURITemplate;
     this._editorConfigureURI = data.editorConfigureURI;
     this._showPathURI = data.showPathURI;
     this._showDirectoryURI = data.showDirectoryURI;
@@ -87,7 +88,7 @@ JX.install('DiffChangeset', {
     _changesetList: null,
     _icon: null,
 
-    _editorURI: null,
+    _editorURITemplate: null,
     _editorConfigureURI: null,
     _showPathURI: null,
     _showDirectoryURI: null,
@@ -102,8 +103,8 @@ JX.install('DiffChangeset', {
     _isSelected: false,
     _viewMenu: null,
 
-    getEditorURI: function() {
-      return this._editorURI;
+    getEditorURITemplate: function() {
+      return this._editorURITemplate;
     },
 
     getEditorConfigureURI: function() {
diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js
index e7cc8b52fe..7293f89fe0 100644
--- a/webroot/rsrc/js/application/diff/DiffChangesetList.js
+++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js
@@ -355,48 +355,11 @@ JX.install('DiffChangesetList', {
         // lines) reply on the old file.
 
         if (cursor.type == 'change') {
-          var origin = cursor.nodes.begin;
-          var target = cursor.nodes.end;
+          var cells = this._getLineNumberCellsForChangeBlock(
+            cursor.nodes.begin,
+            cursor.nodes.end);
 
-          // The "origin" and "target" are entire rows, but we need to find
-          // a range of "<th />" nodes to actually create an inline, so go
-          // fishing.
-
-          var old_list = [];
-          var new_list = [];
-
-          var row = origin;
-          while (row) {
-            var header = row.firstChild;
-            while (header) {
-              if (this.getLineNumberFromHeader(header)) {
-                if (header.className.indexOf('old') !== -1) {
-                  old_list.push(header);
-                } else if (header.className.indexOf('new') !== -1) {
-                  new_list.push(header);
-                }
-              }
-              header = header.nextSibling;
-            }
-
-            if (row == target) {
-              break;
-            }
-
-            row = row.nextSibling;
-          }
-
-          var use_list;
-          if (new_list.length) {
-            use_list = new_list;
-          } else {
-            use_list = old_list;
-          }
-
-          var src = use_list[0];
-          var dst = use_list[use_list.length - 1];
-
-          cursor.changeset.newInlineForRange(src, dst);
+          cursor.changeset.newInlineForRange(cells.src, cells.dst);
 
           this.setFocus(null);
           return;
@@ -407,6 +370,51 @@ JX.install('DiffChangesetList', {
       this._warnUser(pht('You must select a comment or change to reply to.'));
     },
 
+    _getLineNumberCellsForChangeBlock: function(origin, target) {
+      // The "origin" and "target" are entire rows, but we need to find
+      // a range of cell nodes to actually create an inline, so go
+      // fishing.
+
+      var old_list = [];
+      var new_list = [];
+
+      var row = origin;
+      while (row) {
+        var header = row.firstChild;
+        while (header) {
+          if (this.getLineNumberFromHeader(header)) {
+            if (header.className.indexOf('old') !== -1) {
+              old_list.push(header);
+            } else if (header.className.indexOf('new') !== -1) {
+              new_list.push(header);
+            }
+          }
+          header = header.nextSibling;
+        }
+
+        if (row == target) {
+          break;
+        }
+
+        row = row.nextSibling;
+      }
+
+      var use_list;
+      if (new_list.length) {
+        use_list = new_list;
+      } else {
+        use_list = old_list;
+      }
+
+      var src = use_list[0];
+      var dst = use_list[use_list.length - 1];
+
+      return {
+        src: src,
+        dst: dst
+      };
+    },
+
     _onkeyedit: function() {
       var cursor = this._cursorItem;
 
@@ -505,7 +513,7 @@ JX.install('DiffChangesetList', {
       return changeset;
     },
 
-    _onkeyopeneditor: function() {
+    _onkeyopeneditor: function(e) {
       var pht = this.getTranslations();
       var changeset = this._getChangesetForKeyCommand();
 
@@ -514,13 +522,61 @@ JX.install('DiffChangesetList', {
         return;
       }
 
-      var editor_uri = changeset.getEditorURI();
+      this._openEditor(changeset);
+    },
 
-      if (editor_uri === null) {
+    _openEditor: function(changeset) {
+      var pht = this.getTranslations();
+
+      var editor_template = changeset.getEditorURITemplate();
+      if (editor_template === null) {
         this._warnUser(pht('No external editor is configured.'));
         return;
       }
 
+      var line = null;
+
+      // See PHI1749. We aren't exactly sure what the user intends when they
+      // use the keyboard to select a change block and then activate the
+      // "Open in Editor" function: they might mean to open the old or new
+      // offset, and may have the old or new state (or some other state) in
+      // their working copy.
+
+      // For now, pick: the new state line number if one exists; or the old
+      // state line number if one does not. If nothing else, this behavior is
+      // simple.
+
+      // If there's a document engine, just open the file to the first line.
+      // We currently can not map display blocks to source lines.
+
+      // If there's an inline, open the file to that line.
+
+      if (changeset.getResponseDocumentEngineKey() === null) {
+        var cursor = this._cursorItem;
+        if (cursor && (cursor.changeset === changeset)) {
+          if (cursor.type == 'change') {
+            var cells = this._getLineNumberCellsForChangeBlock(
+              cursor.nodes.begin,
+              cursor.nodes.end);
+            line = this.getLineNumberFromHeader(cells.src);
+          }
+
+          if (cursor.type === 'comment') {
+            var inline = cursor.target;
+            line = inline.getLineNumber();
+          }
+        }
+      }
+
+      var variables = {
+        l: line || 1
+      };
+
+      var editor_uri = new JX.ExternalEditorLinkEngine()
+        .setTemplate(editor_template)
+        .setVariables(variables)
+        .newURI();
+
       JX.$U(editor_uri).go();
     },
 
@@ -1029,10 +1085,21 @@ JX.install('DiffChangesetList', {
         changeset.getShowPathURI())
         .setKeyCommand('d');
 
-      var editor_uri = changeset.getEditorURI();
-      if (editor_uri !== null) {
-        add_link('fa-i-cursor', pht('Open in Editor'), editor_uri, true)
-          .setKeyCommand('\\');
+      var editor_template = changeset.getEditorURITemplate();
+      if (editor_template !== null) {
+        var editor_item = new JX.PHUIXActionView()
+          .setIcon('fa-i-cursor')
+          .setName(pht('Open in Editor'))
+          .setKeyCommand('\\')
+          .setHandler(function(e) {
+
+            changeset_list._openEditor(changeset);
+
+            e.prevent();
+            menu.close();
+          });
+
+        list.addItem(editor_item);
       } else {
         var configure_uri = changeset.getEditorConfigureURI();
         if (configure_uri !== null) {
diff --git a/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js b/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js
new file mode 100644
index 0000000000..c43f1c360a
--- /dev/null
+++ b/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js
@@ -0,0 +1,41 @@
+/**
+ * @provides javelin-external-editor-link-engine
+ * @requires javelin-install
+ * @javelin
+ */
+
+JX.install('ExternalEditorLinkEngine', {
+
+  properties: {
+    template: null,
+    variables: null
+  },
+
+  members: {
+    newURI: function() {
+      var template = this.getTemplate();
+      var variables = this.getVariables();
+
+      var parts = [];
+      for (var ii = 0; ii < template.length; ii++) {
+        var part = template[ii];
+        var value = part.value;
+
+        if (part.type === 'literal') {
+          parts.push(value);
+          continue;
+        }
+
+        if (part.type === 'variable') {
+          if (variables.hasOwnProperty(value)) {
+            var replacement = variables[value];
+            replacement = encodeURIComponent(replacement);
+            parts.push(replacement);
+          }
+        }
+      }
+
+      return parts.join('');
+    }
+  }
+});
diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js
index 9042cb0912..fd8510646e 100644
--- a/webroot/rsrc/js/core/behavior-line-linker.js
+++ b/webroot/rsrc/js/core/behavior-line-linker.js
@@ -4,6 +4,7 @@
  *           javelin-stratcom
  *           javelin-dom
  *           javelin-history
+ *           javelin-external-editor-link-engine
  */
 
 JX.behavior('phabricator-line-linker', function() {
@@ -170,32 +171,19 @@ JX.behavior('phabricator-line-linker', function() {
 
       if (editor_link) {
         var data = JX.Stratcom.getData(editor_link);
-        var template = data.template;
 
         var variables = {
           l: parseInt(Math.min(o, t), 10),
         };
 
-        var parts = [];
-        for (var ii = 0; ii < template.length; ii++) {
-          var part = template[ii];
-          var value = part.value;
+        var template = data.template;
 
-          if (part.type === 'literal') {
-            parts.push(value);
-            continue;
-          }
+        var editor_uri = new JX.ExternalEditorLinkEngine()
+          .setTemplate(template)
+          .setVariables(variables)
+          .newURI();
 
-          if (part.type === 'variable') {
-            if (variables.hasOwnProperty(value)) {
-              var replacement = variables[value];
-              replacement = encodeURIComponent(replacement);
-              parts.push(replacement);
-            }
-          }
-        }
-
-        editor_link.href = parts.join('');
+        editor_link.href = editor_uri;
       }
     });
 

From 87fb35abb72e1435a7c2897605988fb26ce30513 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 21 May 2020 15:01:10 -0700
Subject: [PATCH 21/23] Prevent keyboard selection of change blocks inside edit
 suggestions

Summary: Ref T13513. When a revision has inlines with edit suggestions, pressing "j" and "k" can incorrectly select the blocks inside the diffs inside the inlines.

Test Plan: Used "j" to cycle through changes in a revision with inline comments with edit suggestions, didn't get jumped into the suggestion diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21283
---
 resources/celerity/map.php                        |  8 ++++----
 webroot/rsrc/js/application/diff/DiffChangeset.js | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index ea44247b95..1d98650a7e 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '5c459f92',
-    'differential.pkg.js' => 'a7171fb6',
+    'differential.pkg.js' => '2b4a7014',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -379,7 +379,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '3a1ca35b',
+    'rsrc/js/application/diff/DiffChangeset.js' => '39dcf2c3',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
     'rsrc/js/application/diff/DiffInline.js' => '008b6a15',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
@@ -776,7 +776,7 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '3a1ca35b',
+    'phabricator-diff-changeset' => '39dcf2c3',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
     'phabricator-diff-inline' => '008b6a15',
     'phabricator-diff-path-view' => '8207abf9',
@@ -1231,7 +1231,7 @@ return array(
       'trigger-rule',
       'trigger-rule-type',
     ),
-    '3a1ca35b' => array(
+    '39dcf2c3' => array(
       'javelin-dom',
       'javelin-util',
       'javelin-stratcom',
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
index a1d8cba267..212d19f5c6 100644
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -454,9 +454,24 @@ JX.install('DiffChangeset', {
       var blocks = [];
       var block;
       var ii;
+      var parent_node = null;
       for (ii = 0; ii < rows.length; ii++) {
         var type = this._getRowType(rows[ii]);
 
+        // This row might be part of a diff inside an inline comment, showing
+        // an inline edit suggestion. Before we accept it as a possible target
+        // for selection, make sure it's a child of the right parent.
+
+        if (parent_node === null) {
+          parent_node = rows[ii].parentNode;
+        }
+
+        if (type !== null) {
+          if (rows[ii].parentNode !== parent_node) {
+            type = null;
+          }
+        }
+
         if (!block || (block.type !== type)) {
           block = {
             type: type,

From 4fd0628fae4545655f5b84d425c47ef4d3da9302 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 May 2020 11:47:10 -0700
Subject: [PATCH 22/23] Fix two rendering issues with Jupyter notebooks

Summary:
See PHI1752.

  - Early exit of document layout can cause us to fail to populate available rows.
  - Some Jupyter documents have "markdown" cells with plain strings, apparently.

Test Plan: Successfully rendered example diff from PHI1752.

Differential Revision: https://secure.phabricator.com/D21285
---
 .../diff/PhabricatorDocumentEngineBlocks.php  |  1 +
 .../PhabricatorJupyterDocumentEngine.php      | 19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
index d07f341815..3897af2f02 100644
--- a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
+++ b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
@@ -61,6 +61,7 @@ final class PhabricatorDocumentEngineBlocks
     $lists = $this->lists;
 
     if (count($lists) != 2) {
+      $this->layoutAvailableRowCount = 0;
       return array();
     }
 
diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php
index 3e479fdace..753cdf3921 100644
--- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php
+++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php
@@ -63,6 +63,7 @@ final class PhabricatorJupyterDocumentEngine
       $blocks->addBlockList($uref, $u_blocks);
       $blocks->addBlockList($vref, $v_blocks);
     } catch (Exception $ex) {
+      phlog($ex);
       $blocks->addMessage($ex->getMessage());
     }
 
@@ -85,10 +86,14 @@ final class PhabricatorJupyterDocumentEngine
       switch ($utype) {
         case 'markdown':
           $usource = idx($ucell, 'source');
-          $usource = implode('', $usource);
+          if (is_array($usource)) {
+            $usource = implode('', $usource);
+          }
 
           $vsource = idx($vcell, 'source');
-          $vsource = implode('', $vsource);
+          if (is_array($vsource)) {
+            $vsource = implode('', $vsource);
+          }
 
           $diff = id(new PhutilProseDifferenceEngine())
             ->getDiff($usource, $vsource);
@@ -254,7 +259,10 @@ final class PhabricatorJupyterDocumentEngine
           $hash_input = $cell['raw'];
           break;
         case 'markdown':
-          $hash_input = implode('', $cell['source']);
+          $hash_input = $cell['source'];
+          if (is_array($hash_input)) {
+            $hash_input = implode('', $cell['source']);
+          }
           break;
         default:
           $hash_input = serialize($cell);
@@ -367,10 +375,11 @@ final class PhabricatorJupyterDocumentEngine
     $results = array();
     foreach ($cells as $cell) {
       $cell_type = idx($cell, 'cell_type');
-
       if ($cell_type === 'markdown') {
         $source = $cell['source'];
-        $source = implode('', $source);
+        if (is_array($source)) {
+          $source = implode('', $source);
+        }
 
         // Attempt to split contiguous blocks of markdown into smaller
         // pieces.

From 959a835b95dd39bb2463882ce989e2d5e7401b8e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 May 2020 05:42:34 -0700
Subject: [PATCH 23/23] When executing a repository passthru command via
 CommandEngine, don't set a timeout

Summary:
Ref T13541. The passthru future does not have time limit behavior, so if we reach this code we currently fail.

Phabricator never reaches this code normally, but this code is reachable during debugging if you try to foreground a slow fetch to inspect it.

Passthru commands generally only make sense to run interactively, and the caller or control script can enforce their own timeouts (usually by pressing "^C" with their fingers).

Test Plan: Used a debugging script to run ref-by-ref fetches in the foreground.

Maniphest Tasks: T13541

Differential Revision: https://secure.phabricator.com/D21284
---
 .../diffusion/protocol/DiffusionCommandEngine.php          | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/applications/diffusion/protocol/DiffusionCommandEngine.php b/src/applications/diffusion/protocol/DiffusionCommandEngine.php
index a0fe568168..a0f1adaf1f 100644
--- a/src/applications/diffusion/protocol/DiffusionCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionCommandEngine.php
@@ -120,6 +120,7 @@ abstract class DiffusionCommandEngine extends Phobject {
   public function newFuture() {
     $argv = $this->newCommandArgv();
     $env = $this->newCommandEnvironment();
+    $is_passthru = $this->getPassthru();
 
     if ($this->getSudoAsDaemon()) {
       $command = call_user_func_array('csprintf', $argv);
@@ -127,7 +128,7 @@ abstract class DiffusionCommandEngine extends Phobject {
       $argv = array('%C', $command);
     }
 
-    if ($this->getPassthru()) {
+    if ($is_passthru) {
       $future = newv('PhutilExecPassthru', $argv);
     } else {
       $future = newv('ExecFuture', $argv);
@@ -139,7 +140,9 @@ abstract class DiffusionCommandEngine extends Phobject {
     // to try to avoid cases where `git fetch` hangs for some reason and we're
     // left sitting with a held lock forever.
     $repository = $this->getRepository();
-    $future->setTimeout($repository->getEffectiveCopyTimeLimit());
+    if (!$is_passthru) {
+      $future->setTimeout($repository->getEffectiveCopyTimeLimit());
+    }
 
     return $future;
   }