[PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Daniel Barkalow]

If this is set, the url is not required, and the transport always uses
a helper named "git-remote-<value>".

It is a separate configuration option in order to allow a sensible
configuration for foreign systems which either have no meaningful urls
for repositories or which require urls that do not specify the system
used by the repository at that location. However, this only affects
how the name of the helper is determined, not anything about the
interaction with the helper, and the contruction is such that, if the
foreign scm does happen to use a co-named url method, a url with that
method may be used directly.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 Documentation/config.txt |    4 ++++
 remote.c                 |    4 +++-
 remote.h                 |    2 ++
 transport-helper.c       |   14 ++++++++++----
 transport.c              |    5 +++++
 5 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bcaec0d..f0144b1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1380,6 +1380,10 @@ remote.<name>.tagopt::
 	Setting this value to \--no-tags disables automatic tag following when
 	fetching from remote <name>
 
+remote.<name>.vcs::
+	Setting this to a value <vcs> will cause git to interact with
+	the remote with the git-remote-<vcs> helper.
+
 remotes.<group>::
 	The list of remotes which are fetched by "git remote update
 	<group>".  See linkgit:git-remote[1].
diff --git a/remote.c b/remote.c
index e6f5cd2..057ac02 100644
--- a/remote.c
+++ b/remote.c
@@ -50,7 +50,7 @@ static char buffer[BUF_SIZE];
 
 static int valid_remote(const struct remote *remote)
 {
-	return !!remote->url;
+	return remote->url || remote->foreign_vcs;
 }
 
 static const char *alias_url(const char *url)
@@ -427,6 +427,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 	} else if (!strcmp(subkey, ".proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
+	} else if (!strcmp(subkey, ".vcs")) {
+		return git_config_string(&remote->foreign_vcs, key, value);
 	}
 	return 0;
 }
