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] 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,
           ),
         ));
   }