Add additional flags to "bin/repository rebuild-identities" to improve flexibility

Summary:
Ref T13444. Repository identities have, at a minimum, some bugs where they do not update relationships properly after many types of email address changes.

It is currently very difficult to fix this once the damage is done since there's no good way to inspect or rebuild them.

Take some steps toward improving observability and providing repair tools: allow `bin/repository rebuild-identities` to effect more repairs and operate on identities more surgically.

Test Plan: Ran `bin/repository rebuild-identities` with all new flags, saw what looked like reasonable rebuilds occur.

Maniphest Tasks: T13444

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

src/applications/config/check/PhabricatorManualActivitySetupCheck.php

@@ -113,7 +113,8 @@ final class PhabricatorManualActivitySetupCheck
       'pre',
       array(),
       (string)csprintf(
-        'phabricator/ $ ./bin/repository rebuild-identities --all'));
+        'phabricator/ $ '.
+        './bin/repository rebuild-identities --all-repositories'));
 
     $message[] = pht(
       'You can find more information about this new identity mapping '.

src/applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php

@@ -68,6 +68,24 @@ final class DiffusionRepositoryIdentityEngine
   }
 
   private function updateIdentity(PhabricatorRepositoryIdentity $identity) {
+
+    // If we're updating an identity and it has a manual user PHID associated
+    // with it but the user is no longer valid, remove the value. This likely
+    // corresponds to a user that was destroyed.
+
+    $assigned_phid = $identity->getManuallySetUserPHID();
+    $unassigned = DiffusionIdentityUnassignedDatasource::FUNCTION_TOKEN;
+    if ($assigned_phid && ($assigned_phid !== $unassigned)) {
+      $viewer = $this->getViewer();
+      $user = id(new PhabricatorPeopleQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($assigned_phid))
+        ->executeOne();
+      if (!$user) {
+        $identity->setManuallySetUserPHID(null);
+      }
+    }
+
     $resolved_phid = $this->resolveIdentity($identity);
 
     $identity

src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php

@@ -14,38 +14,172 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow
       ->setArguments(
         array(
           array(
-            'name'    => 'repositories',
+            'name' => 'all-repositories',
-            'wildcard' => true,
+            'help' => pht('Rebuild identities across all repositories.'),
           ),
           array(
-            'name'     => 'all',
+            'name' => 'all-identities',
-            'help'     => pht('Rebuild identities across all repositories.'),
+            'help' => pht('Rebuild all currently-known identities.'),
-        ),
+          ),
+          array(
+            'name' => 'repository',
+            'param' => 'repository',
+            'repeat' => true,
+            'help' => pht('Rebuild identities in a repository.'),
+          ),
+          array(
+            'name' => 'commit',
+            'param' => 'commit',
+            'repeat' => true,
+            'help' => pht('Rebuild identities for a commit.'),
+          ),
+          array(
+            'name' => 'user',
+            'param' => 'user',
+            'repeat' => true,
+            'help' => pht('Rebuild identities for a user.'),
+          ),
+          array(
+            'name' => 'email',
+            'param' => 'email',
+            'repeat' => true,
+            'help' => pht('Rebuild identities for an email address.'),
+          ),
+          array(
+            'name' => 'raw',
+            'param' => 'raw',
+            'repeat' => true,
+            'help' => pht('Rebuild identities for a raw commit string.'),
+          ),
         ));
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
+    $viewer = $this->getViewer();
 
-    $all = $args->getArg('all');
+    $rebuilt_anything = false;
-    $repositories = $args->getArg('repositories');
 
-    if ($all xor empty($repositories)) {
+
+    $all_repositories = $args->getArg('all-repositories');
+    $repositories = $args->getArg('repository');
+
+    if ($all_repositories && $repositories) {
       throw new PhutilArgumentUsageException(
-        pht('Specify --all or a list of repositories, but not both.'));
+        pht(
+          'Flags "--all-repositories" and "--repository" are not '.
+          'compatible.'));
     }
 
