From 1b8ed98ddcc9537b1e7d38cc1687b742acc4a0d6 Mon Sep 17 00:00:00 2001
From: Miles Shang <mshang@fb.com>
Date: Wed, 4 Jul 2012 14:04:45 -0700
Subject: [PATCH] Multi-line highlighting in Diffusion.

Summary: Currently, Diffusion supports highlighting of line ranges passed in the URI. It would be helpful to be able to highlight multiple line ranges.

Test Plan: Accessed directly via URL in my sandbox. Seems to work. I'm not sure what other components use this functionality, but this change should be backwards compatible.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D2921
---
 .../DiffusionBrowseFileController.php         | 36 ++++++++++++-------
 .../diffusion/request/DiffusionRequest.php    |  2 +-
 .../__tests__/DiffusionURITestCase.php        | 13 +++++++
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php
index 6207cb9e09..e6822d8667 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php
@@ -293,15 +293,22 @@ final class DiffusionBrowseFileController extends DiffusionController {
       $epoch_range = ($epoch_max - $epoch_min) + 1;
     }
 
-    $min_line = 0;
-    $line = $drequest->getLine();
-    if (strpos($line, '-') !== false) {
-      list($min, $max) = explode('-', $line, 2);
-      $min_line = min($min, $max);
-      $max_line = max($min, $max);
-    } else if (strlen($line)) {
-      $min_line = $line;
-      $max_line = $line;
+    $line_arr = array();
+    $line_str = $drequest->getLine();
+    $ranges = explode(',', $line_str);
+    foreach ($ranges as $range) {
+      if (strpos($range, '-') !== false) {
+        list($min, $max) = explode('-', $range, 2);
+        $line_arr[] = array(
+          'min' => min($min, $max),
+          'max' => max($min, $max),
+        );
+      } else if (strlen($range)) {
+        $line_arr[] = array(
+          'min' => $range,
+          'max' => $range,
+        );
+      }
     }
 
     $display = array();
@@ -366,12 +373,15 @@ final class DiffusionBrowseFileController extends DiffusionController {
         }
       }
 
-      if ($min_line) {
-        if ($line_number == $min_line) {
+      if ($line_arr) {
+        if ($line_number == $line_arr[0]['min']) {
           $display_line['target'] = true;
         }
-        if ($line_number >= $min_line && $line_number <= $max_line) {
-          $display_line['highlighted'] = true;
+        foreach ($line_arr as $range) {
+          if ($line_number >= $range['min'] &&
+              $line_number <= $range['max']) {
+            $display_line['highlighted'] = true;
+          }
         }
       }
 
diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php
index e179c38d12..5a02a88fb8 100644
--- a/src/applications/diffusion/request/DiffusionRequest.php
+++ b/src/applications/diffusion/request/DiffusionRequest.php
@@ -475,7 +475,7 @@ abstract class DiffusionRequest {
     // Consume the back part of the URI, up to the first "$". Use a negative
     // lookbehind to prevent matching '$$'. We double the '$' symbol when
     // encoding so that files with names like "money/$100" will survive.
-    $pattern = '@(?:(?:^|[^$])(?:[$][$])*)[$]([\d-]+)$@';
+    $pattern = '@(?:(?:^|[^$])(?:[$][$])*)[$]([\d-,]+)$@';
     if (preg_match($pattern, $blob, $matches)) {
       $result['line'] = $matches[1];
       $blob = substr($blob, 0, -(strlen($matches[1]) + 1));
diff --git a/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php b/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php
index 84e18a3d33..3a4e3cce32 100644
--- a/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php
+++ b/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php
@@ -55,6 +55,12 @@ final class DiffusionURITestCase extends ArcanistPhutilTestCase {
         'commit' => '$;;semicolon;;$$',
         'line'   => '100',
       ),
+      'branch/path.ext;abc$3-5,7-12,14' => array(
+        'branch'  => 'branch',
+        'path'    => 'path.ext',
+        'commit'  => 'abc',
+        'line'    => '3-5,7-12,14',
+      ),
     );
 
     foreach ($map as $input => $expect) {
@@ -140,6 +146,13 @@ final class DiffusionURITestCase extends ArcanistPhutilTestCase {
         'path'      => 'path/to/file.ext',
         'commit'    => 'abc',
       ),
+      '/diffusion/A/browse/branch/path.ext$3-5%2C7-12%2C14' => array(
+        'action'    => 'browse',
+        'callsign'  => 'A',
+        'branch'    => 'branch',
+        'path'      => 'path.ext',
+        'line'      => '3-5,7-12,14',
+      ),
     );
 
     foreach ($map as $expect => $input) {