SSHStore in bfile

[Greg Ward]

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.

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

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!

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

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.

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.

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.

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.

  * 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