diff --git a/remote.h b/remote.h
index 5db8420..ac0ce2f 100644
--- a/remote.h
+++ b/remote.h
@@ -11,6 +11,8 @@ struct remote {
 	const char *name;
 	int origin;
 
+	const char *foreign_vcs;
+
 	const char **url;
 	int url_nr;
 	int url_alloc;
diff --git a/transport-helper.c b/transport-helper.c
index a5ac575..536dd06 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -162,10 +162,16 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 int transport_helper_init(struct transport *transport)
 {
 	struct helper_data *data = xcalloc(sizeof(*data), 1);
-	char *eom = strchr(transport->url, ':');
-	if (!eom)
-		return -1;
-	data->name = xstrndup(transport->url, eom - transport->url);
+
+	if (transport->remote->foreign_vcs) {
+		data->name = xstrdup(transport->remote->foreign_vcs);
+		transport->url = transport->remote->foreign_vcs;
+	} else {
+		char *eom = strchr(transport->url, ':');
+		if (!eom)
+			return -1;
+		data->name = xstrndup(transport->url, eom - transport->url);
+	}
 
 	transport->data = data;
 	transport->get_refs_list = get_refs_list;
diff --git a/transport.c b/transport.c
index b21e82e..19f330a 100644
--- a/transport.c
+++ b/transport.c
@@ -815,6 +815,11 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		url = remote->url[0];
 	ret->url = url;
 
+	if (remote && remote->foreign_vcs) {
+		transport_helper_init(ret);
+		return ret;
+	}
+
 	if (!prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
-- 
1.6.4.183.g77eb9.dirty

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Johannes Schindelin]

Hi,

On Sun, 9 Aug 2009, Daniel Barkalow wrote:

> If this is set, the url is not required, and the transport always uses
> a helper named "git-remote-<value>".
> 
> It is a separate configuration option in order to allow a sensible
> configuration for foreign systems which either have no meaningful urls
> for repositories or which require urls that do not specify the system
> used by the repository at that location. However, this only affects
> how the name of the helper is determined, not anything about the
> interaction with the helper, and the contruction is such that, if the
> foreign scm does happen to use a co-named url method, a url with that
> method may be used directly.
> 
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> ---

Oh well, just try it often enough.  I give up.

Ciao,
Dscho

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> If this is set, the url is not required, and the transport always uses
> a helper named "git-remote-<value>".
>
> It is a separate configuration option in order to allow a sensible
> configuration for foreign systems which either have no meaningful urls
> for repositories or which require urls that do not specify the system
> used by the repository at that location. However, this only affects
> how the name of the helper is determined, not anything about the
> interaction with the helper, and the contruction is such that, if the
> foreign scm does happen to use a co-named url method, a url with that
> method may be used directly.

Personally, I do not like this.

Why isn't it enough to define the canonical remote name git takes as
"<name of the helper>:<whatever string the helper understands>"?

Then <whatever string the helper understands> part does not have to
resemble URL at all, if the foreign system does not have such a concept
(i.e. "have no meaning urls for repositories").  

Your "let's eject curl based transport out of core" helper (already in
'next') will become something like these in the canonical form:

	curl:http://git.kernel.org/pub/scm/git/git.git
	curl:ftp://git.kernel.org/pub/scm/git/git.git
	curl:https://git.kernel.org/pub/scm/git/git.git

that are handled by a single helper binary git-remote-curl, but nobody has
to use these canonical forms for well-known transports, because we can
have an obvious set of backward-compatible synonyms that are understood by
the transport layer to choose that helper program, so that usual

	http://git.kernel.org/pub/scm/git/git.git

will be understood to choose git-remote-curl backend.  We do not need to
have three git-remote-{http,https,ftp} helpers at all.

That way, a Subversion repository people may want to interact with would
be spelled, if the helper is "git-remote-svn", like this:

	svn:https://scummvm.svn.sourceforge.net/svnroot/scummvm/

and it would be crystal clear that it is not a git native repository that
is accessed over curl based walkers, and also the folks who still have not
migrated to git can simply drop the leading "svn:" and learn the name of
the repository they could access natively with Subversion.  Nobody on the
Subversion side would think svn:https://... is the URL to use with
Subversion (after all they will see that on the communication in the git
circle).

And from the git side, we can tell users: "if you want to interact with
Subversion repositories, you can use traditional git-svn, or you can use
the unified remote mechanism.  To use the latter, just prefix 'svn:' in
front of the URL used to refer to the repository in the Subversion world".

If you spell the "URL" as https://scummvm.svn.sourceforge.net/svnroot/scummvm/
then you wouldn't be able to give that directly to git from the command
line, without using this new configuration.  I do not quite understand why
this indirection is desired.  I think it only confuses users.

I recall somebody earlier mentioned a possibility to have more than one
helper that deals with one type of foreign system.  In such a case, a pair
of URL and vcs configuration can be used to identify what helper to use on
what foreign "repository":

	(https://scummvm.svn.sourceforge.net/svnroot/scummvm/, svn)
	(https://scummvm.svn.sourceforge.net/svnroot/scummvm/, svn-ng)

and it might become easier to switch the helper without changing the URL
part.  But I do not particularly think that would be a practical
advantage, unless we can assume that the next-generation version of the
helper can somehow reuse the metadata the old version of the helper left
in the repository and incrementally operate on the repository.  Even in
that case, I think spelling everything out in a single configuration
(i.e.. remote.$name.url) would make it clearer to see what is going on.

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Daniel Barkalow]

On Sun, 9 Aug 2009, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > If this is set, the url is not required, and the transport always uses
> > a helper named "git-remote-<value>".
> >
> > It is a separate configuration option in order to allow a sensible
> > configuration for foreign systems which either have no meaningful urls
> > for repositories or which require urls that do not specify the system
> > used by the repository at that location. However, this only affects
> > how the name of the helper is determined, not anything about the
> > interaction with the helper, and the contruction is such that, if the
> > foreign scm does happen to use a co-named url method, a url with that
> > method may be used directly.
> 
> Personally, I do not like this.
> 
> Why isn't it enough to define the canonical remote name git takes as
> "<name of the helper>:<whatever string the helper understands>"?

I think that users should never need to know the names of the helpers. I 
mean, the native protocol uses helpers that most users are completely 
unaware of. The users do need to know what the 

> Then <whatever string the helper understands> part does not have to
> resemble URL at all, if the foreign system does not have such a concept
> (i.e. "have no meaning urls for repositories").  

The problem is not that foreign systems don't have "urls" in particular. 
The problem is that there are foreign systems, like perforce, where the 
lines between different repositories in the git sense are drawn in very 
different ways. In perforce, for example, there is a single namespace for 
all branches of all projects hosted on the same server. It's like if 
kernel.org had branches:

 - git-master
 - linux-master
 - git-next
 - linux-next
 - linux-rt
 - git-pu
 - linux-stable
 - sparse-master
(and hundreds more, mixing all the branches of all of the repositories 
for all of the projects, without any conventions beyond what the 
community on the particular server made up)

You really need a different sort of configuration option to specify what 
we think of as "the git.git repository", which includes certain branches 
and doesn't include other projects hosted on kernel.org. And in order to 
find the server, it uses an arbitrary shell command line that acts like 
rsh (and often consists of "ssh" as the command and a bunch of complicated 
options to connect to the right port of the right host to get the right 
restricted shell with the right preset environment). So there's nothing 
you can put in the <whatever string the helper understands> part, because 
the helper really has to figure out what it's doing from a bunch of 
options.

Furthermore, I don't want to just use "p4:" as what people should use in 
remote.*.url in order to select the right helper, because then people are 
going to have different remotes with the same url option value in order to 
access entirely different data.

The only way I've been able to come up with to support this at all 
usefully is to have a bunch of helper-specific options that specify what 
the helper needs to know about the locations you consider to be part of 
the project and an option that tells git that this remote uses the p4 
helper. I'm not sure what makes sense for other helpers, but the case I 
actually use needs something like what's in this patch.

I think it makes sense for svn access to support just having a url 
option like "svn://something (svn native protocol)", or 
"svn+ssh://something (svn protocol over ssh)" or "svn+https://something 
(https access to a svn repo)", or some other similar syntax, but this is a 
poor fit for p4.

In order to support this, there just needs to be a call to check whether 
"remote-<something>" is an available git command (without running it or 
giving an error), and the helper code should be used if it is. This is 
actually required so that people with workstations whose domain is 
.kernel.org and who have cloned "master:/home/linus/something.git" don't 
start getting "remote-master isn't a git command" errors (that is, 
misinterpreting ssh-format location hostnames as helper names. Johan, 
perhaps you could write that for your CVS helper?

> Your "let's eject curl based transport out of core" helper (already in
> 'next') will become something like these in the canonical form:
> 
> 	curl:http://git.kernel.org/pub/scm/git/git.git
> 	curl:ftp://git.kernel.org/pub/scm/git/git.git
> 	curl:https://git.kernel.org/pub/scm/git/git.git
> 
> that are handled by a single helper binary git-remote-curl, but nobody has
> to use these canonical forms for well-known transports, because we can
> have an obvious set of backward-compatible synonyms that are understood by
> the transport layer to choose that helper program, so that usual
> 
> 	http://git.kernel.org/pub/scm/git/git.git
> 
> will be understood to choose git-remote-curl backend.  We do not need to
> have three git-remote-{http,https,ftp} helpers at all.
> 
> That way, a Subversion repository people may want to interact with would
> be spelled, if the helper is "git-remote-svn", like this:
> 
> 	svn:https://scummvm.svn.sourceforge.net/svnroot/scummvm/
> 
> and it would be crystal clear that it is not a git native repository that
> is accessed over curl based walkers, and also the folks who still have not
> migrated to git can simply drop the leading "svn:" and learn the name of
> the repository they could access natively with Subversion.  Nobody on the
> Subversion side would think svn:https://... is the URL to use with
> Subversion (after all they will see that on the communication in the git
> circle).
> 
> And from the git side, we can tell users: "if you want to interact with
> Subversion repositories, you can use traditional git-svn, or you can use
> the unified remote mechanism.  To use the latter, just prefix 'svn:' in
> front of the URL used to refer to the repository in the Subversion world".
> 
> If you spell the "URL" as https://scummvm.svn.sourceforge.net/svnroot/scummvm/
> then you wouldn't be able to give that directly to git from the command
> line, without using this new configuration.  I do not quite understand why
> this indirection is desired.  I think it only confuses users.

As I said above, I think that just picking the start off of the URL is a 
good solution for systems where the remote repository has a location, but 
that's not every system, and not the one that's my personal itch.

> I recall somebody earlier mentioned a possibility to have more than one
> helper that deals with one type of foreign system.  In such a case, a pair
> of URL and vcs configuration can be used to identify what helper to use on
> what foreign "repository":
> 
> 	(https://scummvm.svn.sourceforge.net/svnroot/scummvm/, svn)
> 	(https://scummvm.svn.sourceforge.net/svnroot/scummvm/, svn-ng)
> 
> and it might become easier to switch the helper without changing the URL
> part.  But I do not particularly think that would be a practical
> advantage, unless we can assume that the next-generation version of the
> helper can somehow reuse the metadata the old version of the helper left
> in the repository and incrementally operate on the repository.  Even in
> that case, I think spelling everything out in a single configuration
> (i.e.. remote.$name.url) would make it clearer to see what is going on.

I think that, ideally, helpers for foreign systems would be portable 
across multiple native systems. The svn helper could be a program 
"svn-remote-access-helper", and anything that speaks fast-import (e.g., 
bzr or hg) would be able to use it. When installing it for git, you'd 
symlink it to git-remote-svn; if you decided to install 
"svn-remote-access-helper-ng", you'd change the symlink.

Of course, there's a long way to go before helpers can be portable in this 
direction, but I think it's mostly tractable and good for making the code 
clean.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Johan Herland]

On Monday 10 August 2009, Daniel Barkalow wrote:
> On Sun, 9 Aug 2009, Junio C Hamano wrote:
> > Daniel Barkalow <barkalow@iabervon.org> writes:
> > > If this is set, the url is not required, and the transport always
> > > uses a helper named "git-remote-<value>".
> > >
> > > It is a separate configuration option in order to allow a sensible
> > > configuration for foreign systems which either have no meaningful
> > > urls for repositories or which require urls that do not specify the
> > > system used by the repository at that location. However, this only
> > > affects how the name of the helper is determined, not anything about
> > > the interaction with the helper, and the contruction is such that, if
> > > the foreign scm does happen to use a co-named url method, a url with
> > > that method may be used directly.
> >
> > Personally, I do not like this.
> >
> > Why isn't it enough to define the canonical remote name git takes as
> > "<name of the helper>:<whatever string the helper understands>"?
>
> [...]
>
> The only way I've been able to come up with to support this at all
> usefully is to have a bunch of helper-specific options that specify what
> the helper needs to know about the locations you consider to be part of
> the project and an option that tells git that this remote uses the p4
> helper. I'm not sure what makes sense for other helpers, but the case I
> actually use needs something like what's in this patch.

I'm somewhat agnostic on this issue. At the moment, I follow the P4 cues,
and use a config like this:

    [remote "foo"]
        vcs = cvs
        cvsRoot = ":pserver:user@cvs.server.example.com/var/cvs/cvsroot"
        cvsModule = "bar"
        ...

But I could just as well use a config like this instead:

    [remote "foo"]
        url = "cvs::pserver:user@cvs.server.example.com/var/cvs/cvsroot#bar"
        ...

Either is fine with me, although I suspect users might find the
current/first alternative easier to parse.

> I think it makes sense for svn access to support just having a url
> option like "svn://something (svn native protocol)", or
> "svn+ssh://something (svn protocol over ssh)" or "svn+https://something
> (https access to a svn repo)", or some other similar syntax, but this is
> a poor fit for p4.
>
> In order to support this, there just needs to be a call to check whether
> "remote-<something>" is an available git command (without running it or
> giving an error), and the helper code should be used if it is. This is
> actually required so that people with workstations whose domain is
> .kernel.org and who have cloned "master:/home/linus/something.git" don't
> start getting "remote-master isn't a git command" errors (that is,
> misinterpreting ssh-format location hostnames as helper names. Johan,
> perhaps you could write that for your CVS helper?

Sorry, not following you here. Write exactly what?

- The code in the transport layer for checking if "remote-<something>"
  is an available git command?

- The code in my CVS helper for handling the ssh-format misinterpretation,
  i.e. the case where someone has a git/ssh server called "cvs"? If so,
  how should this be handled?

> I think that, ideally, helpers for foreign systems would be portable
> across multiple native systems. The svn helper could be a program
> "svn-remote-access-helper", and anything that speaks fast-import (e.g.,
> bzr or hg) would be able to use it. When installing it for git, you'd
> symlink it to git-remote-svn; if you decided to install
> "svn-remote-access-helper-ng", you'd change the symlink.

In that case, helpers must keep their metadata in a repo-independent
format. Currently that is outside the scope of my CVS helper, since I'm
leveraging git-notes for most of the CVS helper's metadata.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> I think that users should never need to know the names of the helpers. I 
> mean, the native protocol uses helpers that most users are completely 
> unaware of. The users do need to know what the 

... heck ;-)?

But seriously, why not?

> The problem is not that foreign systems don't have "urls" in particular. 
> The problem is that there are foreign systems, like perforce, where the 
> lines between different repositories in the git sense are drawn in very 
> different ways. In perforce, for example, there is a single namespace for 
> all branches of all projects hosted on the same server. It's like... 
> ...
> (and hundreds more, mixing all the branches of all of the repositories 
> for all of the projects, without any conventions beyond what the 
> community on the particular server made up)

That looks similar to a subversion server with multiple projects with
multiple branches, and they seem to be able to use

    "svn://<whatever svn uses as its own convention>"

just fine.

Why p4 cannot do what svn can?  Doesn't p4 ever "name" a repository/branch
inside that "single namespace" with a simple string that is easy to
remember for users?  And if there is one, I do not see what prevents you
from using "p4://<whatever information p4 users use>"?

> You really need a different sort of configuration option to specify what 
> we think of as "the git.git repository", which includes certain branches 
> and doesn't include other projects hosted on kernel.org. And in order to 
> find the server, it uses an arbitrary shell command line that acts like 
> rsh (and often consists of "ssh" as the command and a bunch of complicated 
> options to connect to the right port of the right host to get the right 
> restricted shell with the right preset environment). So there's nothing 
> you can put in the <whatever string the helper understands> part, because 
> the helper really has to figure out what it's doing from a bunch of 
> options.
>
> Furthermore, I don't want to just use "p4:" as what people should use in 
> remote.*.url in order to select the right helper, because then people are 
> going to have different remotes with the same url option value in order to 
> access entirely different data.

Maybe it is the _primary_ itch you would want to scratch, but this does
sound like a very special case that wants to have multiple pieces of
pre-parsed information to identify a "repository".

Perhaps in such a case it would be equally valid if you said "p4:hello"
and "p4:goodbye" to identify two "repositories", with p4-helper specific
configuration option that is keyed off of of p4.hello.* and p4.goodbye.*
namespace, e.g. (without knowing p4 at all)

	[remote "origin"]
        	url = p4:hello
	[p4 "hello"]
        	find-server-command = ssh bunch of complicated options
		random-p4-option = ...
		random-other-p4-option = ...

Of course it does not have to be called "url", and you could do the same
thing with:

	[remote "origin"]
        	vcs = p4
	[p4 "origin"]
        	find-server-command = ssh bunch of complicated options
		random-p4-option = ...
		random-other-p4-option = ...

And probably there aren't much difference either way between these two
appraoches.  The former gives an extra name ("hello") that may simply be
redundant, or perhaps adds value by descriptive, depending on your
viewpoint.

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Daniel Barkalow]

On Mon, 10 Aug 2009, Johan Herland wrote:

> On Monday 10 August 2009, Daniel Barkalow wrote:
> > On Sun, 9 Aug 2009, Junio C Hamano wrote:
> > > Daniel Barkalow <barkalow@iabervon.org> writes:
> > > > If this is set, the url is not required, and the transport always
> > > > uses a helper named "git-remote-<value>".
> > > >
> > > > It is a separate configuration option in order to allow a sensible
> > > > configuration for foreign systems which either have no meaningful
> > > > urls for repositories or which require urls that do not specify the
> > > > system used by the repository at that location. However, this only
> > > > affects how the name of the helper is determined, not anything about
> > > > the interaction with the helper, and the contruction is such that, if
> > > > the foreign scm does happen to use a co-named url method, a url with
> > > > that method may be used directly.
> > >
> > > Personally, I do not like this.
> > >
> > > Why isn't it enough to define the canonical remote name git takes as
> > > "<name of the helper>:<whatever string the helper understands>"?
> >
> > [...]
> >
> > The only way I've been able to come up with to support this at all
> > usefully is to have a bunch of helper-specific options that specify what
> > the helper needs to know about the locations you consider to be part of
> > the project and an option that tells git that this remote uses the p4
> > helper. I'm not sure what makes sense for other helpers, but the case I
> > actually use needs something like what's in this patch.
> 
> I'm somewhat agnostic on this issue. At the moment, I follow the P4 cues,
> and use a config like this:
> 
>     [remote "foo"]
>         vcs = cvs
>         cvsRoot = ":pserver:user@cvs.server.example.com/var/cvs/cvsroot"
>         cvsModule = "bar"
>         ...
> 
> But I could just as well use a config like this instead:
> 
>     [remote "foo"]
>         url = "cvs::pserver:user@cvs.server.example.com/var/cvs/cvsroot#bar"
>         ...
> 
> Either is fine with me, although I suspect users might find the
> current/first alternative easier to parse.

I suspect there will be some users who want (1) and some who want (2), and 
both ought to work.

> > I think it makes sense for svn access to support just having a url
> > option like "svn://something (svn native protocol)", or
> > "svn+ssh://something (svn protocol over ssh)" or "svn+https://something
> > (https access to a svn repo)", or some other similar syntax, but this is
> > a poor fit for p4.
> >
> > In order to support this, there just needs to be a call to check whether
> > "remote-<something>" is an available git command (without running it or
> > giving an error), and the helper code should be used if it is. This is
> > actually required so that people with workstations whose domain is
> > .kernel.org and who have cloned "master:/home/linus/something.git" don't
> > start getting "remote-master isn't a git command" errors (that is,
> > misinterpreting ssh-format location hostnames as helper names. Johan,
> > perhaps you could write that for your CVS helper?
> 
> Sorry, not following you here. Write exactly what?
> 
> - The code in the transport layer for checking if "remote-<something>"
>   is an available git command?

That's what I was thinking.

> - The code in my CVS helper for handling the ssh-format misinterpretation,
>   i.e. the case where someone has a git/ssh server called "cvs"? If so,
>   how should this be handled?

I'd been thinking that people's servers wouldn't be named "cvs", but they 
might be named things that aren't clearly not plausible names for helpers. 
So I don't think there should be a need to deal with misdirected names 
going to actual helpers, just names for helpers that don't exist.

> > I think that, ideally, helpers for foreign systems would be portable
> > across multiple native systems. The svn helper could be a program
> > "svn-remote-access-helper", and anything that speaks fast-import (e.g.,
> > bzr or hg) would be able to use it. When installing it for git, you'd
> > symlink it to git-remote-svn; if you decided to install
> > "svn-remote-access-helper-ng", you'd change the symlink.
> 
> In that case, helpers must keep their metadata in a repo-independent
> format. Currently that is outside the scope of my CVS helper, since I'm
> leveraging git-notes for most of the CVS helper's metadata.

Yeah, that's one of several tricky issues. I don't think there's enough 
experience yet to design something to support portable helpers, but I 
think it's worth thinking about.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Monday 10 August 2009, Daniel Barkalow wrote:
> ...
>> The only way I've been able to come up with to support this at all
>> usefully is to have a bunch of helper-specific options that specify what
>> the helper needs to know about the locations you consider to be part of
>> the project and an option that tells git that this remote uses the p4
>> helper. I'm not sure what makes sense for other helpers, but the case I
>> actually use needs something like what's in this patch.
>
> I'm somewhat agnostic on this issue. At the moment, I follow the P4 cues,
> and use a config like this:
>
>     [remote "foo"]
>         vcs = cvs
>         cvsRoot = ":pserver:user@cvs.server.example.com/var/cvs/cvsroot"
>         cvsModule = "bar"
>         ...
>
> But I could just as well use a config like this instead:
>
>     [remote "foo"]
>         url = "cvs::pserver:user@cvs.server.example.com/var/cvs/cvsroot#bar"
>         ...
>
> Either is fine with me, although I suspect users might find the
> current/first alternative easier to parse.

Ah, ok, that is a much better (rather, easier to understand for _me_)
example to illustrate what Daniel meant, and I can well imagine P4
counterpart of cvsRoot may resemble an URL even less than your cvsRoot
example does.

If the foreign system uses a single string as "the string to identify
location", like the latter (which is a good example, even though I do not
think a CVS folks write a reference to a module like you wrote), then I
think it makes sense to use that form as remote.$name.url.  But if naming
a location with a single simple string is an alien notion to the foreign
system, I agree with Daniel that we do not gain much by trying to shoehorn
them into a single remote.$name.url configuration.

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Johannes Schindelin]

Hi,

On Mon, 10 Aug 2009, Junio C Hamano wrote:

> Johan Herland <johan@herland.net> writes:
> 
> > I'm somewhat agnostic on this issue. At the moment, I follow the P4 
> > cues, and use a config like this:
> >
> >     [remote "foo"]
> >         vcs = cvs
> >         cvsRoot = ":pserver:user@cvs.server.example.com/var/cvs/cvsroot"
> >         cvsModule = "bar"
> >         ...
> >
> > But I could just as well use a config like this instead:
> >
> >     [remote "foo"]
> >         url = "cvs::pserver:user@cvs.server.example.com/var/cvs/cvsroot#bar"
> >         ...
> >
> > Either is fine with me, although I suspect users might find the
> > current/first alternative easier to parse.
> 
> Ah, ok, that is a much better (rather, easier to understand for _me_) 
> example to illustrate what Daniel meant, and I can well imagine P4 
> counterpart of cvsRoot may resemble an URL even less than your cvsRoot 
> example does.
> 
> If the foreign system uses a single string as "the string to identify
> location", like the latter (which is a good example, even though I do not
> think a CVS folks write a reference to a module like you wrote), then I
> think it makes sense to use that form as remote.$name.url.  But if naming
> a location with a single simple string is an alien notion to the foreign
> system, I agree with Daniel that we do not gain much by trying to shoehorn
> them into a single remote.$name.url configuration.

Of course, it is always nice to be able to tell people: just clone the 
repository with

	git clone cvs::pserver:anonymous@cool.haxx.se:/cvsroot/curl#curl

Rather than telling them: "you know, it is really trivial once you 
understand the inner workings of Git.  Just follow this recipe for the 
moment, and you are all set up:

	1) mkdir curl
	2) cd curl
	3) git init
	4) git remote.origin.vcs cvs
	5) git remote.origin.cvsRoot :pserver:anonymous@cool.haxx.se:/cvsroot/curl
	6) git remote.origin.cvsModule curl
	7) git fetch origin
	8) git checkout --track origin/master

