On Sun, May 30, 2010 at 10:23 AM, Jelmer Vernooij <[email protected]> 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
>
_______________________________________________
Mailing list: https://launchpad.net/~dulwich-users
Post to : [email protected]
Unsubscribe : https://launchpad.net/~dulwich-users
More help : https://help.launchpad.net/ListHelp
