From aa5c023fe8fa4b7a770f1e570870b96cf01a70bb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 28 Oct 2015 08:28:46 -0700
Subject: [PATCH] Make rules for guessing onto/remote more powerful and more
 explicit in `arc land`

Summary:
Fixes T9543. Fixes T9658. Ref T3855.

Major functional change is that you can have a sequence of branches like:

  origin/master -> notmaster -> feature1

...where they track each other, but you named your local master something else. Currently, we resolve only one level of upstreams, so we try to land onto "notmaster" in this case, which is wrong.

Instead, keep resolving upstreams until we either hit a cycle, don't have another upstream to look at, or find someting in a remote. In this case we'll eventually find "origin/master" and select "origin" as the remote and "master" as the target.

Other minor changes:

  - Make this selection process explicit.
  - Make the help 3000x longer.

Also fix a bug where we could incorrectly try to tell Differential to update awith `--preview`.

Test Plan:
  - Landed from a tag.
  - Landed from a tracking branch.
  - Landed from an nth-degree tracking branch.
  - Tried to land from a local branch with a cycle in upstreams.
  - Landed with --remote and --onto.
  - Read `arc help land`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9658, T3855, T9543

Differential Revision: https://secure.phabricator.com/D14357
---
 src/land/ArcanistGitLandEngine.php    |   2 +
 src/repository/api/ArcanistGitAPI.php |  67 ++++++++
 src/workflow/ArcanistLandWorkflow.php | 225 ++++++++++++++++++++++++--
 3 files changed, 278 insertions(+), 16 deletions(-)

diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php
index 9fc08292..1a55e14a 100644
--- a/src/land/ArcanistGitLandEngine.php
+++ b/src/land/ArcanistGitLandEngine.php
@@ -167,6 +167,8 @@ final class ArcanistGitLandEngine
       $original_date,
       $this->getCommitMessageFile());
 
+    $this->getWorkflow()->didCommitMerge();
+
     list($stdout) = $api->execxLocal(
       'rev-parse --verify %s',
       'HEAD');
diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index 84ac017f..74f97bc8 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -1333,4 +1333,71 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     $this->resolvedHeadCommit = null;
   }
 
+  /**
+   * Follow the chain of tracking branches upstream until we reach a remote
+   * or cycle locally.
+   *
+   * @param string Ref to start from.
+   * @return list<wild> Path to an upstream.
+   */
+  public function getPathToUpstream($start) {
+    $cursor = $start;
+    $path = array();
+    while (true) {
+      list($err, $upstream) = $this->execManualLocal(
+        'rev-parse --symbolic-full-name %s@{upstream}',
+        $cursor);
+
+      if ($err) {
+        // We ended up somewhere with no tracking branch, so we're done.
+        break;
+      }
+
+      $upstream = trim($upstream);
+
+      if (preg_match('(^refs/heads/)', $upstream)) {
+        $upstream = preg_replace('(^refs/heads/)', '', $upstream);
+
+        $is_cycle = isset($path[$upstream]);
+
+        $path[$cursor] = array(
+          'type' => 'local',
+          'name' => $upstream,
+          'cycle' => $is_cycle,
+        );
+
+        if ($is_cycle) {
+          // We ran into a local cycle, so we're done.
+          break;
+        }
+
+        // We found another local branch, so follow that one upriver.
+        $cursor = $upstream;
+        continue;
+      }
+
+      if (preg_match('(^refs/remotes/)', $upstream)) {
+        $upstream = preg_replace('(^refs/remotes/)', '', $upstream);
+        list($remote, $branch) = explode('/', $upstream, 2);
+
+        $path[$cursor] = array(
+          'type' => 'remote',
+          'name' => $branch,
+          'remote' => $remote,
+        );
+
+        // We found a remote, so we're done.
+        break;
+      }
+
+      throw new Exception(
+        pht(
+          'Got unrecognized upstream format ("%s") from Git, expected '.
+          '"refs/heads/..." or "refs/remotes/...".',
+          $upstream));
+    }
+
+    return $path;
+  }
+
 }
diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php
index 77db9602..38874331 100644
--- a/src/workflow/ArcanistLandWorkflow.php
+++ b/src/workflow/ArcanistLandWorkflow.php
@@ -38,7 +38,7 @@ final class ArcanistLandWorkflow extends ArcanistWorkflow {
 
   public function getCommandSynopses() {
     return phutil_console_format(<<<EOTEXT
-      **land** [__options__] [__branch__] [--onto __master__]
+      **land** [__options__] [__ref__]
 EOTEXT
       );
   }
@@ -47,18 +47,71 @@ EOTEXT
     return phutil_console_format(<<<EOTEXT
           Supports: git, hg
 
-          Land an accepted change (currently sitting in local feature branch
-          __branch__) onto __master__ and push it to the remote. Then, delete
-          the feature branch. If you omit __branch__, the current branch will
-          be used.
+          Publish an accepted revision after review. This command is the last
+          step in the standard Differential pre-publish code review workflow.
 
-          In mutable repositories, this will perform a --squash merge (the
-          entire branch will be represented by one commit on __master__). In
-          immutable repositories (or when --merge is provided), it will perform
-          a --no-ff merge (the branch will always be merged into __master__ with
-          a merge commit).
+          This command merges and pushes changes associated with an accepted
+          revision that are currently sitting in __ref__, which is usually the
+          name of a local branch. Without __ref__, the current working copy
+          state will be used.
+
+          Under Git: branches, tags, and arbitrary commits (detached HEADs)
+          may be landed.
+
+          Under Mercurial: branches and bookmarks may be landed, but only
+          onto a target of the same type. See T3855.
+
+          The workflow selects a target branch to land onto and a remote where
+          the change will be pushed to.
+
+          A target branch is selected by examining these sources in order:
+
+            - the **--onto** flag;
+            - the upstream of the current branch, recursively (Git only);
+            - the __arc.land.onto.default__ configuration setting;
+            - or by falling back to a standard default:
+              - "master" in Git;
+              - "default" in Mercurial.
+
+          A remote is selected by examining these sources in order:
+
+            - the **--remote** flag;
+            - the upstream of the current branch, recursively (Git only);
+            - or by falling back to a standard default:
+              - "origin" in Git;
+              - the default remote in Mercurial.
+
+          After selecting a target branch and a remote, the commits which will
+          be landed are printed.
+
+          With **--preview**, execution stops here, before the change is
+          merged.
+
+          The change is merged into the target branch, following these rules:
+
+          In mutable repositories or with **--squash**, this will perform a
+          squash merge (the entire branch will be represented as one commit on
+          the target branch).
+
+          In immutable repositories or with **--merge**, this will perform a
+          strict merge (a merge commit will always be created, and local
+          commits will be preserved).
+
+          The resulting commit will be given an up-to-date commit message
+          describing the final state of the revision in Differential.
+
+          With **--hold**, execution stops here, before the change is pushed.
+
+          The change is pushed into the remote.
+
+          Consulting mystical sources of power, the workflow makes a guess
+          about what state you wanted to end up in after the process finishes
+          and the working copy is put into that state.
+
+          The branch which was landed is deleted, unless the **--keep-branch**
+          flag was passed or the landing branch is the same as the target
+          branch.
 
-          Under hg, bookmarks can be landed the same way as branches.
 EOTEXT
       );
   }