Oh, and please ignore that warning in the last step telling you that you 
are already on branch 'master', that is perfectly normal."

I don't know, but this sounds more and more like the complicator's glove 
to me (with the same reactions).

Ciao,
Dscho

[RFC/PATCH 0/6] Graceful handling of missing remote helpers

[Johan Herland]

On Mon, 10 Aug 2009, Daniel Barkalow wrote:
> > > In order to support this, there just needs to be a call to check whether
> > > "remote-<something>" is an available git command (without running it or
> > > giving an error), and the helper code should be used if it is. This is
> > > actually required so that people with workstations whose domain is
> > > .kernel.org and who have cloned "master:/home/linus/something.git" don't
> > > start getting "remote-master isn't a git command" errors (that is,
> > > misinterpreting ssh-format location hostnames as helper names. Johan,
> > > perhaps you could write that for your CVS helper?
> >
> > Sorry, not following you here. Write exactly what?
> >
> > - The code in the transport layer for checking if "remote-<something>"
> >   is an available git command?
>
> That's what I was thinking.

Ok, I've spent way too much time on this, so if this doesn't cut it, I leave it
up to someone else to pick up the pieces.

The following patch series implements what I understand to be the current
consensus (or at least a good compromise) in this thread:

- There are two ways to indicate that a remote helper should be used:
  1. remote.foo.vcs = <something>
  2. remote.foo.url starts with "<something>:"
  If both are present, remote.foo.vcs is preferred.

- Teach transport_helper_init() to check if "remote-<something>" is an
  available git command. If so, use it. If not, gracefully abort the
  transport-helper setup, and fall back to the "native" transport code.

- Skip the transport-helper code is the remote is obviously "native" (i.e.
  URL starts with "git:"/"file:"/"ssh:"/"rsync:")

AFAICS this should make Daniel happy because his P4 setup will still work, and
it should also make Dscho happy, since he can tell people to
  "git clone cvs::pserver:anonymous@cool.haxx.se:/cvsroot/curl#curl"
(of course, this depends on the CVS helper supporting remote.foo.url, which it
will, in a future iteration)

The non-trivial part of the series is in patch #5.

Implementing the rsync handling as another remote helper program is left as an
exercise for the reader...


Have fun! :)

