Make "bin/audit delete" synchronize commit audit status, and improve "bin/audit synchronize" documentation

Summary:
Depends on D20126. See PHI1056. Ref T13244.

  - `bin/audit delete` destroys audit requests, but does not update the overall audit state for associated commits. For example, if you destroy all audit requests for a commit, it does not move to "No Audit Required".
  - `bin/audit synchronize` does this synchronize step, but is poorly documented.

Make `bin/audit delete` synchronize affected commits.

Document `bin/audit synchronize` better.

There's some reasonable argument that `bin/audit synchronize` perhaps shouldn't exist, but it does let you recover from an accidentally (or intentionally) mangled database state. For now, let it live.

Test Plan:
  - Ran `bin/audit delete`, saw audits destroyed and affected commits synchornized.
  - Ran `bin/audit synchronize`, saw behavior unchanged.
  - Ran `bin/audit help`, got better help.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

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

src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php

@@ -105,10 +105,10 @@ final class PhabricatorAuditManagementDeleteWorkflow
       $query->withPHIDs(mpull($commits, 'getPHID'));
     }
 
-    $commits = $query->execute();
-    $commits = mpull($commits, null, 'getPHID');
+    $commit_iterator = new PhabricatorQueryIterator($query);
+
     $audits = array();
-    foreach ($commits as $commit) {
+    foreach ($commit_iterator as $commit) {
       $commit_audits = $commit->getAudits();
       foreach ($commit_audits as $key => $audit) {
         if ($id_map && empty($id_map[$audit->getID()])) {
@@ -131,51 +131,87 @@ final class PhabricatorAuditManagementDeleteWorkflow
           continue;
         }
       }
-      $audits[] = $commit_audits;
-    }
-    $audits = array_mergev($audits);
 
-    $console = PhutilConsole::getConsole();
+      if (!$commit_audits) {
+        continue;
+      }
 
-    if (!$audits) {
-      $console->writeErr("%s\n", pht('No audits match the query.'));
-      return 0;
-    }
+      $handles = id(new PhabricatorHandleQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(mpull($commit_audits, 'getAuditorPHID'))
+        ->execute();
 
-    $handles = id(new PhabricatorHandleQuery())
-      ->setViewer($this->getViewer())
-      ->withPHIDs(mpull($audits, 'getAuditorPHID'))
-      ->execute();
+      foreach ($commit_audits as $audit) {
+        $audit_id = $audit->getID();
 
-
-    foreach ($audits as $audit) {
-      $commit = $commits[$audit->getCommitPHID()];
-
-      $console->writeOut(
-        "%s\n",
-        sprintf(
+        $description = sprintf(
           '%10d %-16s %-16s %s: %s',
-          $audit->getID(),
+          $audit_id,
           $handles[$audit->getAuditorPHID()]->getName(),
           PhabricatorAuditStatusConstants::getStatusName(
             $audit->getAuditStatus()),
           $commit->getRepository()->formatCommitName(
             $commit->getCommitIdentifier()),
-          trim($commit->getSummary())));
+          trim($commit->getSummary()));
+
+        $audits[] = array(
+          'auditID' => $audit_id,
+          'commitPHID' => $commit->getPHID(),
+          'description' => $description,
+        );
+      }
     }
 
-    if (!$is_dry_run) {
-      $message = pht(
-        'Really delete these %d audit(s)? They will be permanently deleted '.
-        'and can not be recovered.',
-        count($audits));
-      if ($console->confirm($message)) {
-        foreach ($audits as $audit) {
-          $id = $audit->getID();
-          $console->writeOut("%s\n", pht('Deleting audit %d...', $id));
-          $audit->delete();
-        }
+    if (!$audits) {
+      echo tsprintf(
+        "%s\n",
+        pht('No audits match the query.'));
+      return 0;
+    }
+
+    foreach ($audits as $audit_spec) {
+      echo tsprintf(
+        "%s\n",
+        $audit_spec['description']);
+    }
+
+    if ($is_dry_run) {
+      echo tsprintf(
+        "%s\n",
+        pht('This is a dry run, so no changes will be made.'));
+      return 0;
+    }
+
+    $message = pht(
+      'Really delete these %s audit(s)? They will be permanently deleted '.
+      'and can not be recovered.',
+      phutil_count($audits));
+    if (!phutil_console_confirm($message)) {
+      echo tsprintf(
+        "%s\n",
+        pht('User aborted the workflow.'));
+      return 1;
+    }
+
+    $audits_by_commit = igroup($audits, 'commitPHID');
+    foreach ($audits_by_commit as $commit_phid => $audit_specs) {
+      $audit_ids = ipull($audit_specs, 'auditID');
+
+      $audits = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere(
+        'id IN (%Ld)',
+        $audit_ids);
+
+      foreach ($audits as $audit) {
+        $id = $audit->getID();
+
+        echo tsprintf(
+          "%s\n",
+          pht('Deleting audit %d...', $id));
+
+        $audit->delete();
       }
+
+      $this->synchronizeCommitAuditState($commit_phid);
     }
 
     return 0;

src/applications/audit/management/PhabricatorAuditManagementWorkflow.php

@@ -87,4 +87,39 @@ abstract class PhabricatorAuditManagementWorkflow
     return $commits;
   }
 
+  protected function synchronizeCommitAuditState($commit_phid) {
+    $viewer = $this->getViewer();
+
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($commit_phid))
+      ->needAuditRequests(true)
+      ->executeOne();
+    if (!$commit) {
+      return;
+    }
+
+    $old_status = $commit->getAuditStatusObject();
+    $commit->updateAuditStatus($commit->getAudits());
+    $new_status = $commit->getAuditStatusObject();
+
+    if ($old_status->getKey() == $new_status->getKey()) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'No synchronization changes for "%s".',
+          $commit->getDisplayName()));
+    } else {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Synchronizing "%s": "%s" -> "%s".',
+          $commit->getDisplayName(),
+          $old_status->getName(),
+          $new_status->getName()));
+
+      $commit->save();
+    }
+  }
+
 }

