Fix an issue where ancestors of permanent refs might not be published during import or if a branch is later made permanent

Summary:
Fixes T13284. See that task for substantial discussion. There are currently two cases where we'll skip over commits which we should publish:

  - if a branch is not permanent, then later made permanent; or
  - in some cases, the first time we examine branches in a repository.

In both cases, this error is one-shot and things work correctly going forward. The root cause is conflation between the states "this ref currently permanent" and "this ref was permanent the last time we updated refs".

Separate these pieces of state and cover all these cases. Also introduce a "--rebuild" flag to fix the state of bad commits.

Test Plan:
See T13284 for the three major cases:

  - initial import;
  - push changes to a nonpermanent branch, update, then make it permanent;
  - push chanegs to a nonpermanent branch, update, push more changes, then make it permanent.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13284

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

resources/sql/autopatches/20190924.diffusion.01.permanent.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_refcursor
+  ADD isPermanent BOOL NOT NULL;

resources/sql/autopatches/20190924.diffusion.02.default.sql

@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_repository.repository_refcursor
+  SET isPermanent = 1;

src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php

@@ -195,13 +195,15 @@ final class DiffusionLowLevelResolveRefsQuery
 
       $alternate = null;
       if ($type == 'tag') {
-        $alternate = $identifier;
-        $identifier = idx($tag_map, $ref);
-        if (!$identifier) {
-          throw new Exception(
-            pht(
-              "Failed to look up tag '%s'!",
-              $ref));
+        $tag_identifier = idx($tag_map, $ref);
+        if ($tag_identifier === null) {
+          // This can happen when we're asked to resolve the hash of a "tag"
+          // object created with "git tag --annotate" that isn't currently
+          // reachable from any ref. Just leave things as they are.
+        } else {
+          // Otherwise, we have a normal named tag.
+          $alternate = $identifier;
+          $identifier = $tag_identifier;
         }
       }
 

src/applications/repository/engine/PhabricatorRepositoryRefEngine.php

@@ -10,7 +10,16 @@ final class PhabricatorRepositoryRefEngine
   private $newPositions = array();
   private $deadPositions = array();
   private $closeCommits = array();
-  private $hasNoCursors;
+  private $rebuild;
+
+  public function setRebuild($rebuild) {
+    $this->rebuild = $rebuild;
+    return $this;
+  }
+
+  public function getRebuild() {
+    return $this->rebuild;
+  }
 
   public function updateRefs() {
     $this->newPositions = array();
@@ -60,15 +69,17 @@ final class PhabricatorRepositoryRefEngine
       ->execute();
     $cursor_groups = mgroup($all_cursors, 'getRefType');
 
-    $this->hasNoCursors = (!$all_cursors);
-
-    // Find all the heads of closing refs.
+    // Find all the heads of permanent refs.
     $all_closing_heads = array();
     foreach ($all_cursors as $cursor) {
-      $should_close = $this->shouldCloseRef(
-        $cursor->getRefType(),
-        $cursor->getRefName());
-      if (!$should_close) {
+
+      // See T13284. Note that we're considering whether this ref was a
+      // permanent ref or not the last time we updated refs for this
+      // repository. This allows us to handle things properly when a ref
+      // is reconfigured from non-permanent to permanent.
+
+      $was_permanent = $cursor->getIsPermanent();
+      if (!$was_permanent) {
         continue;
       }
 
@@ -76,6 +87,7 @@ final class PhabricatorRepositoryRefEngine
         $all_closing_heads[] = $identifier;
       }
     }
+
     $all_closing_heads = array_unique($all_closing_heads);
     $all_closing_heads = $this->removeMissingCommits($all_closing_heads);
 
@@ -88,12 +100,18 @@ final class PhabricatorRepositoryRefEngine
       $this->setCloseFlagOnCommits($this->closeCommits);
     }
 
-    if ($this->newPositions || $this->deadPositions) {
+    $save_cursors = $this->getCursorsForUpdate($all_cursors);
+
+    if ($this->newPositions || $this->deadPositions || $save_cursors) {
       $repository->openTransaction();
 
         $this->saveNewPositions();
         $this->deleteDeadPositions();
 
+        foreach ($save_cursors as $cursor) {
+          $cursor->save();
+        }
+
       $repository->saveTransaction();
     }
 
@@ -103,6 +121,28 @@ final class PhabricatorRepositoryRefEngine
     }
   }
 
