Sometimes discard already-closed revisions in "arc land"

Summary:
Ref T13546. When we find commits in history which are associated with already-closed revisions, and they weren't named explicitly on the command line, and we're using a squash strategy, discard them.

This generally happens when "feature2" is on top of "feature1", but "feature1" gets amended or branched elsewhere and lands independently.

Test Plan: Ran "arc land feature3" where prior revisions had already landed, got discards on the duplicated changes.

Maniphest Tasks: T13546

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

src/land/ArcanistLandCommit.php

@@ -7,13 +7,15 @@ final class ArcanistLandCommit
   private $summary;
   private $displaySummary;
   private $parents;
-  private $symbols = array();
   private $explicitRevisionRef;
   private $revisionRef = false;
   private $parentCommits;
   private $isHeadCommit;
   private $isImplicitCommit;
 
+  private $directSymbols = array();
+  private $indirectSymbols = array();
+
   public function setHash($hash) {
     $this->hash = $hash;
     return $this;
@@ -50,13 +52,22 @@ final class ArcanistLandCommit
     return $this->parents;
   }
 
-  public function addSymbol(ArcanistLandSymbol $symbol) {
+  public function addDirectSymbol(ArcanistLandSymbol $symbol) {
-    $this->symbols[] = $symbol;
+    $this->directSymbols[] = $symbol;
     return $this;
   }
 
-  public function getSymbols() {
+  public function getDirectSymbols() {
-    return $this->symbols;
+    return $this->directSymbols;
+  }
+
+  public function addIndirectSymbol(ArcanistLandSymbol $symbol) {
+    $this->indirectSymbols[] = $symbol;
+    return $this;
+  }
+
+  public function getIndirectSymbols() {
+    return $this->indirectSymbols;
   }
 
   public function setExplicitRevisionref(ArcanistRevisionRef $ref) {

src/land/engine/ArcanistGitLandEngine.php

@@ -1299,6 +1299,7 @@ final class ArcanistGitLandEngine
       }
 
       $commits = phutil_split_lines($commits, false);
+      $is_first = false;
       foreach ($commits as $line) {
         if (!strlen($line)) {
           continue;
@@ -1331,7 +1332,12 @@ final class ArcanistGitLandEngine
         }
 
         $commit = $commit_map[$hash];
-        $commit->addSymbol($symbol);
+        if ($is_first) {
+          $commit->addDirectSymbol($symbol);
+          $is_first = false;
+        }
+
+        $commit->addIndirectSymbol($symbol);
       }
     }
 

src/land/engine/ArcanistLandEngine.php

@@ -866,7 +866,7 @@ abstract class ArcanistLandEngine extends Phobject {
       // TODO: If we have several refs but all but one are abandoned or closed
       // or authored by someone else, we could guess what you mean.
 
-      $symbols = $commit->getSymbols();
+      $symbols = $commit->getIndirectSymbols();
       $raw_symbols = mpull($symbols, 'getSymbol');
       $symbol_list = implode(', ', $raw_symbols);
       $display_hash = $this->getDisplayHash($hash);
@@ -899,11 +899,6 @@ abstract class ArcanistLandEngine extends Phobject {
           $display_hash));
     }
 
-    // TODO: Some of the revisions we've identified may be mapped to an
-    // outdated set of commits. We should look in local branches for a better
-    // set of commits, and try to confirm that the state we're about to land
-    // is the current state in Differential.
-
     if ($force_ref) {
       $phid_map = array();
       foreach ($commit_map as $commit) {
@@ -1155,6 +1150,8 @@ abstract class ArcanistLandEngine extends Phobject {
       $sets[$revision_phid] = $set;
     }
 
+    $sets = $this->filterCommitSets($sets);
+
     if (!$this->getShouldPreview()) {
       $this->confirmImplicitCommits($sets, $symbols);
     }
@@ -1436,4 +1433,62 @@ abstract class ArcanistLandEngine extends Phobject {
     return $strategy;
   }
 
+  private function filterCommitSets(array $sets) {
+    assert_instances_of($sets, 'ArcanistLandCommitSet');
+    $log = $this->getLogEngine();
+
+    // If some of the ancestor revisions are already closed, and the user did
+    // not specifically indicate that we should land them, and we are using
+    // a "squash" strategy, discard those sets.
+
+    if ($this->isSquashStrategy()) {
+      $discard = array();
+      foreach ($sets as $key => $set) {
+        $revision_ref = $set->getRevisionRef();
+
+        if (!$revision_ref->isClosed()) {
+          continue;
+        }
+
+        $symbols = null;
+        foreach ($set->getCommits() as $commit) {
+          $commit_symbols = $commit->getDirectSymbols();
+          if ($commit_symbols) {
+            $symbols = $commit_symbols;
+            break;
+          }
+        }
+
+        if ($symbols) {
+          continue;
+        }
+
+        $discard[] = $set;
+        unset($sets[$key]);
+      }
+
+      if ($discard) {
+        echo tsprintf(
+          "\n%!\n%W\n",
+          pht('DISCARDING ANCESTORS'),
+          pht(
+            'Some ancestor commits are associated with revisions that have '.
+            'already been closed. These changes will be skipped:'));
+
+        foreach ($discard as $set) {
+          $this->printCommitSet($set);
+        }
+
+        echo tsprintf("\n");
+      }
+    }
+
+    // TODO: Some of the revisions we've identified may be mapped to an
+    // outdated set of commits. We should look in local branches for a better
+    // set of commits, and try to confirm that the state we're about to land
+    // is the current state in Differential.
+
+    return $sets;
+  }
+
 }