src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php

@@ -6,8 +6,16 @@ final class PhabricatorAuditSynchronizeManagementWorkflow
   protected function didConstruct() {
     $this
       ->setName('synchronize')
-      ->setExamples('**synchronize** ...')
-      ->setSynopsis(pht('Update audit status for commits.'))
+      ->setExamples(
+        "**synchronize** __repository__ ...\n".
+        "**synchronize** __commit__ ...\n".
+        "**synchronize** --all")
+      ->setSynopsis(
+        pht(
+          'Update commits to make their summary audit state reflect the '.
+          'state of their actual audit requests. This can fix inconsistencies '.
+          'in database state if audit requests have been mangled '.
+          'accidentally (or on purpose).'))
       ->setArguments(
         array_merge(
           $this->getCommitConstraintArguments(),
@@ -21,36 +29,7 @@ final class PhabricatorAuditSynchronizeManagementWorkflow
     foreach ($objects as $object) {
       $commits = $this->loadCommitsForConstraintObject($object);
       foreach ($commits as $commit) {
-        $commit = id(new DiffusionCommitQuery())
-          ->setViewer($viewer)
-          ->withPHIDs(array($commit->getPHID()))
-          ->needAuditRequests(true)
-          ->executeOne();
-        if (!$commit) {
-          continue;
-        }
-
-        $old_status = $commit->getAuditStatusObject();
-        $commit->updateAuditStatus($commit->getAudits());
-        $new_status = $commit->getAuditStatusObject();
-
-        if ($old_status->getKey() == $new_status->getKey()) {
-          echo tsprintf(
-            "%s\n",
-            pht(
-              'No changes for "%s".',
-              $commit->getDisplayName()));
-        } else {
-          echo tsprintf(
-            "%s\n",
-            pht(
-              'Updating "%s": "%s" -> "%s".',
-              $commit->getDisplayName(),
-              $old_status->getName(),
-              $new_status->getName()));
-
-          $commit->save();
-        }
+        $this->synchronizeCommitAuditState($commit->getPHID());
       }
     }
   }

src/docs/user/userguide/audit.diviner

@@ -175,16 +175,13 @@ You can use this command to forcibly delete requests which may have triggered
 incorrectly (for example, because a package or Herald rule was configured in an
 overbroad way).
 
-After deleting audits, you may want to run `bin/audit synchronize` to
-synchronize audit state.
-
 **Synchronize Audit State**: Synchronize the audit state of commits to the
 current open audit requests with `bin/audit synchronize`.
 
 Normally, overall audit state is automatically kept up to date as changes are
-made to an audit. However, if you delete audits or manually update the database
-to make changes to audit request state, the state of corresponding commits may
-no longer be correct.
+made to an audit. However, if you manually update the database to make changes
+to audit request state, the state of corresponding commits may no longer be
+consistent.
 
 This command will update commits so their overall audit state reflects the
 cumulative state of their actual audit requests.