...Johan


Johan Herland (6):
  Minor unrelated fixes
  transport_get(): Don't SEGFAULT on missing url
  Move setup of curl remote helper from transport.c to transport-helper.c
  Add is_git_command_or_alias() for checking availability of a given git command
  Let transport_helper_init() decide if a remote helper program can be used
  Add testcase to verify handling of missing remote helper programs

 help.c                           |   23 ++++++++++++-
 help.h                           |    2 +
 t/t5590-remote-helper-missing.sh |   56 +++++++++++++++++++++++++++++++
 transport-helper.c               |   67 +++++++++++++++++++++++++++++++++++--
 transport.c                      |   56 ++++++-------------------------
 5 files changed, 154 insertions(+), 50 deletions(-)
 create mode 100755 t/t5590-remote-helper-missing.sh

[RFC/PATCH 1/6] Minor unrelated fixes

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 help.c             |    2 +-
 transport-helper.c |    6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 6c46d8b..994561d 100644
--- a/help.c
+++ b/help.c
@@ -302,7 +302,7 @@ const char *help_unknown_cmd(const char *cmd)
 	struct cmdnames main_cmds, other_cmds;
 
 	memset(&main_cmds, 0, sizeof(main_cmds));
-	memset(&other_cmds, 0, sizeof(main_cmds));
+	memset(&other_cmds, 0, sizeof(other_cmds));
 	memset(&aliases, 0, sizeof(aliases));
 
 	git_config(git_unknown_cmd_config, NULL);
diff --git a/transport-helper.c b/transport-helper.c
index cc99368..a901630 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -8,7 +8,7 @@
 
 struct helper_data
 {
-	const char *name;
+	char *name;
 	struct child_process *helper;
 
 	char *marks_filename;
@@ -258,8 +258,10 @@ int transport_helper_init(struct transport *transport)
 		transport->url = transport->remote->foreign_vcs;
 	} else {
 		char *eom = strchr(transport->url, ':');
-		if (!eom)
+		if (!eom) {
+			free(data);
 			return -1;
+		}
 		data->name = xstrndup(transport->url, eom - transport->url);
 	}
 
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC/PATCH 2/6] transport_get(): Don't SEGFAULT on missing url

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 transport.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/transport.c b/transport.c
index 19f330a..26d9999 100644
--- a/transport.c
+++ b/transport.c
@@ -820,14 +820,15 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		return ret;
 	}
 
-	if (!prefixcmp(url, "rsync:")) {
+	if (url && !prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
 
-	} else if (!prefixcmp(url, "http://")
-	        || !prefixcmp(url, "https://")
-	        || !prefixcmp(url, "ftp://")) {
+	} else if (url
+	        && (!prefixcmp(url, "http://")
+	         || !prefixcmp(url, "https://")
+	         || !prefixcmp(url, "ftp://"))) {
 		transport_helper_init(ret);
 #ifdef NO_CURL
 		error("git was compiled without libcurl support.");
@@ -835,7 +836,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->push = curl_transport_push;
 #endif
 
-	} else if (is_local(url) && is_file(url)) {
+	} else if (url && is_local(url) && is_file(url)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
 		ret->get_refs_list = get_refs_from_bundle;
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC/PATCH 3/6] Move setup of curl remote helper from transport.c to transport-helper.c

[Johan Herland]

Since the curl transport is now launched by the transport-helper mechanism,
it makes sense to move the remaining curl-related code (i.e.
curl_transport_push()) from transport.c into transport-helper.c.

The patch also consolidates the two transport_helper_init() call sites
("foreign" helper and curl helper).

Signed-off-by: Johan Herland <johan@herland.net>
---
 transport-helper.c |   39 +++++++++++++++++++++++++++++++++++++++
 transport.c        |   48 ++++++------------------------------------------
 2 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index a901630..d3ce984 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -249,6 +249,34 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 	return ret;
 }
 
+#ifndef NO_CURL
+static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)
+{
+	const char **argv;
+	int argc;
+
+	if (flags & TRANSPORT_PUSH_MIRROR)
+		return error("http transport does not support mirror mode");
+
+	argv = xmalloc((refspec_nr + 12) * sizeof(char *));
+	argv[0] = "http-push";
+	argc = 1;
+	if (flags & TRANSPORT_PUSH_ALL)
+		argv[argc++] = "--all";
+	if (flags & TRANSPORT_PUSH_FORCE)
+		argv[argc++] = "--force";
+	if (flags & TRANSPORT_PUSH_DRY_RUN)
+		argv[argc++] = "--dry-run";
+	if (flags & TRANSPORT_PUSH_VERBOSE)
+		argv[argc++] = "--verbose";
+	argv[argc++] = transport->url;
+	while (refspec_nr--)
+		argv[argc++] = *refspec++;
+	argv[argc] = NULL;
+	return !!run_command_v_opt(argv, RUN_GIT_CMD);
+}
+#endif
+
 int transport_helper_init(struct transport *transport)
 {
 	struct helper_data *data = xcalloc(sizeof(*data), 1);
@@ -269,5 +297,16 @@ int transport_helper_init(struct transport *transport)
 	transport->get_refs_list = get_refs_list;
 	transport->fetch = fetch;
 	transport->disconnect = disconnect_helper;
+
+	if (!strcmp(data->name, "http")
+	 || !strcmp(data->name, "https")
+	 || !strcmp(data->name, "ftp")) {
+#ifdef NO_CURL
+		error("git was compiled without libcurl support.");
+#else
+		transport->push = curl_transport_push;
+#endif
+	}
+
 	return 0;
 }
diff --git a/transport.c b/transport.c
index 26d9999..81a28bc 100644
--- a/transport.c
+++ b/transport.c
@@ -349,35 +349,6 @@ static int rsync_transport_push(struct transport *transport,
 	return result;
 }
 
