From f044c1e999b10939fe6536cd305fdfa7e8cadc95 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 22 Apr 2015 16:48:56 -0700
Subject: [PATCH] Possibly fix issue with subpriority recursion

Summary:
Ref T7664. Currently, when spreading subpriorities we may recurse deeply in certain conditions. Make sure we never recurse more than one level.

To try to mitigate issues with floating point precision, be more aggressive about selecting tasks to reorder.

I wasn't really able to come up with a realistic test case here, and the test cases I found which sort of approximated the behavior took way too long to generate data to actually commit.

This approach is inherently somewhat fragile but hopefully this is approximately good enough. We don't have a durable storage engine which can meaningfully represent double-linked lists right now.

Test Plan:
  - Wrote some (slow) tests which kind of approximately hit the issue.
  - Verified they maxed out at stack depth 2 after the change.
  - Unit tests still pass.
  - Dragged some tasks around.
  - Couldn't come up with any pathological issues here by thinking about it?

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7664

Differential Revision: https://secure.phabricator.com/D12511
---
 .../editor/ManiphestTransactionEditor.php     | 23 ++++++++++++++-----
 .../maniphest/query/ManiphestTaskQuery.php    | 22 ++++++++++++++++++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
index 0b733baf27..abc83369bf 100644
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -632,7 +632,8 @@ final class ManiphestTransactionEditor
    */
   public static function getAdjacentSubpriority(
     ManiphestTask $dst,
-    $is_after) {
+    $is_after,
+    $allow_recursion = true) {
 
     $query = id(new ManiphestTaskQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
@@ -654,20 +655,25 @@ final class ManiphestTransactionEditor
     // If we find an adjacent task, we average the two subpriorities and
     // return the result.
     if ($adjacent) {
+      $epsilon = 0.01;
+
       // If the adjacent task has a subpriority that is identical or very
       // close to the task we're looking at, we're going to move it and all
       // tasks with the same subpriority a little farther down the subpriority
       // scale.
-      if (abs($adjacent->getSubpriority() - $base) < 0.01) {
+      if ($allow_recursion &&
+          (abs($adjacent->getSubpriority() - $base) < $epsilon)) {
         $conn_w = $adjacent->establishConnection('w');
 
-        // Get all of the tasks with the same subpriority as the adjacent
+        $min = ($adjacent->getSubpriority() - ($epsilon));
+        $max = ($adjacent->getSubpriority() + ($epsilon));
+
+        // Get all of the tasks with the similar subpriorities to the adjacent
         // task, including the adjacent task itself.
-        $shift_base = $adjacent->getSubpriority();
         $query = id(new ManiphestTaskQuery())
           ->setViewer(PhabricatorUser::getOmnipotentUser())
           ->withPriorities(array($adjacent->getPriority()))
-          ->withSubpriorities(array($shift_base));
+          ->withSubpriorityBetween($min, $max);
 
         if (!$is_after) {
           $query->setOrderVector(array('-priority', '-subpriority', '-id'));
@@ -678,11 +684,16 @@ final class ManiphestTransactionEditor
         $shift_all = $query->execute();
         $shift_last = last($shift_all);
 
+        // Select the most extreme subpriority in the result set as the
+        // base value.
+        $shift_base = head($shift_all)->getSubpriority();
+
         // Find the subpriority before or after the task at the end of the
         // block.
         list($shift_pri, $shift_sub) = self::getAdjacentSubpriority(
           $shift_last,
-          $is_after);
+          $is_after,
+          $allow_recursion = false);
 
         $delta = ($shift_sub - $shift_base);
         $count = count($shift_all);
diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php
index 3a91e2bbf4..0046bd5163 100644
--- a/src/applications/maniphest/query/ManiphestTaskQuery.php
+++ b/src/applications/maniphest/query/ManiphestTaskQuery.php
@@ -21,6 +21,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
   private $dateCreatedBefore;
   private $dateModifiedAfter;
   private $dateModifiedBefore;
+  private $subpriorityMin;
+  private $subpriorityMax;
 
   private $fullTextSearch   = '';
 
@@ -140,6 +142,12 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return $this;
   }
 
+  public function withSubpriorityBetween($min, $max) {
+    $this->subpriorityMin = $min;
+    $this->subpriorityMax = $max;
+    return $this;
+  }
+
   public function withSubscribers(array $subscribers) {
     $this->subscriberPHIDs = $subscribers;
     return $this;
@@ -381,6 +389,20 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         $this->subpriorities);
     }
 
+    if ($this->subpriorityMin) {
+      $where[] = qsprintf(
+        $conn,
+        'task.subpriority >= %f',
+        $this->subpriorityMin);
+    }
+
+    if ($this->subpriorityMax) {
+      $where[] = qsprintf(
+        $conn,
+        'task.subpriority <= %f',
+        $this->subpriorityMax);
+    }
+
     $where[] = $this->buildPagingClause($conn);
 
     $where = $this->formatWhereClause($where);