In "arc land", if rebasing a range fails, attempt to "reduce" it

Summary:
Ref T13576. See that task for discussion.

When a user runs `arc land --pick A`, they may be selecting a range of commits ("X..Y") which have ancestors ("V..W") that should NOT land.

We must slice "X..Y" out of history before we can merge it, to avoid landing changes from "V..W".

When "X..Y" is simple and linear, we can rebase the range to pick our desired slice out of history.

When "X..Y" includes merge commits, we frequently can not, and I could not identify any simple alternative. The best alternative I came up with is this "reduce" operation:

  - squash "into" onto Y, producing S, to guarantee there are no natural conflicts;
  - squash S onto X^, producing T, to get rid of the merge commits;
  - rebase T onto "into", producing R, to slice "X..Y" out of history;
  - squash R onto "into", producing Q. (R and Q will be the same, but this simplies the code.)

This feels flimsy and fragile, but I can't immediately find a way to break it. See T13576 for more discussion.

Test Plan:
  - Applied conflicting changes to `example.txt` in `master` and `feature1`.
  - Ran `arc land`, got a merge conflict.
  - Resolved the conflict with `git merge master`.
  - Ran `arc land`.
    - Before: merge conflict.
    - After: `arc land` resolves the merge correctly.
  - Stacked `feature2` on `feature1`, and made various mutations to `feature1` and `feature2`, then ran `arc land --pick feature2`. Changes made in `feature1` should not land, and they mostly do not. See T13576.

Maniphest Tasks: T13576

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

src/land/engine/ArcanistGitLandEngine.php

@@ -300,78 +300,186 @@ final class ArcanistGitLandEngine
         $api->getDisplayHash($max_hash),
         $max_commit->getDisplaySummary()));
 
-    $argv = array();
+    // See T13576. We have several different approaches for performing the
-    $argv[] = '--no-stat';
+    // actual merge.
-    $argv[] = '--no-commit';
+    //
+    // In the simplest case, we're using the "merge" strategy. This means
+    // we always want to merge the entire history, and we can just use a
+    // "git merge" to accomplish our goal. No other approach is permissible
+    // here, so if that doesn't work we're all done and just tell the user
+    // to go resolve conflicts.
+    //
+    // If we're using the "squash" strategy, we may be merging a range of
+    // commits that aren't direct descendants of any ancestor of the state
+    // we're merging into. That is, there may be some ancestors of this
+    // range that we do NOT want to merge. A simple way to get into this
+    // state is to use "--pick". We need to slice off only the commits we
+    // want to merge to ensure we don't bring anything extra along.
+    //
+    // If history is simple and linear, we can select this slice with
+    // "git rebase". However, if history includes merge commits, it seems
+    // as though there are many cases where a (non-interactive) rebase is
+    // doomed to failure.
+    //
+    // If a "git rebase" fails, try to "reduce" the slice first, by using
+    // a "git merge --squash" to collapse the whole slice on top of its
+    // parent. This produces a single non-merge commit with all the changes,
+    // which we can then rebase and merge.
 
