From bc4f86d2799681b8e96702d0671245c85ee8215b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 19 Oct 2020 12:21:06 -0700
Subject: [PATCH 01/19] When a new, deleted, draft inline is revived with
 "Undo", undelete it

Summary:
See PHI1876. Normally, deleted inlines are undeleted with an "undelete" operation, which clears the "isDeleted" flag.

However, when an inline is deleted implicitly by using "Cancel" without first saving it, the flag currently isn't cleared properly. This can lead to cases where inlines seem to vanish (they are shown to the user in the UI, but treated as deleted on submission).

Test Plan:
There are two affected sequences here:

  - Create a new inline, type text, cancel, undo.
  - Create a new inline, type text, cancel, undo, save.

The former sequence triggers an "edit" operation. The subsequent "Save" in the second sequence triggers a "save" operation.

It's normally impossible in the UI to execute a "save" without executing an "edit" first, but "save" clearly should undelete the comment if you get there somehow, so this change clears the deleted flag in both cases for completeness.

  - Executed both sequences, saw comment persist in preview, on reload, and after submission.

Differential Revision: https://secure.phabricator.com/D21483
---
 .../diff/PhabricatorInlineCommentController.php            | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index e1b8dc7264..693f66066f 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -189,6 +189,8 @@ abstract class PhabricatorInlineCommentController
           $inline->setIsEditing(false);
 
           if (!$inline->isVoidComment($viewer)) {
+            $inline->setIsDeleted(0);
+
             $this->saveComment($inline);
 
             return $this->buildRenderedCommentResponse(
@@ -217,7 +219,10 @@ abstract class PhabricatorInlineCommentController
 
           $is_dirty = false;
           if (!$inline->getIsEditing()) {
-            $inline->setIsEditing(true);
+            $inline
+              ->setIsDeleted(0)
+              ->setIsEditing(true);
+
             $is_dirty = true;
           }
 

From 671986592bafc4b2f059319d9e320cab46445ac2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2020 12:34:19 -0700
Subject: [PATCH 02/19] Add a missing "GROUP BY" to MailQuery when querying for
 multiple recipients

Summary:
See <https://discourse.phabricator-community.org/t/mail-details-view-broken/4315>. The change in D21400 detects a missing "GROUP BY" in some variations of this query.

Specifically, we may join multiple recipient rows (since mail may have multiple recipients) and then fail to group the results.

Fix this by adding the "GROUP BY". Additionally, remove the special-cased behavior when no authors or recipients are specified -- it's complicated and not entirely correct (e.g., may produce a "no object" instead of a policy error when querying by ID), and likely predates overheating.

Test Plan:
  - Disabled `metamta.one-mail-per-recipient` in Config.
  - Generated a message to 2+ recipients.
  - Viewed the message detail; queried for the message by specifying 2+ recipients.
  - Viewed the unfiltered list of messages, saw the query overheat.

Differential Revision: https://secure.phabricator.com/D21486
---
 .../query/PhabricatorMetaMTAMailQuery.php     | 55 +++++++++----------
 .../policy/PhabricatorPolicyAwareQuery.php    |  2 +-
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
index a1fad69c0b..903b385ceb 100644
--- a/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
+++ b/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
@@ -64,24 +64,6 @@ final class PhabricatorMetaMTAMailQuery
         $this->actorPHIDs);
     }
 
-    if ($this->recipientPHIDs !== null) {
-      $where[] = qsprintf(
-        $conn,
-        'recipient.dst IN (%Ls)',
-        $this->recipientPHIDs);
-    }
-
-    if ($this->actorPHIDs === null && $this->recipientPHIDs === null) {
-      $viewer = $this->getViewer();
-      if (!$viewer->isOmnipotent()) {
-        $where[] = qsprintf(
-          $conn,
-          'edge.dst = %s OR actorPHID = %s',
-          $viewer->getPHID(),
-          $viewer->getPHID());
-      }
-    }
-
     if ($this->createdMin !== null) {
       $where[] = qsprintf(
         $conn,
@@ -102,26 +84,29 @@ final class PhabricatorMetaMTAMailQuery
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
     $joins = parent::buildJoinClauseParts($conn);
 
-    if ($this->actorPHIDs === null && $this->recipientPHIDs === null) {
+    if ($this->shouldJoinRecipients()) {
       $joins[] = qsprintf(
         $conn,
-        'LEFT JOIN %T edge ON mail.phid = edge.src AND edge.type = %d',
+        'JOIN %T recipient
+          ON mail.phid = recipient.src
+            AND recipient.type = %d
+            AND recipient.dst IN (%Ls)',
         PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-        PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST);
-    }
-
-    if ($this->recipientPHIDs !== null) {
-      $joins[] = qsprintf(
-        $conn,
-        'LEFT JOIN %T recipient '.
-        'ON mail.phid = recipient.src AND recipient.type = %d',
-        PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-        PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST);
+        PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST,
+        $this->recipientPHIDs);
     }
 
     return $joins;
   }
 
+  private function shouldJoinRecipients() {
+    if ($this->recipientPHIDs === null) {
+      return false;
+    }
+
+    return true;
+  }
+
   protected function getPrimaryTableAlias() {
     return 'mail';
   }
@@ -134,4 +119,14 @@ final class PhabricatorMetaMTAMailQuery
     return 'PhabricatorMetaMTAApplication';
   }
 
+  protected function shouldGroupQueryResultRows() {
+    if ($this->shouldJoinRecipients()) {
+      if (count($this->recipientPHIDs) > 1) {
+        return true;
+      }
+    }
+
+    return parent::shouldGroupQueryResultRows();
+  }
+
 }
diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
index a770c326f9..c43edaefcb 100644
--- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
@@ -234,7 +234,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
     // T11773 for some discussion.
     $this->isOverheated = false;
 
-    // See T13386. If we on an old offset-based paging workflow, we need
+    // See T13386. If we are on an old offset-based paging workflow, we need
     // to base the overheating limit on both the offset and limit.
     $overheat_limit = $need * 10;
     $total_seen = 0;

From c04147328fa3006c75b6a669d6c0e6ecb335be01 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2020 13:03:34 -0700
Subject: [PATCH 03/19] Fix isValidGitShallowCloneResponse

Summary:
Changes the heuristic method by which non-zero exit statuses from git-http-backend are found to be due to packfile negotiation during shallow fetches, etc.

Instead of checking git-http-backend stderr for a generic "hung up" error message, see if the pack-result response contains a terminating flush packet ("0000"). This should give a greater assurance that the request was handled correctly and the response is complete.

Test Plan: Run `GIT_CURL_VERBOSE=1 git fetch --depth 1 https://host.example/source/repo.git HEAD` to ensure it completes and includes two successful POST requests during packfile negotiation (the last one actually receives the packfile).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, dzduvall

Tags: #diffusion

Differential Revision: https://secure.phabricator.com/D21484
---
 .../controller/DiffusionServeController.php   | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php
