Re: [RFC PATCH 0/4] git_connect: add some flexibility

[Mike Hommey <mh@glandium.org>]

On Thu, Apr 28, 2016 at 10:41:12AM -0700, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > As you may be aware, I'm working on a git remote helper to access
> > mercurial repositories (https://github.com/glandium/git-cinnabar/).
> >
> > At the moment, a small part is written in C, relying on the git code
> > base, but eventually, there would be more C.
> >
> > As I want to get rid of the dependency on Mercurial itself, I'm planning
> > to implement the wire protocol parts in git-cinnabar.
> 
> While all of the above sounds like a good thing to do, what I do not
> understand is why you need to even touch git_connect() at all, and
> we certainly do *not* want you to touch it to make its external
> interface unnecessarily ugly and complex with features that are only
> necessary if it needs to talk to non-git services.
> 
> In other words, why can't this cinnabar thing live on the other side
> of transport API, just like Git transport itself does not know about
> cURL and HTTP when talking with https:// repositories?

It does live on the other side of transport API. The changes are not
about git itself talking to mercurial servers. They are about a remote
helper (git-cinnabar), using native code based on git core code, to talk
to mercurial servers. Because I'd rather bend git_connect a little, if I
can, than copy it (as well as parse_connect_url). Because I want to
benefit from all tweaks it has (handling GIT_SSH/GIT_SSH_COMMAND
properly, handling tortoiseplink/plink properly, handling IPv6 forms of
the ssh url properly, ...)

Now, tweaking git_connect is one possible way to do what I want. Another
would be to make parse_connect_url non-static, and move the ssh command
line construction to a separate (non-static) function. Something with a
signature like:
  void prepare_ssh_command(struct argv_array **command,
                           const char *hostandport)

that would essentially do the part of git_connect that is in the if
(protocol == PROTO_SSH) block (not sure how CONNECT_DIAG_URL would fit
in this, though).

Thoughts?

Mike