-    $query = id(new DiffusionCommitQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->needCommitData(true);
 
-    if ($repositories) {
+    $all_identities = $args->getArg('all-identities');
-      $repos = $this->loadRepositories($args, 'repositories');
+    $raw = $args->getArg('raw');
-      $query->withRepositoryIDs(mpull($repos, 'getID'));
+
+    if ($all_identities && $raw) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Flags "--all-identities" and "--raw" are not '.
+          'compatible.'));
     }
 
-    $iterator = new PhabricatorQueryIterator($query);
+    if ($all_repositories || $repositories) {
-    foreach ($iterator as $commit) {
+      $rebuilt_anything = true;
+
+      if ($repositories) {
+        $repository_list = $this->loadRepositories($args, 'repository');
+      } else {
+        $repository_query = id(new PhabricatorRepositoryQuery())
+          ->setViewer($viewer);
+        $repository_list = new PhabricatorQueryIterator($repository_query);
+      }
+
+      foreach ($repository_list as $repository) {
+        $commit_query = id(new DiffusionCommitQuery())
+          ->setViewer($viewer)
+          ->needCommitData(true)
+          ->withRepositoryIDs(array($repository->getID()));
+
+        $commit_iterator = new PhabricatorQueryIterator($commit_query);
+
+        $this->rebuildCommits($commit_iterator);
+      }
+    }
+
+    $commits = $args->getArg('commit');
+    if ($commits) {
+      $rebuilt_anything = true;
+      $commit_list = $this->loadCommits($args, 'commit');
+
+      // Reload commits to get commit data.
+      $commit_list = id(new DiffusionCommitQuery())
+        ->setViewer($viewer)
+        ->needCommitData(true)
+        ->withIDs(mpull($commit_list, 'getID'))
+        ->execute();
+
+      $this->rebuildCommits($commit_list);
+    }
+
+    $users = $args->getArg('user');
+    if ($users) {
+      $rebuilt_anything = true;
+
+      $user_list = $this->loadUsersFromArguments($users);
+      $this->rebuildUsers($user_list);
+    }
+
+    $emails = $args->getArg('email');
+    if ($emails) {
+      $rebuilt_anything = true;
+      $this->rebuildEmails($emails);
+    }
+
+    if ($all_identities || $raw) {
+      $rebuilt_anything = true;
+
+      if ($raw) {
+        $identities = id(new PhabricatorRepositoryIdentityQuery())
+          ->setViewer($viewer)
+          ->withIdentityNames($raw)
+          ->execute();
+
+        $identities = mpull($identities, null, 'getIdentityNameRaw');
+        foreach ($raw as $raw_identity) {
+          if (!isset($identities[$raw_identity])) {
+            throw new PhutilArgumentUsageException(
+              pht(
+                'No identity "%s" exists. When selecting identities with '.
+                '"--raw", the entire identity must match exactly.',
+                $raw_identity));
+          }
+        }
+
+        $identity_list = $identities;
+      } else {
+        $identity_query = id(new PhabricatorRepositoryIdentityQuery())
+          ->setViewer($viewer);
+
+        $identity_list = new PhabricatorQueryIterator($identity_query);
+
+        $this->logInfo(
+          pht('REBUILD'),
+          pht('Rebuilding all existing identities.'));
+      }
+
+      $this->rebuildIdentities($identity_list);
+    }
+
+    if (!$rebuilt_anything) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Nothing specified to rebuild. Use flags to choose which '.
+          'identities to rebuild, or "--help" for help.'));
+    }
+
+    return 0;
+  }
+
+  private function rebuildCommits($commits) {
+    foreach ($commits as $commit) {
       $needs_update = false;
 
       $data = $commit->getCommitData();
@@ -57,6 +191,8 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow
 
       $author_phid = $commit->getAuthorIdentityPHID();
       $identity_phid = $author_identity->getPHID();
+
+      $aidentity_phid = $identity_phid;
       if ($author_phid !== $identity_phid) {
         $commit->setAuthorIdentityPHID($identity_phid);
         $data->setCommitDetail('authorIdentityPHID', $identity_phid);
@@ -83,16 +219,20 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow
       if ($needs_update) {
         $commit->save();
         $data->save();
-        echo tsprintf(
+
-          "Rebuilt identities for %s.\n",
+        $this->logInfo(
-          $commit->getDisplayName());
+          pht('COMMIT'),
+          pht(
+            'Rebuilt identities for "%s".',
+            $commit->getDisplayName()));
       } else {
-        echo tsprintf(
+        $this->logInfo(
-          "No changes for %s.\n",
+          pht('SKIP'),
-          $commit->getDisplayName());
+          pht(
+            'No changes for commit "%s".',
+            $commit->getDisplayName()));
       }
     }
-
   }
 
   private function getIdentityForCommit(
@@ -113,4 +253,131 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow
     return $this->identityCache[$raw_identity];
   }
 
+
+  private function rebuildUsers($users) {
+    $viewer = $this->getViewer();
+
+    foreach ($users as $user) {
+      $this->logInfo(
+        pht('USER'),
+        pht(
+          'Rebuilding identities for user "%s".',
+          $user->getMonogram()));
+
+      $emails = id(new PhabricatorUserEmail())->loadAllWhere(
+        'userPHID = %s',
+        $user->getPHID());
+      if ($emails) {
+        $this->rebuildEmails(mpull($emails, 'getAddress'));
+      }
+
+      $identities = id(new PhabricatorRepositoryIdentityQuery())
+        ->setViewer($viewer)
+        ->withRelatedPHIDs(array($user->getPHID()))
+        ->execute();
+
+      if (!$identities) {
+        $this->logWarn(
+          pht('NO IDENTITIES'),
+          pht('Found no identities directly related to user.'));
+        continue;
+      }
+
+      $this->rebuildIdentities($identities);
+    }
+  }
+
+  private function rebuildEmails($emails) {
+    $viewer = $this->getViewer();
+
+    foreach ($emails as $email) {
+      $this->logInfo(
+        pht('EMAIL'),
+        pht('Rebuilding identities for email address "%s".', $email));
+
+      $identities = id(new PhabricatorRepositoryIdentityQuery())
+        ->setViewer($viewer)
+        ->withEmailAddresses(array($email))
+        ->execute();
+
+      if (!$identities) {
+        $this->logWarn(
+          pht('NO IDENTITIES'),
+          pht('Found no identities for email address "%s".', $email));
+        continue;
+      }
+
+      $this->rebuildIdentities($identities);
+    }
+  }
+
+  private function rebuildIdentities($identities) {
+    $viewer = $this->getViewer();
+
+    foreach ($identities as $identity) {
+      $raw_identity = $identity->getIdentityName();
+
+      if (isset($this->identityCache[$raw_identity])) {
+        $this->logInfo(
+          pht('SKIP'),
+          pht(
+            'Identity "%s" has already been rebuilt.',
+            $raw_identity));
+        continue;
+      }
+
+      $this->logInfo(
+        pht('IDENTITY'),
+        pht(
+          'Rebuilding identity "%s".',
+          $raw_identity));
+
+      $old_auto = $identity->getAutomaticGuessedUserPHID();
+      $old_assign = $identity->getManuallySetUserPHID();
+
+      $identity = id(new DiffusionRepositoryIdentityEngine())
+        ->setViewer($viewer)
+        ->newUpdatedIdentity($identity);
+
+      $this->identityCache[$raw_identity] = $identity;
+
+      $new_auto = $identity->getAutomaticGuessedUserPHID();
+      $new_assign = $identity->getManuallySetUserPHID();
+
+      $same_auto = ($old_auto === $new_auto);
+      $same_assign = ($old_assign === $new_assign);
+
+      if ($same_auto && $same_assign) {
+        $this->logInfo(
+          pht('UNCHANGED'),
+          pht('No changes to identity.'));
+      } else {
+        if (!$same_auto) {
+          $this->logWarn(
+            pht('AUTOMATIC PHID'),
+            pht(
+              'Automatic user updated from "%s" to "%s".',
+              $this->renderPHID($old_auto),
+              $this->renderPHID($new_auto)));
+        }
+        if (!$same_assign) {
+          $this->logWarn(
+            pht('ASSIGNED PHID'),
+            pht(
+              'Assigned user updated from "%s" to "%s".',
+              $this->renderPHID($old_assign),
+              $this->renderPHID($new_assign)));
+        }
+      }
+    }
+  }
+
+  private function renderPHID($phid) {
+    if ($phid == null) {
+      return pht('NULL');
+    } else {
+      return $phid;
+    }
+  }
+
 }

