dulwich-users team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

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. 

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. 

Cheers,

Jelmer