index 60d5c1578d..1a1383368f 100644
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@@ -922,18 +922,26 @@ final class DiffusionServeController extends DiffusionController {
     // This is a pretty funky fix: it would be nice to more precisely detect
     // that a request is a `--depth N` clone request, but we don't have any code
     // to decode protocol frames yet. Instead, look for reasonable evidence
-    // in the error and output that we're looking at a `--depth` clone.
+    // in the output that we're looking at a `--depth` clone.
 
-    // For evidence this isn't completely crazy, see:
-    // https://github.com/schacon/grack/pull/7
+    // A valid x-git-upload-pack-result response during packfile negotiation
+    // should end with a flush packet ("0000"). As long as that packet
+    // terminates the response body in the response, we'll assume the response
+    // is correct and complete.
+
+    // See https://git-scm.com/docs/pack-protocol#_packfile_negotiation
 
     $stdout_regexp = '(^Content-Type: application/x-git-upload-pack-result)m';
-    $stderr_regexp = '(The remote end hung up unexpectedly)';
 
     $has_pack = preg_match($stdout_regexp, $stdout);
-    $is_hangup = preg_match($stderr_regexp, $stderr);
 
-    return $has_pack && $is_hangup;
+    if (strlen($stdout) >= 4) {
+      $has_flush_packet = (substr($stdout, -4) === "0000");
+    } else {
+      $has_flush_packet = false;
+    }
+
+    return ($has_pack && $has_flush_packet);
   }
 
   private function getCommonEnvironment(PhabricatorUser $viewer) {

From ae5a38f3349fdd718033619c4e908137f1634502 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 3 Nov 2020 10:32:26 -0800
Subject: [PATCH 04/19] Guarantee terms in PhabricatorAuthPasswordEngine are
 strings

Summary:
Ref T2312. Numeric strings are read out of arrays as integers, and modern PHP raises appropriate warnings when they're then treated as strings.

For now, cast the keys to strings explicitly (we know we inserted only strings). In the future, introduction of a `StringMap` type or similar might be appropriate.

Test Plan:
  - Added "abc.12345.xyz" to the blocklist, changed my VCS password.
  - Before: fatal when trying to "strpos()" an integer.
  - After: password change worked correctly.

Maniphest Tasks: T2312

Differential Revision: https://secure.phabricator.com/D21487
---
 .../auth/engine/PhabricatorAuthPasswordEngine.php           | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
index f763b0987f..a1fec4a6d2 100644
--- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
@@ -181,6 +181,12 @@ final class PhabricatorAuthPasswordEngine
     $normal_password = phutil_utf8_strtolower($raw_password);
     if (strlen($normal_password) >= $minimum_similarity) {
       foreach ($normal_map as $term => $source) {
+
+        // See T2312. This may be required if the term list includes numeric
+        // strings like "12345", which will be cast to integers when used as
+        // array keys.
+        $term = phutil_string_cast($term);
+
         if (strpos($term, $normal_password) === false &&
             strpos($normal_password, $term) === false) {
           continue;

From bf8707d3a9bf9670ce44fdfe8c7812033cf014a1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 3 Nov 2020 12:20:10 -0800
Subject: [PATCH 05/19] Add a basic "harbormaster.step.search" API method

Summary: Ref T13585. This isn't particularly useful (notably, it does not include custom field values and isn't searchable by build plan PHID) but get the basics into place.

Test Plan: Used the web UI to make API calls, reviewed results.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13585

Differential Revision: https://secure.phabricator.com/D21488
---
 src/__phutil_library_map__.php                |  5 ++
 .../HarbormasterBuildStepSearchAPIMethod.php  | 18 ++++++
 .../HarbormasterBuildStepSearchEngine.php     | 58 +++++++++++++++++++
 .../configuration/HarbormasterBuildStep.php   | 43 +++++++++++++-
 4 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 src/applications/harbormaster/conduit/HarbormasterBuildStepSearchAPIMethod.php
 create mode 100644 src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 4f6ae109bb..0bb84f38b9 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1436,6 +1436,8 @@ phutil_register_library_map(array(
     'HarbormasterBuildStepImplementationTestCase' => 'applications/harbormaster/step/__tests__/HarbormasterBuildStepImplementationTestCase.php',
     'HarbormasterBuildStepPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php',
     'HarbormasterBuildStepQuery' => 'applications/harbormaster/query/HarbormasterBuildStepQuery.php',
+    'HarbormasterBuildStepSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildStepSearchAPIMethod.php',
+    'HarbormasterBuildStepSearchEngine' => 'applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php',
     'HarbormasterBuildStepTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php',
     'HarbormasterBuildStepTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildStepTransactionQuery.php',
     'HarbormasterBuildTarget' => 'applications/harbormaster/storage/build/HarbormasterBuildTarget.php',
@@ -7627,6 +7629,7 @@ phutil_register_library_map(array(
       'PhabricatorApplicationTransactionInterface',
       'PhabricatorPolicyInterface',
       'PhabricatorCustomFieldInterface',
+      'PhabricatorConduitResultInterface',
     ),
     'HarbormasterBuildStepCoreCustomField' => array(
       'HarbormasterBuildStepCustomField',
@@ -7639,6 +7642,8 @@ phutil_register_library_map(array(
     'HarbormasterBuildStepImplementationTestCase' => 'PhabricatorTestCase',
     'HarbormasterBuildStepPHIDType' => 'PhabricatorPHIDType',
     'HarbormasterBuildStepQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
+    'HarbormasterBuildStepSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
+    'HarbormasterBuildStepSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'HarbormasterBuildStepTransaction' => 'PhabricatorApplicationTransaction',
     'HarbormasterBuildStepTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'HarbormasterBuildTarget' => array(
diff --git a/src/applications/harbormaster/conduit/HarbormasterBuildStepSearchAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterBuildStepSearchAPIMethod.php
new file mode 100644
index 0000000000..9305cf0abd
--- /dev/null
+++ b/src/applications/harbormaster/conduit/HarbormasterBuildStepSearchAPIMethod.php
@@ -0,0 +1,18 @@
+<?php
+
+final class HarbormasterBuildStepSearchAPIMethod
+  extends PhabricatorSearchEngineAPIMethod {
+
+  public function getAPIMethodName() {
+    return 'harbormaster.step.search';
+  }
+
+  public function newSearchEngine() {
+    return new HarbormasterBuildStepSearchEngine();
+  }
+
+  public function getMethodSummary() {
+    return pht('Retrieve information about Harbormaster build steps.');
+  }
+
+}
diff --git a/src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php b/src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php
new file mode 100644
index 0000000000..b866a1dbc9
--- /dev/null
+++ b/src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php
@@ -0,0 +1,58 @@
+<?php
+
+final class HarbormasterBuildStepSearchEngine
+  extends PhabricatorApplicationSearchEngine {
+
+  public function getResultTypeDescription() {
+    return pht('Harbormaster Build Steps');
+  }
+
+  public function getApplicationClassName() {
+    return 'PhabricatorHarbormasterApplication';
+  }
+
+  public function newQuery() {
+    return new HarbormasterBuildStepQuery();
+  }
+
+  protected function buildCustomSearchFields() {
+    return array();
+  }
+
+  protected function buildQueryFromParameters(array $map) {
+    $query = $this->newQuery();
+
+    return $query;
+  }
+
+  protected function getURI($path) {
+    return '/harbormaster/step/'.$path;
+  }
+
+  protected function getBuiltinQueryNames() {
+    return array(
+      'all' => pht('All Steps'),
+    );
+  }
+
+  public function buildSavedQueryFromBuiltin($query_key) {
+    $query = $this->newSavedQuery();
+    $query->setQueryKey($query_key);
+
+    switch ($query_key) {
+      case 'all':
+        return $query;
+    }
+
+    return parent::buildSavedQueryFromBuiltin($query_key);
+  }
+
+  protected function renderResultList(
+    array $plans,
+    PhabricatorSavedQuery $query,
+    array $handles) {
+    assert_instances_of($plans, 'HarbormasterBuildStep');
+    return null;
+  }
+
+}
diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
index dd0ebdc507..71b050adff 100644
--- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
+++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
@@ -4,7 +4,8 @@ final class HarbormasterBuildStep extends HarbormasterDAO
   implements
     PhabricatorApplicationTransactionInterface,
     PhabricatorPolicyInterface,
-    PhabricatorCustomFieldInterface {
+    PhabricatorCustomFieldInterface,
+    PhabricatorConduitResultInterface {
 
   protected $name;
   protected $description;
@@ -169,5 +170,45 @@ final class HarbormasterBuildStep extends HarbormasterDAO
     return $this;
   }
 
+/* -(  PhabricatorConduitResultInterface  )---------------------------------- */
+
+
+  public function getFieldSpecificationsForConduit() {
+    return array(
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('name')
+        ->setType('string')
+        ->setDescription(pht('The name of the build step.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('description')
+        ->setType('remarkup')
+        ->setDescription(pht('The build step description.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('buildPlanPHID')
+        ->setType('phid')
+        ->setDescription(
+          pht(
+            'The PHID of the build plan this build step belongs to.')),
+    );
+  }
+
+  public function getFieldValuesForConduit() {
+    // T6203: This can be removed once the field becomes non-nullable.
+    $name = $this->getName();
+    $name = phutil_string_cast($name);
+
+    return array(
+      'name' => $name,
+      'description' => array(
+        'raw' => $this->getDescription(),
+      ),
+      'buildPlanPHID' => $this->getBuildPlanPHID(),
+    );
+  }
+
+  public function getConduitSearchAttachments() {
+    return array();
+  }
+
 
 }

From 34082efb02c7815ecce80957ecddee93aedd7571 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 3 Nov 2020 12:22:16 -0800
Subject: [PATCH 06/19] Add a basic "harbormaster.step.edit" API method

Summary: Ref T13585. Provide a minimal but technically functional "harbormaster.step.edit" API method.

Test Plan: Used the web console to modify the URI for a "Make HTTP Request" build step.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13585

Differential Revision: https://secure.phabricator.com/D21489
---
 src/__phutil_library_map__.php                |   4 +
 .../HarbormasterBuildStepEditAPIMethod.php    |  20 ++++
 .../HarbormasterBuildStepEditEngine.php       | 107 ++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php
 create mode 100644 src/applications/harbormaster/editor/HarbormasterBuildStepEditEngine.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 0bb84f38b9..12a5ad9333 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1430,6 +1430,8 @@ phutil_register_library_map(array(
     'HarbormasterBuildStep' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStep.php',
     'HarbormasterBuildStepCoreCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php',
     'HarbormasterBuildStepCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php',
+    'HarbormasterBuildStepEditAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php',
+    'HarbormasterBuildStepEditEngine' => 'applications/harbormaster/editor/HarbormasterBuildStepEditEngine.php',
     'HarbormasterBuildStepEditor' => 'applications/harbormaster/editor/HarbormasterBuildStepEditor.php',
     'HarbormasterBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterBuildStepGroup.php',
     'HarbormasterBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterBuildStepImplementation.php',
@@ -7636,6 +7638,8 @@ phutil_register_library_map(array(
       'PhabricatorStandardCustomFieldInterface',
     ),
     'HarbormasterBuildStepCustomField' => 'PhabricatorCustomField',
+    'HarbormasterBuildStepEditAPIMethod' => 'PhabricatorEditEngineAPIMethod',
+    'HarbormasterBuildStepEditEngine' => 'PhabricatorEditEngine',
     'HarbormasterBuildStepEditor' => 'PhabricatorApplicationTransactionEditor',
     'HarbormasterBuildStepGroup' => 'Phobject',
     'HarbormasterBuildStepImplementation' => 'Phobject',
diff --git a/src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php
new file mode 100644
index 0000000000..e29f27cb10
--- /dev/null
+++ b/src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php
@@ -0,0 +1,20 @@
+<?php
+
+final class HarbormasterBuildStepEditAPIMethod
+  extends PhabricatorEditEngineAPIMethod {
+
+  public function getAPIMethodName() {
+    return 'harbormaster.step.edit';
+  }
+
+  public function newEditEngine() {
+    return new HarbormasterBuildStepEditEngine();
+  }
+
+  public function getMethodSummary() {
+    return pht(
+      'Apply transactions to create a new build step or edit an existing '.
+      'one.');
+  }
+
+}
diff --git a/src/applications/harbormaster/editor/HarbormasterBuildStepEditEngine.php b/src/applications/harbormaster/editor/HarbormasterBuildStepEditEngine.php
new file mode 100644
index 0000000000..d54832cad1
--- /dev/null
+++ b/src/applications/harbormaster/editor/HarbormasterBuildStepEditEngine.php
@@ -0,0 +1,107 @@
+<?php
+
+final class HarbormasterBuildStepEditEngine
+  extends PhabricatorEditEngine {
+
+  const ENGINECONST = 'harbormaster.buildstep';
+
+  private $buildPlan;
+
+  public function setBuildPlan(HarbormasterBuildPlan $build_plan) {
+    $this->buildPlan = $build_plan;
+    return $this;
+  }
+
+  public function getBuildPlan() {
+    if ($this->buildPlan === null) {
+      throw new PhutilInvalidStateException('setBuildPlan');
+    }
+
+    return $this->buildPlan;
+  }
+
+  public function isEngineConfigurable() {
+    return false;
+  }
+
+  public function getEngineName() {
+    return pht('Harbormaster Build Steps');
+  }
+
+  public function getSummaryHeader() {
+    return pht('Edit Harbormaster Build Step Configurations');
+  }
+
+  public function getSummaryText() {
+    return pht('This engine is used to edit Harbormaster build steps.');
+  }
+
+  public function getEngineApplicationClass() {
+    return 'PhabricatorHarbormasterApplication';
+  }
+
+  protected function newEditableObject() {
+    $viewer = $this->getViewer();
+
+
+    $plan = HarbormasterBuildPlan::initializeNewBuildPlan($viewer);
+    $this->setBuildPlan($plan);
+
+    $plan = $this->getBuildPlan();
+
+    $step = HarbormasterBuildStep::initializeNewStep($viewer);
+
+    $step->setBuildPlanPHID($plan->getPHID());
+    $step->attachBuildPlan($plan);
+
+    return $step;
+  }
+
+  protected function newObjectQuery() {
+    return new HarbormasterBuildStepQuery();
+  }
+
+  protected function getObjectCreateTitleText($object) {
+    return pht('Create Build Step');
+  }
+
+  protected function getObjectCreateButtonText($object) {
+    return pht('Create Build Step');
+  }
+
+  protected function getObjectEditTitleText($object) {
+    return pht('Edit Build Step: %s', $object->getName());
+  }
+
+  protected function getObjectEditShortText($object) {
+    return pht('Edit Build Step');
+  }
+
+  protected function getObjectCreateShortText() {
+    return pht('Create Build Step');
+  }
+
+  protected function getObjectName() {
+    return pht('Build Step');
+  }
+
+  protected function getEditorURI() {
+    return '/harbormaster/step/edit/';
+  }
+
+  protected function getObjectCreateCancelURI($object) {
+    return '/harbormaster/step/';
+  }
+
+  protected function getObjectViewURI($object) {
+    $id = $object->getID();
+    return "/harbormaster/step/{$id}/";
+  }
+
+  protected function buildCustomEditFields($object) {
+    $fields = array();
+
+    return $fields;
+  }
+
+}

From b2ab18f8f3d0cbab55b92da7a5fcbc0e148a4c99 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin.mckinley@robinhood.com>
Date: Thu, 19 Nov 2020 21:48:33 +0000
Subject: [PATCH 07/19] Change baseURI for Packages to avoid 404

Summary: Without this change, the landing page for the Packages app is https://secure.phabricator.com/packages, which is a 404. There's probably a better way to fix this, but this was the fewest characters.

Test Plan: doitlive

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21494
---
 .../packages/application/PhabricatorPackagesApplication.php     | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/packages/application/PhabricatorPackagesApplication.php b/src/applications/packages/application/PhabricatorPackagesApplication.php
index 9de74dddc0..874a9b5f31 100644
--- a/src/applications/packages/application/PhabricatorPackagesApplication.php
+++ b/src/applications/packages/application/PhabricatorPackagesApplication.php
@@ -15,7 +15,7 @@ final class PhabricatorPackagesApplication extends PhabricatorApplication {
   }
 
   public function getBaseURI() {
-    return '/packages/';
+    return '/packages/package/';
   }
 
   public function getIcon() {

From 18f049a282f4085de5f68db42a9fe4dc3ef393ce Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Jan 2021 10:29:58 -0800
Subject: [PATCH 08/19] Fix reading of the request path when running the PHP
 builtin webserver

Summary:
Ref T13575. Since PHP builtin webserver support was added, the pathway for parsing request parameters became more complex. We now rebuild "$_REQUEST" later, and this rebuild will destroy any mutations made to it here, so the assignment to "__path__" is lost.

Instead of "validating" the request path, make this method "read" the request path and store it explicitly, so it will survive any later request mutations.

Test Plan:
  - Submitted any POST form while running Phabricator under the builtin PHP webserver. Old behavior was an error when accessing "__path__"; new behavior is a working application.
  - Loaded normal pages, etc.

Maniphest Tasks: T13575

Differential Revision: https://secure.phabricator.com/D21506
---
 .../AphrontApplicationConfiguration.php       |  4 +-
 support/startup/PhabricatorStartup.php        | 51 ++++++++++++++++---
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index 5bed835d8f..31b3db9a7f 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -176,7 +176,7 @@ final class AphrontApplicationConfiguration
     }
 
     $host = AphrontRequest::getHTTPHeader('Host');
-    $path = $_REQUEST['__path__'];
+    $path = PhabricatorStartup::getRequestPath();
 
     $application = new self();
 
@@ -759,7 +759,7 @@ final class AphrontApplicationConfiguration
   }
 
   private static function newSelfCheckResponse() {
-    $path = idx($_REQUEST, '__path__', '');
+    $path = PhabricatorStartup::getRequestPath();
     $query = idx($_SERVER, 'QUERY_STRING', '');
 
     $pairs = id(new PhutilQueryStringParser())
diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php
index 5dac7505d9..870d342464 100644
--- a/support/startup/PhabricatorStartup.php
+++ b/support/startup/PhabricatorStartup.php
@@ -35,6 +35,7 @@
  * @task validation   Validation
  * @task ratelimit    Rate Limiting
  * @task phases       Startup Phase Timers
+ * @task request-path Request Path
  */
 final class PhabricatorStartup {
 
@@ -47,6 +48,7 @@ final class PhabricatorStartup {
   private static $phases;
 
   private static $limits = array();
+  private static $requestPath;
 
 
 /* -(  Accessing Request Information  )-------------------------------------- */
@@ -119,6 +121,7 @@ final class PhabricatorStartup {
     self::$phases = array();
 
     self::$accessLog = null;
+    self::$requestPath = null;
 
     static $registered;
     if (!$registered) {
@@ -140,7 +143,7 @@ final class PhabricatorStartup {
 
     self::normalizeInput();
 
-    self::verifyRewriteRules();
+    self::readRequestPath();
 
     self::beginOutputCapture();
   }
@@ -552,17 +555,29 @@ final class PhabricatorStartup {
 
 
   /**
-   * @task validation
+   * @task request-path
    */
-  private static function verifyRewriteRules() {
+  private static function readRequestPath() {
+
+    // See T13575. The request path may be provided in:
+    //
+    //  - the "$_GET" parameter "__path__" (normal for Apache and nginx); or
+    //  - the "$_SERVER" parameter "REQUEST_URI" (normal for the PHP builtin
+    //    webserver).
+    //
+    // Locate it wherever it is, and store it for later use. Note that writing
+    // to "$_REQUEST" here won't always work, because later code may rebuild
+    // "$_REQUEST" from other sources.
+
     if (isset($_REQUEST['__path__']) && strlen($_REQUEST['__path__'])) {
+      self::setRequestPath($_REQUEST['__path__']);
       return;
     }
 
+    // Compatibility with PHP 5.4+ built-in web server.
     if (php_sapi_name() == 'cli-server') {
-      // Compatibility with PHP 5.4+ built-in web server.
-      $url = parse_url($_SERVER['REQUEST_URI']);
-      $_REQUEST['__path__'] = $url['path'];
+      $path = parse_url($_SERVER['REQUEST_URI']);
+      self::setRequestPath($path['path']);
       return;
     }
 
@@ -580,6 +595,30 @@ final class PhabricatorStartup {
     }
   }
 
+  /**
+   * @task request-path
+   */
+  public static function getRequestPath() {
+    $path = self::$requestPath;
+
+    if ($path === null) {
+      self::didFatal(
+        'Request attempted to access request path, but no request path is '.
+        'available for this request. You may be calling web request code '.
+        'from a non-request context, or your webserver may not be passing '.
+        'a request path to Phabricator in a format that it understands.');
+    }
+
+    return $path;
+  }
+
+  /**
+   * @task request-path
+   */
+  public static function setRequestPath($path) {
+    self::$requestPath = $path;
+  }
+
 
 /* -(  Rate Limiting  )------------------------------------------------------ */
 

From 04c1f67a020ccd8cd1755e6a4df5d5d9aa670182 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Jan 2021 10:47:48 -0800
Subject: [PATCH 09/19] Add "M<digit>" and "P<digit>" to the default Remarkup
 ignore list

Summary: Ref T13575. Particularly with the new Apple silicon, I think there are enough domain collisions for `M1`, `M2`, `P1`, etc., to justify adding them to the default ignore list.

Test Plan: Created a mock, then wrote a comment referencing an object on the list (`M1`) and an object not on the list (`T1`). Got text and a link respectively.

Maniphest Tasks: T13575

Differential Revision: https://secure.phabricator.com/D21507
---
 .../option/PhabricatorCoreConfigOptions.php      | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/applications/config/option/PhabricatorCoreConfigOptions.php b/src/applications/config/option/PhabricatorCoreConfigOptions.php
index 10f3e75b62..77c48115a4 100644
--- a/src/applications/config/option/PhabricatorCoreConfigOptions.php
+++ b/src/applications/config/option/PhabricatorCoreConfigOptions.php
@@ -169,7 +169,21 @@ EOREMARKUP
             'Maniphest. If you\'d prefer more traditional UI strings like '.
             '"Add Comment", you can set this flag to disable most of the '.
             'extra flavor.')),
-      $this->newOption('remarkup.ignored-object-names', 'string', '/^(Q|V)\d$/')
+      $this->newOption(
+        'remarkup.ignored-object-names',
+        'string',
+
+        // Q1, Q2, etc., are common abbreviations for "Quarter".
+        // V1, V2, etc., are common abbreviations for "Version".
+        // P1, P2, etc., are common abbreviations for "Priority".
+
+        // M1 is a computer chip manufactured by Apple.
+        // M2 (commonly spelled "M.2") is an expansion slot on motherboards.
+        // M4 is a carbine.
+        // M8 is a phonetic spelling of "mate", used in culturally significant
+        // copypasta about navy seals.
+
+        '/^(Q|V|M|P)\d$/')
         ->setSummary(
           pht('Text values that match this regex and are also object names '.
           'will not be linked.'))

From c63c2aadef3a317c738fe23b6c73d6a81313e8f4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Jan 2021 07:48:15 -0800
Subject: [PATCH 10/19] Support "control" and "return/enter" in the remarkup
 rule for keystrokes

Summary: These characters are missing support in `{key ...}` but are reasonable to include.

Test Plan: {F8302969}

Differential Revision: https://secure.phabricator.com/D21508
---
 .../rule/PhabricatorKeyboardRemarkupRule.php     | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php b/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php
index cdbfea7b74..56bab68e44 100644
--- a/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php
+++ b/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php
@@ -61,6 +61,22 @@ final class PhabricatorKeyboardRemarkupRule extends PhutilRemarkupRule {
           'escape',
         ),
       ),
+      array(
+        'name' => pht('Enter'),
+        'symbol' => "\xE2\x8F\x8E",
+        'aliases' => array(
+          'enter',
+          'return',
+        ),
+      ),
+      array(
+        'name' => pht('Control'),
+        'symbol' => "\xE2\x8C\x83",
+        'aliases' => array(
+          'ctrl',
+          'control',
+        ),
+      ),
       array(
         'name' => pht('Up'),
         'symbol' => "\xE2\x86\x91",

From ea9cb0b625fb6922c45aecbfdebacc60788ed92d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Jan 2021 11:31:55 -0800
Subject: [PATCH 11/19] Disambiguate Git ref selectors in some Git command line
 invocations

Summary: Ref T13589. See that task for discussion.

Test Plan: Executed most commands via "bin/conduit" or in isolation.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21510
---
 .../DiffusionBrowseQueryConduitAPIMethod.php      | 15 ++++++++-------
 .../DiffusionExistsQueryConduitAPIMethod.php      |  2 +-
 .../DiffusionHistoryQueryConduitAPIMethod.php     |  2 +-
 ...DiffusionLastModifiedQueryConduitAPIMethod.php |  2 +-
 ...iffusionMergedCommitsQueryConduitAPIMethod.php |  8 ++++----
 .../DiffusionQueryPathsConduitAPIMethod.php       |  2 +-
 .../DiffusionSearchQueryConduitAPIMethod.php      |  2 +-
 .../query/blame/DiffusionGitBlameQuery.php        |  2 +-
 .../filecontent/DiffusionGitFileContentQuery.php  |  2 +-
 .../lowlevel/DiffusionLowLevelParentsQuery.php    |  4 ++--
 .../query/rawdiff/DiffusionGitRawDiffQuery.php    |  6 +++---
 11 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
index 0b6ae19e32..477a89c88b 100644
--- a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
@@ -48,7 +48,7 @@ final class DiffusionBrowseQueryConduitAPIMethod
     } else {
       try {
         list($stdout) = $repository->execxLocalCommand(
-          'cat-file -t %s:%s',
+          'cat-file -t -- %s:%s',
           $commit,
           $path);
       } catch (CommandException $e) {
@@ -62,7 +62,7 @@ final class DiffusionBrowseQueryConduitAPIMethod
 
         list($sub_err, $sub_stdout) = $repository->execLocalCommand(
           'ls-tree %s -- %s',
-          $commit,
+          gitsprintf('%s', $commit),
           $path);
         if (!$sub_err) {
           // If the path failed "cat-file" but "ls-tree" worked, we assume it
@@ -86,8 +86,9 @@ final class DiffusionBrowseQueryConduitAPIMethod
         if (preg_match('/^fatal: Not a valid object name/', $stderr)) {
           // Grab two logs, since the first one is when the object was deleted.
           list($stdout) = $repository->execxLocalCommand(
-            'log -n2 --format="%%H" %s -- %s',
-            $commit,
+            'log -n2 %s %s -- %s',
+            '--format=%H',
+            gitsprintf('%s', $commit),
             $path);
           $stdout = trim($stdout);
           if ($stdout) {
@@ -121,8 +122,8 @@ final class DiffusionBrowseQueryConduitAPIMethod
     }
 
     list($stdout) = $repository->execxLocalCommand(
-      'ls-tree -z -l %s:%s',
-      $commit,
+      'ls-tree -z -l %s -- %s',
+      gitsprintf('%s', $commit),
       $path);
 
     $submodules = array();
@@ -207,7 +208,7 @@ final class DiffusionBrowseQueryConduitAPIMethod
       // the wild.
 
       list($err, $contents) = $repository->execLocalCommand(
-        'cat-file blob %s:.gitmodules',
+        'cat-file blob -- %s:.gitmodules',
         $commit);
 
       if (!$err) {
diff --git a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
index 2d4a221171..7a7dbff598 100644
--- a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
@@ -25,7 +25,7 @@ final class DiffusionExistsQueryConduitAPIMethod
     $repository = $this->getDiffusionRequest()->getRepository();
     $commit = $request->getValue('commit');
     list($err, $merge_base) = $repository->execLocalCommand(
-      'cat-file -t %s',
+      'cat-file -t -- %s',
       $commit);
     return !$err;
   }
diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
index ebce21dd6f..223aef6a7d 100644
--- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
@@ -64,7 +64,7 @@ final class DiffusionHistoryQueryConduitAPIMethod
       $offset,
       $limit,
       '%H:%P',
-      $commit_range,
+      gitsprintf('%s', $commit_range),
       // Git omits merge commits if the path is provided, even if it is empty.
       (strlen($path) ? csprintf('%s', $path) : ''));
 
diff --git a/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php
index 1135420a6e..a15000bd97 100644
--- a/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php
@@ -34,7 +34,7 @@ final class DiffusionLastModifiedQueryConduitAPIMethod
       }
       list($hash) = $repository->execxLocalCommand(
         'log -n1 --format=%%H %s -- %s',
-        $commit,
+        gitsprintf('%s', $commit),
         $path);
       $results[$path] = trim($hash);
     }
diff --git a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
index 79587a2e5e..e02580dc73 100644
--- a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
@@ -35,9 +35,9 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
     $limit = $this->getLimit($request);
 
     list($parents) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s',
+      'log -n 1 --format=%s %s --',
       '%P',
-      $commit);
+      gitsprintf('%s', $commit));
 
     $parents = preg_split('/\s+/', trim($parents));
     if (count($parents) < 2) {
@@ -54,8 +54,8 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
       // NOTE: "+ 1" accounts for the merge commit itself.
       $limit + 1,
       '%H',
-      $commit,
-      '^'.$first_parent);
+      gitsprintf('%s', $commit),
+      gitsprintf('%s', '^'.$first_parent));
 
     $hashes = explode("\n", trim($logs));
 
diff --git a/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php
index 09c07ec28f..98cc006419 100644
--- a/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php
@@ -45,7 +45,7 @@ final class DiffusionQueryPathsConduitAPIMethod
 
     $future = $repository->getLocalCommandFuture(
       'ls-tree --name-only -r -z %s -- %s',
-      $commit,
+      gitsprintf('%s', $commit),
       $path);
 
     $lines = id(new LinesOfALargeExecFuture($future))->setDelimiter("\0");
diff --git a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
index e2c56bb0f8..ba4c824061 100644
--- a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
@@ -64,7 +64,7 @@ final class DiffusionSearchQueryConduitAPIMethod
     $future = $repository->getLocalCommandFuture(
       // NOTE: --perl-regexp is available only with libpcre compiled in.
       'grep --extended-regexp --null -n --no-color -f - %s -- %s',
-      $drequest->getStableCommit(),
+      gitsprintf('%s', $drequest->getStableCommit()),
       $path);
 
     // NOTE: We're writing the pattern on stdin to avoid issues with UTF8
diff --git a/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php b/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php
index 0eb15cd018..3525546111 100644
--- a/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php
+++ b/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php
@@ -13,7 +13,7 @@ final class DiffusionGitBlameQuery extends DiffusionBlameQuery {
 
     return $repository->getLocalCommandFuture(
       '--no-pager blame --root -s -l %s -- %s',
-      $commit,
+      gitsprintf('%s', $commit),
       $path);
   }
 
diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php
index e0fb8bd9ee..8c8e362e60 100644
--- a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php
+++ b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php
@@ -10,7 +10,7 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
     $commit = $drequest->getCommit();
 
     return $repository->getLocalCommandFuture(
-      'cat-file blob %s:%s',
+      'cat-file blob -- %s:%s',
       $commit,
       $path);
   }
diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
index a2673b0ce2..e0d9ef14b6 100644
--- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
+++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
@@ -37,9 +37,9 @@ final class DiffusionLowLevelParentsQuery
     $repository = $this->getRepository();
 
     list($stdout) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s',
+      'log -n 1 --format=%s %s --',
       '%P',
-      $this->identifier);
+      gitsprintf('%s', $this->identifier));
 
     return preg_split('/\s+/', trim($stdout));
   }
diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
index 41d91c00ca..f535724093 100644
--- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
+++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
@@ -25,7 +25,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
       list($parents) = $repository->execxLocalCommand(
         'log -n 1 --format=%s %s --',
         '%P',
-        $commit);
+        gitsprintf('%s', $commit));
 
       if (strlen(trim($parents))) {
         $against = $commit.'^';
@@ -42,8 +42,8 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
     return $repository->getLocalCommandFuture(
       'diff %Ls %s %s -- %s',
       $options,
-      $against,
-      $commit,
+      gitsprintf('%s', $against),
+      gitsprintf('%s', $commit),
       $path);
   }
 

From 0e28105ff76bf3960d35e204476c22d81414e653 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 Jan 2021 10:49:41 -0800
Subject: [PATCH 12/19] Further correct and disambigutate ref selectors passed
 to Git on the CLI

Summary:
Ref T13589. In D21510, not every ref selector got touched, and this isn't a valid construction in Git:

```
$ git ls-tree ... -- ''
```

Thus:

  - Disambiguate more (all?) ref selectors.
  - Correct the construction of "git ls-tree" when there is no path.
  - Clean some stuff up: make the construction of some flags and arguments more explicit, get rid of a needless "%C", prefer "%Ls" over acrobatics, etc.

Test Plan: Browsed/updated a local Git repository. (This change is somewhat difficult to test exhaustively, as evidenced by the "ls-tree" issue in D21510.)

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21511
---
 .../DiffusionBrowseQueryConduitAPIMethod.php  | 32 +++++++++++------
 .../DiffusionHistoryQueryConduitAPIMethod.php | 36 +++++++++++++------
 ...nternalGitRawDiffQueryConduitAPIMethod.php | 16 ++++-----
 ...usionLastModifiedQueryConduitAPIMethod.php |  3 +-
 ...sionMergedCommitsQueryConduitAPIMethod.php |  8 ++---
 .../DiffusionRefsQueryConduitAPIMethod.php    |  6 ++--
 .../engine/DiffusionCommitHookEngine.php      |  6 ++--
 .../DiffusionGitFileContentQuery.php          |  5 ++-
 .../lowlevel/DiffusionLowLevelCommitQuery.php | 26 +++++++++-----
 .../DiffusionLowLevelFilesizeQuery.php        |  2 +-
 .../DiffusionLowLevelParentsQuery.php         |  4 +--
 .../rawdiff/DiffusionGitRawDiffQuery.php      |  4 +--
 .../daemon/PhabricatorGitGraphStream.php      | 10 +++---
 .../engine/PhabricatorRepositoryRefEngine.php | 10 +++---
 ...habricatorWorkingCopyDiscoveryTestCase.php |  2 ++
 15 files changed, 103 insertions(+), 67 deletions(-)

diff --git a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
index 477a89c88b..49a34ac250 100644
--- a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
@@ -35,22 +35,26 @@ final class DiffusionBrowseQueryConduitAPIMethod
   protected function getGitResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
+
     $path = $request->getValue('path');
+    if (!strlen($path)) {
+      $path = null;
+    }
+
     $commit = $request->getValue('commit');
     $offset = (int)$request->getValue('offset');
     $limit = (int)$request->getValue('limit');
     $result = $this->getEmptyResultSet();
 
-    if ($path == '') {
+    if ($path === null) {
       // Fast path to improve the performance of the repository view; we know
       // the root is always a tree at any commit and always exists.
       $stdout = 'tree';
     } else {
       try {
         list($stdout) = $repository->execxLocalCommand(
-          'cat-file -t -- %s:%s',
-          $commit,
-          $path);
+          'cat-file -t -- %s',
+          sprintf('%s:%s', $commit, $path));
       } catch (CommandException $e) {
         // The "cat-file" command may fail if the path legitimately does not
         // exist, but it may also fail if the path is a submodule. This can
@@ -121,14 +125,20 @@ final class DiffusionBrowseQueryConduitAPIMethod
       return $result;
     }
 
-    list($stdout) = $repository->execxLocalCommand(
-      'ls-tree -z -l %s -- %s',
-      gitsprintf('%s', $commit),
-      $path);
+    if ($path === null) {
+      list($stdout) = $repository->execxLocalCommand(
+        'ls-tree -z -l %s --',
+        gitsprintf('%s', $commit));
+    } else {
+      list($stdout) = $repository->execxLocalCommand(
+        'ls-tree -z -l %s -- %s',
+        gitsprintf('%s', $commit),
+        $path);
+    }
 
     $submodules = array();
 
-    if (strlen($path)) {
+    if ($path !== null) {
       $prefix = rtrim($path, '/').'/';
     } else {
       $prefix = '';
@@ -226,8 +236,8 @@ final class DiffusionBrowseQueryConduitAPIMethod
           $dict[$key] = $value;
         }
 
-        foreach ($submodules as $path) {
-          $full_path = $path->getFullPath();
+        foreach ($submodules as $submodule_path) {
+          $full_path = $submodule_path->getFullPath();
           $key = 'submodule.'.$full_path.'.url';
           if (isset($dict[$key])) {
             $path->setExternalURI($dict[$key]);
diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
index 223aef6a7d..8c53ff0555 100644
--- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
@@ -45,7 +45,12 @@ final class DiffusionHistoryQueryConduitAPIMethod
     $repository = $drequest->getRepository();
     $commit_hash = $request->getValue('commit');
     $against_hash = $request->getValue('against');
+
     $path = $request->getValue('path');
+    if (!strlen($path)) {
+      $path = null;
+    }
+
     $offset = $request->getValue('offset');
     $limit = $request->getValue('limit');
 
@@ -55,18 +60,27 @@ final class DiffusionHistoryQueryConduitAPIMethod
       $commit_range = $commit_hash;
     }
 
+    $argv = array();
+
+    $argv[] = '--skip';
+    $argv[] = $offset;
+
+    $argv[] = '--max-count';
+    $argv[] = $limit;
+
+    $argv[] = '--format=%H:%P';
+
+    $argv[] = gitsprintf('%s', $commit_range);
+
+    $argv[] = '--';
+
+    if ($path !== null) {
+      $argv[] = $path;
+    }
+
     list($stdout) = $repository->execxLocalCommand(
-      'log '.
-        '--skip=%d '.
-        '-n %d '.
-        '--pretty=format:%s '.
-        '%s -- %C',
-      $offset,
-      $limit,
-      '%H:%P',
-      gitsprintf('%s', $commit_range),
-      // Git omits merge commits if the path is provided, even if it is empty.
-      (strlen($path) ? csprintf('%s', $path) : ''));
+      'log %Ls',
+      $argv);
 
     $lines = explode("\n", trim($stdout));
     $lines = array_filter($lines);
diff --git a/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php
index ac241a282f..ea94ff71c4 100644
--- a/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php
@@ -43,9 +43,9 @@ final class DiffusionInternalGitRawDiffQueryConduitAPIMethod
     // it requires the commit to have a parent that we can diff against. The
     // first commit doesn't, so "commit^" is not a valid ref.
     list($parents) = $repository->execxLocalCommand(
-      'log -n1 --format=%s %s',
-      '%P',
-      $commit);
+      'log -n1 %s %s --',
+      '--format=%P',
+      gitsprintf('%s', $commit));
     $use_log = !strlen(trim($parents));
 
     // First, get a fast raw diff without "--find-copies-harder". This flag
@@ -96,18 +96,18 @@ final class DiffusionInternalGitRawDiffQueryConduitAPIMethod
       // NOTE: "--pretty=format: " is to disable diff output, we only want the
       // part we get from "--raw".
       $future = $repository->getLocalCommandFuture(
-        'log %Ls --pretty=format: %s',
+        'log %Ls --pretty=format: %s --',
         $flags,
-        $commit);
+        gitsprintf('%s', $commit));
     } else {
       // Otherwise, we can use "diff", which will give us output for merges.
       // We diff against the first parent, as this is generally the expectation
       // and results in sensible behavior.
       $future = $repository->getLocalCommandFuture(
-        'diff %Ls %s^1 %s',
+        'diff %Ls %s %s --',
         $flags,
-        $commit,
-        $commit);
+        gitsprintf('%s^1', $commit),
+        gitsprintf('%s', $commit));
     }
 
     // Don't spend more than 30 seconds generating the slower output.
diff --git a/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php
index a15000bd97..8d404c656a 100644
--- a/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php
@@ -33,7 +33,8 @@ final class DiffusionLastModifiedQueryConduitAPIMethod
         continue;
       }
       list($hash) = $repository->execxLocalCommand(
-        'log -n1 --format=%%H %s -- %s',
+        'log -n1 %s %s -- %s',
+        '--format=%H',
         gitsprintf('%s', $commit),
         $path);
       $results[$path] = trim($hash);
diff --git a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
index e02580dc73..b3851802fc 100644
--- a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
@@ -35,8 +35,8 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
     $limit = $this->getLimit($request);
 
     list($parents) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s --',
-      '%P',
+      'log -n 1 %s %s --',
+      '--format=%P',
       gitsprintf('%s', $commit));
 
     $parents = preg_split('/\s+/', trim($parents));
@@ -50,10 +50,10 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
 
     $first_parent = head($parents);
     list($logs) = $repository->execxLocalCommand(
-      'log -n %d --format=%s %s %s --',
+      'log -n %d %s %s %s --',
       // NOTE: "+ 1" accounts for the merge commit itself.
       $limit + 1,
-      '%H',
+      '--format=%H',
       gitsprintf('%s', $commit),
       gitsprintf('%s', '^'.$first_parent));
 
diff --git a/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php
index 00bb28b385..8bdd2d8571 100644
--- a/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php
@@ -28,9 +28,9 @@ final class DiffusionRefsQueryConduitAPIMethod
     $commit = $request->getValue('commit');
 
     list($stdout) = $repository->execxLocalCommand(
-      'log --format=%s -n 1 %s --',
-      '%d',
-      $commit);
+      'log -n 1 %s %s --',
+      '--format=%d',
+      gitsprintf('%s', $commit));
 
     // %d, gives a weird output format
     // similar to (remote/one, remote/two, remote/three)
diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
index 6ebaf6bdc7..0f6b05b578 100644
--- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
+++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php
@@ -608,9 +608,9 @@ final class DiffusionCommitHookEngine extends Phobject {
       // repository. Particularly, this will cover the cases of a new branch, a
       // completely moved tag, etc.
       $futures[$key] = $this->getRepository()->getLocalCommandFuture(
-        'log --format=%s %s --not --all',
-        '%H',
-        $ref_update->getRefNew());
+        'log %s %s --not --all --',
+        '--format=%H',
+        gitsprintf('%s', $ref_update->getRefNew()));
     }
 
     $content_updates = array();
diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php
index 8c8e362e60..f450a8adeb 100644
--- a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php
+++ b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php
@@ -10,9 +10,8 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
     $commit = $drequest->getCommit();
 
     return $repository->getLocalCommandFuture(
-      'cat-file blob -- %s:%s',
-      $commit,
-      $path);
+      'cat-file blob -- %s',
+      sprintf('%s:%s', $commit, $path));
   }
 
 }
diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php
index f4c31f7c24..8b9233f128 100644
--- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php
+++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php
@@ -55,12 +55,15 @@ final class DiffusionLowLevelCommitQuery
       $split_body = true;
     }
 
-    // Even though we pass --encoding here, git doesn't always succeed, so
-    // we try a little harder, since git *does* tell us what the actual encoding
-    // is correctly (unless it doesn't; encoding is sometimes empty).
-    list($info) = $repository->execxLocalCommand(
-      'log -n 1 --encoding=%s --format=%s %s --',
-      'UTF-8',
+    $argv = array();
+
+    $argv[] = '-n';
+    $argv[] = '1';
+
+    $argv[] = '--encoding=UTF-8';
+
+    $argv[] = sprintf(
+      '--format=%s',
       implode(
         '%x00',
         array(
@@ -78,8 +81,15 @@ final class DiffusionLowLevelCommitQuery
           // so include an explicit terminator: this makes sure the exact
           // body text is surrounded by "\0" characters.
           '~',
-        )),
-      $this->identifier);
+        )));
+
+    // Even though we pass --encoding here, git doesn't always succeed, so
+    // we try a little harder, since git *does* tell us what the actual encoding
+    // is correctly (unless it doesn't; encoding is sometimes empty).
+    list($info) = $repository->execxLocalCommand(
+      'log -n 1 %Ls %s --',
+      $argv,
+      gitsprintf('%s', $this->identifier));
 
     $parts = explode("\0", $info);
     $encoding = array_shift($parts);
diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php
index 9d97a134ca..8984003711 100644
--- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php
+++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php
@@ -33,7 +33,7 @@ final class DiffusionLowLevelFilesizeQuery
 
     $paths_future = $repository->getLocalCommandFuture(
       'diff-tree -z -r --no-commit-id %s --',
-      $identifier);
+      gitsprintf('%s', $identifier));
 
     // With "-z" we get "<fields>\0<filename>\0" for each line. Process the
     // delimited text as "<fields>, <filename>" pairs.
diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
index e0d9ef14b6..193f45a029 100644
--- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
+++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
@@ -37,8 +37,8 @@ final class DiffusionLowLevelParentsQuery
     $repository = $this->getRepository();
 
     list($stdout) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s --',
-      '%P',
+      'log -n 1 %s %s --',
+      '--format=%P',
       gitsprintf('%s', $this->identifier));
 
     return preg_split('/\s+/', trim($stdout));
diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
index f535724093..5983b2723e 100644
--- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
+++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
@@ -23,8 +23,8 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
       // Check if this is the root commit by seeing if it has parents, since
       // `git diff X^ X` does not work if "X" is the initial commit.
       list($parents) = $repository->execxLocalCommand(
-        'log -n 1 --format=%s %s --',
-        '%P',
+        'log -n 1 %s --',
+        '--format=%P',
         gitsprintf('%s', $commit));
 
       if (strlen(trim($parents))) {
diff --git a/src/applications/repository/daemon/PhabricatorGitGraphStream.php b/src/applications/repository/daemon/PhabricatorGitGraphStream.php
index b175053528..39b1fe39ed 100644
--- a/src/applications/repository/daemon/PhabricatorGitGraphStream.php
+++ b/src/applications/repository/daemon/PhabricatorGitGraphStream.php
@@ -19,13 +19,13 @@ final class PhabricatorGitGraphStream
 
     if ($start_commit !== null) {
       $future = $repository->getLocalCommandFuture(
-        'log --format=%s %s --',
-        '%H%x01%P%x01%ct',
-        $start_commit);
+        'log %s %s --',
+        '--format=%H%x01%P%x01%ct',
+        gitsprintf('%s', $start_commit));
     } else {
       $future = $repository->getLocalCommandFuture(
-        'log --format=%s --all --',
-        '%H%x01%P%x01%ct');
+        'log %s --all --',
+        '--format=%H%x01%P%x01%ct');
     }
 
     $this->iterator = new LinesOfALargeExecFuture($future);
diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
index a1b7ae5f55..0166e8e53c 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
@@ -483,17 +483,17 @@ final class PhabricatorRepositoryRefEngine
           $ref_list = implode("\n", $ref_list)."\n";
 
           $future = $this->getRepository()->getLocalCommandFuture(
-            'log --format=%s --stdin',
-            '%H');
+            'log %s --stdin --',
+            '--format=%H');
 
           list($stdout) = $future
             ->write($ref_list)
             ->resolvex();
         } else {
           list($stdout) = $this->getRepository()->execxLocalCommand(
-            'log --format=%s %s',
-            '%H',
-            $new_head);
+            'log %s %s --',
+            '--format=%H',
+            gitsprintf('%s', $new_head));
         }
 
         $stdout = trim($stdout);
diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php
index a15cb3d79f..cc94e558e4 100644
--- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php
+++ b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php
@@ -4,6 +4,8 @@ final class PhabricatorWorkingCopyDiscoveryTestCase
   extends PhabricatorWorkingCopyTestCase {
 
   public function testSubversionCommitDiscovery() {
+    $this->requireBinaryForTest('svn');
+
     $refs = $this->discoverRefs('ST');
     $this->assertEqual(
       array(

From e7e8ef7e390a8a777087974c6e1a281c107ca565 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 Jan 2021 15:02:06 -0800
Subject: [PATCH 13/19] Correct a straggling CLI format string after ref
 selector changes

Summary: Ref T13589. This is missing a "%s" conversion.

Test Plan: Will view a commit with a diff.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21512
---
 .../diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php        | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
index 5983b2723e..9c894437ef 100644
--- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
+++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php
@@ -23,7 +23,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
       // Check if this is the root commit by seeing if it has parents, since
       // `git diff X^ X` does not work if "X" is the initial commit.
       list($parents) = $repository->execxLocalCommand(
-        'log -n 1 %s --',
+        'log -n 1 %s %s --',
         '--format=%P',
         gitsprintf('%s', $commit));
 

From 16a14af2bb17dc37fcfb2035fac40ee6017330e5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Jan 2021 12:22:50 -0800
Subject: [PATCH 14/19] Correct the behavior of "bin/repository discover
 --repair"

Summary:
Ref T13591. Since D8781, this flag does not function correctly in Git and Mercurial repositories, since ref discovery pre-fills the cache.

Move the "don't look at the database" behavior the flag enables into the cache lookup. D8781 should have been slightly more aggressive and done this, it was just overlooked.

Test Plan:
  - Ran `bin/repository discover --help` and read the updated help text.
  - Ran `bin/repository discover --repair` in a fully-discovered Git repository.
    - Before: no effect.
    - After: full rediscovery.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21513
---
 .../PhabricatorRepositoryDiscoveryEngine.php    | 14 +++++++-------
 ...atorRepositoryManagementDiscoverWorkflow.php | 17 +++++++++--------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
index c3e4169b53..8ac52b0b57 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
@@ -459,13 +459,6 @@ final class PhabricatorRepositoryDiscoveryEngine
       return true;
     }
 
-    if ($this->repairMode) {
-      // In repair mode, rediscover the entire repository, ignoring the
-      // database state. We can hit the local cache above, but if we miss it
-      // stop the script from going to the database cache.
-      return false;
-    }
-
     $this->fillCommitCache(array($identifier));
 
     return isset($this->commitCache[$identifier]);
@@ -476,6 +469,13 @@ final class PhabricatorRepositoryDiscoveryEngine
       return;
     }
 
+    if ($this->repairMode) {
+      // In repair mode, rediscover the entire repository, ignoring the
+      // database state. The engine still maintains a local cache (the
+      // "Working Set") but we just give up before looking in the database.
+      return;
+    }
+
     $max_size = self::MAX_COMMIT_CACHE_SIZE;
 
     // If we're filling more identifiers than would fit in the cache, ignore
diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php
index c534edf907..eb9437dbf9 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php
@@ -7,21 +7,22 @@ final class PhabricatorRepositoryManagementDiscoverWorkflow
     $this
       ->setName('discover')
       ->setExamples('**discover** [__options__] __repository__ ...')
-      ->setSynopsis(pht('Discover __repository__.'))
+      ->setSynopsis(pht('Discover commits in __repository__.'))
       ->setArguments(
         array(
           array(
-            'name'        => 'verbose',
-            'help'        => pht('Show additional debugging information.'),
+            'name' => 'verbose',
+            'help' => pht('Show additional debugging information.'),
           ),
           array(
-            'name'        => 'repair',
-            'help'        => pht(
-              'Repair a repository with gaps in commit history.'),
+            'name' => 'repair',
+            'help' => pht(
+              'Discover all commits, even if they are ancestors of known '.
+              'commits. This can repair gaps in repository history.'),
           ),
           array(
-            'name'        => 'repos',
-            'wildcard'    => true,
+            'name' => 'repos',
+            'wildcard' => true,
           ),
         ));
   }

From 2d0e7c37e1f599111ea199d502d236788a63341f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Jan 2021 13:32:37 -0800
Subject: [PATCH 15/19] Rename "IMPORTED_CLOSEABLE" to "IMPORTED_PERMANENT" to
 clarify the meaning of the flag

Summary:
Ref T13591. This is an old flag with an old name, and there's an import bug because the outdated concept of "closable" is confusing two different behaviors.

This flag should mean only "is this commit reachable from a permanent ref?". Rename it to "IMPORTED_PERMANENT" to make that more clear.

Rename the "Unpublished" query to "Permanent" to make that more clear, as well.

Test Plan:
  - Grepped for all affected symbols.
  - Queried for all commmits, permament commits, and impermanent commits.
  - Ran repository discovery.
  - See also further changes in this change series for more extensive tests.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21514
---
 .../query/PhabricatorCommitSearchEngine.php   | 14 +++++-----
 .../diffusion/query/DiffusionCommitQuery.php  | 24 ++++++++---------
 .../engine/PhabricatorRepositoryCommitRef.php | 10 +++----
 .../PhabricatorRepositoryDiscoveryEngine.php  | 20 +++++++-------
 .../engine/PhabricatorRepositoryRefEngine.php | 26 +++++++++----------
 .../storage/PhabricatorRepositoryCommit.php   |  4 +--
 6 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
index 37e8d563b4..8b7751c795 100644
--- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php
+++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
@@ -54,8 +54,8 @@ final class PhabricatorCommitSearchEngine
       $query->withUnreachable($map['unreachable']);
     }
 
