From 8858b6cf8d178a5835a86f1598345ca01ed30e22 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 10 Mar 2016 16:05:08 -0800 Subject: [PATCH] When replying to a ghost comment, attach the reply to the same place Summary: Fixes T10562. I left this behavior sort of ambiguous in the original implementation because I didn't anticipate or stumble across this situation. It's easy to fix: when you reply to a ghost, just put the reply in the exact same place as the ghost (even if it's a different diff), so they always move/ghost/port/thread together. Test Plan: See T10562 for reproduction steps and a "before" picture. Here's the after picture: {F1168983} The two comments at the bottom are pre-fix, and exhibit the bug. The comment at the top is post-fix, and appears adjacent to the original correctly. Reviewers: chad Reviewed By: chad Subscribers: eadler Maniphest Tasks: T10562 Differential Revision: https://secure.phabricator.com/D15458 --- .../diff/PhabricatorInlineCommentController.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index aa37d29c72..1c92b167b0 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -305,13 +305,18 @@ abstract class PhabricatorInlineCommentController pht('Failed to load comment "%s".', $reply_phid)); } - // NOTE: It's fine to reply to a comment from a different changeset, so - // the reply comment may not appear on the same changeset that the new - // comment appears on. This is expected in the case of ghost comments. - // We currently put the new comment on the visible changeset, not the - // original comment's changeset. + // When replying, force the new comment into the same location as the + // old comment. If we don't do this, replying to a ghost comment from + // diff A while viewing diff B can end up placing the two comments in + // different places while viewing diff C, because the porting algorithm + // makes a different decision. Forcing the comments to bind to the same + // place makes sure they stick together no matter which diff is being + // viewed. See T10562 for discussion. + $this->changesetID = $reply_comment->getChangesetID(); $this->isNewFile = $reply_comment->getIsNewFile(); + $this->lineNumber = $reply_comment->getLineNumber(); + $this->lineLength = $reply_comment->getLineLength(); } }