Re: [Monotone-devel] merging branch to allow 'automate stdio' over the network

[Stephen Leake]

Timothy Brownawell <address@hidden> writes:

> On Tue, Sep 29, 2009 at 2:14 AM, Stephen Leake
> <address@hidden> wrote:
>> Thomas Keller <address@hidden> writes:
>>
>>> Timothy Brownawell wrote:
>>>
>>>> This splits netsync.cc into multiple files, and splits the negotiation
>>>> and teardown out of the 'session' class. Then it adds 'automate_cmd'
>>>> as an alternative to 'anonymous_cmd'/'auth_cmd' so that network
>>>> clients can request either an automate connection or a netsync
>>>> connection.
>>>
>>> The split is definitely welcome, what I miss (and to be honest already
>>> missed before) is some chatter about the roles of the different classes
>>> and their inner workings. I know that monotone's source code is mostly
>>> documented, erm, "as is", so either I read and understand it or it is my
>>> problem, but still I see your refactoring as chance to improve the
>>> documentation of this crucial part of monotone.
>
> I've put a few comments in the header files, does this help?

Yes, thanks.

>> I agree. For example, why do we need the notion of "wrapped session"?
>> It would seem deriving automate_session and netsync_session from
>> session would be simpler.
>
> That would be much more convenient, but it puts the binding time too early.
>
> On the server, a 'session' is created at accept() time. We don't know
> whether that 'session' should actually be a 'netsync_session' or an
> 'automate_session' until we get the client's reply to our hello
> packet, so we need something that can be instantiated separately and
> inserted into an already-existing 'session' to give it a personality.

That makes sense, and the comment in the file makes it clear.

Another solution would be to create a session object that is used
during the handshake, and replace it with a netsync_session or
automate_session object later. I'm not clear this would be better;
you'd have to capture all of the state of the original object when
creating the latter.

-- 
-- Stephe