-#ifndef NO_CURL
-static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)
-{
-	const char **argv;
-	int argc;
-
-	if (flags & TRANSPORT_PUSH_MIRROR)
-		return error("http transport does not support mirror mode");
-
-	argv = xmalloc((refspec_nr + 12) * sizeof(char *));
-	argv[0] = "http-push";
-	argc = 1;
-	if (flags & TRANSPORT_PUSH_ALL)
-		argv[argc++] = "--all";
-	if (flags & TRANSPORT_PUSH_FORCE)
-		argv[argc++] = "--force";
-	if (flags & TRANSPORT_PUSH_DRY_RUN)
-		argv[argc++] = "--dry-run";
-	if (flags & TRANSPORT_PUSH_VERBOSE)
-		argv[argc++] = "--verbose";
-	argv[argc++] = transport->url;
-	while (refspec_nr--)
-		argv[argc++] = *refspec++;
-	argv[argc] = NULL;
-	return !!run_command_v_opt(argv, RUN_GIT_CMD);
-}
-
-#endif
-
 struct bundle_transport_data {
 	int fd;
 	struct bundle_header header;
@@ -815,26 +786,19 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		url = remote->url[0];
 	ret->url = url;
 
-	if (remote && remote->foreign_vcs) {
-		transport_helper_init(ret);
-		return ret;
-	}
+	if (remote && remote->foreign_vcs)
+		url = NULL;
 
 	if (url && !prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
 
-	} else if (url
-	        && (!prefixcmp(url, "http://")
-	         || !prefixcmp(url, "https://")
-	         || !prefixcmp(url, "ftp://"))) {
+	} else if (!url
+	        || !prefixcmp(url, "http://")
+	        || !prefixcmp(url, "https://")
+	        || !prefixcmp(url, "ftp://")) {
 		transport_helper_init(ret);
-#ifdef NO_CURL
-		error("git was compiled without libcurl support.");
-#else
-		ret->push = curl_transport_push;
-#endif
 
 	} else if (url && is_local(url) && is_file(url)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC/PATCH 4/6] Add is_git_command_or_alias() for checking availability of a given git command

[Johan Herland]

The transport-helper mechanism requires us to gracefully handle the case
where a git command is not available for some reason.

This patch introduces a simple function for querying the availability of a
git command, without attempting to execute said command.

The new function is very similar to the static is_git_command() function in
builtin-help.c, except that this function also accepts a matching alias.

Signed-off-by: Johan Herland <johan@herland.net>
---
 help.c |   21 +++++++++++++++++++++
 help.h |    2 ++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/help.c b/help.c
index 994561d..a616277 100644
--- a/help.c
+++ b/help.c
@@ -296,6 +296,27 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
 	old->names = NULL;
 }
 
+int is_git_command_or_alias(const char *cmd)
+{
+	struct cmdnames main_cmds, other_cmds;
+
+	memset(&main_cmds, 0, sizeof(main_cmds));
+	memset(&other_cmds, 0, sizeof(other_cmds));
+	memset(&aliases, 0, sizeof(aliases));
+
+	git_config(git_unknown_cmd_config, NULL);
+
+	load_command_list("git-", &main_cmds, &other_cmds);
+
+	add_cmd_list(&main_cmds, &aliases);
+	add_cmd_list(&main_cmds, &other_cmds);
+	qsort(main_cmds.names, main_cmds.cnt,
+	      sizeof(main_cmds.names), cmdname_compare);
+	uniq(&main_cmds);
+
+	return is_in_cmdlist(&main_cmds, cmd);
+}
+
 const char *help_unknown_cmd(const char *cmd)
 {
 	int i, n, best_similarity = 0;
diff --git a/help.h b/help.h
index 56bc154..6c43452 100644
--- a/help.h
+++ b/help.h
@@ -26,4 +26,6 @@ int is_in_cmdlist(struct cmdnames *c, const char *s);
 void list_commands(const char *title, struct cmdnames *main_cmds,
 		   struct cmdnames *other_cmds);
 
+int is_git_command_or_alias(const char *cmd);
+
 #endif /* HELP_H */
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC/PATCH 5/6] Let transport_helper_init() decide if a remote helper program can be used

[Johan Herland]

Teach the transport-helper mechanism to verify that the remote helper
program is available, before finalizing the transport setup.
If the remote helper is NOT available, transport_helper_init() returns -1,
and transport_get() falls back to trying a native transport.

This patch introduces a subtle, but important change in the handling of
remotes and their URLs:

Before this patch, we only invoke transport_helper_init() _after_ we find
that the transport-helper mechanism is appropriate for this remote
(i.e. the remote is "foreign", or is handled by the curl helper).

This patch moves the decision point into transport_helper_init(): If the
remote is not obviously using a native transport (URL starts with "file:",
"git:" or "ssh:"), then we first call transport_helper_init(), and only if
it returns error (meaning that no appropriate remote helper program was
found) do we fall back to native transport handling.

Signed-off-by: Johan Herland <johan@herland.net>
---
 transport-helper.c |   22 ++++++++++++++++++++--
 transport.c        |   11 ++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index d3ce984..de30727 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -5,6 +5,7 @@
 #include "commit.h"
 #include "diff.h"
 #include "revision.h"
