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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html