-    if ($map['unpublished'] !== null) {
-      $query->withUnpublished($map['unpublished']);
+    if ($map['permanent'] !== null) {
+      $query->withPermanent($map['permanent']);
     }
 
     if ($map['ancestorsOf']) {
@@ -132,15 +132,15 @@ final class PhabricatorCommitSearchEngine
             'Find or exclude unreachable commits which are not ancestors of '.
             'any branch, tag, or ref.')),
       id(new PhabricatorSearchThreeStateField())
-        ->setLabel(pht('Unpublished'))
-        ->setKey('unpublished')
+        ->setLabel(pht('Permanent'))
+        ->setKey('permanent')
         ->setOptions(
           pht('(Show All)'),
-          pht('Show Only Unpublished Commits'),
-          pht('Hide Unpublished Commits'))
+          pht('Show Only Permanent Commits'),
+          pht('Hide Permanent Commits'))
         ->setDescription(
           pht(
-            'Find or exclude unpublished commits which are not ancestors of '.
+            'Find or exclude permanent commits which are ancestors of '.
             'any permanent branch, tag, or ref.')),
       id(new PhabricatorSearchStringListField())
         ->setLabel(pht('Ancestors Of'))
diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 2854aecbdd..7b4e80bfec 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -15,7 +15,7 @@ final class DiffusionCommitQuery
   private $statuses;
   private $packagePHIDs;
   private $unreachable;
