Improve Drydock errors for empty commits and missing changes

Summary: Ref T10093. Show better errors when a commit fails because it has already been merged and when a fetch fails because the ref isn't present in the remote.

Test Plan:
{F1160794}

{F1160795}

Reviewers: chad

Reviewed By: chad

Subscribers: michaeljs1990, yelirekim

Maniphest Tasks: T10093

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

src/__phutil_library_map__.php

@@ -886,7 +886,7 @@ phutil_register_library_map(array(
     'DrydockBlueprintTransactionQuery' => 'applications/drydock/query/DrydockBlueprintTransactionQuery.php',
     'DrydockBlueprintViewController' => 'applications/drydock/controller/DrydockBlueprintViewController.php',
     'DrydockCommand' => 'applications/drydock/storage/DrydockCommand.php',
-    'DrydockCommandError' => 'applications/drydock/DrydockCommandError/DrydockCommandError.php',
+    'DrydockCommandError' => 'applications/drydock/exception/DrydockCommandError.php',
     'DrydockCommandInterface' => 'applications/drydock/interface/command/DrydockCommandInterface.php',
     'DrydockCommandQuery' => 'applications/drydock/query/DrydockCommandQuery.php',
     'DrydockConsoleController' => 'applications/drydock/controller/DrydockConsoleController.php',
@@ -5014,6 +5014,7 @@ phutil_register_library_map(array(
       'DrydockDAO',
       'PhabricatorPolicyInterface',
     ),
+    'DrydockCommandError' => 'Phobject',
     'DrydockCommandInterface' => 'DrydockInterface',
     'DrydockCommandQuery' => 'DrydockQuery',
     'DrydockConsoleController' => 'DrydockController',

src/applications/drydock/DrydockCommandError/DrydockCommandError.php

@@ -1,18 +0,0 @@
-<?php
-
-class DrydockCommandError {
-  public static function newFromCommandException(
-    $phase,
-    $command,
-    CommandException $ex) {
-    $error = array(
-      'phase' => $phase,
-      'command' => (string)$command,
-      'raw' => (string)$ex->getCommand(),
-      'err' => $ex->getError(),
-      'stdout' => $ex->getStdout(),
-      'stderr' => $ex->getStderr(),
-    );
-    return $error;
-  }
-}

src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php

@@ -4,6 +4,8 @@ final class DrydockWorkingCopyBlueprintImplementation
   extends DrydockBlueprintImplementation {
 
   const PHASE_SQUASHMERGE = 'squashmerge';
+  const PHASE_REMOTEFETCH = 'blueprint.workingcopy.fetch.remote';
+  const PHASE_MERGEFETCH = 'blueprint.workingcopy.fetch.staging';
 
   public function isEnabled() {
     return true;
@@ -240,11 +242,11 @@ final class DrydockWorkingCopyBlueprintImplementation
 
     $default = null;
     foreach ($map as $directory => $spec) {
+      $interface->pushWorkingDirectory("{$root}/repo/{$directory}/");
+
       $cmd = array();
       $arg = array();
 
-      $interface->pushWorkingDirectory("{$root}/repo/{$directory}/");
-
       $cmd[] = 'git clean -d --force';
       $cmd[] = 'git fetch';
 
@@ -266,7 +268,20 @@ final class DrydockWorkingCopyBlueprintImplementation
 
         $cmd[] = 'git reset --hard origin/%s';
         $arg[] = $branch;
-      } else if ($ref) {
+      }
+
+      $this->execxv($interface, $cmd, $arg);
+
+      if (idx($spec, 'default')) {
+        $default = $directory;
+      }
+
+      // If we're fetching a ref from a remote, do that separately so we can
+      // raise a more tailored error.
+      if ($ref) {
+        $cmd = array();
+        $arg = array();
+
         $ref_uri = $ref['uri'];
         $ref_ref = $ref['ref'];
 
@@ -277,17 +292,25 @@ final class DrydockWorkingCopyBlueprintImplementation
 
         $cmd[] = 'git checkout %s --';
         $arg[] = $ref_ref;
+
+        try {
+          $this->execxv($interface, $cmd, $arg);
+        } catch (CommandException $ex) {
+          $display_command = csprintf(
+            'git fetch %R %R',
+            $ref_uri,
+            $ref_ref);
+
+          $error = DrydockCommandError::newFromCommandException($ex)
+            ->setPhase(self::PHASE_REMOTEFETCH)
+            ->setDisplayCommand($display_command);
+
+          $lease->setAttribute(
+            'workingcopy.vcs.error',
+            $error->toDictionary());
+
+          throw $ex;
         }
-
-      $cmd = implode(' && ', $cmd);
-      $argv = array_merge(array($cmd), $arg);
-
-      $result = call_user_func_array(
-        array($interface, 'execx'),
-        $argv);
-
-      if (idx($spec, 'default')) {
-        $default = $directory;
       }
 
       $merges = idx($spec, 'merges');
@@ -428,11 +451,29 @@ final class DrydockWorkingCopyBlueprintImplementation
     $src_uri = $merge['src.uri'];
     $src_ref = $merge['src.ref'];
 
+
+    try {
       $interface->execx(
         'git fetch --no-tags -- %s +%s:%s',
         $src_uri,
         $src_ref,
         $src_ref);
+    } catch (CommandException $ex) {
+      $display_command = csprintf(
+        'git fetch %R +%R:%R',
+        $src_uri,
+        $src_ref,
+        $src_ref);
+
+      $error = DrydockCommandError::newFromCommandException($ex)
+        ->setPhase(self::PHASE_MERGEFETCH)
+        ->setDisplayCommand($display_command);
+
+      $lease->setAttribute('workingcopy.vcs.error', $error->toDictionary());
+
+      throw $ex;
+    }
+
 
     // NOTE: This can never actually generate a commit because we pass
     // "--squash", but git sometimes runs code to check that a username and
@@ -443,32 +484,36 @@ final class DrydockWorkingCopyBlueprintImplementation
       'drydock@phabricator',
       $src_ref);
 
-    // Show the user a simplified command if the operation fails and we need to
-    // report an error.
-    $show_command = csprintf(
-      'git merge --squash -- %R',
-      $src_ref);
-
     try {
       $interface->execx('%C', $real_command);
     } catch (CommandException $ex) {
-      $error = DrydockCommandError::newFromCommandException(
-        self::PHASE_SQUASHMERGE,
-        $show_command,
-        $ex);
+      $display_command = csprintf(
+        'git merge --squash %R',
+        $src_ref);
 
-      $lease->setAttribute('workingcopy.vcs.error', $error);
+      $error = DrydockCommandError::newFromCommandException($ex)
+        ->setPhase(self::PHASE_SQUASHMERGE)
+        ->setDisplayCommand($display_command);
+
+      $lease->setAttribute('workingcopy.vcs.error', $error->toDictionary());
       throw $ex;
     }
   }
 
   public function getCommandError(DrydockLease $lease) {
-    $error = $lease->getAttribute('workingcopy.vcs.error');
-    if (!$error) {
-      return null;
-    } else {
-      return $error;
-    }
+    return $lease->getAttribute('workingcopy.vcs.error');
   }
 
+  private function execxv(
+    DrydockCommandInterface $interface,
+    array $commands,
+    array $arguments) {
+
+    $commands = implode(' && ', $commands);
+    $argv = array_merge(array($commands), $arguments);
+
+    return call_user_func_array(array($interface, 'execx'), $argv);
+  }
+
+
 }

src/applications/drydock/exception/DrydockCommandError.php

@@ -0,0 +1,58 @@
+<?php
+
+final class DrydockCommandError extends Phobject {
+
+  private $phase;
+  private $displayCommand;
+  private $command;
+  private $error;
+  private $stdout;
+  private $stderr;
+
+  public static function newFromCommandException(CommandException $ex) {
+    $error = new self();
+
+    $error->command = (string)$ex->getCommand();
+
+    $error->error = $ex->getError();
+    $error->stdout = $ex->getStdout();
+    $error->stderr = $ex->getStderr();
+
+    return $error;
+  }
+
+  public function setPhase($phase) {
+    $this->phase = $phase;
+    return $this;
+  }
+
+  public function getPhase() {
+    return $this->phase;
+  }
+
+  public function setDisplayCommand($display_command) {
+    $this->displayCommand = (string)$display_command;
+    return $this;
+  }
+
+  public function getDisplayCommand() {
+    return $this->displayCommand;
+  }
+
+  public function toDictionary() {
+    $display_command = $this->getDisplayCommand();
+    if ($display_command === null) {
+      $display_command = $this->command;
+    }
+
+    return array(
+      'phase' => $this->getPhase(),
+      'command' => $display_command,
+      'raw' => $this->command,
+      'err' => $this->error,
+      'stdout' => $this->stdout,
+      'stderr' => $this->stderr,
+    );
+  }
+
+}

src/applications/drydock/operation/DrydockLandRepositoryOperation.php

@@ -4,7 +4,9 @@ final class DrydockLandRepositoryOperation
   extends DrydockRepositoryOperationType {
 
   const OPCONST = 'land';
-  const PHASE_PUSH = 'push';
+
+  const PHASE_PUSH = 'op.land.push';
+  const PHASE_COMMIT = 'op.land.commit';
 
   public function getOperationDescription(
     DrydockRepositoryOperation $operation,
@@ -119,9 +121,24 @@ final class DrydockLandRepositoryOperation
       $committer_info['email'],
       "{$author_name} <{$author_email}>");
 
-    $future
-      ->write($commit_message)
-      ->resolvex();
+    $future->write($commit_message);
+
+    try {
+      $future->resolvex();
+    } catch (CommandException $ex) {
+      $display_command = csprintf('git commit');
+
+      // TODO: One reason this can fail is if the changes have already been
+      // merged. We could try to detect that.
+
+      $error = DrydockCommandError::newFromCommandException($ex)
+        ->setPhase(self::PHASE_COMMIT)
+        ->setDisplayCommand($display_command);
+
+      $operation->setCommandError($error->toDictionary());
+
+      throw $ex;
+    }
 
     try {
       $interface->execx(
@@ -129,15 +146,17 @@ final class DrydockLandRepositoryOperation
         'HEAD',
         $push_dst);
     } catch (CommandException $ex) {
-      $show_command = csprintf(
-      'git push origin -- %s:%s',
+      $display_command = csprintf(
+        'git push origin %R:%R',
         'HEAD',
         $push_dst);
-      $error = DrydockCommandError::newFromCommandException(
-        self::PHASE_PUSH,
-        $show_command,
-        $ex);
-      $operation->setCommandError($error);
+
+      $error = DrydockCommandError::newFromCommandException($ex)
+        ->setPhase(self::PHASE_PUSH)
+        ->setDisplayCommand($display_command);
+
+      $operation->setCommandError($error->toDictionary());
+
       throw $ex;
     }
   }

src/applications/drydock/view/DrydockRepositoryOperationStatusView.php

@@ -82,6 +82,20 @@ final class DrydockRepositoryOperationStatusView
               'This change did not merge cleanly. This usually indicates '.
               'that the change is out of date and needs to be updated.');
             break;
+          case DrydockWorkingCopyBlueprintImplementation::PHASE_REMOTEFETCH:
+            $message = pht(
+              'This change could not be fetched from the remote.');
+            break;
+          case DrydockWorkingCopyBlueprintImplementation::PHASE_MERGEFETCH:
+            $message = pht(
+              'This change could not be fetched from the remote staging '.
+              'area. It may not have been pushed, or may have been removed.');
+            break;
+          case DrydockLandRepositoryOperation::PHASE_COMMIT:
+            $message = pht(
+              'Committing this change failed. It may already have been '.
+              'merged.');
+            break;
           case DrydockLandRepositoryOperation::PHASE_PUSH:
             $message = pht(
               'The push failed. This usually indicates '.
@@ -123,10 +137,23 @@ final class DrydockRepositoryOperationStatusView
 
   private function renderVCSErrorTable(array $vcs_error) {
     $rows = array();
-    $rows[] = array(pht('Command'), $vcs_error['command']);
+
+    $rows[] = array(
+      pht('Command'),
+      phutil_censor_credentials($vcs_error['command']),
+    );
+
     $rows[] = array(pht('Error'), $vcs_error['err']);
-    $rows[] = array(pht('Stdout'), $vcs_error['stdout']);
-    $rows[] = array(pht('Stderr'), $vcs_error['stderr']);
+
+    $rows[] = array(
+      pht('Stdout'),
+      phutil_censor_credentials($vcs_error['stdout']),
+    );
+
+    $rows[] = array(
+      pht('Stderr'),
+      phutil_censor_credentials($vcs_error['stderr']),
+    );
 
     $table = id(new AphrontTableView($rows))
       ->setColumnClasses(