+#include "help.h"
 
 struct helper_data
 {
@@ -279,11 +280,18 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 
 int transport_helper_init(struct transport *transport)
 {
-	struct helper_data *data = xcalloc(sizeof(*data), 1);
+	struct helper_data *data;
+	struct strbuf buf = STRBUF_INIT;
+	char *cmd;
+
+	if (!transport->remote)
+		return -1;
 
+	data = xcalloc(sizeof(*data), 1);
 	if (transport->remote->foreign_vcs) {
 		data->name = xstrdup(transport->remote->foreign_vcs);
-		transport->url = transport->remote->foreign_vcs;
+		if (!transport->url)
+			transport->url = transport->remote->foreign_vcs;
 	} else {
 		char *eom = strchr(transport->url, ':');
 		if (!eom) {
@@ -293,6 +301,16 @@ int transport_helper_init(struct transport *transport)
 		data->name = xstrndup(transport->url, eom - transport->url);
 	}
 
+	strbuf_addf(&buf, "remote-%s", data->name);
+	cmd = strbuf_detach(&buf, NULL);
+	if (!is_git_command_or_alias(cmd)) {
+		warning("Could not find remote helper command \"git %s\". Assuming native remote.", cmd);
+		free(cmd);
+		free(data->name);
+		free(data);
+		return -1;
+	}
+
 	transport->data = data;
 	transport->get_refs_list = get_refs_list;
 	transport->fetch = fetch;
diff --git a/transport.c b/transport.c
index 81a28bc..b7033eb 100644
--- a/transport.c
+++ b/transport.c
@@ -794,11 +794,12 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
 
-	} else if (!url
-	        || !prefixcmp(url, "http://")
-	        || !prefixcmp(url, "https://")
-	        || !prefixcmp(url, "ftp://")) {
-		transport_helper_init(ret);
+	} else if ((!url
+	         || (prefixcmp(url, "git:")
+	          && prefixcmp(url, "ssh:")
+	          && prefixcmp(url, "file:")))
+	        && !transport_helper_init(ret)) {
+		/* no-op, ret is initialized by transport_helper_init() */
 
 	} else if (url && is_local(url) && is_file(url)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC/PATCH 6/6] Add testcase to verify handling of missing remote helper programs

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t5590-remote-helper-missing.sh |   56 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)
 create mode 100755 t/t5590-remote-helper-missing.sh

diff --git a/t/t5590-remote-helper-missing.sh b/t/t5590-remote-helper-missing.sh
new file mode 100755
index 0000000..3bc2bc2
--- /dev/null
+++ b/t/t5590-remote-helper-missing.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+
+test_description='test graceful failure when missing remote helper program'
+. ./test-lib.sh
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	git config remote.missing.vcs foo &&
+	git remote add missing2 foo:/nonexisting/path
+'
+
+test_expect_success 'fetch changes from "missing" remote' '
+	cat <<EOF >expect &&
+warning: Could not find remote helper command "git remote-foo". Assuming native remote.
+EOF
+	! git fetch missing >actual 2>&1 &&
+	head -n1 actual >actual.first &&
+	test_cmp expect actual.first
+'
+
+test_expect_success 'fetch changes from "missing2" remote' '
+	cat <<EOF >expect &&
+warning: Could not find remote helper command "git remote-foo". Assuming native remote.
+EOF
+	! git fetch missing2 >actual 2>&1 &&
+	head -n1 actual >actual.first &&
+	test_cmp expect actual.first
+'
+
+test_expect_success 'push changes to "missing" remote' '
+	echo "more content" >>file &&
+	git add file &&
+	git commit -m two &&
+	cat <<EOF >expect &&
+warning: Could not find remote helper command "git remote-foo". Assuming native remote.
+EOF
+	! git push --all missing >actual 2>&1 &&
+	head -n1 actual >actual.first &&
+	test_cmp expect actual.first
+'
+
+test_expect_success 'push changes to "missing2" remote' '
+	echo "even more content" >>file &&
+	git add file &&
+	git commit -m three &&
+	cat <<EOF >expect &&
+warning: Could not find remote helper command "git remote-foo". Assuming native remote.
+EOF
+	! git push --all missing2 >actual 2>&1 &&
+	head -n1 actual >actual.first &&
+	test_cmp expect actual.first
+'
+
+test_done
-- 
1.6.4.rc3.138.ga6b98.dirty

Re: [RFC/PATCH 5/6] Let transport_helper_init() decide if a remote helper program can be used

[Johannes Schindelin]

Hi,

On Tue, 11 Aug 2009, Johan Herland wrote:

> diff --git a/transport-helper.c b/transport-helper.c
> index d3ce984..de30727 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -5,6 +5,7 @@
>  #include "commit.h"
>  #include "diff.h"
>  #include "revision.h"
> +#include "help.h"
>  
>  struct helper_data
>  {
> @@ -279,11 +280,18 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
>  
>  int transport_helper_init(struct transport *transport)
>  {
> -	struct helper_data *data = xcalloc(sizeof(*data), 1);
> +	struct helper_data *data;
> +	struct strbuf buf = STRBUF_INIT;
> +	char *cmd;
> +
> +	if (!transport->remote)
> +		return -1;
>  
> +	data = xcalloc(sizeof(*data), 1);
>  	if (transport->remote->foreign_vcs) {
>  		data->name = xstrdup(transport->remote->foreign_vcs);
> -		transport->url = transport->remote->foreign_vcs;
> +		if (!transport->url)
> +			transport->url = transport->remote->foreign_vcs;
>  	} else {
>  		char *eom = strchr(transport->url, ':');
>  		if (!eom) {

IMHO it would be better to decide early if there is no vcs helper, rather 
than doing all kinds of set up, only to free() the data we worked so hard 
in setting up later.  Something like

	if (!transport->remote->foreign_vcs) {
		const char *colon = transport->url ?
			strchr(transport->url, ':') : NULL;

		if (!colon)
			return -1;

		transport->remote->foreign_vcs =
			xstrndup(transport->url, colon - transport->url);
	}

	strbuf_addf(&buf, "remote-%s", transport->remote->foreign_vcs);
	if (!is_git_command_or_alias(buf.buf)) {
		error("Could not find remote helper '%s'", buf.buf);
		strbuf_release(&buf);
		return -1;
	}

> diff --git a/transport.c b/transport.c
> index 81a28bc..b7033eb 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -794,11 +794,12 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  		ret->fetch = fetch_objs_via_rsync;
>  		ret->push = rsync_transport_push;
>  
> -	} else if (!url
> -	        || !prefixcmp(url, "http://")
> -	        || !prefixcmp(url, "https://")
> -	        || !prefixcmp(url, "ftp://")) {
> -		transport_helper_init(ret);
> +	} else if ((!url
> +	         || (prefixcmp(url, "git:")
> +	          && prefixcmp(url, "ssh:")
> +	          && prefixcmp(url, "file:")))
> +	        && !transport_helper_init(ret)) {
> +		/* no-op, ret is initialized by transport_helper_init() */
>  
>  	} else if (url && is_local(url) && is_file(url)) {

Confusing...

When is transport_helper_init(ret) called now?  What is done in the code 
block?  Ah, nothing is done, but we usually write that this way:

		; /* do nothing */

And a comment would have been in order to say that we fall back to native 
Git transport, which will probably silently fail when there is no URL (I 
_know_ that allowing two different ways to specify the same thing are not 
only inconsistent and confusing, they lead to errors -- if not here then 
somewhere else).

Also, you missed two cases mentioned in connect.c: "git+ssh" and 
"ssh+git".  Which brings me to another thing: I'd rather handle the known 
protocols and fall back to a remote helper.  The other way round seems 
pretty backwards.

Sidenote: I have to admit that I find it distracting that "ret" is passed 
to transport_helper_init(), either, it's probably not an "int" but an 
instance of struct transport.)

Ciao,
Dscho

Re: [RFC/PATCH 4/6] Add is_git_command_or_alias() for checking availability of a given git command

[Johannes Schindelin]

Hi,

On Tue, 11 Aug 2009, Johan Herland wrote:

> diff --git a/help.c b/help.c
> index 994561d..a616277 100644
> --- a/help.c
> +++ b/help.c
> @@ -296,6 +296,27 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
>  	old->names = NULL;
>  }
>  
> +int is_git_command_or_alias(const char *cmd)
> +{
> +	struct cmdnames main_cmds, other_cmds;
> +
> +	memset(&main_cmds, 0, sizeof(main_cmds));
> +	memset(&other_cmds, 0, sizeof(other_cmds));
> +	memset(&aliases, 0, sizeof(aliases));
> +
> +	git_config(git_unknown_cmd_config, NULL);
> +
> +	load_command_list("git-", &main_cmds, &other_cmds);
> +
> +	add_cmd_list(&main_cmds, &aliases);
> +	add_cmd_list(&main_cmds, &other_cmds);
> +	qsort(main_cmds.names, main_cmds.cnt,
> +	      sizeof(main_cmds.names), cmdname_compare);
> +	uniq(&main_cmds);
> +
> +	return is_in_cmdlist(&main_cmds, cmd);
> +}

Sorting a list is an O(n log n) operation, searching through a list 
linearly is O(n).  You throw away main_cmds (without freeing) after that, 
so I think it is not a good trade-off.

Ciao,
Dscho

Re: [RFC/PATCH 3/6] Move setup of curl remote helper from transport.c to transport-helper.c

[Johannes Schindelin]

Hi,

On Tue, 11 Aug 2009, Johan Herland wrote:

> diff --git a/transport-helper.c b/transport-helper.c
> index a901630..d3ce984 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -249,6 +249,34 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
>  	return ret;
>  }
>  
> +#ifndef NO_CURL
> +static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)
> +{
> +	const char **argv;
> +	int argc;
> +
> +	if (flags & TRANSPORT_PUSH_MIRROR)
> +		return error("http transport does not support mirror mode");
> +
> +	argv = xmalloc((refspec_nr + 12) * sizeof(char *));
> +	argv[0] = "http-push";
> +	argc = 1;
> +	if (flags & TRANSPORT_PUSH_ALL)
> +		argv[argc++] = "--all";
> +	if (flags & TRANSPORT_PUSH_FORCE)
> +		argv[argc++] = "--force";
> +	if (flags & TRANSPORT_PUSH_DRY_RUN)
> +		argv[argc++] = "--dry-run";
> +	if (flags & TRANSPORT_PUSH_VERBOSE)
> +		argv[argc++] = "--verbose";
> +	argv[argc++] = transport->url;
> +	while (refspec_nr--)
> +		argv[argc++] = *refspec++;
> +	argv[argc] = NULL;
> +	return !!run_command_v_opt(argv, RUN_GIT_CMD);
> +}
> +#endif
> +
>  int transport_helper_init(struct transport *transport)
>  {
>  	struct helper_data *data = xcalloc(sizeof(*data), 1);
> @@ -269,5 +297,16 @@ int transport_helper_init(struct transport *transport)
>  	transport->get_refs_list = get_refs_list;
>  	transport->fetch = fetch;
>  	transport->disconnect = disconnect_helper;
> +
> +	if (!strcmp(data->name, "http")
> +	 || !strcmp(data->name, "https")
> +	 || !strcmp(data->name, "ftp")) {
> +#ifdef NO_CURL
> +		error("git was compiled without libcurl support.");
> +#else
> +		transport->push = curl_transport_push;
> +#endif
> +	}
> +
>  	return 0;
>  }

I still find it rather, uhm, confusing that copying git-http-push and/or 
git-remote-http into place will still give you the error message "Git was 
compiled without libcurl support".

It's just as well that I do not need to maintain RPMs or DEBs.

Ciao,
Dscho

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Bert Wesarg]

On Mon, Aug 10, 2009 at 03:15, Junio C Hamano<gitster@pobox.com> wrote:
> Daniel Barkalow <barkalow@iabervon.org> writes:
>
>> If this is set, the url is not required, and the transport always uses
>> a helper named "git-remote-<value>".
>>
>> It is a separate configuration option in order to allow a sensible
>> configuration for foreign systems which either have no meaningful urls
>> for repositories or which require urls that do not specify the system
>> used by the repository at that location. However, this only affects
>> how the name of the helper is determined, not anything about the
>> interaction with the helper, and the contruction is such that, if the
>> foreign scm does happen to use a co-named url method, a url with that
>> method may be used directly.
>
> Personally, I do not like this.
>
> Why isn't it enough to define the canonical remote name git takes as
> "<name of the helper>:<whatever string the helper understands>"?

May I ask what will happen to these supported URL notations:


       o   [user@]host.xz:/path/to/repo.git/

       o   [user@]host.xz:~user/path/to/repo.git/

       o   [user@]host.xz:path/to/repo.git

this will bite you, if you have an ssh host alias named "<your
favorite helper name>".

Bert

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Johannes Schindelin]

Hi,

On Tue, 11 Aug 2009, Bert Wesarg wrote:

