From bcc90d8c6b8c0c556d9105e669cdba9db4d44370 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 25 Nov 2018 15:39:15 -0800
Subject: [PATCH] Fix an off-by-one error affecting mail rendering of inlines
 on the final line of a file

Summary: Depends on D19837. Ref T13216. See PHI985. There's an off-by-one error here between how inline comments store "length" and how context rendering treats "length". We need to add 1 to the length, but currently do it a little too early. Do it slightly later so that inlines on the final line of a file render properly.

Test Plan: Left an inline on the final line of a new file, saw it render properly in HTML mail.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19838
---
 .../mail/DifferentialInlineCommentMailView.php           | 2 +-
 .../differential/parser/DifferentialChangesetParser.php  | 9 ++++++++-
 src/applications/feed/PhabricatorFeedStoryPublisher.php  | 8 ++++----
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/applications/differential/mail/DifferentialInlineCommentMailView.php b/src/applications/differential/mail/DifferentialInlineCommentMailView.php
index 0bc914914b..c1430320e3 100644
--- a/src/applications/differential/mail/DifferentialInlineCommentMailView.php
+++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php
@@ -361,7 +361,7 @@ final class DifferentialInlineCommentMailView
 
     return $parser->render(
       $start - $context,
-      $length + 1 + (2 * $context),
+      $length + (2 * $context),
       array());
   }
 
diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
index 596ea0e426..e214aa16a4 100644
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -874,9 +874,16 @@ final class DifferentialChangesetParser extends Phobject {
         $offset_map = $this->old;
       }
 
+      // NOTE: Inline comments use zero-based lengths. For example, a comment
+      // that starts and ends on line 123 has length 0. Rendering considers
+      // this range to have length 1. Probably both should agree, but that
+      // ship likely sailed long ago. Tweak things here to get the two systems
+      // to agree. See PHI985, where this affected mail rendering of inline
+      // comments left on the final line of a file.
+
       $range_end = $this->getOffset($offset_map, $range_start + $range_len);
       $range_start = $this->getOffset($offset_map, $range_start);
-      $range_len = ($range_end - $range_start);
+      $range_len = ($range_end - $range_start) + 1;
     }
 
     $render_pch = $this->shouldRenderPropertyChangeHeader($this->changeset);
diff --git a/src/applications/feed/PhabricatorFeedStoryPublisher.php b/src/applications/feed/PhabricatorFeedStoryPublisher.php
index 47dde8c98a..70d9a2f61c 100644
--- a/src/applications/feed/PhabricatorFeedStoryPublisher.php
+++ b/src/applications/feed/PhabricatorFeedStoryPublisher.php
@@ -133,9 +133,9 @@ final class PhabricatorFeedStoryPublisher extends Phobject {
 
       queryfx(
         $conn,
-        'INSERT INTO %T (objectPHID, chronologicalKey) VALUES %Q',
+        'INSERT INTO %T (objectPHID, chronologicalKey) VALUES %LQ',
         $ref->getTableName(),
-        implode(', ', $sql));
+        $sql);
     }
 
     $subscribed_phids = $this->subscribedPHIDs;
@@ -191,9 +191,9 @@ final class PhabricatorFeedStoryPublisher extends Phobject {
         $conn,
         'INSERT INTO %T '.
         '(primaryObjectPHID, userPHID, chronologicalKey, hasViewed) '.
-        'VALUES %Q',
+        'VALUES %LQ',
         $notif->getTableName(),
-        implode(', ', $sql));
+        $sql);
     }
 
     PhabricatorUserCache::clearCaches(