src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php

@@ -11,6 +11,7 @@ final class PhabricatorRepositoryIdentityQuery
   private $effectivePHIDs;
   private $identityNameLike;
   private $hasEffectivePHID;
+  private $relatedPHIDs;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -47,6 +48,11 @@ final class PhabricatorRepositoryIdentityQuery
     return $this;
   }
 
+  public function withRelatedPHIDs(array $related) {
+    $this->relatedPHIDs = $related;
+    return $this;
+  }
+
   public function withHasEffectivePHID($has_effective_phid) {
     $this->hasEffectivePHID = $has_effective_phid;
     return $this;
@@ -57,7 +63,7 @@ final class PhabricatorRepositoryIdentityQuery
   }
 
   protected function getPrimaryTableAlias() {
-     return 'repository_identity';
+     return 'identity';
    }
 
   protected function loadPage() {
@@ -70,28 +76,28 @@ final class PhabricatorRepositoryIdentityQuery
     if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn,
-        'repository_identity.id IN (%Ld)',
+        'identity.id IN (%Ld)',
         $this->ids);
     }
 
     if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
-        'repository_identity.phid IN (%Ls)',
+        'identity.phid IN (%Ls)',
         $this->phids);
     }
 
     if ($this->assignedPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'repository_identity.manuallySetUserPHID IN (%Ls)',
+        'identity.manuallySetUserPHID IN (%Ls)',
         $this->assignedPHIDs);
     }
 
     if ($this->effectivePHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'repository_identity.currentEffectiveUserPHID IN (%Ls)',
+        'identity.currentEffectiveUserPHID IN (%Ls)',
         $this->effectivePHIDs);
     }
 
