Merge branch "master" into "experimental"

src/__phutil_library_map__.php

@@ -51,6 +51,7 @@ phutil_register_library_map(array(
     'ArcanistBrowseURIHardpointLoader' => 'browse/loader/ArcanistBrowseURIHardpointLoader.php',
     'ArcanistBrowseURIRef' => 'browse/ref/ArcanistBrowseURIRef.php',
     'ArcanistBrowseWorkflow' => 'browse/workflow/ArcanistBrowseWorkflow.php',
+    'ArcanistBuildPlanRef' => 'ref/ArcanistBuildPlanRef.php',
     'ArcanistBuildRef' => 'ref/ArcanistBuildRef.php',
     'ArcanistBundle' => 'parser/ArcanistBundle.php',
     'ArcanistBundleTestCase' => 'parser/__tests__/ArcanistBundleTestCase.php',
@@ -502,6 +503,7 @@ phutil_register_library_map(array(
     'ArcanistBrowseURIHardpointLoader' => 'ArcanistHardpointLoader',
     'ArcanistBrowseURIRef' => 'ArcanistRef',
     'ArcanistBrowseWorkflow' => 'ArcanistWorkflow',
+    'ArcanistBuildPlanRef' => 'Phobject',
     'ArcanistBuildRef' => 'Phobject',
     'ArcanistBundle' => 'Phobject',
     'ArcanistBundleTestCase' => 'PhutilTestCase',

src/ref/ArcanistBuildPlanRef.php

@@ -0,0 +1,25 @@
+<?php
+
+final class ArcanistBuildPlanRef
+  extends Phobject {
+
+  private $parameters;
+
+  public static function newFromConduit(array $data) {
+    $ref = new self();
+    $ref->parameters = $data;
+    return $ref;
+  }
+
+  public function getPHID() {
+    return $this->parameters['phid'];
+  }
+
+  public function getBehavior($behavior_key, $default = null) {
+    return idxv(
+      $this->parameters,
+      array('fields', 'behaviors', $behavior_key, 'value'),
+      $default);
+  }
+
+}

src/ref/ArcanistBuildRef.php

@@ -69,6 +69,10 @@ final class ArcanistBuildRef
     return $this->parameters['id'];
   }
 
+  public function getPHID() {
+    return $this->parameters['phid'];
+  }
+
   public function getName() {
     if (isset($this->parameters['fields']['name'])) {
       return $this->parameters['fields']['name'];
@@ -96,11 +100,32 @@ final class ArcanistBuildRef
     return pht('Build %d', $this->getID());
   }
 
+  public function getBuildPlanPHID() {
+    return idxv($this->parameters, array('fields', 'buildPlanPHID'));
+  }
+
+  public function isComplete() {
+    switch ($this->getStatus()) {
+      case 'passed':
+      case 'failed':
+      case 'aborted':
+      case 'error':
+      case 'deadlocked':
+        return true;
+      default:
+        return false;
+    }
+  }
+
+  public function isPassed() {
+    return ($this->getStatus() === 'passed');
+  }
+
   public function getStatusSortVector() {
     $status = $this->getStatus();
 
     // For now, just sort passed builds first.
-    if ($this->getStatus() == 'passed') {
+    if ($this->isPassed()) {
       $status_class = 1;
     } else {
       $status_class = 2;

src/workflow/ArcanistDiffWorkflow.php

@@ -2205,8 +2205,8 @@ EOTEXT
     // we always get this 100% right, we're just trying to do something
     // reasonable.
 
-    $local = $this->loadActiveLocalCommitInfo();
+    $hashes = $this->loadActiveDiffLocalCommitHashes();
-    $hashes = ipull($local, null, 'commit');
+    $hashes = array_fuse($hashes);
 
     $usable = array();
     foreach ($commit_messages as $message) {
@@ -2253,8 +2253,8 @@ EOTEXT
       return null;
     }
 
-    $local = $this->loadActiveLocalCommitInfo();
+    $hashes = $this->loadActiveDiffLocalCommitHashes();
-    $hashes = ipull($local, null, 'commit');
+    $hashes = array_fuse($hashes);
 
     $usable = array();
     foreach ($messages as $rev => $message) {
@@ -2302,7 +2302,63 @@ EOTEXT
     return implode('', $default);
   }
 
-  private function loadActiveLocalCommitInfo() {
+  private function loadActiveDiffLocalCommitHashes() {
+    // The older "differential.querydiffs" method includes the full diff text,
+    // which can be very slow for large diffs. If we can, try to use
+    // "differential.diff.search" instead.
+
+    // We expect this to fail if the Phabricator version on the server is
+    // older than April 2018 (D19386), which introduced the "commits"
+    // attachment for "differential.revision.search".
+
+    // TODO: This can be optimized if we're able to learn the "revisionPHID"
+    // before we get here. See PHI1104.
+
+    try {
+      $revisions_raw = $this->getConduit()->callMethodSynchronous(
+        'differential.revision.search',
+        array(
+          'constraints' => array(
+            'ids' => array(
+              $this->revisionID,
+            ),
+          ),
+        ));
+
+      $revisions = $revisions_raw['data'];
+      $revision = head($revisions);
+      if ($revision) {
+        $revision_phid = $revision['phid'];
+
+        $diffs_raw = $this->getConduit()->callMethodSynchronous(
+          'differential.diff.search',
+          array(
+            'constraints' => array(
+              'revisionPHIDs' => array(
+                $revision_phid,
+              ),
+            ),
+            'attachments' => array(
+              'commits' => true,
+            ),
+            'limit' => 1,
+          ));
+
+        $diffs = $diffs_raw['data'];
+        $diff = head($diffs);
+
+        if ($diff) {
+          $commits = idxv($diff, array('attachments', 'commits', 'commits'));
+          if ($commits !== null) {
+            $hashes = ipull($commits, 'identifier');
+            return array_values($hashes);
+          }
+        }
+      }
+    } catch (Exception $ex) {
+      // If any of this fails, fall back to the older method below.
+    }
+
     $current_diff = $this->getConduit()->callMethodSynchronous(
       'differential.querydiffs',
       array(
@@ -2311,7 +2367,10 @@ EOTEXT
     $current_diff = head($current_diff);
 
     $properties = idx($current_diff, 'properties', array());
-    return idx($properties, 'local:commits', array());
+    $local = idx($properties, 'local:commits', array());
+    $hashes = ipull($local, 'commit');
+
+    return array_values($hashes);
   }
 
 

src/workflow/ArcanistLandWorkflow.php

@@ -1369,6 +1369,19 @@ EOTEXT
    * before landing if it does.
    */
   private function checkForBuildables($diff_phid) {
+    // Try to use the more modern check which respects the "Warn on Land"
+    // behavioral flag on build plans if we can. This newer check won't work
+    // unless the server is running code from March 2019 or newer since the
+    // API methods we need won't exist yet. We'll fall back to the older check
+    // if this one doesn't work out.
+    try {
+      $this->checkForBuildablesWithPlanBehaviors($diff_phid);
+    } catch (ArcanistUserAbortException $abort_ex) {
+      throw $abort_ex;
+    } catch (Exception $ex) {
+      // Continue with the older approach, below.
+    }
+
     // NOTE: Since Harbormaster is still beta and this stuff all got added
     // recently, just bail if we can't find a buildable. This is just an
     // advisory check intended to prevent human error.
@@ -1449,6 +1462,166 @@ EOTEXT
     }
   }
 
+  private function checkForBuildablesWithPlanBehaviors($diff_phid) {
+    // TODO: These queries should page through all results instead of fetching
+    // only the first page, but we don't have good primitives to support that
+    // in "master" yet.
+
+    $this->writeInfo(
+      pht('BUILDS'),
+      pht('Checking build status...'));
+
+    $raw_buildables = $this->getConduit()->callMethodSynchronous(
+      'harbormaster.buildable.search',
+      array(
+        'constraints' => array(
+          'objectPHIDs' => array(
+            $diff_phid,
+          ),
+          'manual' => false,
+        ),
+      ));
+
+    if (!$raw_buildables['data']) {
+      return;
+    }
+
+    $buildables = $raw_buildables['data'];
+    $buildable_phids = ipull($buildables, 'phid');
+
+    $raw_builds = $this->getConduit()->callMethodSynchronous(
+      'harbormaster.build.search',
+      array(
+        'constraints' => array(
+          'buildables' => $buildable_phids,
+        ),
+      ));
+
+    if (!$raw_builds['data']) {
+      return;
+    }
+
+    $builds = array();
+    foreach ($raw_builds['data'] as $raw_build) {
+      $build_ref = ArcanistBuildRef::newFromConduit($raw_build);
+      $build_phid = $build_ref->getPHID();
+      $builds[$build_phid] = $build_ref;
+    }
+
+    $plan_phids = mpull($builds, 'getBuildPlanPHID');
+    $plan_phids = array_values($plan_phids);
+
+    $raw_plans = $this->getConduit()->callMethodSynchronous(
+      'harbormaster.buildplan.search',
+      array(
+        'constraints' => array(
+          'phids' => $plan_phids,
+        ),
+      ));
+
+    $plans = array();
+    foreach ($raw_plans['data'] as $raw_plan) {
+      $plan_ref = ArcanistBuildPlanRef::newFromConduit($raw_plan);
+      $plan_phid = $plan_ref->getPHID();
+      $plans[$plan_phid] = $plan_ref;
+    }
+
+    $ongoing_builds = array();
+    $failed_builds = array();
+
+    $builds = msort($builds, 'getStatusSortVector');
+    foreach ($builds as $build_ref) {
+      $plan = idx($plans, $build_ref->getBuildPlanPHID());
+      if (!$plan) {
+        continue;
+      }
+
+      $plan_behavior = $plan->getBehavior('arc-land', 'always');
+      $if_building = ($plan_behavior == 'building');
+      $if_complete = ($plan_behavior == 'complete');
+      $if_never = ($plan_behavior == 'never');
+
+      // If the build plan "Never" warns when landing, skip it.
+      if ($if_never) {
+        continue;
+      }
+
+      // If the build plan warns when landing "If Complete" but the build is
+      // not complete, skip it.
+      if ($if_complete && !$build_ref->isComplete()) {
+        continue;
+      }
+
+      // If the build plan warns when landing "If Building" but the build is
+      // complete, skip it.
+      if ($if_building && $build_ref->isComplete()) {
+        continue;
+      }
+
+      // Ignore passing builds.
+      if ($build_ref->isPassed()) {
+        continue;
+      }
+
+      if (!$build_ref->isComplete()) {
+        $ongoing_builds[] = $build_ref;
+      } else {
+        $failed_builds[] = $build_ref;
+      }
+    }
+
+    if (!$ongoing_builds && !$failed_builds) {
+      return;
+    }
+
+    if ($failed_builds) {
+      $this->writeWarn(
+        pht('BUILD FAILURES'),
+        pht(
+          'Harbormaster failed to build the active diff for this revision:'));
+      $prompt = pht('Land revision anyway, despite build failures?');
+    } else if ($ongoing_builds) {
+      $this->writeWarn(
+        pht('ONGOING BUILDS'),
+        pht(
+          'Harbormaster is still building the active diff for this revision:'));
+      $prompt = pht('Land revision anyway, despite ongoing build?');
+    }
+
+    $show_builds = array_merge($failed_builds, $ongoing_builds);
+    echo "\n";
+    foreach ($show_builds as $build_ref) {
+      $ansi_color = $build_ref->getStatusANSIColor();
+      $status_name = $build_ref->getStatusName();
+      $object_name = $build_ref->getObjectName();
+      $build_name = $build_ref->getName();
+
+      echo tsprintf(
+        "    **<bg:".$ansi_color."> %s </bg>** %s: %s\n",
+        $status_name,
+        $object_name,
+        $build_name);
+    }
+
+    echo tsprintf(
+      "\n%s\n\n",
+      pht('You can review build details here:'));
+
+    foreach ($buildables as $buildable) {
+      $buildable_uri = id(new PhutilURI($this->getConduitURI()))
+        ->setPath(sprintf('/B%d', $buildable['id']));
+
+      echo tsprintf(
+        "          **%s**: __%s__\n",
+        pht('Buildable %d', $buildable['id']),
+        $buildable_uri);
+    }
+
+    if (!phutil_console_confirm($prompt)) {
+      throw new ArcanistUserAbortException();
+    }
+  }
+
   public function buildEngineMessage(ArcanistLandEngine $engine) {
     // TODO: This is oh-so-gross.
     $this->findRevision();

src/workflow/ArcanistPatchWorkflow.php

@@ -307,6 +307,14 @@ EOTEXT
         $repository_api->execxLocal('checkout -b %s', $branch_name);
       }
 
+      // Synchronize submodule state, since the checkout may have modified
+      // submodule references. See PHI1083.
+
+      // Note that newer versions of "git checkout" include a
+      // "--recurse-submodules" flag which accomplishes this goal a little
+      // more simply. For now, use the more compatible form.
+      $repository_api->execPassthru('submodule update --init --recursive');
+
       echo phutil_console_format(
         "%s\n",
         pht(
@@ -714,35 +722,67 @@ EOTEXT
         throw new ArcanistUsageException(pht('Unable to apply patch!'));
       }
 
+      // See PHI1083 and PHI648. If the patch applied changes to submodules,
+      // it only updates the submodule pointer, not the actual submodule. We're
+      // left with the pointer update staged in the index, and the unmodified
+      // submodule on disk.
+
+      // If we then "git commit --all" or "git add --all", the unmodified
+      // submodule on disk is added to the index as a change, which effectively
+      // undoes the patch we just applied and reverts the submodule back to
+      // the previous state.
+
+      // To avoid this, do a submodule update before we continue.
+
+      // We could also possibly skip the "--all" flag so we don't have to do
+      // this submodule update, but we want to leave the working copy in a
+      // clean state anyway, so we're going to have to do an update at some
+      // point. This usually doesn't cost us anything.
+      $repository_api->execPassthru('submodule update --init --recursive');
+
       if ($this->shouldCommit()) {
+        $flags = array();
         if ($bundle->getFullAuthor()) {
-          $author_cmd = csprintf('--author=%s', $bundle->getFullAuthor());
+          $flags[] = csprintf('--author=%s', $bundle->getFullAuthor());
-        } else {
-          $author_cmd = '';
         }
 
         $commit_message = $this->getCommitMessage($bundle);
+
         $future = $repository_api->execFutureLocal(
-          'commit -a %C -F - --no-verify',
+          'commit -a %Ls -F - --no-verify',
-          $author_cmd);
+          $flags);
         $future->write($commit_message);
         $future->resolvex();
-        $verb = pht('committed');
+
+        $this->writeOkay(
+          pht('COMMITTED'),
+          pht('Successfully committed patch.'));
       } else {
-        $verb = pht('applied');
+        $this->writeOkay(
+          pht('APPLIED'),
+          pht('Successfully applied patch.'));
       }
 
       if ($this->canBranch() &&
           !$this->shouldBranch() &&
           $this->shouldCommit() && $has_base_revision) {
-        $repository_api->execxLocal('checkout %s', $original_branch);
+
+        // See PHI1083 and PHI648. Synchronize submodule state after mutating
+        // the working copy.
+
+        $repository_api->execxLocal('checkout %s --', $original_branch);
+        $repository_api->execPassthru('submodule update --init --recursive');
+
         $ex = null;
         try {
-          $repository_api->execxLocal('cherry-pick %s', $new_branch);
+          $repository_api->execxLocal('cherry-pick -- %s', $new_branch);
+          $repository_api->execPassthru('submodule update --init --recursive');
         } catch (Exception $ex) {
           // do nothing
         }
-        $repository_api->execxLocal('branch -D %s', $new_branch);
+
+        $repository_api->execxLocal('branch -D -- %s', $new_branch);
+
         if ($ex) {
           echo phutil_console_format(
             "\n<bg:red>** %s**</bg>\n",
@@ -751,15 +791,6 @@ EOTEXT
         }
       }
 
-      // Synchronize submodule state, since the patch may have made changes
-      // to ".gitmodules". We do this after we finish managing branches so
-      // the behavior is correct under "--nobranch"; see PHI648.
-      $repository_api->execPassthru('submodule update --init --recursive');
-
-      echo phutil_console_format(
-        "<bg:green>** %s **</bg> %s\n",
-        pht('OKAY'),
-        pht('Successfully %s patch.', $verb));
     } else if ($repository_api instanceof ArcanistMercurialAPI) {
       $future = $repository_api->execFutureLocal('import --no-commit -');
       $future->write($bundle->toGitPatch());