From 3a4e14431fce5b80c57c1f62ed6003ee93575ac6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 12:46:38 -0800 Subject: [PATCH] Remove an obsolete comment about Mercurial SSH error behavior Summary: Depends on D18855. Ref T13036. This comment no longer seems to be accurate: anything we send over `stderr` is faithfully shown to the user with recent clients. From [[ https://www.mercurial-scm.org/repo/hg/file/default/mercurial/help/internals/wireprotocol.txt | this document ]], the missing sauce may have been: ``` A generic error response type is also supported. It consists of a an error message written to ``stderr`` followed by ``\n-\n``. In addition, ``\n`` is written to ``stdout``. ``` That is, writing "\n" to stdout in addition to writing the error to stderr. However, this no longer appears to be necessary. I think the modern client behavior is generally sensible (and consistent with the behavior of Git and Subversion) so this //probably// isn't a bug or me making a mistake. Test Plan: With a modern client, threw some arbitrary exception during execution. Observed a helpful message on the client with no additional steps. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13036 Differential Revision: https://secure.phabricator.com/D18856 --- .../ssh/DiffusionMercurialServeSSHWorkflow.php | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php index da877b7314..5701a4bac1 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php @@ -66,17 +66,6 @@ final class DiffusionMercurialServeSSHWorkflow ->setWillWriteCallback(array($this, 'willWriteMessageCallback')) ->execute(); - // TODO: It's apparently technically possible to communicate errors to - // Mercurial over SSH by writing a special "\n\n-\n" string. However, - // my attempt to implement that resulted in Mercurial closing the socket and - // then hanging, without showing the error. This might be an issue on our - // side (we need to close our half of the socket?), or maybe the code - // for this in Mercurial doesn't actually work, or maybe something else - // is afoot. At some point, we should look into doing this more cleanly. - // For now, when we, e.g., reject writes for policy reasons, the user will - // see "abort: unexpected response: empty string" after the diagnostically - // useful, e.g., "remote: This repository is read-only over SSH." message. - if (!$err && $this->didSeeWrite) { $repository->writeStatusMessage( PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,