Page MenuHomePhabricator

Enable Mercurial reads and writes over SSH
ClosedPublic

Authored by epriestley on Nov 11 2013, 12:10 AM.
Tags
None
Referenced Files
F13788415: D7553.diff
Thu, Sep 12, 9:48 PM
F13788411: D7553.id17067.diff
Thu, Sep 12, 9:46 PM
F13788410: D7553.id17044.diff
Thu, Sep 12, 9:46 PM
F13788409: D7553.id.diff
Thu, Sep 12, 9:46 PM
Unknown Object (File)
Mon, Sep 2, 5:05 PM
Unknown Object (File)
Fri, Aug 30, 9:30 PM
Unknown Object (File)
Fri, Aug 30, 9:30 PM
Unknown Object (File)
Fri, Aug 30, 9:29 PM
Subscribers

Details

Reviewers
asherkin
btrahan
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP8840f60218ef: Enable Mercurial reads and writes over SSH
Summary

Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol.

Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong.

A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct.

Test Plan
  • Ran hg clone over SSH.
  • Ran hg fetch over SSH.
  • Ran hg push over SSH, to a read-only repo (error) and a read-write repo (success).

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

src/applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php
60

To clarify this comment, the errors are perfectly visible and usable, we just ship them over stderr instead of actually using the special protocol error mechanism. This doesn't appreciably impact the usability, it would just be nice to have eventually.

src/applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php
4

good luck reviewing this

This all looks sane (and awesome!) at a once-over, nice work.

Don't think I'm crazy enough to hit Accept on it though, as I don't fully understand all the existing channel stuff.

btrahan added inline comments.
src/applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php
4

haha, this wasn't so bad actually. decodeStream has lots of nice comments to hold your hand.