diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -886,7 +886,7 @@ '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 @@ 'DrydockDAO', 'PhabricatorPolicyInterface', ), + 'DrydockCommandError' => 'Phobject', 'DrydockCommandInterface' => 'DrydockInterface', 'DrydockCommandQuery' => 'DrydockQuery', 'DrydockConsoleController' => 'DrydockController', diff --git a/src/applications/drydock/DrydockCommandError/DrydockCommandError.php b/src/applications/drydock/DrydockCommandError/DrydockCommandError.php deleted file mode 100644 --- a/src/applications/drydock/DrydockCommandError/DrydockCommandError.php +++ /dev/null @@ -1,18 +0,0 @@ - $phase, - 'command' => (string)$command, - 'raw' => (string)$ex->getCommand(), - 'err' => $ex->getError(), - 'stdout' => $ex->getStdout(), - 'stderr' => $ex->getStderr(), - ); - return $error; - } -} diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -4,6 +4,8 @@ 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 @@ $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 @@ $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 @@ $cmd[] = 'git checkout %s --'; $arg[] = $ref_ref; - } - $cmd = implode(' && ', $cmd); - $argv = array_merge(array($cmd), $arg); + try { + $this->execxv($interface, $cmd, $arg); + } catch (CommandException $ex) { + $display_command = csprintf( + 'git fetch %R %R', + $ref_uri, + $ref_ref); - $result = call_user_func_array( - array($interface, 'execx'), - $argv); + $error = DrydockCommandError::newFromCommandException($ex) + ->setPhase(self::PHASE_REMOTEFETCH) + ->setDisplayCommand($display_command); - if (idx($spec, 'default')) { - $default = $directory; + $lease->setAttribute( + 'workingcopy.vcs.error', + $error->toDictionary()); + + throw $ex; + } } $merges = idx($spec, 'merges'); @@ -428,11 +451,29 @@ $src_uri = $merge['src.uri']; $src_ref = $merge['src.ref']; - $interface->execx( - 'git fetch --no-tags -- %s +%s:%s', - $src_uri, - $src_ref, - $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 @@ '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); + + $error = DrydockCommandError::newFromCommandException($ex) + ->setPhase(self::PHASE_SQUASHMERGE) + ->setDisplayCommand($display_command); - $lease->setAttribute('workingcopy.vcs.error', $error); + $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); + } + + } diff --git a/src/applications/drydock/exception/DrydockCommandError.php b/src/applications/drydock/exception/DrydockCommandError.php new file mode 100644 --- /dev/null +++ b/src/applications/drydock/exception/DrydockCommandError.php @@ -0,0 +1,58 @@ +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, + ); + } + +} diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -4,7 +4,9 @@ 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,25 +121,42 @@ $committer_info['email'], "{$author_name} <{$author_email}>"); - $future - ->write($commit_message) - ->resolvex(); + $future->write($commit_message); try { - $interface->execx( - 'git push origin -- %s:%s', - 'HEAD', - $push_dst); + $future->resolvex(); } catch (CommandException $ex) { - $show_command = csprintf( - 'git push origin -- %s:%s', - 'HEAD', - $push_dst); - $error = DrydockCommandError::newFromCommandException( - self::PHASE_PUSH, - $show_command, - $ex); - $operation->setCommandError($error); + $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( + 'git push origin -- %s:%s', + 'HEAD', + $push_dst); + } catch (CommandException $ex) { + $display_command = csprintf( + 'git push origin %R:%R', + 'HEAD', + $push_dst); + + $error = DrydockCommandError::newFromCommandException($ex) + ->setPhase(self::PHASE_PUSH) + ->setDisplayCommand($display_command); + + $operation->setCommandError($error->toDictionary()); + throw $ex; } } diff --git a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php --- a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php +++ b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php @@ -82,6 +82,20 @@ '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 @@ 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(