+  private function getCursorsForUpdate(array $cursors) {
+    assert_instances_of($cursors, 'PhabricatorRepositoryRefCursor');
+
+    $results = array();
+
+    foreach ($cursors as $cursor) {
+      $ref_type = $cursor->getRefType();
+      $ref_name = $cursor->getRefName();
+
+      $is_permanent = $this->isPermanentRef($ref_type, $ref_name);
+
+      if ($is_permanent == $cursor->getIsPermanent()) {
+        continue;
+      }
+
+      $cursor->setIsPermanent((int)$is_permanent);
+      $results[] = $cursor;
+    }
+
+    return $results;
+  }
+
   private function updateBranchStates(
     PhabricatorRepository $repository,
     array $branches) {
@@ -301,11 +341,41 @@ final class PhabricatorRepositoryRefEngine
         $this->markPositionNew($new_position);
       }
 
-      if ($this->shouldCloseRef($ref_type, $name)) {
-        foreach ($added_commits as $identifier) {
+      if ($this->isPermanentRef($ref_type, $name)) {
+
+        // See T13284. If this cursor was already marked as permanent, we
+        // only need to publish the newly created ref positions. However, if
+        // this cursor was not previously permanent but has become permanent,
+        // we need to publish all the ref positions.
+
+        // This corresponds to users reconfiguring a branch to make it
+        // permanent without pushing any new commits to it.
+
+        $is_rebuild = $this->getRebuild();
+        $was_permanent = $ref_cursor->getIsPermanent();
+
+        if ($is_rebuild || !$was_permanent) {
+          $update_all = true;
+        } else {
+          $update_all = false;
+        }
+
+        if ($update_all) {
+          $update_commits = $new_commits;
+        } else {
+          $update_commits = $added_commits;
+        }
+
+        if ($is_rebuild) {
+          $exclude = array();
+        } else {
+          $exclude = $all_closing_heads;
+        }
+
+        foreach ($update_commits as $identifier) {
           $new_identifiers = $this->loadNewCommitIdentifiers(
             $identifier,
-            $all_closing_heads);
+            $exclude);
 
           $this->markCloseCommits($new_identifiers);
         }
@@ -334,19 +404,11 @@ final class PhabricatorRepositoryRefEngine
     }
   }
 
-  private function shouldCloseRef($ref_type, $ref_name) {
+  private function isPermanentRef($ref_type, $ref_name) {
     if ($ref_type !== PhabricatorRepositoryRefCursor::TYPE_BRANCH) {
       return false;
     }
 
-    if ($this->hasNoCursors) {
-      // If we don't have any cursors, don't close things. Particularly, this
-      // corresponds to the case where you've just updated to this code on an
-      // existing repository: we don't want to requeue message steps for every
-      // commit on a closeable ref.
-      return false;
-    }
-
     return $this->getRepository()->isBranchPermanentRef($ref_name);
   }
 
@@ -505,10 +567,13 @@ 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);
+      ->setRefName($ref_name)
+      ->setIsPermanent((int)$is_permanent);
 
     try {
       return $cursor->save();

src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php

@@ -14,6 +14,12 @@ final class PhabricatorRepositoryManagementRefsWorkflow
             'name'        => 'verbose',
             'help'        => pht('Show additional debugging information.'),
           ),
+          array(
+            'name' => 'rebuild',
+            'help' => pht(
+              'Publish commits currently reachable from any permanent ref, '.
+              'ignoring the cached ref state.'),
+          ),
           array(
             'name'        => 'repos',
             'wildcard'    => true,
@@ -41,6 +47,7 @@ final class PhabricatorRepositoryManagementRefsWorkflow
       $engine = id(new PhabricatorRepositoryRefEngine())
         ->setRepository($repo)
         ->setVerbose($args->getArg('verbose'))
+        ->setRebuild($args->getArg('rebuild'))
         ->updateRefs();
     }
 

src/applications/repository/storage/PhabricatorRepositoryRefCursor.php

@@ -19,6 +19,7 @@ final class PhabricatorRepositoryRefCursor
   protected $refNameHash;
   protected $refNameRaw;
   protected $refNameEncoding;
+  protected $isPermanent;
 
   private $repository = self::ATTACHABLE;
   private $positions = self::ATTACHABLE;
@@ -34,6 +35,7 @@ final class PhabricatorRepositoryRefCursor
         'refType' => 'text32',
         'refNameHash' => 'bytes12',
         'refNameEncoding' => 'text16?',
+        'isPermanent' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'key_ref' => array(