From 554359203467986d2873f274364f80e2bc640239 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 4 Jan 2018 14:11:54 -0800
Subject: [PATCH] Add a couple of clarifying comments to the Mercurial protocol
 parser

Summary: See D18857. Ref T13036. See PHI275. Explain what's going on here a little better since it isn't entirely obvious and debugging these stream parsers is a gigantic pain.

Test Plan: Read text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18859
---
 ...nMercurialWireClientSSHProtocolChannel.php | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php b/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php
index 4c16fb9f0f..8eb026d8fe 100644
--- a/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php
+++ b/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php
@@ -180,6 +180,15 @@ final class DiffusionMercurialWireClientSSHProtocolChannel
           $this->state = 'arguments';
         }
       } else if ($this->state == 'data-length') {
+
+        // We're reading the length of a chunk of raw data. It looks like
+        // this:
+        //
+        //   <length-in-bytes>\n
+        //
+        // The length is human-readable text (for example, "4096"), and
+        // may be 0.
+
         $line = $this->readProtocolLine();
         if ($line === null) {
           break;
@@ -192,6 +201,9 @@ final class DiffusionMercurialWireClientSSHProtocolChannel
           $this->state = 'data-bytes';
         }
       } else if ($this->state == 'data-bytes') {
+
+        // We're reading some known, nonzero number of raw bytes of data.
+
         // If we don't have any more bytes on the buffer yet, just bail:
         // otherwise, we'll emit a pointless and possibly harmful 0-byte data
         // frame. See T13036 for discussion.
@@ -203,6 +215,13 @@ final class DiffusionMercurialWireClientSSHProtocolChannel
         $this->buffer = substr($this->buffer, strlen($bytes));
         $this->expectBytes -= strlen($bytes);
 
+        // NOTE: We emit a data frame as soon as we read some data. This can
+        // cause us to repackage frames: for example, if we receive one large
+        // frame slowly, we may emit it as several smaller frames. In theory
+        // this is good; in practice, Mercurial never seems to select a frame
+        // size larger than 4096 bytes naturally and this may be more
+        // complexity and trouble than it is worth. See T13036.
+
         $messages[] = $this->newDataMessage($bytes);
 
         if (!$this->expectBytes) {