Fix double file content in files in new subdirectories in Subversion

Summary:
Fixes T5555. Normally, when we `svn diff subdir/`, we use `--depth empty` to get only changes for the directory itself (usually, property changes).

However, this flag has no effect if the directory is newly added.

Adjust the diff parser so that if two sets of hunks are specified for a single file in a raw diff, we let the last one win instead of including both. This approach is a broadly more reasonable interpretation of these diffs.

Test Plan:
  - Added a new file in a new subdirectory in Subversion.
  - Ran `arc diff --only`.
  - No double file content in resulting diff.
  - Added unit test.
  - There's fairly comprehensive unit test coverage for this stuff.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5555

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

src/__phutil_library_map__.php

@@ -5,7 +5,6 @@
  * @generated
  * @phutil-library-version 2
  */
-
 phutil_register_library_map(array(
   '__library_version__' => 2,
   'class' =>

src/parser/ArcanistDiffParser.php

@@ -852,6 +852,14 @@ final class ArcanistDiffParser {
   }
 
   protected function parseChangeset(ArcanistDiffChange $change) {
+    // If a diff includes two sets of changes to the same file, let the
+    // second one win. In particular, this occurs when adding subdirectories
+    // in Subversion that contain files: the file text will be present in
+    // both the directory diff and the file diff. See T5555. Dropping the
+    // hunks lets whichever one shows up later win instead of showing changes
+    // twice.
+    $change->dropHunks();
+
     $all_changes = array();
     do {
       $hunk = new ArcanistDiffHunk();

src/parser/__tests__/ArcanistDiffParserTestCase.php

@@ -588,6 +588,13 @@ EOTEXT
           ArcanistDiffChangeType::TYPE_CHANGE,
           $change->getType());
         break;
+      case 'svn-double-diff.svndiff':
+        $this->assertEqual(1, count($changes));
+
+        $change = array_shift($changes);
+        $hunks = $change->getHunks();
+        $this->assertEqual(1, count($hunks));
+        break;
       default:
         throw new Exception("No test block for diff file {$diff_file}.");
         break;

src/parser/__tests__/diff/svn-double-diff.svndiff

@@ -0,0 +1,16 @@
+Index: subdir/newfile
+===================================================================
+--- subdir/newfile	(revision 0)
++++ subdir/newfile	(working copy)
+@@ -0,0 +1,3 @@
++apple
++banana
++cherry
+Index: subdir/newfile
+===================================================================
+--- subdir/newfile	(revision 0)
++++ subdir/newfile	(working copy)
+@@ -0,0 +1,3 @@
++apple
++banana
++cherry

src/parser/diff/ArcanistDiffChange.php

@@ -192,6 +192,11 @@ final class ArcanistDiffChange {
     return $this;
   }
 
+  public function dropHunks() {
+    $this->hunks = array();
+    return $this;
+  }
+
   public function getHunks() {
     return $this->hunks;
   }

src/repository/api/ArcanistSubversionAPI.php

@@ -272,6 +272,8 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI {
   }
 
   public function buildDiffFuture($path) {
+    $root = phutil_get_library_root('arcanist');
+
     // The "--depth empty" flag prevents us from picking up changes in
     // children when we run 'diff' against a directory. Specifically, when a
     // user has added or modified some directory "example/", we want to return
@@ -279,8 +281,9 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI {
     // without "--depth empty", svn will give us changes to the directory
     // itself (such as property changes) and also give us changes to any
     // files within the directory (basically, implicit recursion). We don't
-    // want that, so prevent recursive diffing.
-    $root = phutil_get_library_root('arcanist');
+    // want that, so prevent recursive diffing. This flag does not work if the
+    // directory is newly added (see T5555) so we need to filter the results
+    // out later as well.
 
     if (phutil_is_windows()) {
       // TODO: Provide a binary_safe_diff script for Windows.