-    // When we're merging into the empty state, Git refuses to perform the
+    $try = array();
-    // merge until we tell it explicitly that we're doing something unusual.
-    if ($is_empty) {
-      $argv[] = '--allow-unrelated-histories';
-    }
-
-    if ($this->isSquashStrategy()) {
-      // NOTE: We're explicitly specifying "--ff" to override the presence
-      // of "merge.ff" options in user configuration.
-      $argv[] = '--ff';
-      $argv[] = '--squash';
-    } else {
-      $argv[] = '--no-ff';
-    }
-
-    $argv[] = '--';
-
-    $is_rebasing = false;
-    $is_merging = false;
-    try {
     if ($this->isSquashStrategy() && !$is_empty) {
-        // If we're performing a squash merge, we're going to rebase the
+      $try[] = 'rebase-merge';
-        // commit range first. We only want to merge the specific commits
+      $try[] = 'reduce-rebase-merge';
-        // in the range, and merging too much can create conflicts.
-
-        $api->execxLocal('checkout %s --', $max_hash);
-
-        $is_rebasing = true;
-        $api->execxLocal(
-          'rebase --onto %s -- %s',
-          $into_commit,
-          $min_hash.'^');
-        $is_rebasing = false;
-
-        $merge_hash = $api->getCanonicalRevisionName('HEAD');
     } else {
-        $merge_hash = $max_hash;
+      $try[] = 'merge';
     }
 
-      $api->execxLocal('checkout %s --', $into_commit);
+    $merge_exceptions = array();
+    $merge_complete = false;
+    foreach ($try as $approach) {
+      $reduce_min = null;
+      $reduce_max = null;
 
-      $argv[] = $merge_hash;
+      $rebase_min = null;
+      $rebase_max = null;
 
-      $is_merging = true;
+      $merge_hash = null;
-      $api->execxLocal('merge %Ls', $argv);
+      $force_resolve = false;
-      $is_merging = false;
+
+      switch ($approach) {
+        case 'reduce-rebase-merge':
+          $reduce_min = $min_hash;
+          $reduce_max = $max_hash;
+
+          $log->writeStatus(
+            pht('MERGE'),
+            pht('Attempting to reduce and rebase changes.'));
+          break;
+        case 'rebase-merge':
+          $rebase_min = $min_hash;
+          $rebase_max = $max_hash;
+
+          $log->writeStatus(
+            pht('MERGE'),
+            pht('Attempting to rebase changes.'));
+          break;
+        case 'merge':
+          $merge_hash = $max_hash;
+
+          $log->writeStatus(
+            pht('MERGE'),
+            pht('Attempting to merge changes.'));
+          break;
+        default:
+          throw new Exception(
+            pht(
+              'Unknown merge approach "%s".',
+              $approach));
+      }
+
+      try {
+        if ($reduce_max) {
+          $reduce_dst = $reduce_min.'^';
+
+          // Squash the "into" state on top of the range. The goal is to
+          // guarantee that there are no unresolved conflicts between the
+          // maximum commit and the "into" state, because we're going to
+          // force conflicts to resolve in our favor later.
+
+          $this->applyMergeOperation(
+            $into_commit,
+            $reduce_max,
+            true,
+            $is_empty);
+
+          $join_hash = $this->applyCommitOperation(
+            sprintf(
+              'arc land: join (%s -> %s)',
+              $api->getDisplayHash($into_commit),
+              $api->getDisplayHash($reduce_max)),
+            null,
+            null,
+            $allow_empty = true);
+
+          // Squash the range, including the new merge, into a single
+          // commit. The goal here is to produce a new range with no merge
+          // commits so we can rebase it (we'll produce a sequence exactly
+          // one commit long).
+
+          $this->applyMergeOperation(
+            $join_hash,
+            $reduce_dst,
+            true,
+            $is_empty);
+
+          $reduce_hash = $this->applyCommitOperation(
+            sprintf(
+              'arc land: reduce (%s..%s -> %s)',
+              $api->getDisplayHash($reduce_min),
+              $api->getDisplayHash($reduce_max),
+              $reduce_dst));
+
+          // We've reduced the range into a new range that is one commit
+          // long, has no merge commits, and has no conflicts against the
+          // "into" state.
+
+          // We'll rebase it and force conflicts to resolve in favor of the
+          // reduced state. The hope is that we've taken sufficient steps to
+          // guarantee this resolution is always reasonable.
+
+          $rebase_min = $reduce_hash;
+          $rebase_max = $reduce_hash;
+          $force_resolve = true;
+        }
+
+        if ($rebase_max) {
+          $merge_hash = $this->applyRebaseOperation(
+            $rebase_min,
+            $rebase_max,
+            $into_commit,
+            $force_resolve);
+        }
+
+        $this->applyMergeOperation(
+          $merge_hash,
+          $into_commit,
+          $this->isSquashStrategy(),
+          $is_empty);
+
+        $log->writeStatus(pht('DONE'), pht('Merge succeeded.'));
+
+        $merge_complete = true;
       } catch (CommandException $ex) {
+        $merge_exceptions[] = $ex;
+      }
+
+      if ($merge_complete) {
+        break;
+      }
+    }
+
+    if (!$merge_complete) {
       $direct_symbols = $max_commit->getDirectSymbols();
       $indirect_symbols = $max_commit->getIndirectSymbols();
       if ($direct_symbols) {
         $message = pht(
           'Local commit "%s" (%s) does not merge cleanly into "%s". '.
-          'Merge or rebase local changes so they can merge cleanly.',
+          'Rebase or merge local changes so they can merge cleanly.',
           $api->getDisplayHash($max_hash),
           $this->getDisplaySymbols($direct_symbols),
           $api->getDisplayHash($into_commit));
       } else if ($indirect_symbols) {
         $message = pht(
           'Local commit "%s" (reachable from: %s) does not merge cleanly '.
-          'into "%s". Merge or rebase local changes so they can merge '.
+          'into "%s". Rebase or merge local changes so they can merge '.
           'cleanly.',
           $api->getDisplayHash($max_hash),
           $this->getDisplaySymbols($indirect_symbols),
           $api->getDisplayHash($into_commit));
       } else {
         $message = pht(
-          'Local commit "%s" does not merge cleanly into "%s". Merge or '.
+          'Local commit "%s" does not merge cleanly into "%s". Rebase or '.
-          'rebase local changes so they can merge cleanly.',
+          'merge local changes so they can merge cleanly.',
           $api->getDisplayHash($max_hash),
           $api->getDisplayHash($into_commit));
       }
@@ -388,18 +496,6 @@ final class ArcanistGitLandEngine
             'Use "--incremental" to merge and push changes one by one.'));
       }
 
