recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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

epriestley 3a4e1443 0f02d79f

-11
-11
src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php
··· 66 66 ->setWillWriteCallback(array($this, 'willWriteMessageCallback')) 67 67 ->execute(); 68 68 69 - // TODO: It's apparently technically possible to communicate errors to 70 - // Mercurial over SSH by writing a special "\n<error>\n-\n" string. However, 71 - // my attempt to implement that resulted in Mercurial closing the socket and 72 - // then hanging, without showing the error. This might be an issue on our 73 - // side (we need to close our half of the socket?), or maybe the code 74 - // for this in Mercurial doesn't actually work, or maybe something else 75 - // is afoot. At some point, we should look into doing this more cleanly. 76 - // For now, when we, e.g., reject writes for policy reasons, the user will 77 - // see "abort: unexpected response: empty string" after the diagnostically 78 - // useful, e.g., "remote: This repository is read-only over SSH." message. 79 - 80 69 if (!$err && $this->didSeeWrite) { 81 70 $repository->writeStatusMessage( 82 71 PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,