@@ -99,11 +105,11 @@ final class PhabricatorRepositoryIdentityQuery
       if ($this->hasEffectivePHID) {
         $where[] = qsprintf(
           $conn,
-          'repository_identity.currentEffectiveUserPHID IS NOT NULL');
+          'identity.currentEffectiveUserPHID IS NOT NULL');
       } else {
         $where[] = qsprintf(
           $conn,
-          'repository_identity.currentEffectiveUserPHID IS NULL');
+          'identity.currentEffectiveUserPHID IS NULL');
       }
     }
 
@@ -115,24 +121,35 @@ final class PhabricatorRepositoryIdentityQuery
 
       $where[] = qsprintf(
         $conn,
-        'repository_identity.identityNameHash IN (%Ls)',
+        'identity.identityNameHash IN (%Ls)',
         $name_hashes);
     }
 
     if ($this->emailAddresses !== null) {
       $where[] = qsprintf(
         $conn,
-        'repository_identity.emailAddress IN (%Ls)',
+        'identity.emailAddress IN (%Ls)',
         $this->emailAddresses);
     }
 
     if ($this->identityNameLike != null) {
       $where[] = qsprintf(
         $conn,
-        'repository_identity.identityNameRaw LIKE %~',
+        'identity.identityNameRaw LIKE %~',
         $this->identityNameLike);
     }
 
+    if ($this->relatedPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        '(identity.manuallySetUserPHID IN (%Ls) OR
+          identity.currentEffectiveUserPHID IN (%Ls) OR
+          identity.automaticGuessedUserPHID IN (%Ls))',
+        $this->relatedPHIDs,
+        $this->relatedPHIDs,
+        $this->relatedPHIDs);
+    }
+
     return $where;
   }
 

