Improve "thread" rendering of unusually-shaped graphs

Summary:
Ref T4788. This fixes all the bugs I was immediately able to catch:

  - "Directory-Like" graph shapes could draw too many vertical lines.
  - "Reverse-Directory-Like" graph shapes could draw too few vertical lines.
  - Terminated, branched graph shapes drew the very last line to the wrong place.

This covers the behavior with tests, so we should be able to fix more stuff later without breaking anything.

Test Plan:
  - Added failing tests and made them pass.

{F1708158}

{F1708159}

{F1708160}

{F1708161}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788

Differential Revision: https://secure.phabricator.com/D16216

Author: epriestley

resources/celerity/map.php

@@ -13,7 +13,7 @@
     'differential.pkg.css' => '3e81ae60',
     'differential.pkg.js' => '634399e9',
     'diffusion.pkg.css' => '91c5d3a6',
-    'diffusion.pkg.js' => '3a9a8bfa',
+    'diffusion.pkg.js' => '84c8f8fd',
     'maniphest.pkg.css' => '4845691a',
     'maniphest.pkg.js' => '949a7498',
     'rsrc/css/aphront/aphront-bars.css' => '231ac33c',
@@ -394,7 +394,7 @@
     'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => 'b42eddc7',
     'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'd835b03a',
     'rsrc/js/application/diffusion/behavior-commit-branches.js' => 'bdaf4d04',
-    'rsrc/js/application/diffusion/behavior-commit-graph.js' => '5a0b1a64',
+    'rsrc/js/application/diffusion/behavior-commit-graph.js' => '49ae8328',
     'rsrc/js/application/diffusion/behavior-diffusion-browse-file.js' => '054a0f0b',
     'rsrc/js/application/diffusion/behavior-jump-to.js' => '73d09eef',
     'rsrc/js/application/diffusion/behavior-load-blame.js' => '42126667',
@@ -619,7 +619,7 @@
     'javelin-behavior-differential-user-select' => 'a8d8459d',
     'javelin-behavior-diffusion-browse-file' => '054a0f0b',
     'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04',
-    'javelin-behavior-diffusion-commit-graph' => '5a0b1a64',
+    'javelin-behavior-diffusion-commit-graph' => '49ae8328',
     'javelin-behavior-diffusion-jump-to' => '73d09eef',
     'javelin-behavior-diffusion-locate-file' => '6d3e1947',
     'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc',
@@ -1217,6 +1217,11 @@
       'javelin-uri',
       'phabricator-notification',
     ),
+    '49ae8328' => array(
+      'javelin-behavior',
+      'javelin-dom',
+      'javelin-stratcom',
+    ),
     '4b700e9e' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1343,11 +1348,6 @@
       'javelin-vector',
       'javelin-dom',
     ),
