From ec9eaabfbdac857cdea57f08b24421bbd941fbd2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Aug 2014 12:25:24 -0700
Subject: [PATCH] Allow repo updates to recover after force push + garbage
 collection

Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).

We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.

Instead, validate all the `<old hash>` values before we try to make use of them.

Test Plan:
  - Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
  - Applied patch.
  - `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
  - Saw new commits recognized correctly.
  - Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5839

Differential Revision: https://secure.phabricator.com/D10226
---
 .../DiffusionLowLevelResolveRefsQuery.php     |  6 ----
 .../engine/PhabricatorRepositoryRefEngine.php | 30 +++++++++++++++++++
 ...atorRepositoryManagementUpdateWorkflow.php |  6 ++++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
index 0d18d9601e..ca47c04ca1 100644
--- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
+++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
@@ -135,12 +135,6 @@ final class DiffusionLowLevelResolveRefsQuery
 
     $futures = array();
     foreach ($this->refs as $ref) {
-      // TODO: There was a note about `--rev 'a b'` not working for branches
-      // with spaces in their names in older code, but I suspect this was
-      // misidentified and resulted from the branch name being interpeted as
-      // a revset. Use hgsprintf() to avoid that. If this doesn't break for a
-      // bit, remove this comment. Otherwise, consider `-b %s --limit 1`.
-
       $futures[$ref] = $repository->getLocalCommandFuture(
         'log --template=%s --rev %s',
         '{node}',
diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
index cfcf3b7041..eddf26b33d 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
@@ -63,6 +63,7 @@ final class PhabricatorRepositoryRefEngine
       }
     }
     $all_closing_heads = array_unique($all_closing_heads);
+    $all_closing_heads = $this->removeMissingCommits($all_closing_heads);
 
     foreach ($maps as $type => $refs) {
       $cursor_group = idx($cursor_groups, $type, array());
@@ -105,6 +106,35 @@ final class PhabricatorRepositoryRefEngine
     return $this;
   }
 
+  /**
+   * Remove commits which no longer exist in the repository from a list.
+   *
+   * After a force push and garbage collection, we may have branch cursors which
+   * point at commits which no longer exist. This can make commands issued later
+   * fail. See T5839 for discussion.
+   *
+   * @param list<string>    List of commit identifiers.
+   * @return list<string>   List with nonexistent identifiers removed.
+   */
+  private function removeMissingCommits(array $identifiers) {
+    if (!$identifiers) {
+      return array();
+    }
+
+    $resolved = id(new DiffusionLowLevelResolveRefsQuery())
+      ->setRepository($this->getRepository())
+      ->withRefs($identifiers)
+      ->execute();
+
+    foreach ($identifiers as $key => $identifier) {
+      if (empty($resolved[$identifier])) {
+        unset($identifiers[$key]);
+      }
+    }
+
+    return $identifiers;
+  }
+
   private function updateCursors(
     array $cursors,
     array $new_refs,
diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
index d677162714..163a826a31 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
@@ -42,6 +42,7 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
 
   public function execute(PhutilArgumentParser $args) {
     $this->setVerbose($args->getArg('verbose'));
+    $console = PhutilConsole::getConsole();
 
     $repos = $this->loadRepositories($args, 'repos');
     if (count($repos) !== 1) {
@@ -103,6 +104,11 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
 
     $lock->unlock();
 
+    $console->writeOut(
+      pht(
+        'Updated repository **%s**.',
+        $repository->getMonogram())."\n");
+
     return 0;
   }