dulwich-users team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxxxx>]

Hi Ray Chuan,

On Tue, 2010-12-21 at 15:13 +0100, Jelmer Vernooij wrote:
> On Tue, 2010-12-21 at 07:05 -0600, Augie Fackler wrote:
> > On Dec 21, 2010, at 4:22 AM, Jelmer Vernooij wrote:
> > > On Tue, 2010-12-21 at 12:46 +0800, Tay Ray Chuan wrote:
> > >> Allow the vendor to be specified on the client class. That way, when
> > >> users wish to use a separate vendor, they can just subclass
> > >> SSHGitClient, instead of replacing (globally) client.get_ssh_vendor.
> > >> ---
> > >> dulwich/client.py                   |    7 +++----
> > >> dulwich/tests/compat/test_client.py |   25 +++++++++++++------------
> > >> 2 files changed, 16 insertions(+), 16 deletions(-)
> > >> 
> > >> diff --git a/dulwich/client.py b/dulwich/client.py
> > >> index 2b69852..e89879f 100644
> > >> --- a/dulwich/client.py
> > >> +++ b/dulwich/client.py
> > >> @@ -338,12 +338,11 @@ class SSHVendor(object):
> > >>                                 stdout=subprocess.PIPE)
> > >>         return SubprocessWrapper(proc)
> > >> 
> > >> -# Can be overridden by users
> > >> -get_ssh_vendor = SSHVendor
> > >> -
> > >> 
> > >> class SSHGitClient(GitClient):
> > >> 
> > >> +    ssh_vendor_class = SSHVendor
> > >> +
> > > My main concern is about keeping compatibility with the Bazaar SSHVendor
> > > interface - both so bzr-git can easily hook it in, but also to keep the
> > > option open of stealing that code easily in the future. This patch
> > > doesn't change the SSHVendor class, so that's not really an issue here. 
> > > 
> > > I'm happy to merge this but would prefer to keep calling this variable
> > > get_vendor as ideally it selects a vendor (openssh, sshcorp, plink, lsh,
> > > paramiko, etc) and returns an instance of that rather than being
> > > hardcoded to a single class. 
> 
> > I won't bikeshed what to call it, but IMO we need to not remove
> > get_ssh_vendor in the same version of dulwich that we move to some
> > other interface. Did you see my feedback on this patch? Do you have any
> > thoughts on that? (I wanted to make it possible for clients to not
> > *have* to implement bzr's ssh vendor scheme if that's undesirable.)
> No, I hadn't seen that at the time I replied. I think that's a good
> suggestion.
> 
> I presume you'd like to override it with whatever mechanism you have in
> Mercurial for creating a SSH connection?
Do you have a patch for this in progress? If not, I wouldn't mind having
a look so it can land before 0.7.0.

Thanks!

Jelmer