-    '5a0b1a64' => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-stratcom',
-    ),
     '5a13c79f' => array(
       'javelin-install',
       'javelin-dom',

src/__phutil_library_map__.php

@@ -1602,6 +1602,7 @@
     'PHUICurtainPanelView' => 'view/layout/PHUICurtainPanelView.php',
     'PHUICurtainView' => 'view/layout/PHUICurtainView.php',
     'PHUIDiffGraphView' => 'infrastructure/diff/view/PHUIDiffGraphView.php',
+    'PHUIDiffGraphViewTestCase' => 'infrastructure/diff/view/__tests__/PHUIDiffGraphViewTestCase.php',
     'PHUIDiffInlineCommentDetailView' => 'infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php',
     'PHUIDiffInlineCommentEditView' => 'infrastructure/diff/view/PHUIDiffInlineCommentEditView.php',
     'PHUIDiffInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php',
@@ -6139,6 +6140,7 @@
     'PHUICurtainPanelView' => 'AphrontTagView',
     'PHUICurtainView' => 'AphrontTagView',
     'PHUIDiffGraphView' => 'Phobject',
+    'PHUIDiffGraphViewTestCase' => 'PhabricatorTestCase',
     'PHUIDiffInlineCommentDetailView' => 'PHUIDiffInlineCommentView',
     'PHUIDiffInlineCommentEditView' => 'PHUIDiffInlineCommentView',
     'PHUIDiffInlineCommentRowScaffold' => 'AphrontView',

src/infrastructure/diff/view/PHUIDiffGraphView.php

@@ -23,7 +23,7 @@ public function getIsTail() {
     return $this->isTail;
   }
 
-  public function renderGraph(array $parents) {
+  public function renderRawGraph(array $parents) {
     // This keeps our accumulated information about each line of the
     // merge/branch graph.
     $graph = array();
@@ -47,7 +47,10 @@ public function renderGraph(array $parents) {
       $line = '';
       $found = false;
       $pos = count($threads);
-      for ($n = 0; $n < $count; $n++) {
+
+      $thread_count = $pos;
+      for ($n = 0; $n < $thread_count; $n++) {
+
         if (empty($threads[$n])) {
           $line .= ' ';
           continue;
@@ -147,16 +150,30 @@ public function renderGraph(array $parents) {
         $line = $graph[$key]['line'];
         $len = strlen($line);
         for ($ii = 0; $ii < $len; $ii++) {
-          if (isset($terminated[$ii])) {
-            continue;
-          }
-
           $c = $line[$ii];
           if ($c == 'o') {
+            // If we've already terminated this thread, we don't need to add
+            // a terminator.
+            if (isset($terminated[$ii])) {
+              continue;
+            }
+
             $terminated[$ii] = true;
+
+            // If this thread is joinining some other node here, we don't want
+            // to terminate it.
+            if (isset($graph[$key + 1])) {
+              $joins = $graph[$key + 1]['join'];
+              if (in_array($ii, $joins)) {
+                continue;
+              }
+            }
+
             $graph[$key]['line'][$ii] = 'x';
           } else if ($c != ' ') {
             $terminated[$ii] = true;
+          } else {
+            unset($terminated[$ii]);
           }
         }
       }
@@ -166,6 +183,12 @@ public function renderGraph(array $parents) {
       $graph[] = $last;
     }
 
+    return array($graph, $count);
+  }
+
+  public function renderGraph(array $parents) {
+    list($graph, $count) = $this->renderRawGraph($parents);
+
     // Render into tags for the behavior.
 
     foreach ($graph as $k => $meta) {

src/infrastructure/diff/view/__tests__/PHUIDiffGraphViewTestCase.php

@@ -0,0 +1,94 @@
+<?php
+
+final class PHUIDiffGraphViewTestCase extends PhabricatorTestCase {
+
+  public function testTailTermination() {
+    $nodes = array(
+      'A' => array('B'),
+      'B' => array('C', 'D', 'E'),
+      'E' => array(),
+      'D' => array(),
+      'C' => array('F', 'G'),
+      'G' => array(),
+      'F' => array(),
+    );
+
+    $graph = $this->newGraph($nodes);
+
+    $picture = array(
+      '^',
+      'o',
+      '||x',
+      '|x ',
+      'o  ',
+      '|x ',
+      'x  ',
+    );
+
+    $this->assertGraph($picture, $graph, pht('Terminating Tree'));
+  }
+
+  public function testReverseTree() {
+    $nodes = array(
+      'A' => array('B'),
+      'C' => array('B'),
+      'B' => array('D'),
+      'E' => array('D'),
+      'F' => array('D'),
+      'D' => array('G'),
+      'G' => array(),
+    );
+
+    $graph = $this->newGraph($nodes);
+
+    $picture = array(
+      '^',
+      '|^',
+      'o ',
+      '|^',
+      '||^',
+      'o  ',
+      'x',
+    );
+
+    $this->assertGraph($picture, $graph, pht('Reverse Tree'));
+  }
+
+  public function testJoinTerminateTree() {
+    $nodes = array(
+      'A' => array('D'),
+      'B' => array('C'),
+      'C' => array('D'),
+      'D' => array(),
+    );
+
+    $graph = $this->newGraph($nodes);
+
+    $picture = array(
+      '^',
+      '|^',
+      '|o',
+      'x ',
+    );
+
+    $this->assertGraph($picture, $graph, pht('Reverse Tree'));
+  }
+
+  private function newGraph(array $nodes) {
+    return id(new PHUIDiffGraphView())
+      ->setIsHead(true)
+      ->setIsTail(true)
+      ->renderRawGraph($nodes);
+  }
+
+  private function assertGraph($picture, $graph, $label) {
+    list($data, $count) = $graph;
+    $lines = ipull($data, 'line');
+
+    $picture = implode("\n", $picture);
+    $lines = implode("\n", $lines);
+
+    $this->assertEqual($picture, $lines, $label);
+  }
+
+}

webroot/rsrc/js/application/diffusion/behavior-commit-graph.js

@@ -79,6 +79,7 @@ JX.behavior('diffusion-commit-graph', function(config) {
       c = data.line.charAt(jj);
       switch (c) {
         case 'o':
+        case 'x':
         case '^':
           origin = xpos(jj);
           break;
@@ -91,6 +92,7 @@ JX.behavior('diffusion-commit-graph', function(config) {
     for (jj = 0; jj < data.join.length; jj++) {
       var join = data.join[jj];
       x = xpos(join);
+
       cxt.beginPath();
         cxt.moveTo(x, 0);
         cxt.bezierCurveTo(x, h/4, origin, h/4, origin, h/2);