> On Mon, Aug 10, 2009 at 03:15, Junio C Hamano<gitster@pobox.com> wrote:
> > Daniel Barkalow <barkalow@iabervon.org> writes:
> >
> >> If this is set, the url is not required, and the transport always uses
> >> a helper named "git-remote-<value>".
> >>
> >> It is a separate configuration option in order to allow a sensible
> >> configuration for foreign systems which either have no meaningful urls
> >> for repositories or which require urls that do not specify the system
> >> used by the repository at that location. However, this only affects
> >> how the name of the helper is determined, not anything about the
> >> interaction with the helper, and the contruction is such that, if the
> >> foreign scm does happen to use a co-named url method, a url with that
> >> method may be used directly.
> >
> > Personally, I do not like this.
> >
> > Why isn't it enough to define the canonical remote name git takes as
> > "<name of the helper>:<whatever string the helper understands>"?
> 
> May I ask what will happen to these supported URL notations:
> 
> 
>        o   [user@]host.xz:/path/to/repo.git/
> 
>        o   [user@]host.xz:~user/path/to/repo.git/
> 
>        o   [user@]host.xz:path/to/repo.git
> 
> this will bite you, if you have an ssh host alias named "<your
> favorite helper name>".

That is a valid concern.  I'd suggest to disallow @ and . in the protocol 
name (maybe even everything non-alnum).  For shortcuts that really read 
like "svn", I think it is not too much asked for to require adjusting the 
URL (by prefixing ssh:// and adjusting the path).

If there is really anybody who uses Git via ssh to access a server called 
"svn", we could just as well have a little fun with them, don't you agree?

Ciao,
Dscho

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Jeff King]

On Tue, Aug 11, 2009 at 06:20:54PM +0200, Johannes Schindelin wrote:

> >        o   [user@]host.xz:/path/to/repo.git/
> 
> That is a valid concern.  I'd suggest to disallow @ and . in the protocol 
> name (maybe even everything non-alnum).  For shortcuts that really read 
> like "svn", I think it is not too much asked for to require adjusting the 
> URL (by prefixing ssh:// and adjusting the path).
> 
> If there is really anybody who uses Git via ssh to access a server called 
> "svn", we could just as well have a little fun with them, don't you agree?

It is not actually that unreasonable. I have remotes which point to:

  vcs:git/foo.git

where "vcs" is a shorthand for vcs.gtisc.gatech.edu, defined in my
.ssh/config (it's a machine which hosts several different version
control systems). So I could see somebody having something like "svn" if
they were, e.g., hosting a git-to-svn conversion on their company's svn
server.

And as far as asking people in such a situation to change, consider:

  1. No matter how small a change, asking for a change means you are
     breaking people's current config.

  2. ssh:// really is inferior, especially if you are typing it on the
     command-line as part of a one-off pull. It is really convenient to
     assume you start in your home directory (as above), and to be able
     to use tilde-notation to point to the home directories of others
     (e.g., "git pull vcs:~mike/foo.git").

-Peff

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Jeff King]

On Wed, Aug 12, 2009 at 07:59:14AM +0900, Nanako Shiraishi wrote:

> Then how about using a prefix that has been invalid, "vcs::$string",
> for example?

I have no problem with that, and I think it makes it even more visually
obvious what is going. For example:

  svn::http://server/path/to/repo

makes the "svn" prefix jump out a bit more.

-Peff

Re: [RFC/PATCH 5/6] Let transport_helper_init() decide if a remote helper program can be used

[Daniel Barkalow]

On Tue, 11 Aug 2009, Johan Herland wrote:

> @@ -293,6 +301,16 @@ int transport_helper_init(struct transport *transport)
>  		data->name = xstrndup(transport->url, eom - transport->url);
>  	}
>  
> +	strbuf_addf(&buf, "remote-%s", data->name);
> +	cmd = strbuf_detach(&buf, NULL);
> +	if (!is_git_command_or_alias(cmd)) {
> +		warning("Could not find remote helper command \"git %s\". Assuming native remote.", cmd);

Won't this give you a warning for:

	url = master.kernel.org:something

of 'Could not find (...) "git remote-master.kernel.org" (...)'? That 
would be certain to upset some people. I think we can assume that people's 
scheme parts of their URLs that are actually URLs don't contain '.', and 
that people with:

	url = master:something

will append their domains if the warning gets annoying.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Johannes Schindelin]

Hi,

On Tue, 11 Aug 2009, Jeff King wrote:

> On Tue, Aug 11, 2009 at 06:20:54PM +0200, Johannes Schindelin wrote:
> 
> > >        o   [user@]host.xz:/path/to/repo.git/
> > 
> > That is a valid concern.  I'd suggest to disallow @ and . in the protocol 
> > name (maybe even everything non-alnum).  For shortcuts that really read 
> > like "svn", I think it is not too much asked for to require adjusting the 
> > URL (by prefixing ssh:// and adjusting the path).
> > 
> > If there is really anybody who uses Git via ssh to access a server called 
> > "svn", we could just as well have a little fun with them, don't you agree?
> 
> It is not actually that unreasonable. I have remotes which point to:
> 
>   vcs:git/foo.git

That is still not "svn".

> where "vcs" is a shorthand for vcs.gtisc.gatech.edu, defined in my 
> .ssh/config (it's a machine which hosts several different version 
> control systems). So I could see somebody having something like "svn" if 
> they were, e.g., hosting a git-to-svn conversion on their company's svn 
> server.

If _I_ were to judge whether to make it convenient for computer-savvy 
people like you who would have _no_ problem diagnosing the problem (_if_ 
they have the problem, having edited .ssh/config themselves!), who would 
curse briefly, and then go on fixing the problem, or in the alternative 
make it convenient for people who do not know their way around .ssh/config 
as well as you (and who happen to make up the _vast_ majority of Git users 
by now [*1*]), and who would really prefer to have an easy way to clone 
"foreign" repositories, I have _no_ problem deciding which way to go.

So I'm a bastard.  Big news.  But I'm a pragmatic one.

Ciao,
Dscho

Footnote [*1*]: Maybe I was helping to increase the number of Git users 
who do not know .ssh/config with my work on Git for Windows, but that was 
not my original intention.  Maybe I should start to feel better about 
it...

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Johannes Schindelin]

Hi,

On Tue, 11 Aug 2009, Jeff King wrote:

> On Wed, Aug 12, 2009 at 07:59:14AM +0900, Nanako Shiraishi wrote:
> 
> > Then how about using a prefix that has been invalid, "vcs::$string",
> > for example?
> 
> I have no problem with that, and I think it makes it even more visually
> obvious what is going. For example:
> 
>   svn::http://server/path/to/repo
> 
> makes the "svn" prefix jump out a bit more.

... and http:://repo.or.cz/r/git.git ?  Thanks.  But no, thanks.

Ciao,
Dscho

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I have no problem with that, and I think it makes it even more visually
>> obvious what is going. For example:
>> 
>>   svn::http://server/path/to/repo
>> 
>> makes the "svn" prefix jump out a bit more.
>
> ... and http:://repo.or.cz/r/git.git ?  Thanks.  But no, thanks.

Huh?

If you meant a "canonical format that always spells out the name of the
helper, and then whatever string the chosen helper uses to identify the
repository", that would be spelled as:

	libcurl::http://repo.or.cz/r/git.git/

and will be handled by a single helper, git-remote-libcurl, that is
essentially what Linus and Daniel ejected from the builtin.

And in fact, that would be vastly more sensible than "we have one helper
that uses libcurl, but we hide the implementation detail and call the
helper with three names git-remote-{http,https,ftp}, so you would spell
the repository http://repo.or.cz/r/git.git/", which is what we have queued
in 'next/pu'.

And of course the use of "canonical format" for transports that git
traditionally has known about is only for consistency; we would want to
give shortcut for them.  Obviously we would want "http://<anything>" to be
a short-hand for "curl::http://<anything>".

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Jeff King]

On Wed, Aug 12, 2009 at 01:53:38AM +0200, Johannes Schindelin wrote:

> > It is not actually that unreasonable. I have remotes which point to:
> > 
> >   vcs:git/foo.git
> 
> That is still not "svn".

No, but you snipped the part where I explain how that leads me to
believe "svn" is plausible. Remember that you and I are just a
representative sample of a much larger userbase.

There is also a related question: should the the meaning of the URL rely
purely on _syntax_, or must we understand the _semantics_ of the
individual tokens? That is, given $X:$Y, does that syntactically mean
that $X _must_ be a remote helper, or must I understand what helpers git
knows about to know what it is?

I tend to think purely syntactic systems are more robust and easier to
understand. The downside is that it's less DWIM, which can often mean
more typing.

> If _I_ were to judge whether to make it convenient for computer-savvy 
> people like you who would have _no_ problem diagnosing the problem (_if_ 
> they have the problem, having edited .ssh/config themselves!), who would 
> curse briefly, and then go on fixing the problem, or in the alternative 
> make it convenient for people who do not know their way around .ssh/config 
> as well as you (and who happen to make up the _vast_ majority of Git users 
> by now [*1*]), and who would really prefer to have an easy way to clone 
> "foreign" repositories, I have _no_ problem deciding which way to go.
> 
> So I'm a bastard.  Big news.  But I'm a pragmatic one.

You didn't quote the part of my email about how ssh:// sucks. It is not
just about having my config break, figuring it out, and fixing it. You
are losing a useful construct that I might be using on the command line.