-      if ($is_rebasing) {
-        $api->execManualLocal('rebase --abort');
-      }
-
-      if ($is_merging) {
-        $api->execManualLocal('merge --abort');
-      }
-
-      if ($is_merging || $is_rebasing) {
-        $api->execManualLocal('reset --hard HEAD --');
-      }
-
       throw new PhutilArgumentUsageException(
         pht('Encountered a merge conflict.'));
     }
@@ -410,15 +506,10 @@ final class ArcanistGitLandEngine
     $revision_ref = $set->getRevisionRef();
     $commit_message = $revision_ref->getCommitMessage();
 
-    $future = $api->execFutureLocal(
+    $new_cursor = $this->applyCommitOperation(
-      'commit --author %s --date %s -F - --',
+      $commit_message,
       $original_author,
       $original_date);
-    $future->write($commit_message);
-    $future->resolvex();
-
-    list($stdout) = $api->execxLocal('rev-parse --verify %s', 'HEAD');
-    $new_cursor = trim($stdout);
 
     if ($is_empty) {
       // See T12876. If we're landing into the empty state, we just did a fake
@@ -1601,4 +1692,114 @@ final class ArcanistGitLandEngine
         'Desired merge strategy is ambiguous, choose an explicit strategy.'));
   }
 
+  private function applyRebaseOperation(
+    $src_min,
+    $src_max,
+    $dst,
+    $force_resolve) {
+
+    $api = $this->getRepositoryAPI();
+
+    $api->execxLocal('checkout %s --', $src_max);
+
+    $argv = array();
+    $argv[] = '--onto';
+    $argv[] = gitsprintf('%s', $dst);
+
+    if ($force_resolve) {
+      $argv[] = '--strategy';
+      $argv[] = 'recursive';
+      $argv[] = '--strategy-option';
+      $argv[] = 'theirs';
+    }
+
+    $argv[] = '--';
+    $argv[] = gitsprintf('%s', $src_min.'^');
+
+    try {
+      $api->execxLocal('rebase %Ls', $argv);
+    } catch (CommandException $ex) {
+      $api->execManualLocal('rebase --abort');
+      $api->execManualLocal('reset --hard HEAD --');
+      throw $ex;
+    }
+
+    $merge_hash = $api->getCanonicalRevisionName('HEAD');
+
+    return $merge_hash;
+  }
+
+  private function applyMergeOperation($src, $dst, $is_squash, $is_empty) {
+    $api = $this->getRepositoryAPI();
+
+    $api->execxLocal('checkout %s --', $dst);
+
+    $argv = array();
+    $argv[] = '--no-stat';
+    $argv[] = '--no-commit';
+
+    // When we're merging into the empty state, Git refuses to perform the
+    // merge until we tell it explicitly that we're doing something unusual.
+    if ($is_empty) {
+      $argv[] = '--allow-unrelated-histories';
+    }
+
+    if ($is_squash) {
+      // NOTE: We're explicitly specifying "--ff" to override the presence
+      // of "merge.ff" options in user configuration.
+      $argv[] = '--ff';
+      $argv[] = '--squash';
+    } else {
+      $argv[] = '--no-ff';
+    }
+
+    $argv[] = '--';
+
+    $argv[] = $src;
+
+    try {
+      $api->execxLocal('merge %Ls', $argv);
+    } catch (CommandException $ex) {
+      $api->execManualLocal('merge --abort');
+      $api->execManualLocal('reset --hard HEAD');
+      throw $ex;
+    }
+  }
+
+  private function applyCommitOperation(
+    $message,
+    $author = null,
+    $date = null,
+    $allow_empty = false) {
+
+    $api = $this->getRepositoryAPI();
+
+    $argv = array();
+    if ($author !== null) {
+      $argv[] = '--author';
+      $argv[] = $author;
+    }
+
+    if ($date !== null) {
+      $argv[] = '--date';
+      $argv[] = $date;
+    }
+
+    if ($allow_empty) {
+      $argv[] = '--allow-empty';
+    }
+
+    $future = $api->execFutureLocal(
+      'commit %Ls -F - --',
+      $argv);
+    $future->write($message);
+    $future->resolvex();
+
+    list($stdout) = $api->execxLocal('rev-parse --verify %s', 'HEAD');
+    $new_commit = trim($stdout);
+
+    return $new_commit;
+  }
+
+
 }