-  private $unpublished;
+  private $permanent;
 
   private $needAuditRequests;
   private $needAuditAuthority;
@@ -154,8 +154,8 @@ final class DiffusionCommitQuery
     return $this;
   }
 
-  public function withUnpublished($unpublished) {
-    $this->unpublished = $unpublished;
+  public function withPermanent($permanent) {
+    $this->permanent = $permanent;
     return $this;
   }
 
@@ -859,18 +859,18 @@ final class DiffusionCommitQuery
       }
     }
 
-    if ($this->unpublished !== null) {
-      if ($this->unpublished) {
-        $where[] = qsprintf(
-          $conn,
-          '(commit.importStatus & %d) = 0',
-          PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE);
-      } else {
+    if ($this->permanent !== null) {
+      if ($this->permanent) {
         $where[] = qsprintf(
           $conn,
           '(commit.importStatus & %d) = %d',
-          PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE,
-          PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE);
+          PhabricatorRepositoryCommit::IMPORTED_PERMANENT,
+          PhabricatorRepositoryCommit::IMPORTED_PERMANENT);
+      } else {
+        $where[] = qsprintf(
+          $conn,
+          '(commit.importStatus & %d) = 0',
+          PhabricatorRepositoryCommit::IMPORTED_PERMANENT);
       }
     }
 
diff --git a/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php b/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php
index 31c859d5d8..148b99f3e0 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php
@@ -5,7 +5,7 @@ final class PhabricatorRepositoryCommitRef extends Phobject {
   private $identifier;
   private $epoch;
   private $branch;
-  private $canCloseImmediately;
+  private $isPermanent;
   private $parents = array();
 
   public function setIdentifier($identifier) {
@@ -35,13 +35,13 @@ final class PhabricatorRepositoryCommitRef extends Phobject {
     return $this->branch;
   }
 
-  public function setCanCloseImmediately($can_close_immediately) {
-    $this->canCloseImmediately = $can_close_immediately;
+  public function setIsPermanent($is_permanent) {
+    $this->isPermanent = $is_permanent;
     return $this;
   }
 
-  public function getCanCloseImmediately() {
-    return $this->canCloseImmediately;
+  public function getIsPermanent() {
+    return $this->isPermanent;
   }
 
   public function setParents(array $parents) {
diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
index 8ac52b0b57..1c27e38e99 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
@@ -101,7 +101,7 @@ final class PhabricatorRepositoryDiscoveryEngine
         $repository,
         $ref->getIdentifier(),
         $ref->getEpoch(),
-        $ref->getCanCloseImmediately(),
+        $ref->getIsPermanent(),
         $ref->getParents(),
         $task_priority);
 
@@ -250,7 +250,7 @@ final class PhabricatorRepositoryDiscoveryEngine
         $refs[$identifier] = id(new PhabricatorRepositoryCommitRef())
           ->setIdentifier($identifier)
           ->setEpoch($epoch)
-          ->setCanCloseImmediately(true);
+          ->setIsPermanent(true);
 
         if ($upper_bound === null) {
           $upper_bound = $identifier;
@@ -354,7 +354,7 @@ final class PhabricatorRepositoryDiscoveryEngine
       $branch_refs = $this->discoverStreamAncestry(
         new PhabricatorMercurialGraphStream($repository, $commit),
         $commit,
-        $close_immediately = true);
+        $is_permanent = true);
 
       $this->didDiscoverRefs($branch_refs);
 
@@ -371,7 +371,7 @@ final class PhabricatorRepositoryDiscoveryEngine
   private function discoverStreamAncestry(
     PhabricatorRepositoryGraphStream $stream,
     $commit,
-    $close_immediately) {
+    $is_permanent) {
 
     $discover = array($commit);
     $graph = array();
@@ -424,7 +424,7 @@ final class PhabricatorRepositoryDiscoveryEngine
       $refs[] = id(new PhabricatorRepositoryCommitRef())
         ->setIdentifier($commit)
         ->setEpoch($epoch)
-        ->setCanCloseImmediately($close_immediately)
+        ->setIsPermanent($is_permanent)
         ->setParents($stream->getParents($commit));
     }
 
@@ -507,8 +507,8 @@ final class PhabricatorRepositoryDiscoveryEngine
   }
 
   /**
-   * Sort branches so we process closeable branches first. This makes the
-   * whole import process a little cheaper, since we can close these commits
+   * Sort branches so we process permanent branches first. This makes the
+   * whole import process a little cheaper, since we can publish these commits
    * the first time through rather than catching them in the refs step.
    *
    * @task internal
@@ -538,7 +538,7 @@ final class PhabricatorRepositoryDiscoveryEngine
     PhabricatorRepository $repository,
     $commit_identifier,
     $epoch,
-    $close_immediately,
+    $is_permanent,
     array $parents,
     $task_priority) {
 
@@ -570,8 +570,8 @@ final class PhabricatorRepositoryDiscoveryEngine
     $commit->setRepositoryID($repository->getID());
     $commit->setCommitIdentifier($commit_identifier);
     $commit->setEpoch($epoch);
-    if ($close_immediately) {
-      $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE);
+    if ($is_permanent) {
+      $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_PERMANENT);
     }
 
     $data = new PhabricatorRepositoryCommitData();
diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
index 0166e8e53c..18dbf754f9 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
@@ -9,7 +9,7 @@ final class PhabricatorRepositoryRefEngine
 
   private $newPositions = array();
   private $deadPositions = array();
-  private $closeCommits = array();
+  private $permanentCommits = array();
   private $rebuild;
 
   public function setRebuild($rebuild) {
@@ -24,7 +24,7 @@ final class PhabricatorRepositoryRefEngine
   public function updateRefs() {
     $this->newPositions = array();
     $this->deadPositions = array();
-    $this->closeCommits = array();
+    $this->permanentCommits = array();
 
     $repository = $this->getRepository();
     $viewer = $this->getViewer();
@@ -96,8 +96,8 @@ final class PhabricatorRepositoryRefEngine
       $this->updateCursors($cursor_group, $refs, $type, $all_closing_heads);
     }
 
-    if ($this->closeCommits) {
-      $this->setCloseFlagOnCommits($this->closeCommits);
+    if ($this->permanentCommits) {
+      $this->setPermanentFlagOnCommits($this->permanentCommits);
     }
 
     $save_cursors = $this->getCursorsForUpdate($all_cursors);
@@ -217,9 +217,9 @@ final class PhabricatorRepositoryRefEngine
     return $this;
   }
 
-  private function markCloseCommits(array $identifiers) {
+  private function markPermanentCommits(array $identifiers) {
     foreach ($identifiers as $identifier) {
-      $this->closeCommits[$identifier] = $identifier;
+      $this->permanentCommits[$identifier] = $identifier;
     }
     return $this;
   }
@@ -377,7 +377,7 @@ final class PhabricatorRepositoryRefEngine
             $identifier,
             $exclude);
 
-          $this->markCloseCommits($new_identifiers);
+          $this->markPermanentCommits($new_identifiers);
         }
       }
     }
@@ -507,10 +507,10 @@ final class PhabricatorRepositoryRefEngine
   }
 
   /**
-   * Mark a list of commits as closeable, and queue workers for those commits
+   * Mark a list of commits as permanent, and queue workers for those commits
    * which don't already have the flag.
    */
-  private function setCloseFlagOnCommits(array $identifiers) {
+  private function setPermanentFlagOnCommits(array $identifiers) {
     $repository = $this->getRepository();
     $commit_table = new PhabricatorRepositoryCommit();
     $conn = $commit_table->establishConnection('w');
@@ -552,7 +552,7 @@ final class PhabricatorRepositoryRefEngine
       }
     }
 
-    $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE;
+    $permanent_flag = PhabricatorRepositoryCommit::IMPORTED_PERMANENT;
     $published_flag = PhabricatorRepositoryCommit::IMPORTED_PUBLISH;
 
     $all_commits = ipull($all_commits, null, 'commitIdentifier');
@@ -568,9 +568,9 @@ final class PhabricatorRepositoryRefEngine
       }
 
       $import_status = $row['importStatus'];
-      if (!($import_status & $closeable_flag)) {
-        // Set the "closeable" flag.
-        $import_status = ($import_status | $closeable_flag);
+      if (!($import_status & $permanent_flag)) {
+        // Set the "permanent" flag.
+        $import_status = ($import_status | $permanent_flag);
 
         // See T13580. Clear the "published" flag, so publishing executes
         // again. We may have previously performed a no-op "publish" on the
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 586f1e2d0a..5fb089953d 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -36,7 +36,7 @@ final class PhabricatorRepositoryCommit
   const IMPORTED_PUBLISH = 8;
   const IMPORTED_ALL = 11;
 
-  const IMPORTED_CLOSEABLE = 1024;
+  const IMPORTED_PERMANENT = 1024;
   const IMPORTED_UNREACHABLE = 2048;
 
   private $commitData = self::ATTACHABLE;
@@ -467,7 +467,7 @@ final class PhabricatorRepositoryCommit
   }
 
   public function isPermanentCommit() {
-    return (bool)$this->isPartiallyImported(self::IMPORTED_CLOSEABLE);
+    return (bool)$this->isPartiallyImported(self::IMPORTED_PERMANENT);
   }
 
   public function newCommitAuthorView(PhabricatorUser $viewer) {

From 6716d4f6ae17ca606799d9a89bedc3929828a15b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Jan 2021 13:53:52 -0800
Subject: [PATCH 16/19] Separate "shouldPublishRef()" from "isPermanentRef()"
 and set "IMPORTED_PERMANENT" more narrowly

Summary:
Ref T13591. Currently, the "IMPORTED_PERMANENT" flag (previously "IMPORTED_CLOSEABLE", until D21514) flag is set by using the result of "shouldPublishRef()".

This method returns the wrong value for the flag when there is a repository-level reason not to publish the ref (most commonly, because the repository is currently importing).

Although it's correct that commits should not be published in an importing repository, that's already handled in the "PublishWorker" by testing "shouldPublishCommit()". The "IMPORTED_PERMANENT" flag should only reflect whether a commit is reachable from a permanent ref or not.

  - Move the relevant logic to a new method in Publisher.
  - Fill "IMPORTED_PERMANENT" narrowly from "isPermanentRef()", rather than broadly from "shouldPublishRef()".
  - Deduplicate some logic in "PhabricatorRepositoryRefEngine" which has the same intent as the logic in the Publisher.

Test Plan:
  - Ran discovery on a new repository, saw permanent commits marked as permanent from the beginning.
  - See later changes in this patch series for additional testing.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21515
---
 .../PhabricatorRepositoryDiscoveryEngine.php  | 10 ++---
 .../engine/PhabricatorRepositoryRefEngine.php | 26 ++++++-------
 .../query/PhabricatorRepositoryPublisher.php  | 39 +++++++++++++------
 .../PhabricatorRepositoryRefCursor.php        |  6 +++
 4 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
index 1c27e38e99..dedb9de93a 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
@@ -189,7 +189,7 @@ final class PhabricatorRepositoryDiscoveryEngine
       $head_refs = $this->discoverStreamAncestry(
         new PhabricatorGitGraphStream($repository, $commit),
         $commit,
-        $publisher->shouldPublishRef($ref));
+        $publisher->isPermanentRef($ref));
 
       $this->didDiscoverRefs($head_refs);
 
@@ -507,9 +507,9 @@ final class PhabricatorRepositoryDiscoveryEngine
   }
 
   /**
-   * Sort branches so we process permanent branches first. This makes the
-   * whole import process a little cheaper, since we can publish these commits
-   * the first time through rather than catching them in the refs step.
+   * Sort refs so we process permanent refs first. This makes the whole import
+   * process a little cheaper, since we can publish these commits the first
+   * time through rather than catching them in the refs step.
    *
    * @task internal
    *
@@ -523,7 +523,7 @@ final class PhabricatorRepositoryDiscoveryEngine
     $head_refs = array();
     $tail_refs = array();
     foreach ($refs as $ref) {
-      if ($publisher->shouldPublishRef($ref)) {
+      if ($publisher->isPermanentRef($ref)) {
         $head_refs[] = $ref;
       } else {
         $tail_refs[] = $ref;
diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
index 18dbf754f9..390e6e569d 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
@@ -100,7 +100,7 @@ final class PhabricatorRepositoryRefEngine
       $this->setPermanentFlagOnCommits($this->permanentCommits);
     }
 
-    $save_cursors = $this->getCursorsForUpdate($all_cursors);
+    $save_cursors = $this->getCursorsForUpdate($repository, $all_cursors);
 
     if ($this->newPositions || $this->deadPositions || $save_cursors) {
       $repository->openTransaction();
@@ -121,17 +121,19 @@ final class PhabricatorRepositoryRefEngine
     }
   }
 
-  private function getCursorsForUpdate(array $cursors) {
+  private function getCursorsForUpdate(
+    PhabricatorRepository $repository,
+    array $cursors) {
     assert_instances_of($cursors, 'PhabricatorRepositoryRefCursor');
 
+    $publisher = $repository->newPublisher();
+
     $results = array();
 
     foreach ($cursors as $cursor) {
-      $ref_type = $cursor->getRefType();
-      $ref_name = $cursor->getRefName();
-
-      $is_permanent = $this->isPermanentRef($ref_type, $ref_name);
+      $diffusion_ref = $cursor->newDiffusionRepositoryRef();
 
+      $is_permanent = $publisher->isPermanentRef($diffusion_ref);
       if ($is_permanent == $cursor->getIsPermanent()) {
         continue;
       }
@@ -259,6 +261,7 @@ final class PhabricatorRepositoryRefEngine
     $ref_type,
     array $all_closing_heads) {
     $repository = $this->getRepository();
+    $publisher = $repository->newPublisher();
 
     // NOTE: Mercurial branches may have multiple branch heads; this logic
     // is complex primarily to account for that.
@@ -341,7 +344,8 @@ final class PhabricatorRepositoryRefEngine
         $this->markPositionNew($new_position);
       }
 
-      if ($this->isPermanentRef($ref_type, $name)) {
+      $diffusion_ref = head($refs)->newDiffusionRepositoryRef();
+      if ($publisher->isPermanentRef($diffusion_ref)) {
 
         // See T13284. If this cursor was already marked as permanent, we
         // only need to publish the newly created ref positions. However, if
@@ -404,14 +408,6 @@ final class PhabricatorRepositoryRefEngine
     }
   }
 
-  private function isPermanentRef($ref_type, $ref_name) {
-    if ($ref_type !== PhabricatorRepositoryRefCursor::TYPE_BRANCH) {
-      return false;
-    }
-
-    return $this->getRepository()->isBranchPermanentRef($ref_name);
-  }
-
   /**
    * Find all ancestors of a new closing branch head which are not ancestors
    * of any old closing branch head.
diff --git a/src/applications/repository/query/PhabricatorRepositoryPublisher.php b/src/applications/repository/query/PhabricatorRepositoryPublisher.php
index ad374bd034..fae1b07f00 100644
--- a/src/applications/repository/query/PhabricatorRepositoryPublisher.php
+++ b/src/applications/repository/query/PhabricatorRepositoryPublisher.php
@@ -38,6 +38,10 @@ final class PhabricatorRepositoryPublisher
     return !$this->getCommitHoldReasons($commit);
   }
 
+  public function isPermanentRef(DiffusionRepositoryRef $ref) {
+    return !$this->getRefImpermanentReasons($ref);
+  }
+
 /* -(  Hold Reasons  )------------------------------------------------------- */
 
   public function getRepositoryHoldReasons() {
@@ -59,18 +63,8 @@ final class PhabricatorRepositoryPublisher
     $repository = $this->getRepository();
     $reasons = $this->getRepositoryHoldReasons();
 
-    if (!$ref->isBranch()) {
-      $reasons[] = self::HOLD_REF_NOT_BRANCH;
-    } else {
-      $branch_name = $ref->getShortName();
-
-      if (!$repository->shouldTrackBranch($branch_name)) {
-        $reasons[] = self::HOLD_UNTRACKED;
-      }
-
-      if (!$repository->isBranchPermanentRef($branch_name)) {
-        $reasons[] = self::HOLD_NOT_PERMANENT_REF;
-      }
+    foreach ($this->getRefImpermanentReasons($ref) as $reason) {
+      $reasons[] = $reason;
     }
 
     return $reasons;
@@ -89,4 +83,25 @@ final class PhabricatorRepositoryPublisher
     return $reasons;
   }
 
+  public function getRefImpermanentReasons(DiffusionRepositoryRef $ref) {
+    $repository = $this->getRepository();
+    $reasons = array();
+
+    if (!$ref->isBranch()) {
+      $reasons[] = self::HOLD_REF_NOT_BRANCH;
+    } else {
+      $branch_name = $ref->getShortName();
+
+      if (!$repository->shouldTrackBranch($branch_name)) {
+        $reasons[] = self::HOLD_UNTRACKED;
+      }
+
+      if (!$repository->isBranchPermanentRef($branch_name)) {
+        $reasons[] = self::HOLD_NOT_PERMANENT_REF;
+      }
+    }
+
+    return $reasons;
+  }
+
 }
diff --git a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php
index 91261f2b93..e661e94295 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php
@@ -88,6 +88,12 @@ final class PhabricatorRepositoryRefCursor
     return mpull($this->getPositions(), 'getCommitIdentifier');
   }
 
+  public function newDiffusionRepositoryRef() {
+    return id(new DiffusionRepositoryRef())
+      ->setRefType($this->getRefType())
+      ->setShortName($this->getRefName());
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

From 3cb543ef8fc6c58288ec1e7775a142a2e0a0c6a8 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Jan 2021 14:36:34 -0800
Subject: [PATCH 17/19] Lift logic for queueing commit import tasks into
 RepositoryEngine

Summary:
Ref T13591. There are currently two pathways to queue an import task for a commit: via repository discovery, or via a ref becoming permanent.

These pathways duplicate some logic and have behavioral differences: one does not set `objectPHID` properly, one does not set the priority correctly.

Unify these pathways, make them both set `objectPHID`, and make them both use the same priority logic.

Test Plan:
  - Discovered refs.
  - See later changes in this series for more complete test cases.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21516
---
 .../PhabricatorRepositoryDiscoveryEngine.php  | 104 +----------------
 .../engine/PhabricatorRepositoryEngine.php    | 110 ++++++++++++++++++
 .../engine/PhabricatorRepositoryRefEngine.php |  33 ++++--
 3 files changed, 138 insertions(+), 109 deletions(-)

diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
index dedb9de93a..21d95b227d 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
@@ -655,7 +655,12 @@ final class PhabricatorRepositoryDiscoveryEngine
     $epoch,
     $task_priority) {
 
-    $this->insertTask($repository, $commit, $task_priority);
+    $this->queueCommitImportTask(
+      $repository,
+      $commit->getID(),
+      $commit->getPHID(),
+      $task_priority,
+      $via = 'discovery');
 
     // Update the repository summary table.
     queryfx(
@@ -679,36 +684,6 @@ final class PhabricatorRepositoryDiscoveryEngine
     }
   }
 
-  private function insertTask(
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit,
-    $task_priority,
-    $data = array()) {
-
-    $vcs = $repository->getVersionControlSystem();
-    switch ($vcs) {
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-        $class = 'PhabricatorRepositoryGitCommitMessageParserWorker';
-        break;
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-        $class = 'PhabricatorRepositorySvnCommitMessageParserWorker';
-        break;
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-        $class = 'PhabricatorRepositoryMercurialCommitMessageParserWorker';
-        break;
-      default:
-        throw new Exception(pht("Unknown repository type '%s'!", $vcs));
-    }
-
-    $data['commitID'] = $commit->getID();
-
-    $options = array(
-      'priority' => $task_priority,
-    );
-
-    PhabricatorWorker::scheduleTask($class, $data, $options);
-  }
-
   private function isInitialImport(array $refs) {
     $commit_count = count($refs);
 
@@ -926,71 +901,4 @@ final class PhabricatorRepositoryDiscoveryEngine
       $data['epoch']);
   }
 
-  private function getImportTaskPriority(
-    PhabricatorRepository $repository,
-    array $refs) {
-
-    // If the repository is importing for the first time, we schedule tasks
-    // at IMPORT priority, which is very low. Making progress on importing a
-    // new repository for the first time is less important than any other
-    // daemon task.
-
-    // If the repository has finished importing and we're just catching up
-    // on recent commits, we usually schedule discovery at COMMIT priority,
-    // which is slightly below the default priority.
-
-    // Note that followup tasks and triggered tasks (like those generated by
-    // Herald or Harbormaster) will queue at DEFAULT priority, so that each
-    // commit tends to fully import before we start the next one. This tends
-    // to give imports fairly predictable progress. See T11677 for some
-    // discussion.
-
-    if ($repository->isImporting()) {
-      $this->log(
-        pht(
-          'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '.
-          'because this repository is still importing.',
-          phutil_count($refs)));
-
-      return PhabricatorWorker::PRIORITY_IMPORT;
-    }
-
-    // See T13369. If we've discovered a lot of commits at once, import them
-    // at lower priority.
-
-    // This is mostly aimed at reducing the impact that synchronizing thousands
-    // of commits from a remote upstream has on other repositories. The queue
-    // is "mostly FIFO", so queueing a thousand commit imports can stall other
-    // repositories.
-
-    // In a perfect world we'd probably give repositories round-robin queue
-    // priority, but we don't currently have the primitives for this and there
-    // isn't a strong case for building them.
-
-    // Use "a whole lot of commits showed up at once" as a heuristic for
-    // detecting "someone synchronized an upstream", and import them at a lower
-    // priority to more closely approximate fair scheduling.
-
-    if (count($refs) >= PhabricatorRepository::LOWPRI_THRESHOLD) {
-      $this->log(
-        pht(
-          'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '.
-          'because many commits were discovered at once.',
-          phutil_count($refs)));
-
-      return PhabricatorWorker::PRIORITY_IMPORT;
-    }
-
-    // Otherwise, import at normal priority.
-
-    if ($refs) {
-      $this->log(
-        pht(
-          'Importing %s commit(s) at normal priority ("PRIORITY_COMMIT").',
-          phutil_count($refs)));
-    }
-
-    return PhabricatorWorker::PRIORITY_COMMIT;
-  }
-
 }