src/infrastructure/management/PhabricatorManagementWorkflow.php

@@ -67,4 +67,125 @@ abstract class PhabricatorManagementWorkflow extends PhutilArgumentWorkflow {
     fprintf(STDERR, '%s', $message);
   }
 
+  final protected function loadUsersFromArguments(array $identifiers) {
+    if (!$identifiers) {
+      return array();
+    }
+
+    $ids = array();
+    $phids = array();
+    $usernames = array();
+
+    $user_type = PhabricatorPeopleUserPHIDType::TYPECONST;
+
+    foreach ($identifiers as $identifier) {
+      // If the value is a user PHID, treat as a PHID.
+      if (phid_get_type($identifier) === $user_type) {
+        $phids[$identifier] = $identifier;
+        continue;
+      }
+
+      // If the value is "@..." and then some text, treat it as a username.
+      if ((strlen($identifier) > 1) && ($identifier[0] == '@')) {
+        $usernames[$identifier] = substr($identifier, 1);
+        continue;
+      }
+
+      // If the value is digits, treat it as both an ID and a username.
+      // Entirely numeric usernames, like "1234", are valid.
+      if (ctype_digit($identifier)) {
+        $ids[$identifier] = $identifier;
+        $usernames[$identifier] = $identifier;
+        continue;
+      }
+
+      // Otherwise, treat it as an unescaped username.
+      $usernames[$identifier] = $identifier;
+    }
+
+    $viewer = $this->getViewer();
+    $results = array();
+
+    if ($phids) {
+      $users = id(new PhabricatorPeopleQuery())
+        ->setViewer($viewer)
+        ->withPHIDs($phids)
+        ->execute();
+      foreach ($users as $user) {
+        $phid = $user->getPHID();
+        $results[$phid][] = $user;
+      }
+    }
+
+    if ($usernames) {
+      $users = id(new PhabricatorPeopleQuery())
+        ->setViewer($viewer)
+        ->withUsernames($usernames)
+        ->execute();
+
+      $reverse_map = array();
+      foreach ($usernames as $identifier => $username) {
+        $username = phutil_utf8_strtolower($username);
+        $reverse_map[$username][] = $identifier;
+      }
+
+      foreach ($users as $user) {
+        $username = $user->getUsername();
+        $username = phutil_utf8_strtolower($username);
+
+        $reverse_identifiers = idx($reverse_map, $username, array());
+
+        if (count($reverse_identifiers) > 1) {
+          throw new PhutilArgumentUsageException(
+            pht(
+              'Multiple user identifiers (%s) correspond to the same user. '.
+              'Identify each user exactly once.',
+              implode(', ', $reverse_identifiers)));
+        }
+
+        foreach ($reverse_identifiers as $reverse_identifier) {
+          $results[$reverse_identifier][] = $user;
+        }
+      }
+    }
+
+    if ($ids) {
+      $users = id(new PhabricatorPeopleQuery())
+        ->setViewer($viewer)
+        ->withIDs($ids)
+        ->execute();
+
+      foreach ($users as $user) {
+        $id = $user->getID();
+        $results[$id][] = $user;
+      }
+    }
+
+    $list = array();
+    foreach ($identifiers as $identifier) {
+      $users = idx($results, $identifier, array());
+      if (!$users) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'No user "%s" exists. Specify users by username, ID, or PHID.',
+            $identifier));
+      }
+
+      if (count($users) > 1) {
+        // This can happen if you have a user "@25", a user with ID 25, and
+        // specify "--user 25". You can disambiguate this by specifying
+        // "--user @25".
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Identifier "%s" matches multiple users. Specify each user '.
+            'unambiguously with "@username" or by using user PHIDs.',
+            $identifier));
+      }
+
+      $list[] = head($users);
+    }
+
+    return $list;
+  }
+
 }