That being said, I am not 100% opposed to the proposal. I just think it
is worth considering this breakage as a downside, and considering

  1. Is there some other syntax that _doesn't_ have this breakage
     but that similarly helps the "vast majority of Git users".

  2. Should such a breakage follow a deprecation schedule, and if so,
     what schedule?

-Peff

Re: [RFC/PATCH 5/6] Let transport_helper_init() decide if a remote helper program can be used

[Jeff King]

On Tue, Aug 11, 2009 at 07:28:19PM -0400, Daniel Barkalow wrote:

> of 'Could not find (...) "git remote-master.kernel.org" (...)'? That 
> would be certain to upset some people. I think we can assume that people's 
> scheme parts of their URLs that are actually URLs don't contain '.', and 
> that people with:
> 
> 	url = master:something
> 
> will append their domains if the warning gets annoying.

Keep in mind that these URLs should be usable from the command-line,
too. So it is not just appending the domain in the config, but appending
it every time you want to do a one-off pull.

-Peff

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

>   1. Is there some other syntax that _doesn't_ have this breakage
>      but that similarly helps the "vast majority of Git users".

Well, proposed possible syntax was:

1. <vcs>:<repository location>

   e.g.:

     svn:http://svn.example.com/project

   but

     host:path/to/repo

2. <vcs>::<repository location>

   e.g.

     svn::http://svn.example.com/project

3. <vcs>+<repository location>

   e.g.

     svn+http://svn.example.com/project

   but

     http+svn://svn.example.com/project
     svn+path/to/repo

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC/PATCH 5/6] Let transport_helper_init() decide if a remote helper program can be used

[Daniel Barkalow]

On Wed, 12 Aug 2009, Jeff King wrote:

> On Tue, Aug 11, 2009 at 07:28:19PM -0400, Daniel Barkalow wrote:
> 
> > of 'Could not find (...) "git remote-master.kernel.org" (...)'? That 
> > would be certain to upset some people. I think we can assume that people's 
> > scheme parts of their URLs that are actually URLs don't contain '.', and 
> > that people with:
> > 
> > 	url = master:something
> > 
> > will append their domains if the warning gets annoying.
> 
> Keep in mind that these URLs should be usable from the command-line,
> too. So it is not just appending the domain in the config, but appending
> it every time you want to do a one-off pull.

For that sort of thing, I think that url.*.insteadOf in your global config 
is the way to go. It applies before anything else, and can supply parts of 
the path as well as the hostname.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Jeff King <peff@peff.net> writes:
>
>>   1. Is there some other syntax that _doesn't_ have this breakage
>>      but that similarly helps the "vast majority of Git users".
>
> Well, proposed possible syntax was:
> 1. <vcs>:<repository location>
> ...
> 2. <vcs>::<repository location>
> ...
> 3. <vcs>+<repository location>
>
>    e.g.
>
>      svn+http://svn.example.com/project
>
>    but
>
>      http+svn://svn.example.com/project
>      svn+path/to/repo

I do not think these are valid examples to demonstrate that 3 is bad.

We do not have (and we will not create) "http+svn://" native transport, so
the former can only mean "Feed 'svn://svn.example.com/project' to the vcs
helper whose name is 'http'".  Similarly I do not see any way to read the
latter other than "Feed 'path/to/repo' to 'svn' vcs helper".

We do have a pair of synonyms "git+ssh://foo" and "ssh+git://bar" that
could make the use of '+' ambiguous.  They could be feeding 'ssh://foo'
and 'git://bar' to 'git' and 'ssh' vcs helpers respectively, but

 (1) they are not even advertised in Documentation/ anywhere as far as I
     can see; and

 (2) these are the only two existing ones that are misdesigned, and we can
     easily special case them to keep backward compatibility.

Double-colon (your 2) is also workable.  It probably is slightly better
than plus because it does not have to grandfather "git+ssh" and "ssh+git"
and that would be beneficial for requiring less complexity in both code
(i.e. special case logic) and more importantly in mental burden to the end
users (i.e. '::' would stand out more than '+' and clearly different from
traditional git URLs in all cases).

As Jeff said (your 1.), a single colon ':' has a rather bad ambiguity
between <vcs> and hostname part in the existing scp-style repository
naming.

Re: [RFC/PATCH 2/6] transport_get(): Don't SEGFAULT on missing url

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Signed-off-by: Johan Herland <johan@herland.net>

How does url end up to be NULL?  At the beginning of transport_get(), you
do this:

	ret->remote = remote;
	if (!url && remote && remote->url)
		url = remote->url[0];
	ret->url = url;
	if (remote && remote->foreign_vcs) {
		transport_helper_init(ret);
		return ret;
	}

and the case where remote.$name.vcs is defined, we do not need
remote.$name.url.

When (!url && remote && remote->url), is remote->url[0] allowed to be
NULL?  I am guessing it would be a bug in whoever prepared the remote, and
if that is indeed the case, the patch shifts the symptoms without fixing
the cause.

When (remote && remote->foreign_vcs) does not hold, iow, if no remote is
defined or the remote is defined but lacks remote.$name.url, you will go
to the last else clause in the function that sets up a git_transport_data
for the native transport, but it has ret->url == NULL.  

Whom does that transport talk with?  Is such a transport of any use, or
does it cause a segfault downstream in the call chain?

In other words, I am wondering if this patch should just diagnose the case
as an error, instead of pretending all is well.

Re: [RFC/PATCH 2/6] transport_get(): Don't SEGFAULT on missing url

[Johan Herland]

On Thursday 13 August 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > Signed-off-by: Johan Herland <johan@herland.net>
>
> How does url end up to be NULL?  At the beginning of transport_get(), you
> do this:
>
> 	ret->remote = remote;
> 	if (!url && remote && remote->url)
> 		url = remote->url[0];
> 	ret->url = url;
> 	if (remote && remote->foreign_vcs) {
> 		transport_helper_init(ret);
> 		return ret;
> 	}
>
> and the case where remote.$name.vcs is defined, we do not need
> remote.$name.url.
>
> When (!url && remote && remote->url), is remote->url[0] allowed to be
> NULL?  I am guessing it would be a bug in whoever prepared the remote,
> and if that is indeed the case, the patch shifts the symptoms without
> fixing the cause.
>
> When (remote && remote->foreign_vcs) does not hold, iow, if no remote is
> defined or the remote is defined but lacks remote.$name.url, you will go
> to the last else clause in the function that sets up a git_transport_data
> for the native transport, but it has ret->url == NULL.
>
> Whom does that transport talk with?  Is such a transport of any use, or
> does it cause a segfault downstream in the call chain?
>
> In other words, I am wondering if this patch should just diagnose the
> case as an error, instead of pretending all is well.

You are right. Instead of pushing the segfault downstream, we should verify 
that the url is non-NULL before returning it as part of ret (modulo the 
foreign case, of course).


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs

[Jakub Narebski]

On Wed, 12 August 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>>   1. Is there some other syntax that _doesn't_ have this breakage
>>>      but that similarly helps the "vast majority of Git users".
>>
>> Well, proposed possible syntax was:
>> 1. <vcs>:<repository location>
>> ...
>> 2. <vcs>::<repository location>
>> ...
>> 3. <vcs>+<repository location>
>>
>>    e.g.
>>
>>      svn+http://svn.example.com/project
>>
>>    but
>>
>>      http+svn://svn.example.com/project
>>      svn+path/to/repo
> 
> I do not think these are valid examples to demonstrate that 3 is bad.
> 
> We do not have (and we will not create) "http+svn://" native transport, so
> the former can only mean "Feed 'svn://svn.example.com/project' to the vcs
> helper whose name is 'http'".  Similarly I do not see any way to read the
> latter other than "Feed 'path/to/repo' to 'svn' vcs helper".

And not "Use 'svn+path/to/repo' as local filesystem path to repository?
On the other hand you can always use here './svn+path/to/repo'

> Double-colon (your 2) is also workable.  It probably is slightly better
> than plus because it does not have to grandfather "git+ssh" and "ssh+git"
> and that would be beneficial for requiring less complexity in both code
> (i.e. special case logic) and more importantly in mental burden to the end
> users (i.e. '::' would stand out more than '+' and clearly different from
> traditional git URLs in all cases).
> 
> As Jeff said (your 1.), a single colon ':' has a rather bad ambiguity
> between <vcs> and hostname part in the existing scp-style repository
> naming.

Also double colon is better for scp-like repository location, as e.g.

  svn+example.com:path/to/repo

might be 'path/to/repo' on 'svn+example.com' host; there is no "escape"
mechanism like for './svn+path/to/repo' relative path.  On the other 
hand unescaped ':' cannot be present in hostname, therefore the following
is unambiguous:

  svn::example.com:path/to/repo

But on another hand svn+http://svn.example.com/project/trunk/ looks IMVHO
better that svn::http://svn.example.com/project/trunk/

-- 
Jakub Narebski
Poland