SSHStore in bfile

[David Douard]

Le Monday 02 November 2009 22:46:33 Greg Ward, vous avez écrit :
> On Sun, Nov 1, 2009 at 4:14 PM, David Douard <david.douard at logilab.fr> 
wrote:
> > Here is a bundle implementing this latter solution. In fact, I've been
> > playing a bit with raw scp commands, and I found it to be not that easy.
> > The solution proposed here, using hg's serve mecanism offers more
> > flexibility.
>
> OK, I have gone over your code in more detail now and I have more
> substantial feedback.  In no particular order...
>
> 1) You're right, the way you've wrapped sshserver.do_hello() is a vile
> hack.  I can't think of another way to do it apart from factoring a
> "get capabilities" method out of sshserver.do_hello(), and then
> wrapping or overriding it.

I did not think about fixing upstream hg's code for this, but you're right, 
that hte best way of achieving this.

> 2) I don't see why you have to strip() capability names before sending
> them.  The real do_hello() doesn't do that.

Your right, I don't know why I put this useless piece of code.

> 3) Your comments for _sshserver_do_bfput() and _sshserver_do_bfget()
> appear to have been copied from the forest extension.  Please fix them
> so they actually describe what is done in bfiles!

Correct, this is now fixed. 

> 4) Those do_bfput() and do_bfget() methods should probably be added to
> sshserver late (e.g. in reposetup()) rather than early.

Fixed.

> 5) SSHStore is largely a copy of mercurial.sshrepo.sshrepository.
> Yuck!  Again, I can see why you did it this way; the protocol code in
> sshrepository is not terribly reusable, presumably because no one has
> wanted to reuse it until now.  As above, I would prefer to refactor
> sshrepository for reusability rather than duplicate its code.  In the
> meantime, can you add comments like "# XXX copied from sshrepository"
> or "# XXX copied from sshrepository and modified" to every method of
> SSHStore that was copied?  Then we'll know what needs to be refactored
> and what code can be removed from bfiles when/if it depends on a
> refactored sshrepo module.

Done (the XXX comments).

> To clarify: I am willing to put up with duplicate code in bfiles.py in
> order to make it work with Mercurial 1.3.  But I would like to
> refactor Mercurial 1.x so that the duplication is no longer necessary.
>  Once bfiles requires Mercurial 1.x or later, the duplicate code can
> be removed.  Hopefully 1.x will be 1.4, but time is tight for that.

Oh, good news, I did't know bfiles was candidate for hg's inclusion. Knowing 
that, your previous remarks about sending patches against hg's code make much 
more sense to me.

> Finally, now that I have tried to use your patch, I see that it
> expects the central store to be a Mercurial repository.  That doesn't
> make a lot of sense, since the whole point of bfiles is to get large
> binary files out of the repository.

Yes, this is a drawback I'm quite unhappy with, but I did not see any other 
way of doing it with hg's current code. In my implementation, the creation of 
the repo in the store is managed by the __init__ of the SSHStore class. If 
the "validate_repo" fails, a repo is created on the remote machine.

The only way I see to fix this actually useless machinery is to fix hg's 
sshserve code to be able to be executed without a distant repo. We may thus 
provide a new server command like 'do_connect' to specify a new repo 
location. I'll ivestigate this a bit further however.

For now, it is required to have a repo, even if it is not used for the store 
beside the ability to run a 'hg serve' command from there.

> How did you intend this to be used?  I can see a couple of ways:
>
>   * Run 'hg init' in the central store, but never commit anything
> there.  That is, the store directory will contain an empty .hg
> directory plus all the big files.

As say above, this is the current situation with my code.

>   * Set 'bfiles.store' to the URL of a real Mercurial repository,
> where the actual store is under that repo in .hg/bfiles/store
>
>   * Set 'bfiles.store' to the URL of a real Mercurial repository,
> where the actual store location on the remote machine is configured in
> that repo's bfiles.store.
>
> Am I missing a simpler way?
>
> Greg



-- 
David Douard                        LOGILAB, Paris (France), +33 1 45 32 03 12
Formations Python, Zope, Debian :   http://www.logilab.fr/formations
Développement logiciel sur mesure : http://www.logilab.fr/services
Informatique scientifique :         http://www.logilab.fr/science