[PATCH 1 of 1] sshstore: reimplemented adding a new bfserve command

[Greg Ward]

This looks great.  One procedural note: to avoid distracting/confusing
people looking for patches to Mercurial, can you use "hg email --flag
bfiles"?  So far no one has complained about us polluting the main
devel list with patches for bfiles, so I presume they don't mind too
much.  Flagging the patches will make it even less likely that we'll
annoy anyone.

> +def bfserve(ui, **opts):
> +    """export the bfile Store via SSH
> +    """

Lowercase for consistency, please.

> +    # XXX *snip* method vastly inspired by sshrepository.validate_repo
> +    def validate_cnx(self, ui, sshcmd, args, remotecmd):

What does "cnx" stand for?  (Don't tell me, just expand it in the code.)

Also, "vastly inspired by" should be "heavily based on" or "derived
from" or similar.

> -        if "bfstore" not in self.capabilities:
> -            self.abort(util.Abort(_("remote hg does not support bfiles")))
> +            self.abort(error.RepoError(_("no suitable response from remote hg; check remote hg supports bfiles")))

I changed this from RepoError to Abort for a good reason.  I think
it's because RepoError was swallowed one or two layers up, and the
user got a lousy error message if the central store was not a repo.
That's obsolete, but I *think* Abort still makes more sense here.  I
think.

Also, a better error message would be
  no suitable response from remote hg; check that remote hg supports bfiles
or
  no suitable response from remote hg: does remote hg support bfiles?

Either one works for me.

> +# -- Private helper Store classes --------------------------------------------
> +
> +class sshstoreserver(object):
> +    """A simple ssh server serving files for bfile's sshstore
> +    """
> +    def __init__(self, ui):
> +        self.ui = ui
> +        self.fin = sys.stdin
> +        self.fout = sys.stdout
> +
> +        sys.stdout = sys.stderr
> +
> +        # Prevent insertion/deletion of CRs
> +        util.set_binary(self.fin)
> +        util.set_binary(self.fout)
[...]

Hmmmm.  That class does look awfully familiar.  If there's no good way
to reuse code from mercurial.sshserver, can you at least add a comment
saying 1) "heavily based on" or "derived from" that module and 2) why
it's better to copy than to reuse?

(I'm not convinced that it *is* better to copy!  If it's possible to
do this with a subclass, even if it's a weird subclass that removes
functionality from the parent, I think I would prefer that.  But if
you're uncomfortable doing that, stick with the blatant copy and we'll
see about refactoring in a later patch.)

Other than those minor issues, this looks great!  Can you resend to me
privately?  No need to clutter up the list with a nearly identical
patch to code that not many people are looking at right now.

Thanks!

Greg