dulwich-users team mailing list archive

[Augie Fackler <durin42@xxxxxxxxx>]

On Sun, May 30, 2010 at 10:23 AM, Jelmer Vernooij <jelmer@xxxxxxxxx> wrote:
> Hi Augie,
>
> On Thu, May 20, 2010 at 05:07:22PM -0500, Augie Fackler wrote:
>> http://github.com/durin42/dulwich now has two branches:
>> d42-send-fix: the send-pack fix along with some already-passing compat
>> tests, for 0.6.0
> Thanks, this one was merged for 0.6.0. It also appears to be different with a different
> Git SHA on d42-refactor-clients.
>
>> d42-refactor-clients: the larger client refactor, for after 0.6.0,
>> pending discussion
> I'm now looking at this one in more detail.
>
> With regard to connect_ssh / SSHVendor I agree it's a good idea to get rid of the current approach.
> I'll need to fix bzr-git to pass in a single function, but I'm happy to do that. FWIW the reason
> we had a SSHVendor class was to match what bzr has so I could hook that into it easily.
> Bazaar has more methods than connect_ssh on SSHVendor, most prominently connect_sftp, which
> is why it is an object rather than a method.

Actually, having thought about it a little, I think that the default
argument doesn't make a ton of sense either. Would it make sense to
have a dict of connection type to connection method? That is,

{'tcp': connect_tcp, 'ssh': connect_ssh, ...}

and then there can be a registration function? That allows registering
new protocol formats fairly easily, and lets us define some kind of
function for "I want to override this method" that'd update the dict.

>
> This branch changes GitClient instances to be re-usable - previously they could really only be used once,
> after which they would have to be discarded, as the server disconnects you after each request. This seems
> like the right approach to take - it makes it possible for us to support the server keeping
> the connection open after a request, and it makes it easier for users of the Dulwich API since they don't have
> to bother with re-initializing GitClient objects.
>
> My only concern is with putting can_read on Protocol, is that really necessary ? I'd rather just
> see _connect()/connect_ssh() return a Protocol instance and a can_read() method. can_read() seems
> like a mismatch for Protocol to me - it won't tell you whether you can actually
> do an operation without it blocking. It's just used as a hackish mechanism in GitClient to
> let it peek whether it should look at the next element in the graph or read more info from
> the server. Not having Protocol.can_read would also reduce the number of places
> that are affected by this patch.

Yeah, I wasn't thrilled with adding can_read everywhere - I like the
idea of splitting off the can_read method and returning it separately
from _connect. I'll rework the patches to do that and ping this thread
when it's ready.

Thanks,
Augie

>
> Cheers,
>
> Jelmer
>