diff --git a/src/applications/repository/engine/PhabricatorRepositoryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryEngine.php
index 12cd8f1ed6..c1ad203f0b 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryEngine.php
@@ -83,4 +83,114 @@ abstract class PhabricatorRepositoryEngine extends Phobject {
     return $this;
   }
 
+  final protected function queueCommitImportTask(
+    PhabricatorRepository $repository,
+    $commit_id,
+    $commit_phid,
+    $task_priority,
+    $via = null) {
+
+    $vcs = $repository->getVersionControlSystem();
+    switch ($vcs) {
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
+        $class = 'PhabricatorRepositoryGitCommitMessageParserWorker';
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+        $class = 'PhabricatorRepositorySvnCommitMessageParserWorker';
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+        $class = 'PhabricatorRepositoryMercurialCommitMessageParserWorker';
+        break;
+      default:
+        throw new Exception(
+          pht(
+            'Unknown repository type "%s"!',
+            $vcs));
+    }
+
+    $data = array(
+      'commitID' => $commit_id,
+    );
+
+    if ($via !== null) {
+      $data['via'] = $via;
+    }
+
+    $options = array(
+      'priority' => $task_priority,
+      'objectPHID' => $commit_phid,
+    );
+
+    PhabricatorWorker::scheduleTask($class, $data, $options);
+  }
+
+  final protected function getImportTaskPriority(
+    PhabricatorRepository $repository,
+    array $refs) {
+    assert_instances_of($refs, 'PhabricatorRepositoryCommitRef');
+
+    // If the repository is importing for the first time, we schedule tasks
+    // at IMPORT priority, which is very low. Making progress on importing a
+    // new repository for the first time is less important than any other
+    // daemon task.
+
+    // If the repository has finished importing and we're just catching up
+    // on recent commits, we usually schedule discovery at COMMIT priority,
+    // which is slightly below the default priority.
+
+    // Note that followup tasks and triggered tasks (like those generated by
+    // Herald or Harbormaster) will queue at DEFAULT priority, so that each
+    // commit tends to fully import before we start the next one. This tends
+    // to give imports fairly predictable progress. See T11677 for some
+    // discussion.
+
+    if ($repository->isImporting()) {
+      $this->log(
+        pht(
+          'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '.
+          'because this repository is still importing.',
+          phutil_count($refs)));
+
+      return PhabricatorWorker::PRIORITY_IMPORT;
+    }
+
+    // See T13369. If we've discovered a lot of commits at once, import them
+    // at lower priority.
+
+    // This is mostly aimed at reducing the impact that synchronizing thousands
+    // of commits from a remote upstream has on other repositories. The queue
+    // is "mostly FIFO", so queueing a thousand commit imports can stall other
+    // repositories.
+
+    // In a perfect world we'd probably give repositories round-robin queue
+    // priority, but we don't currently have the primitives for this and there
+    // isn't a strong case for building them.
+
+    // Use "a whole lot of commits showed up at once" as a heuristic for
+    // detecting "someone synchronized an upstream", and import them at a lower
+    // priority to more closely approximate fair scheduling.
+
+    if (count($refs) >= PhabricatorRepository::LOWPRI_THRESHOLD) {
+      $this->log(
+        pht(
+          'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '.
+          'because many commits were discovered at once.',
+          phutil_count($refs)));
+
+      return PhabricatorWorker::PRIORITY_IMPORT;
+    }
+
+    // Otherwise, import at normal priority.
+
+    if ($refs) {
+      $this->log(
+        pht(
+          'Importing %s commit(s) at normal priority ("PRIORITY_COMMIT").',
+          phutil_count($refs)));
+    }
+
+    return PhabricatorWorker::PRIORITY_COMMIT;
+  }
+
+
 }
diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
index 390e6e569d..679cbcdf67 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
@@ -548,6 +548,22 @@ final class PhabricatorRepositoryRefEngine
       }
     }
 
+    $commit_refs = array();
+    foreach ($identifiers as $identifier) {
+
+      // See T13591. This construction is a bit ad-hoc, but the priority
+      // function currently only cares about the number of refs we have
+      // discovered, so we'll get the right result even without filling
+      // these records out in detail.
+
+      $commit_refs[] = id(new PhabricatorRepositoryCommitRef())
+        ->setIdentifier($identifier);
+    }
+
+    $task_priority = $this->getImportTaskPriority(
+      $repository,
+      $commit_refs);
+
     $permanent_flag = PhabricatorRepositoryCommit::IMPORTED_PERMANENT;
     $published_flag = PhabricatorRepositoryCommit::IMPORTED_PUBLISH;
 
@@ -580,17 +596,12 @@ final class PhabricatorRepositoryRefEngine
           $import_status,
           $row['id']);
 
-        $data = array(
-          'commitID' => $row['id'],
-        );
-
-        PhabricatorWorker::scheduleTask(
-          $class,
-          $data,
-          array(
-            'priority' => PhabricatorWorker::PRIORITY_COMMIT,
-            'objectPHID' => $row['phid'],
-          ));
+        $this->queueCommitImportTask(
+          $repository,
+          $row['id'],
+          $row['phid'],
+          $task_priority,
+          $via = 'ref');
       }
     }
 

From 15e022d64870fc281887ab43b5da63ade05fd791 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Jan 2021 16:08:42 -0800
Subject: [PATCH 18/19] Support an "--active" flag for selecting active tasks

Summary: Ref T13591. This is mostly a workaround for Big Sur not having pcntl/posix installed by default and the mess with M1 / Homebrew / SIP / Code Signing (see T13232) so I can't easily run actual daemons and need to fake them with `bin/worker execute --active`, but it's a reasonable flag on its own.

Test Plan:
  - Ran `bin/worker execute --active` and `bin/worker cancel --active`.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21517
---
 .../PhabricatorWorkerManagementWorkflow.php   | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php
index 2f4ee2f4f5..113f3348d5 100644
--- a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php
+++ b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php
@@ -21,6 +21,10 @@ abstract class PhabricatorWorkerManagementWorkflow
         'param' => 'int',
         'help' => pht('Limit to tasks with at least this many failures.'),
       ),
+      array(
+        'name' => 'active',
+        'help' => pht('Select all active tasks.'),
+      ),
     );
   }
 
@@ -28,10 +32,13 @@ abstract class PhabricatorWorkerManagementWorkflow
     $ids = $args->getArg('id');
     $class = $args->getArg('class');
     $min_failures = $args->getArg('min-failure-count');
+    $active = $args->getArg('active');
 
-    if (!$ids && !$class && !$min_failures) {
+    if (!$ids && !$class && !$min_failures && !$active) {
       throw new PhutilArgumentUsageException(
-        pht('Use --id, --class, or --min-failure-count to select tasks.'));
+        pht(
+          'Use "--id", "--class", "--active", and/or "--min-failure-count" '.
+          'to select tasks.'));
     }
 
     $active_query = new PhabricatorWorkerActiveTaskQuery();
@@ -56,7 +63,13 @@ abstract class PhabricatorWorkerManagementWorkflow
     }
 
     $active_tasks = $active_query->execute();
-    $archive_tasks = $archive_query->execute();
+
+    if ($active) {
+      $archive_tasks = array();
+    } else {
+      $archive_tasks = $archive_query->execute();
+    }
+
     $tasks =
       mpull($active_tasks, null, 'getID') +
       mpull($archive_tasks, null, 'getID');

From 1da94dcf499e6ea702e4ac91f31816fcf7a6031d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Jan 2021 16:36:46 -0800
Subject: [PATCH 19/19] Correct some issues around IMPORTED_PERMANENT in
 RefEngine

Summary: Ref T13591. Fixes a few issues with the recent updates here discovered in more thorough testing.

Test Plan:
- Stopped the daemons.
- Created a new copy of Phabricator in Diffusion.
- Pulled it with `bin/repository pull ...`.
  - Got 17,278 commits on disk with `git log --all --format=%H`.
- Set permanent refs to "master".
- Discovered it with `bin/repository discover ...`.
  - This took 31.5s and inserted 17,278 tasks.
  - Verified that all tasks have priority 4,000 (PRIORITY_IMPORT).
  - Observed that 16,799 commits have IMPORTED_PERMANENT and 479 commits do not.
    - This matches `git log master --format=%H` exactly.
- Ran `bin/repository refs ...`. Expected no changes and saw no changes.
- Ran `bin/worker execute --active` for a minute or two. It processed all the impermanent changes first (since `bin/worker` is LIFO and these are supposed to process last).
  - Ran `bin/repository refs`. Expected no changes and saw no changes.
  - Marked all refs as permanent.
  - Starting state: 16,009 message tasks, all at priority 4000.
  - Ran `bin/repository refs`, expecting 479 new tasks at priority 4000.
  - Saw count rise to 16,488 as expected.
  - Saw all the new tasks have priority 4000 and all commits now have the IMPORTED_PERMANENT flag.

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21518
---
 .../engine/PhabricatorRepositoryRefEngine.php     | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
index 679cbcdf67..aafea1f81a 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
@@ -344,8 +344,7 @@ final class PhabricatorRepositoryRefEngine
         $this->markPositionNew($new_position);
       }
 
-      $diffusion_ref = head($refs)->newDiffusionRepositoryRef();
-      if ($publisher->isPermanentRef($diffusion_ref)) {
+      if ($publisher->isPermanentRef(head($refs))) {
 
         // See T13284. If this cursor was already marked as permanent, we
         // only need to publish the newly created ref positions. However, if
@@ -613,13 +612,17 @@ final class PhabricatorRepositoryRefEngine
     $ref_type,
     $ref_name) {
 
-    $is_permanent = $this->isPermanentRef($ref_type, $ref_name);
-
     $cursor = id(new PhabricatorRepositoryRefCursor())
       ->setRepositoryPHID($repository->getPHID())
       ->setRefType($ref_type)
-      ->setRefName($ref_name)
-      ->setIsPermanent((int)$is_permanent);
+      ->setRefName($ref_name);
+
+    $publisher = $repository->newPublisher();
+
+    $diffusion_ref = $cursor->newDiffusionRepositoryRef();
+    $is_permanent = $publisher->isPermanentRef($diffusion_ref);
+
+    $cursor->setIsPermanent((int)$is_permanent);
 
     try {
       return $cursor->save();