@@ -203,6 +256,8 @@ EOTEXT
     }
 
     if ($engine) {
+      $this->readEngineArguments();
+
       $obsolete = array(
         'delete-remote',
         'update-with-merge',
@@ -236,7 +291,7 @@ EOTEXT
 
       $engine->execute();
 
-      if (!$should_hold) {
+      if (!$should_hold && !$this->preview) {
         $this->didPush();
       }
 
@@ -303,6 +358,137 @@ EOTEXT
     return null;
   }
 
+  private function readEngineArguments() {
+    // NOTE: This is hard-coded for Git right now.
+    // TODO: Clean this up and move it into LandEngines.
+
+    $onto = $this->getEngineOnto();
+    $remote = $this->getEngineRemote();
+
+    // This just overwrites work we did earlier, but it has to be up in this
+    // class for now because other parts of the workflow still depend on it.
+    $this->onto = $onto;
+    $this->remote = $remote;
+    $this->ontoRemoteBranch = $this->remote.'/'.$onto;
+  }
+
+  private function getEngineOnto() {
+    $onto = $this->getArgument('onto');
+    if ($onto !== null) {
+      $this->writeInfo(
+        pht('TARGET'),
+        pht(
+          'Landing onto "%s", selected by the --onto flag.',
+          $onto));
+      return $onto;
+    }
+
+    $api = $this->getRepositoryAPI();
+    $path = $api->getPathToUpstream($this->branch);
+
+    if ($path) {
+      $last = last($path);
+      if (isset($last['cycle'])) {
+        $this->writeWarn(
+          pht('LOCAL CYCLE'),
+          pht(
+            'Local branch tracks an upstream, but following it leads to a '.
+            'local cycle; ignoring branch upstream.'));
+
+        echo tsprintf(
+          "\n    %s\n\n",
+          $this->formatUpstreamPathCycle($path));
+
+      } else {
+        if ($last['type'] == 'remote') {
+          $onto = $last['name'];
+          $this->writeInfo(
+            pht('TARGET'),
+            pht(
+              'Landing onto "%s", selected by following tracking branches '.
+              'upstream to the closest remote.',
+              $onto));
+          return $onto;
+        } else {
+          $this->writeInfo(
+            pht('NO PATH TO UPSTREAM'),
+            pht(
+              'Local branch tracks an upstream, but there is no path '.
+              'to a remote; ignoring branch upstream.'));
+        }
+      }
+    }
+
+    $config_key = 'arc.land.onto.default';
+    $onto = $this->getConfigFromAnySource($config_key);
+    if ($onto !== null) {
+      $this->writeInfo(
+        pht('TARGET'),
+        pht(
+          'Landing onto "%s", selected by "%s" configuration.',
+          $onto,
+          $config_key));
+      return $onto;
+    }
+
+    $onto = 'master';
+    $this->writeInfo(
+      pht('TARGET'),
+      pht(
+        'Landing onto "%s", the default target under git.',
+        $onto));
+    return $onto;
+  }
+
+  private function getEngineRemote() {
+    $remote = $this->getArgument('remote');
+    if ($remote !== null) {
+      $this->writeInfo(
+        pht('REMOTE'),
+        pht(
+          'Using remote "%s", selected by the --remote flag.',
+          $remote));
+      return $remote;
+    }
+
+    $api = $this->getRepositoryAPI();
+    $path = $api->getPathToUpstream($this->branch);
+
+    if ($path) {
+      $last = last($path);
+      if ($last['type'] == 'remote') {
+        $remote = $last['remote'];
+        $this->writeInfo(
+          pht('REMOTE'),
+          pht(
+            'Using remote "%s", selected by following tracking branches '.
+            'upstream to the closest remote.',
+            $remote));
+        return $remote;
+      }
+    }
+
+    $remote = 'origin';
+    $this->writeInfo(
+      pht('REMOTE'),
+      pht(
+        'Using remote "%s", the default remote under git.',
+        $remote));
+    return $remote;
+  }
+
+  private function formatUpstreamPathCycle(array $cycle) {
+    $parts = array();
+    foreach ($cycle as $key => $value) {
+      $parts[] = $key;
+    }
+    $parts[] = idx(last($cycle), 'name');
+    $parts[] = pht('...');
+
+    return implode(' -> ', $parts);
+  }
+
+
   private function readArguments() {
     $repository_api = $this->getRepositoryAPI();
     $this->isGit = $repository_api instanceof ArcanistGitAPI;
@@ -320,9 +506,12 @@ EOTEXT
     $branch = $this->getArgument('branch');
     if (empty($branch)) {
       $branch = $this->getBranchOrBookmark();
-
       if ($branch) {
         $this->branchType = $this->getBranchType($branch);
+
+        // TODO: This message is misleading when landing a detached head or
+        // a tag in Git.
+
         echo pht("Landing current %s '%s'.", $this->branchType, $branch), "\n";
         $branch = array($branch);
       }
@@ -1079,9 +1268,7 @@ EOTEXT
       // We dispatch this event so we can run checks on the merged revision,
       // right before it gets pushed out. It's easier to do this in arc land
       // than to try to hook into git/hg.
-      $this->dispatchEvent(
-        ArcanistEventType::TYPE_LAND_WILLPUSHREVISION,
-        array());
+      $this->didCommitMerge();
     } catch (Exception $ex) {
       $this->executeCleanupAfterFailedPush();
       throw $ex;
@@ -1366,6 +1553,12 @@ EOTEXT
     $engine->setCommitMessageFile($this->messageFile);
   }
 
+  public function didCommitMerge() {
+    $this->dispatchEvent(
+      ArcanistEventType::TYPE_LAND_WILLPUSHREVISION,
+      array());
+  }
+
   public function didPush() {
     $this->askForRepositoryUpdate();