From 53f8ad14fa929077658445bf4cd82f01f267f636 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 20 Jun 2019 11:10:52 -0700
Subject: [PATCH] Fix an issue in Owners where a transaction change could show
 too many effects

Summary:
Fixes T13324. Ref PHI1288. Currently, if you edit an Owners package that has some paths with no trailing slashes (like `README.md`) so their internal names and display names differ (`/README.md` display, vs `/README.md/` internal), the "Show Details" in the transaction log shows the path as re-normalized even if you didn't touch it.

Instead, be more careful about handling display paths vs internal paths.

(This code on the whole is significantly less clear than it probably could be, but this issue is so minor that I'm hesitant to start ripping things out.)

Test Plan:
  - In a package with some paths like `/src/` and some paths like `/src`:
  - Added new paths.
  - Removed paths.
  - Changed paths from `/src/` to `/src`.
  - Changed paths from `/src` to `/src/`.

In all cases, the "paths" list and the transaction record identically reflected the edit in the way I expected them to.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13324

Differential Revision: https://secure.phabricator.com/D20596
---
 .../owners/storage/PhabricatorOwnersPath.php  | 16 +++++++--
 ...abricatorOwnersPackagePathsTransaction.php | 36 ++++++++++++++++---
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index 7022cb887c..5bb32f8407 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -79,15 +79,27 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
   public static function getSetFromTransactionValue(array $v) {
     $set = array();
     foreach ($v as $ref) {
-      $set[$ref['repositoryPHID']][$ref['path']][$ref['excluded']] = true;
+      $key = self::getScalarKeyForRef($ref);
+      $set[$key] = true;
     }
     return $set;
   }
 
   public static function isRefInSet(array $ref, array $set) {
-    return isset($set[$ref['repositoryPHID']][$ref['path']][$ref['excluded']]);
+    $key = self::getScalarKeyForRef($ref);
+    return isset($set[$key]);
   }
 
+  private static function getScalarKeyForRef(array $ref) {
+    return sprintf(
+      'repository=%s path=%s display=%s excluded=%d',
+      $ref['repositoryPHID'],
+      $ref['path'],
+      $ref['display'],
+      $ref['excluded']);
+  }
+
+
   /**
    * Get the number of directory matches between this path specification and
    * some real path.
diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
index 753b6ff9e9..a8bb1cc259 100644
--- a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
@@ -12,12 +12,33 @@ final class PhabricatorOwnersPackagePathsTransaction
 
   public function generateNewValue($object, $value) {
     $new = $value;
+
     foreach ($new as $key => $info) {
-      $new[$key]['excluded'] = (int)idx($info, 'excluded');
+      $info['excluded'] = (int)idx($info, 'excluded');
+
+      // The input has one "path" key with the display path.
+      // Move it to "display", then normalize the value in "path".
+
+      $display_path = $info['path'];
+      $raw_path = rtrim($display_path, '/').'/';
+
+      $info['path'] = $raw_path;
+      $info['display'] = $display_path;
+
+      $new[$key] = $info;
     }
+
     return $new;
   }
 
+  public function getTransactionHasEffect($object, $old, $new) {
+    list($add, $rem) = PhabricatorOwnersPath::getTransactionValueChanges(
+      $old,
+      $new);
+
+    return ($add || $rem);
+  }
+
   public function validateTransactions($object, array $xactions) {
     $errors = array();
 
@@ -110,8 +131,8 @@ final class PhabricatorOwnersPackagePathsTransaction
     $display_map = array();
     $seen_map = array();
     foreach ($new as $key => $spec) {
-      $display_path = $spec['path'];
-      $raw_path = rtrim($display_path, '/').'/';
+      $raw_path = $spec['path'];
+      $display_path = $spec['display'];
 
       // If the user entered two paths in the same repository which normalize
       // to the same value (like "src/main.c" and "src/main.c/"), discard the
@@ -193,11 +214,18 @@ final class PhabricatorOwnersPackagePathsTransaction
     $rowc = array();
     foreach ($rows as $key => $row) {
       $rowc[] = $row['class'];
+
+      if (array_key_exists('display', $row)) {
+        $display_path = $row['display'];
+      } else {
+        $display_path = $row['path'];
+      }
+
       $rows[$key] = array(
         $row['change'],
         $row['excluded'] ? pht('Exclude') : pht('Include'),
         $this->renderHandle($row['repositoryPHID']),
-        $row['path'],
+        $display_path,
       );
     }