[RFC PATCH 0/8] Git remote helpers to implement smart transports.

[RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

This series implements extensions to remote helpers for carrying smary
transports. It is against next, because master doesn't contain necressary
patches (the allow specifying remote helper in url one).

First patch reworks URL handling so that unknown protocols are passed
to remote helpers. This allows having remote helpers implement git
transports without duplicating the protocol part.

Second patch refactors git transport option parsing to split smart
transport option to own structure and keep this structure up to date
with encountered options. This is needed if transport turns out to
be smart transport.

Third patch adds capabilty to have git smart transport code take
over connection, replacing "layer 7" with git smart transport protocols.

Fourth patch actually adds the extensions to external transport code to
allow helpers signal that transport should be taken over (become smart
transport).

Fifth patch extends 'git archive' to allow snapshotting off any transport
that uses git smart transport code, not just file://, git:// and ssh://

Sixth patch removes special casing of http, https and ftp. And while
at it, adds ftps, since CURL supports it.

Seventh patch adds debug mode for remote helpers. Might be useful for
debugging deadlocks by showing command traffic between git executable
and remote helper.

Eighth patch adds support for remote helper to signal that it requires
some capability and have git complain if it doesn't know it.


Misc remarks:

Underlying network link is assumed to be full-duplex since most of the
time if the underlying link isn't HTTP, it will be full-duplex (most of the
time even TCP).

Simplest deadlock-free buffering is just to read incoming pipe from git
when there's no data to send to remote end. This gives adequate performance
in all cases except when sending large initial ref adverts (and those are 
ended by flush anyway, so those can be safely buffered). So no extensions
to add missing flushes are needed.

Ilari Liusvaara (8):
  Pass unknown protocols to external protocol handlers
  Refactor git transport options parsing
  Support taking over transports
  Support remote helpers implementing smart transports
  Support remote archive from external protocol helpers
  Remove special casing of http, https and ftp
  Add remote helper debug mode
  Support mandatory capabilities

 .gitignore                           |    5 +-
 Documentation/git-remote-helpers.txt |   35 ++++-
 Makefile                             |   16 ++-
 builtin-archive.c                    |   17 ++-
 transport-helper.c                   |  270 ++++++++++++++++++++++++++++-----
 transport.c                          |  258 ++++++++++++++++++++++++++------
 transport.h                          |   32 ++++
 7 files changed, 533 insertions(+), 100 deletions(-)

[RFC PATCH 1/8] Pass unknown protocols to external protocol handlers

[Ilari Liusvaara]

Change URL handling to allow external protocol handlers to provde
new protocols without the '::' syntax if helper name does not conflict
with any builtin protocol.

foo:// now invokes git-remote-foo with foo:// URL.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport-helper.c |   57 ++++++++++++++++++++++++++++++++----
 transport.c        |   82 +++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 122 insertions(+), 17 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 6182413..a499751 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -22,6 +22,26 @@ struct helper_data
 	int refspec_nr;
 };
 
+const char* remove_ext_force(const char* url)
+{
+	const char* url2 = url;
+	const char* first_colon = NULL;
+
+	if(!url)
+		return NULL;
+
+	while(*url2 && !first_colon)
+		if(*url2 == ':')
+			first_colon = url2;
+		else
+			url2++;
+
+	if(first_colon && first_colon[1] == ':')
+		return first_colon + 2;
+	else
+		return url;
+}
+
 static struct child_process *get_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
@@ -30,6 +50,8 @@ static struct child_process *get_helper(struct transport *transport)
 	const char **refspecs = NULL;
 	int refspec_nr = 0;
 	int refspec_alloc = 0;
+	int duped;
+	int seen_line = 0;
 
 	if (data->helper)
 		return data->helper;
@@ -39,21 +61,43 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->out = -1;
 	helper->err = 0;
 	helper->argv = xcalloc(4, sizeof(*helper->argv));
-	strbuf_addf(&buf, "remote-%s", data->name);
+	/* We use the dashed form because git <unknown helper>
+	   would run and print totally inapporiate error message. */
+	strbuf_addf(&buf, "git-remote-%s", data->name);
 	helper->argv[0] = strbuf_detach(&buf, NULL);
 	helper->argv[1] = transport->remote->name;
-	helper->argv[2] = transport->url;
-	helper->git_cmd = 1;
+	helper->argv[2] = remove_ext_force(transport->url);
+	helper->git_cmd = 0;
 	if (start_command(helper))
-		die("Unable to run helper: git %s", helper->argv[0]);
+		die("Unable to run helper: %s", helper->argv[0]);
 	data->helper = helper;
 
+	/* Open the output as FILE* so strbuf_getline() can be used.
+	   Do this with duped fd because fclose() will close the fd,
+	   and stuff like disowning will require the fd to remain.
+
+	   Set the stream to unbuffered because some reads are critical
+	   in sense that any overreading will cause deadlocks.
+	*/
+	if((duped = dup(helper->out)) < 0)
+		die_errno("Can't dup helper output fd");
+	data->out = xfdopen(duped, "r");
+	setvbuf(data->out, NULL, _IONBF, 0);
+
 	write_str_in_full(helper->in, "capabilities\n");
 
-	data->out = xfdopen(helper->out, "r");
 	while (1) {
-		if (strbuf_getline(&buf, data->out, '\n') == EOF)
+		if (strbuf_getline(&buf, data->out, '\n') == EOF) {
+			/* If we haven't seen line yet, try to finish the
+			   command so we get error message about failed
+			   execution. */
+			if(!seen_line)
+				finish_command(helper);
+
 			exit(128); /* child died, message supplied already */
+		}
+
+		seen_line = 1;
 
 		if (!*buf.buf)
 			break;
@@ -91,6 +135,7 @@ static int disconnect_helper(struct transport *transport)
 	if (data->helper) {
 		write_str_in_full(data->helper->in, "\n");
 		close(data->helper->in);
+		close(data->helper->out);
 		fclose(data->out);
 		finish_command(data->helper);
 		free((char *)data->helper->argv[0]);
diff --git a/transport.c b/transport.c
index 3eea836..148b9e1 100644
--- a/transport.c
+++ b/transport.c
@@ -780,6 +780,55 @@ static int is_file(const char *url)
 	return S_ISREG(buf.st_mode);
 }
 
+static const char* strchrc(const char* str, int c)
+{
+	while(*str)
+		if(*str == c)
+			return str;
+		else
+			str++;
+	return NULL;
+}
+
+static int is_url(const char* url)
+{
+	if(!url)
+		return 0;
+
+	const char* url2 = url;
+	const char* first_slash = strchrc(url, '/');
+
+	/* Input with no slash at all or slash first can't be URL. */
+	if(!first_slash || first_slash == url)
+		return 0;
+	/* Character before must be : and next must be /. */
+	if(first_slash[-1] != ':' || first_slash[1] != '/')
+		return 0;
+	/* There must be something before the :// */
+	if(first_slash == url + 1)
+		return 0;
+	/* Check all characters up to first slash. Only alpha, num and
+	   : are allowed. : must be followed by : or / */
+	url2 = url;
+	while(url2 < first_slash) {
+		if(*url2 != ':' && !isalnum((unsigned char)*url2))
+			return 0;
+		if(*url2 == ':' && url2[1] != ':' && url2[1] != '/')
+			return 0;
+		if(*url2 == ':')
+			url2++;		/* Skip second : */
+		url2++;
+	}
+
+	/* Valid enough. */
+	return 1;
+}
+
+static int external_specification_len(const char* url)
+{
+	return strchrc(url, ':') - url;
+}
+
 struct transport *transport_get(struct remote *remote, const char *url)
 {
 	struct transport *ret = xcalloc(1, sizeof(*ret));
@@ -812,23 +861,19 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		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://")) {
-		transport_helper_init(ret, "curl");
-#ifdef NO_CURL
-		error("git was compiled without libcurl support.");
-#endif
-
 	} else if (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;
 		ret->fetch = fetch_refs_from_bundle;
 		ret->disconnect = close_bundle;
-
-	} else {
+	} else if(!is_url(url)
+		|| !prefixcmp(url, "file://")
+		|| !prefixcmp(url, "git://")
+		|| !prefixcmp(url, "ssh://")
+		|| !prefixcmp(url, "git+ssh://")
+		|| !prefixcmp(url, "ssh+git://")) {
+		/* These are builtin smart transports. */
 		struct git_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
 		ret->set_option = set_git_option;
@@ -845,6 +890,21 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		data->receivepack = "git-receive-pack";
 		if (remote->receivepack)
 			data->receivepack = remote->receivepack;
+	} else if (!prefixcmp(url, "http://")
+		|| !prefixcmp(url, "https://")
+		|| !prefixcmp(url, "ftp://")) {
+		/* These three are just plain special. */
+		transport_helper_init(ret, "curl");
+#ifdef NO_CURL
+		error("git was compiled without libcurl support.");
+#else
+	} else {
+		/* Unknown protocol in URL. Pass to external handler. */
+		int len = external_specification_len(url);
+		char* handler = xmalloc(len + 1);
+		handler[len] = 0;
+		strncpy(handler, url, len);
+		transport_helper_init(ret, handler);
 	}
 
 	return ret;
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 2/8] Refactor git transport options parsing

[Ilari Liusvaara]

Refactor the transport options parsing so that protocols that aren't
directly smart transports (file://, git://, ssh:// & co) can record
the smart transport options for the case if it turns that transport
can actually be smart.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport.c |   78 +++++++++++++++++++++++++++++++++++-----------------------
 transport.h |   12 +++++++++
 2 files changed, 59 insertions(+), 31 deletions(-)

diff --git a/transport.c b/transport.c
index 148b9e1..7956892 100644
--- a/transport.c
+++ b/transport.c
@@ -395,41 +395,35 @@ static int close_bundle(struct transport *transport)
 }
 
 struct git_transport_data {
-	unsigned thin : 1;
-	unsigned keep : 1;
-	unsigned followtags : 1;
-	int depth;
+	struct git_transport_options options;
 	struct child_process *conn;
 	int fd[2];
-	const char *uploadpack;
-	const char *receivepack;
 	struct extra_have_objects extra_have;
 };
 
-static int set_git_option(struct transport *connection,
+static int set_git_option(struct git_transport_options *opts,
 			  const char *name, const char *value)
 {
-	struct git_transport_data *data = connection->data;
 	if (!strcmp(name, TRANS_OPT_UPLOADPACK)) {
-		data->uploadpack = value;
+		opts->uploadpack = value;
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_RECEIVEPACK)) {
-		data->receivepack = value;
+		opts->receivepack = value;
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_THIN)) {
-		data->thin = !!value;
+		opts->thin = !!value;
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_FOLLOWTAGS)) {
-		data->followtags = !!value;
+		opts->followtags = !!value;
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_KEEP)) {
-		data->keep = !!value;
+		opts->keep = !!value;
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_DEPTH)) {
 		if (!value)
-			data->depth = 0;
+			opts->depth = 0;
 		else
-			data->depth = atoi(value);
+			opts->depth = atoi(value);
 		return 0;
 	}
 	return 1;
@@ -439,7 +433,8 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
 {
 	struct git_transport_data *data = transport->data;
 	data->conn = git_connect(data->fd, transport->url,
-				 for_push ? data->receivepack : data->uploadpack,
+				 for_push ? data->options.receivepack :
+				 data->options.uploadpack,
 				 verbose ? CONNECT_VERBOSE : 0);
 	return 0;
 }
@@ -469,15 +464,15 @@ static int fetch_refs_via_pack(struct transport *transport,
 	struct ref *refs_tmp = NULL;
 
 	memset(&args, 0, sizeof(args));
-	args.uploadpack = data->uploadpack;
-	args.keep_pack = data->keep;
+	args.uploadpack = data->options.uploadpack;
+	args.keep_pack = data->options.keep;
 	args.lock_pack = 1;
-	args.use_thin_pack = data->thin;
-	args.include_tag = data->followtags;
+	args.use_thin_pack = data->options.thin;
+	args.include_tag = data->options.followtags;
 	args.verbose = (transport->verbose > 0);
 	args.quiet = (transport->verbose < 0);
 	args.no_progress = args.quiet || (!transport->progress && !isatty(1));
-	args.depth = data->depth;
+	args.depth = data->options.depth;
 
 	for (i = 0; i < nr_heads; i++)
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
@@ -734,7 +729,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	memset(&args, 0, sizeof(args));
 	args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
 	args.force_update = !!(flags & TRANSPORT_PUSH_FORCE);
-	args.use_thin_pack = data->thin;
+	args.use_thin_pack = data->options.thin;
 	args.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);
 	args.quiet = !!(flags & TRANSPORT_PUSH_QUIET);
 	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
@@ -861,12 +856,14 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
+		ret->smart_options = NULL;
 	} else if (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;
 		ret->fetch = fetch_refs_from_bundle;
 		ret->disconnect = close_bundle;
+		ret->smart_options = NULL;
 	} else if(!is_url(url)
 		|| !prefixcmp(url, "file://")
 		|| !prefixcmp(url, "git://")
@@ -876,20 +873,14 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		/* These are builtin smart transports. */
 		struct git_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
-		ret->set_option = set_git_option;
+		ret->set_option = NULL;
 		ret->get_refs_list = get_refs_via_connect;
 		ret->fetch = fetch_refs_via_pack;
 		ret->push_refs = git_transport_push;
 		ret->disconnect = disconnect_git;
+		ret->smart_options = &(data->options);
 
-		data->thin = 1;
 		data->conn = NULL;
-		data->uploadpack = "git-upload-pack";
-		if (remote->uploadpack)
-			data->uploadpack = remote->uploadpack;
-		data->receivepack = "git-receive-pack";
-		if (remote->receivepack)
-			data->receivepack = remote->receivepack;
 	} else if (!prefixcmp(url, "http://")
 		|| !prefixcmp(url, "https://")
 		|| !prefixcmp(url, "ftp://")) {
@@ -907,14 +898,39 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		transport_helper_init(ret, handler);
 	}
 
+	if(ret->smart_options) {
+		ret->smart_options->thin = 1;
+		ret->smart_options->uploadpack = "git-upload-pack";
+		if (remote->uploadpack)
+			ret->smart_options->uploadpack = remote->uploadpack;
+		ret->smart_options->receivepack = "git-receive-pack";
+		if (remote->receivepack)
+			ret->smart_options->receivepack = remote->receivepack;
+	}
+
 	return ret;
 }
 
 int transport_set_option(struct transport *transport,
 			 const char *name, const char *value)
 {
+	int git_reports = 1, protocol_reports = 1;
+
+	if (transport->smart_options)
+		git_reports = set_git_option(transport->smart_options,
+					     name, value);
+
 	if (transport->set_option)
-		return transport->set_option(transport, name, value);
+		protocol_reports = transport->set_option(transport, name,
+							value);
+
+	/* If either report is 0, report 0 (success). */
+	if(!git_reports || !protocol_reports)
+		return 0;
+	/* If either reports -1 (invalid value), report -1. */
+	if((git_reports == -1) || (protocol_reports == -1))
+		return -1;
+	/* Otherwise if both report unknown, report unknown. */
 	return 1;
 }
 
diff --git a/transport.h b/transport.h
index 9e74406..5949132 100644
--- a/transport.h
+++ b/transport.h
@@ -4,6 +4,15 @@
 #include "cache.h"
 #include "remote.h"
 
+struct git_transport_options {
+	unsigned thin : 1;
+	unsigned keep : 1;
+	unsigned followtags : 1;
+	int depth;
+	const char *uploadpack;
+	const char *receivepack;
+};
+
 struct transport {
 	struct remote *remote;
 	const char *url;
@@ -65,6 +74,9 @@ struct transport {
 	signed verbose : 3;
 	/* Force progress even if the output is not a tty */
 	unsigned progress : 1;
+	/* If transport is at least potentially smart, this points to git_transport_options
+	   structure to use in case transport actually turns out to be smart. */
+	struct git_transport_options* smart_options;
 };
 
 #define TRANSPORT_PUSH_ALL 1
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 3/8] Support taking over transports

[Ilari Liusvaara]

Add support for taking over transports that turn out to be smart.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport-helper.c |   12 +++++++
 transport.c        |   89 +++++++++++++++++++++++++++++++++++++++++++++++----
 transport.h        |   15 +++++++++
 3 files changed, 109 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index a499751..777ecbb 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -20,8 +20,18 @@ struct helper_data
 	/* These go from remote name (as in "list") to private name */
 	struct refspec *refspecs;
 	int refspec_nr;
+	struct git_transport_options gitoptions;
 };
 
+static struct child_process* helper_disown(struct transport *transport)
+{
+	struct helper_data *data = transport->data;
+	struct child_process *child = data->helper;
+	fclose(data->out);
+	free(data);
+	return child;
+}
+
 const char* remove_ext_force(const char* url)
 {
 	const char* url2 = url;
@@ -562,5 +572,7 @@ int transport_helper_init(struct transport *transport, const char *name)
 	transport->fetch = fetch;
 	transport->push_refs = push_refs;
 	transport->disconnect = release_helper;
+	transport->disown = helper_disown;
+	transport->smart_options = &(data->gitoptions);
 	return 0;
 }
diff --git a/transport.c b/transport.c
index 7956892..09e4c97 100644
--- a/transport.c
+++ b/transport.c
@@ -9,6 +9,8 @@
 #include "dir.h"
 #include "refs.h"
 
+struct ref special_transport_layer6_ready;
+
 /* rsync support */
 
 /*
@@ -398,6 +400,8 @@ struct git_transport_data {
 	struct git_transport_options options;
 	struct child_process *conn;
 	int fd[2];
+	/* Connection is fully up. */
+	unsigned virtual_connected : 1;
 	struct extra_have_objects extra_have;
 };
 
@@ -432,10 +436,21 @@ static int set_git_option(struct git_transport_options *opts,
 static int connect_setup(struct transport *transport, int for_push, int verbose)
 {
 	struct git_transport_data *data = transport->data;
+
+	if(!data->virtual_connected && data->conn) {
+		/* Just mark it connected. */
+		data->virtual_connected = 1;
+		return 0;
+	}
+
 	data->conn = git_connect(data->fd, transport->url,
 				 for_push ? data->options.receivepack :
 				 data->options.uploadpack,
 				 verbose ? CONNECT_VERBOSE : 0);
+
+	if(data->conn)
+		data->virtual_connected = 1;
+
 	return 0;
 }
 
@@ -477,7 +492,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	for (i = 0; i < nr_heads; i++)
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
 
-	if (!data->conn) {
+	if (!data->virtual_connected) {
 		connect_setup(transport, 0, 0);
 		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
 	}
@@ -490,6 +505,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	if (finish_connect(data->conn))
 		refs = NULL;
 	data->conn = NULL;
+	data->virtual_connected = 0;
 
 	free_refs(refs_tmp);
 
@@ -718,7 +734,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	struct send_pack_args args;
 	int ret;
 
-	if (!data->conn) {
+	if (!data->virtual_connected) {
 		struct ref *tmp_refs;
 		connect_setup(transport, 1, 0);
 
@@ -741,6 +757,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	close(data->fd[0]);
 	ret |= finish_connect(data->conn);
 	data->conn = NULL;
+	data->virtual_connected = 0;
 
 	return ret;
 }
@@ -749,7 +766,8 @@ static int disconnect_git(struct transport *transport)
 {
 	struct git_transport_data *data = transport->data;
 	if (data->conn) {
-		packet_flush(data->fd[1]);
+		if(data->virtual_connected)
+			packet_flush(data->fd[1]);
 		close(data->fd[0]);
 		close(data->fd[1]);
 		finish_connect(data->conn);
@@ -759,6 +777,35 @@ static int disconnect_git(struct transport *transport)
 	return 0;
 }
 
+static void git_take_over_transport(struct transport *transport)
+{
+	struct git_transport_data *data;
+
+	if(!transport->disown)
+		die("Bug detected: Taking over transport requires non-NULL "
+		    "disown method.");
+	if(!transport->smart_options)
+		die("Bug detected: Taking over transport requires non-NULL "
+		    "smart_options field.");
+
+	data = xcalloc(1, sizeof(*data));
+	data->options = *transport->smart_options;
+	data->conn = transport->disown(transport);
+	data->fd[0] = data->conn->out;
+	data->fd[1] = data->conn->in;
+	data->virtual_connected = 0;
+	transport->data = data;
+
+	transport->set_option = NULL;
+	transport->get_refs_list = get_refs_via_connect;
+	transport->fetch = fetch_refs_via_pack;
+	transport->push = NULL;
+	transport->push_refs = git_transport_push;
+	transport->disconnect = disconnect_git;
+	transport->smart_options = &(data->options);
+	transport->disown = NULL;
+}
+
 static int is_local(const char *url)
 {
 	const char *colon = strchr(url, ':');
@@ -857,6 +904,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
 		ret->smart_options = NULL;
+		ret->disown = NULL;
 	} else if (is_local(url) && is_file(url)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
@@ -864,6 +912,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->fetch = fetch_refs_from_bundle;
 		ret->disconnect = close_bundle;
 		ret->smart_options = NULL;
+		ret->disown = NULL;
 	} else if(!is_url(url)
 		|| !prefixcmp(url, "file://")
 		|| !prefixcmp(url, "git://")
@@ -879,8 +928,10 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->push_refs = git_transport_push;
 		ret->disconnect = disconnect_git;
 		ret->smart_options = &(data->options);
+		ret->disown = NULL;
 
 		data->conn = NULL;
+		data->virtual_connected = 0;
 	} else if (!prefixcmp(url, "http://")
 		|| !prefixcmp(url, "https://")
 		|| !prefixcmp(url, "ftp://")) {
@@ -938,14 +989,25 @@ int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
 		   int *nonfastforward)
 {
+	int rc = 0;
 	*nonfastforward = 0;
 	verify_remote_names(refspec_nr, refspec);
 
-	if (transport->push)
-		return transport->push(transport, refspec_nr, refspec, flags);
-	if (transport->push_refs) {
+retry:
+	if (transport->push) {
+		rc = transport->push(transport, refspec_nr, refspec, flags);
+		if(rc == TRANSPORT_LAYER6_READY) {
+			git_take_over_transport(transport);
+			goto retry;
+		}
+		return rc;
+	} else if (transport->push_refs) {
 		struct ref *remote_refs =
 			transport->get_refs_list(transport, 1);
+		if(remote_refs == &special_transport_layer6_ready) {
+			git_take_over_transport(transport);
+			goto retry;
+		}
 		struct ref *local_refs = get_local_heads();
 		int match_flags = MATCH_REFS_NONE;
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
@@ -985,8 +1047,15 @@ int transport_push(struct transport *transport,
 
 const struct ref *transport_get_remote_refs(struct transport *transport)
 {
-	if (!transport->remote_refs)
+	if (!transport->remote_refs) {
+retry:
 		transport->remote_refs = transport->get_refs_list(transport, 0);
+		if(transport->remote_refs == &special_transport_layer6_ready) {
+			git_take_over_transport(transport);
+			goto retry;
+		}
+	}
+
 	return transport->remote_refs;
 }
 
@@ -1020,7 +1089,13 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
 			heads[nr_heads++] = rm;
 	}
 
+retry:
 	rc = transport->fetch(transport, nr_heads, heads);
+	if(rc == TRANSPORT_LAYER6_READY) {
+		git_take_over_transport(transport);
+		goto retry;
+	}
+
 	free(heads);
 	return rc;
 }
diff --git a/transport.h b/transport.h
index 5949132..f3ee890 100644
--- a/transport.h
+++ b/transport.h
@@ -65,6 +65,15 @@ struct transport {
 	int (*push_refs)(struct transport *transport, struct ref *refs, int flags);
 	int (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);
 
+	/**
+	 * Disown the transport helper. Releases all resources used
+	 * by field pointed by member data, except that the child
+	 * process is not released but returned and whatever is pointed
+	 * by smart transport options structure is not freed (but the
+	 * smart transport options structure itself is).
+	 **/
+	struct child_process* (*disown)(struct transport* connection);
+
 	/** get_refs_list(), fetch(), and push_refs() can keep
 	 * resources (such as a connection) reserved for futher
 	 * use. disconnect() releases these resources.
@@ -79,6 +88,12 @@ struct transport {
 	struct git_transport_options* smart_options;
 };
 
+/* Returned by get_refs_list, fetch or push methods of struct transport: Layer 6 is ready,
+   take over the transport and retry operation. */
+#define TRANSPORT_LAYER6_READY -42
+extern struct ref special_transport_layer6_ready;
+
+
 #define TRANSPORT_PUSH_ALL 1
 #define TRANSPORT_PUSH_FORCE 2
 #define TRANSPORT_PUSH_DRY_RUN 4
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 4/8] Support remote helpers implementing smart transports

[Ilari Liusvaara]

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 Documentation/git-remote-helpers.txt |   30 ++++++++++-
 transport-helper.c                   |   94 +++++++++++++++++++++++++++++++--
 transport.c                          |   21 ++++++++
 transport.h                          |    5 ++
 4 files changed, 143 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 5cfdc0c..adf815c 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -90,6 +90,28 @@ Supported if the helper has the "push" capability.
 +
 Supported if the helper has the "import" capability.
 
+'connect-r' <service>::
+	Connects to given service. Stdin and stdout of helper are
+	connected to specified service (no git or git- prefixes are used,
+	so e.g. fetching uses 'upload-pack' as service) on remote side.
+	Valid replies to this command are 'OK' (connection established),
+	'FALLBACK' (no smart transport support, fall back to dumb
+	transports) and 'ERROR' (can't connect, don't bother trying to
+	fall back). After line feed terminating the OK response, the
+	output of service starts. After the connection ends, the remote
+	helper exits. Note that to prevent deadlocking, all read data
+	should be immediately flushed to outgoing connection.
++
+Supported if the helper has the "connect-r" capability. Not used if
+helper has the "invoke-r" capability, as invoke is preferred to connect.
+
+'invoke-r' <cmdlength> <cmd>::
+	Like connect-r command, but instead of service name, command
+	line is given. The length of command field is given in command
+	length field.
++
+Supported if the helper has the "invoke-r" capability.
+
 If a fatal error occurs, the program writes the error message to
 stderr and exits. The caller should expect that a suitable error
 message has been printed if the child closes the connection without
@@ -123,6 +145,12 @@ CAPABILITIES
 	all, it must cover all refs reported by the list command; if
 	it is not used, it is effectively "*:*"
 
+'connect-r'::
+	This helper supports the 'connect-r' command.
+
+'invoke-r'::
+	This helper supports the 'invoke-r' command.
+
 REF LIST ATTRIBUTES
 -------------------
 
@@ -167,7 +195,7 @@ OPTIONS
 
 Documentation
 -------------
-Documentation by Daniel Barkalow.
+Documentation by Daniel Barkalow and Ilari Liusvaara
 
 GIT
 ---
diff --git a/transport-helper.c b/transport-helper.c
index 777ecbb..0e4da79 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -16,7 +16,9 @@ struct helper_data
 	unsigned fetch : 1,
 		import : 1,
 		option : 1,
-		push : 1;
+		push : 1,
+		connect_r : 1,
+		invoke_r : 1;
 	/* These go from remote name (as in "list") to private name */
 	struct refspec *refspecs;
 	int refspec_nr;
@@ -75,7 +77,10 @@ static struct child_process *get_helper(struct transport *transport)
 	   would run and print totally inapporiate error message. */
 	strbuf_addf(&buf, "git-remote-%s", data->name);
 	helper->argv[0] = strbuf_detach(&buf, NULL);
-	helper->argv[1] = transport->remote->name;
+	if(transport->remote)
+		helper->argv[1] = transport->remote->name;
+	else
+		helper->argv[1] = "";
 	helper->argv[2] = remove_ext_force(transport->url);
 	helper->git_cmd = 0;
 	if (start_command(helper))
@@ -125,6 +130,10 @@ static struct child_process *get_helper(struct transport *transport)
 				   refspec_alloc);
 			refspecs[refspec_nr++] = strdup(buf.buf + strlen("refspec "));
 		}
+		if (!strcmp(buf.buf, "connect-r"))
+			data->connect_r = 1;
+		if (!strcmp(buf.buf, "invoke-r"))
+			data->invoke_r = 1;
 	}
 	if (refspecs) {
 		int i;
@@ -344,12 +353,83 @@ static int fetch_with_import(struct transport *transport,
 	return 0;
 }
 
+static int _process_connect_or_invoke(struct transport *transport,
+				      const char *name, const char *exec)
+{
+	struct helper_data *data = transport->data;
+	struct strbuf cmdbuf = STRBUF_INIT;
+	struct child_process *helper;
+
+	helper = get_helper(transport);
+
+	if(data->invoke_r) {
+		strbuf_addf(&cmdbuf, "invoke-r %i %s\n",
+			    strlen(exec), exec);
+	} else if(data->connect_r) {
+		strbuf_addf(&cmdbuf, "connect-r %s\n", name);
+	} else
+		return 0;
+
+	write_in_full(helper->in, cmdbuf.buf, cmdbuf.len);
+	strbuf_reset(&cmdbuf);
+	if (strbuf_getline(&cmdbuf, data->out, '\n') == EOF)
+		exit(128); /* child died, message supplied already */
+	if(!strcmp(cmdbuf.buf, "OK"))
+		return 1;
+	else if(!strcmp(cmdbuf.buf, "FALLBACK"))
+		return 0;
+	else if(!strcmp(cmdbuf.buf, "ERROR"))
+		exit(128); /* Error already suppiled. */
+	else
+		die("Unknown response to invoke/connect: %s",
+			cmdbuf.buf);
+
+	return 0;	/* Shouldn't be here. */
+}
+
+static int process_connect_or_invoke(struct transport* transport,
+				     int for_push)
+{
+	struct helper_data *data = transport->data;
+	const char *name;
+	const char *exec;
+
+	name = for_push ? "receive-pack" : "upload-pack";
+	if(for_push)
+		exec = data->gitoptions.receivepack;
+	else
+		exec = data->gitoptions.uploadpack;
+
+	return _process_connect_or_invoke(transport, name, exec);
+}
+
+static int connect_helper(struct transport *transport, const char *name,
+		   const char *exec, int fd[2])
+{
+	struct helper_data *data = transport->data;
+
+	/* Get_helper so invoke_r and connect_r are inited. */
+	get_helper(transport);
+	if(!data->invoke_r && !data->connect_r)
+		die("Operation not supported by protocol.");
+
+	if(!_process_connect_or_invoke(transport, name, exec))
+		die("Can't connect to subservice %s.", name);
+
+	fd[0] = data->helper->out;
+	fd[1] = data->helper->in;
+	return 0;
+}
+
 static int fetch(struct transport *transport,
 		 int nr_heads, struct ref **to_fetch)
 {
 	struct helper_data *data = transport->data;
 	int i, count;
 
+	if(process_connect_or_invoke(transport, 0))
+		return TRANSPORT_LAYER6_READY;
+
 	count = 0;
 	for (i = 0; i < nr_heads; i++)
 		if (!(to_fetch[i]->status & REF_STATUS_UPTODATE))
@@ -377,6 +457,9 @@ static int push_refs(struct transport *transport,
 	struct child_process *helper;
 	struct ref *ref;
 
+	if(process_connect_or_invoke(transport, 1))
+		return TRANSPORT_LAYER6_READY;
+
 	if (!remote_refs)
 		return 0;
 
@@ -520,10 +603,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	helper = get_helper(transport);
 
-	if (data->push && for_push)
-		write_str_in_full(helper->in, "list for-push\n");
-	else
-		write_str_in_full(helper->in, "list\n");
+	if(process_connect_or_invoke(transport, for_push))
+		return &special_transport_layer6_ready;
 
 	while (1) {
 		char *eov, *eon;
@@ -572,6 +653,7 @@ int transport_helper_init(struct transport *transport, const char *name)
 	transport->fetch = fetch;
 	transport->push_refs = push_refs;
 	transport->disconnect = release_helper;
+	transport->connect = connect_helper;
 	transport->disown = helper_disown;
 	transport->smart_options = &(data->gitoptions);
 	return 0;
diff --git a/transport.c b/transport.c
index 09e4c97..a32f405 100644
--- a/transport.c
+++ b/transport.c
@@ -762,6 +762,17 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	return ret;
 }
 
+static int connect_git(struct transport *transport, const char* name,
+		       const char* executable, int fd[2])
+{
+	struct git_transport_data *data = transport->data;
+	data->conn = git_connect(data->fd, transport->url,
+				 executable, 0);
+	fd[0] = data->fd[0];
+	fd[1] = data->fd[1];
+	return 0;
+}
+
 static int disconnect_git(struct transport *transport)
 {
 	struct git_transport_data *data = transport->data;
@@ -926,6 +937,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->get_refs_list = get_refs_via_connect;
 		ret->fetch = fetch_refs_via_pack;
 		ret->push_refs = git_transport_push;
+		ret->connect = connect_git;
 		ret->disconnect = disconnect_git;
 		ret->smart_options = &(data->options);
 		ret->disown = NULL;
@@ -1109,6 +1121,15 @@ void transport_unlock_pack(struct transport *transport)
 	}
 }
 
+int transport_connect(struct transport *transport, const char *name,
+		      const char* exec, int fd[2])
+{
+	if(transport->connect) {
+		return transport->connect(transport, name, exec, fd);
+	} else
+		die("Operation not supported by protocol");
+}
+
 int transport_disconnect(struct transport *transport)
 {
 	int ret = 0;
diff --git a/transport.h b/transport.h
index f3ee890..c86329a 100644
--- a/transport.h
+++ b/transport.h
@@ -64,6 +64,8 @@ struct transport {
 	 **/
 	int (*push_refs)(struct transport *transport, struct ref *refs, int flags);
 	int (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);
+	int (*connect)(struct transport *connection, const char* name, const char* executable,
+		       int fd[2]);
 
 	/**
 	 * Disown the transport helper. Releases all resources used
@@ -143,6 +145,9 @@ void transport_unlock_pack(struct transport *transport);
 int transport_disconnect(struct transport *transport);
 char *transport_anonymize_url(const char *url);
 
+int transport_connect(struct transport *transport, const char *name,
+		      const char* exec, int fd[2]);
+
 /* Transport methods defined outside transport.c */
 int transport_helper_init(struct transport *transport, const char *name);
 
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 5/8] Support remote archive from external protocol helpers

[Ilari Liusvaara]

Helpers which support invoke/connect also should support remote archive
snapshot (or at least there's only one way to attempt it). So support
remote snapshotting for protocol helpers.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 builtin-archive.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 12351e9..3c053b4 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -5,6 +5,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "archive.h"
+#include "transport.h"
 #include "parse-options.h"
 #include "pkt-line.h"
 #include "sideband.h"
@@ -25,12 +26,16 @@ static void create_output_file(const char *output_file)
 static int run_remote_archiver(int argc, const char **argv,
 			       const char *remote, const char *exec)
 {
-	char *url, buf[LARGE_PACKET_MAX];
+	char buf[LARGE_PACKET_MAX];
 	int fd[2], i, len, rv;
-	struct child_process *conn;
+	struct transport *transport;
+	struct remote *_remote;
 
-	url = xstrdup(remote);
-	conn = git_connect(fd, url, exec, 0);
+	_remote = remote_get(remote);
+	if (!_remote->url[0])
+		die("git archive: Remote with no URL");
+	transport = transport_get(_remote, _remote->url[0]);
+	transport_connect(transport, "upload-archive", exec, fd);
 
 	for (i = 1; i < argc; i++)
 		packet_write(fd[1], "argument %s\n", argv[i]);
@@ -53,9 +58,7 @@ static int run_remote_archiver(int argc, const char **argv,
 
 	/* Now, start reading from fd[0] and spit it out to stdout */
 	rv = recv_sideband("archive", fd[0], 1);
-	close(fd[0]);
-	close(fd[1]);
-	rv |= finish_connect(conn);
+	rv |= transport_disconnect(transport);
 
 	return !!rv;
 }
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 6/8] Remove special casing of http, https and ftp

[Ilari Liusvaara]

HTTP, HTTPS and FTP are no longer special to transport code. Also
add support for FTPS (curl supports it so it is easy).

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 .gitignore  |    5 ++++-
 Makefile    |   16 ++++++++++++++--
 transport.c |    8 --------
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7cc54b4..65508ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,7 +106,10 @@
 /git-reflog
 /git-relink
 /git-remote
-/git-remote-curl
+/git-remote-http
+/git-remote-https
+/git-remote-ftp
+/git-remote-ftps
 /git-repack
 /git-replace
 /git-repo-config
diff --git a/Makefile b/Makefile
index 42744a4..be0be87 100644
--- a/Makefile
+++ b/Makefile
@@ -1097,7 +1097,7 @@ else
 	else
 		CURL_LIBCURL = -lcurl
 	endif
-	PROGRAMS += git-remote-curl$X git-http-fetch$X
+	PROGRAMS += git-remote-http$X git-remote-https$X git-remote-ftp$X git-remote-ftps$X git-http-fetch$X
 	curl_check := $(shell (echo 070908; curl-config --vernum) | sort -r | sed -ne 2p)
 	ifeq "$(curl_check)" "070908"
 		ifndef NO_EXPAT
@@ -1676,7 +1676,19 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
-git-remote-curl$X: remote-curl.o http.o http-walker.o $(GITLIBS)
+git-remote-http$X: remote-curl.o http.o http-walker.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
+
+git-remote-https$X: remote-curl.o http.o http-walker.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
+
+git-remote-ftp$X: remote-curl.o http.o http-walker.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
+
+git-remote-ftps$X: remote-curl.o http.o http-walker.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
diff --git a/transport.c b/transport.c
index a32f405..872cc30 100644
--- a/transport.c
+++ b/transport.c
@@ -944,14 +944,6 @@ struct transport *transport_get(struct remote *remote, const char *url)
 
 		data->conn = NULL;
 		data->virtual_connected = 0;
-	} else if (!prefixcmp(url, "http://")
-		|| !prefixcmp(url, "https://")
-		|| !prefixcmp(url, "ftp://")) {
-		/* These three are just plain special. */
-		transport_helper_init(ret, "curl");
-#ifdef NO_CURL
-		error("git was compiled without libcurl support.");
-#else
 	} else {
 		/* Unknown protocol in URL. Pass to external handler. */
 		int len = external_specification_len(url);
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 7/8] Add remote helper debug mode

[Ilari Liusvaara]

Remote helpers deadlock easily, so support debug mode which shows the
interaction steps.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport-helper.c |  110 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 72 insertions(+), 38 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 0e4da79..697f026 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -8,6 +8,8 @@
 #include "quote.h"
 #include "remote.h"
 
+static int debug = 0;
+
 struct helper_data
 {
 	const char *name;
@@ -25,6 +27,47 @@ struct helper_data
 	struct git_transport_options gitoptions;
 };
 
+static void sendline(struct helper_data *helper, struct strbuf *buffer)
+{
+	if(debug)
+		fprintf(stderr, "Debug: Remote helper: -> %s", buffer->buf);
+	if(write_in_full(helper->helper->in, buffer->buf, buffer->len)
+		!= buffer->len) {
+		die_errno("Full write to remote helper failed");
+	}
+}
+
+static int recvline(struct helper_data *helper, struct strbuf *buffer,
+		    int supress_finish)
+{
+	strbuf_reset(buffer);
+	if (strbuf_getline(buffer, helper->out, '\n') == EOF) {
+		if(debug)
+			fprintf(stderr, "Debug: Remote helper quit.\n");
+		if(!supress_finish)
+			finish_command(helper->helper);
+		exit(128);
+	}
+
+	if(debug)
+		fprintf(stderr, "Debug: Remote helper: <- %s\n", buffer->buf);
+	return 0;
+}
+
+static void xchgline(struct helper_data *helper, struct strbuf *buffer)
+{
+	sendline(helper, buffer);
+	recvline(helper, buffer, 1);
+}
+
+static void write_constant(int fd, const char *str)
+{
+	if(debug)
+		fprintf(stderr, "Debug: Remote helper: -> %s", str);
+	if(write_in_full(fd, str, strlen(str)) != strlen(str))
+		die_errno("Full write to remote helper failed");
+}
+
 static struct child_process* helper_disown(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
@@ -99,23 +142,15 @@ static struct child_process *get_helper(struct transport *transport)
 	data->out = xfdopen(duped, "r");
 	setvbuf(data->out, NULL, _IONBF, 0);
 
-	write_str_in_full(helper->in, "capabilities\n");
+	write_constant(helper->in, "capabilities\n");
 
 	while (1) {
-		if (strbuf_getline(&buf, data->out, '\n') == EOF) {
-			/* If we haven't seen line yet, try to finish the
-			   command so we get error message about failed
-			   execution. */
-			if(!seen_line)
-				finish_command(helper);
-
-			exit(128); /* child died, message supplied already */
-		}
-
+		recvline(data, &buf, seen_line);
 		seen_line = 1;
 
 		if (!*buf.buf)
 			break;
+		if(debug) fprintf(stderr, "Debug: Got cap %s\n", buf.buf);
 		if (!strcmp(buf.buf, "fetch"))
 			data->fetch = 1;
 		if (!strcmp(buf.buf, "option"))
@@ -145,14 +180,19 @@ static struct child_process *get_helper(struct transport *transport)
 		free(refspecs);
 	}
 	strbuf_release(&buf);
+	if(debug) fprintf(stderr, "Debug: Capabilities complete.\n");
 	return data->helper;
 }
 
 static int disconnect_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
+	struct strbuf buf = STRBUF_INIT;
+
 	if (data->helper) {
-		write_str_in_full(data->helper->in, "\n");
+		if(debug) fprintf(stderr, "Debug: Disconnecting.\n");
+		strbuf_addf(&buf, "\n");
+		sendline(data, &buf);
 		close(data->helper->in);
 		close(data->helper->out);
 		fclose(data->out);
@@ -182,10 +222,11 @@ static int set_helper_option(struct transport *transport,
 			  const char *name, const char *value)
 {
 	struct helper_data *data = transport->data;
-	struct child_process *helper = get_helper(transport);
 	struct strbuf buf = STRBUF_INIT;
 	int i, ret, is_bool = 0;
 
+	get_helper(transport);
+
 	if (!data->option)
 		return 1;
 
@@ -208,12 +249,7 @@ static int set_helper_option(struct transport *transport,
 		quote_c_style(value, &buf, NULL, 0);
 	strbuf_addch(&buf, '\n');
 
-	if (write_in_full(helper->in, buf.buf, buf.len) != buf.len)
-		die_errno("cannot send option to %s", data->name);
-
-	strbuf_reset(&buf);
-	if (strbuf_getline(&buf, data->out, '\n') == EOF)
-		exit(128); /* child died, message supplied already */
+	xchgline(data, &buf);
 
 	if (!strcmp(buf.buf, "ok"))
 		ret = 0;
@@ -273,13 +309,10 @@ static int fetch_with_fetch(struct transport *transport,
 	}
 
 	strbuf_addch(&buf, '\n');
-	if (write_in_full(data->helper->in, buf.buf, buf.len) != buf.len)
-		die_errno("cannot send fetch to %s", data->name);
+	sendline(data, &buf);
 
 	while (1) {
-		strbuf_reset(&buf);
-		if (strbuf_getline(&buf, data->out, '\n') == EOF)
-			exit(128); /* child died, message supplied already */
+		recvline(data, &buf, 1);
 
 		if (!prefixcmp(buf.buf, "lock ")) {
 			const char *name = buf.buf + 5;
@@ -314,12 +347,13 @@ static int fetch_with_import(struct transport *transport,
 			     int nr_heads, struct ref **to_fetch)
 {
 	struct child_process fastimport;
-	struct child_process *helper = get_helper(transport);
 	struct helper_data *data = transport->data;
 	int i;
 	struct ref *posn;
 	struct strbuf buf = STRBUF_INIT;
 
+	get_helper(transport);
+
 	if (get_importer(transport, &fastimport))
 		die("Couldn't run fast-import");
 
@@ -329,7 +363,7 @@ static int fetch_with_import(struct transport *transport,
 			continue;
 
 		strbuf_addf(&buf, "import %s\n", posn->name);
-		write_in_full(helper->in, buf.buf, buf.len);
+		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
 	disconnect_helper(transport);
@@ -370,12 +404,12 @@ static int _process_connect_or_invoke(struct transport *transport,
 	} else
 		return 0;
 
-	write_in_full(helper->in, cmdbuf.buf, cmdbuf.len);
-	strbuf_reset(&cmdbuf);
-	if (strbuf_getline(&cmdbuf, data->out, '\n') == EOF)
-		exit(128); /* child died, message supplied already */
-	if(!strcmp(cmdbuf.buf, "OK"))
+	xchgline(data, &cmdbuf);
+	if(!strcmp(cmdbuf.buf, "OK")) {
+		if(debug) fprintf(stderr, "Debug: Layer 6 link ready, "
+				  "starting layer 7...\n");
 		return 1;
+	}
 	else if(!strcmp(cmdbuf.buf, "FALLBACK"))
 		return 0;
 	else if(!strcmp(cmdbuf.buf, "ERROR"))
@@ -508,17 +542,14 @@ static int push_refs(struct transport *transport,
 	}
 
 	strbuf_addch(&buf, '\n');
-	if (write_in_full(helper->in, buf.buf, buf.len) != buf.len)
-		exit(128);
+	sendline(data, &buf);
 
 	ref = remote_refs;
 	while (1) {
 		char *refname, *msg;
 		int status;
 
-		strbuf_reset(&buf);
-		if (strbuf_getline(&buf, data->out, '\n') == EOF)
-			exit(128); /* child died, message supplied already */
+		recvline(data, &buf, 1);
 		if (!buf.len)
 			break;
 
@@ -608,8 +639,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	while (1) {
 		char *eov, *eon;
-		if (strbuf_getline(&buf, data->out, '\n') == EOF)
-			exit(128); /* child died, message supplied already */
+		recvline(data, &buf, 1);
 
 		if (!*buf.buf)
 			break;
@@ -634,6 +664,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 		}
 		tail = &((*tail)->next);
 	}
+	if(debug) fprintf(stderr, "Debug: Read ref listing.\n");
 	strbuf_release(&buf);
 
 	for (posn = ret; posn; posn = posn->next)
@@ -647,6 +678,9 @@ int transport_helper_init(struct transport *transport, const char *name)
 	struct helper_data *data = xcalloc(sizeof(*data), 1);
 	data->name = name;
 
+	if(getenv("GIT_TRANSPORT_HELPER_DEBUG"))
+		debug = 1;
+
 	transport->data = data;
 	transport->set_option = set_helper_option;
 	transport->get_refs_list = get_refs_list;
-- 
1.6.6.rc0.64.g5593e

[RFC PATCH 8/8] Support mandatory capabilities

[Ilari Liusvaara]

Add support for marking capability as mandatory for hosting git version
to understand. This is useful for helpers which require various types
of assistance from main git binary.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 Documentation/git-remote-helpers.txt |    5 ++++-
 transport-helper.c                   |   31 +++++++++++++++++++++++--------
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index adf815c..eab9c03 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -25,7 +25,10 @@ Commands are given by the caller on the helper's standard input, one per line.
 
 'capabilities'::
 	Lists the capabilities of the helper, one per line, ending
-	with a blank line.
+	with a blank line. Each capability may be preceeded with '*'.
+	This marks them mandatory for git version using the remote
+	helper to understand (unknown mandatory capability is fatal
+	error).
 
 'list'::
 	Lists the refs, one per line, in the format "<value> <name>
diff --git a/transport-helper.c b/transport-helper.c
index 697f026..a128560 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -145,30 +145,45 @@ static struct child_process *get_helper(struct transport *transport)
 	write_constant(helper->in, "capabilities\n");
 
 	while (1) {
+		const char* capname;
+		int mandatory = 0;
 		recvline(data, &buf, seen_line);
 		seen_line = 1;
 
 		if (!*buf.buf)
 			break;
-		if(debug) fprintf(stderr, "Debug: Got cap %s\n", buf.buf);
-		if (!strcmp(buf.buf, "fetch"))
+
+		if(*buf.buf == '*') {
+			capname = buf.buf + 1;
+			mandatory = 1;
+		} else
+			capname = buf.buf;
+
+		if(debug) fprintf(stderr, "Debug: Got cap %s\n", capname);
+		if (!strcmp(capname, "fetch"))
 			data->fetch = 1;
-		if (!strcmp(buf.buf, "option"))
+		else if (!strcmp(capname, "option"))
 			data->option = 1;
-		if (!strcmp(buf.buf, "push"))
+		else if (!strcmp(capname, "push"))
 			data->push = 1;
-		if (!strcmp(buf.buf, "import"))
+		else if (!strcmp(capname, "import"))
 			data->import = 1;
-		if (!data->refspecs && !prefixcmp(buf.buf, "refspec ")) {
+		else if (!data->refspecs && !prefixcmp(capname, "refspec ")) {
 			ALLOC_GROW(refspecs,
 				   refspec_nr + 1,
 				   refspec_alloc);
 			refspecs[refspec_nr++] = strdup(buf.buf + strlen("refspec "));
 		}
-		if (!strcmp(buf.buf, "connect-r"))
+		else if (!strcmp(capname, "connect-r"))
 			data->connect_r = 1;
-		if (!strcmp(buf.buf, "invoke-r"))
+		else if (!strcmp(capname, "invoke-r"))
 			data->invoke_r = 1;
+		else if (mandatory) {
+			fflush(stderr);
+			die("Unknown madatory capability %s. This remote "
+			    "helper probably needs newer version of Git.\n",
+			    capname);
+		}
 	}
 	if (refspecs) {
 		int i;
-- 
1.6.6.rc0.64.g5593e

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Sverre Rabbelier]

Heya,

On Tue, Dec 1, 2009 at 14:57, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:
> This series implements extensions to remote helpers for carrying smary
> transports. It is against next, because master doesn't contain necressary
> patches (the allow specifying remote helper in url one).

Could you please explain how this relates to Shawn's smart http series
and the sr/vcs-helper series?

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Dec 1, 2009 at 14:57, Ilari Liusvaara
> <ilari.liusvaara@elisanet.fi> wrote:
> > This series implements extensions to remote helpers for carrying smary
> > transports. It is against next, because master doesn't contain necressary
> > patches (the allow specifying remote helper in url one).
> 
> Could you please explain how this relates to Shawn's smart http series
> and the sr/vcs-helper series?

Or better, why this is even necessary?

I thought git:// over TCP is pretty simple and efficient, and fairly
widely deployed.  Smart http(s):// will be in 1.6.6 and available
soon, and isn't all that ugly.

Since the introduction of git:// nobody has asked for another
protocol... other than wanting to make http:// as efficient as
git:// is.  Which is now done.

So why do we need this?

The sr/vcs-helper series makes sense if you want to make SVN, Hg,
or P4 remotes act transparently like Git remotes.  But that's not
embedding the git:// protocol inside of another protocol, its doing a
full up conversion from a non-Git set of semantics to Git semantics.

-- 
Shawn.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 08:52:45AM -0800, Shawn O. Pearce wrote:
> Sverre Rabbelier <srabbelier@gmail.com> wrote:
> > On Tue, Dec 1, 2009 at 14:57, Ilari Liusvaara
> > <ilari.liusvaara@elisanet.fi> wrote:
> > > This series implements extensions to remote helpers for carrying smary
> > > transports. It is against next, because master doesn't contain necressary
> > > patches (the allow specifying remote helper in url one).
> > 
> > Could you please explain how this relates to Shawn's smart http series
> > and the sr/vcs-helper series?

Ability to easily implement smart transports with underlying full-duplex
connection. The smart http stuff has loads of code to reimplement smart
transport client side.

> Or better, why this is even necessary?
> 
> I thought git:// over TCP is pretty simple and efficient, and fairly
> widely deployed.  Smart http(s):// will be in 1.6.6 and available
> soon, and isn't all that ugly.

I consider the authentication parts of smart http pretty ugly. TLS has some
nicer methods, but support for those is nonexistent. Also, I consider piggy-
backing on HTTP when you can have full-duplex connectivity ugly.
 
> Since the introduction of git:// nobody has asked for another
> protocol... other than wanting to make http:// as efficient as
> git:// is.  Which is now done.

Incorrect. I have seen requests for gits:// (and in fact, I have plans to
implement that protocol).

> So why do we need this?

For instance, to support new types of authentication for smart transports
without patching client git binaries (SSH has lots of failure modes that
are quite nasty to debug) or abusing GIT_PROXY (yuck). 

If the server can also handle authentication, it has a lot better idea
where things go wrong and can give better errors to logs or to client
(of course, not too much can be leaked to client or it will be too useful
for attack, but that's seperate topic).

> The sr/vcs-helper series makes sense if you want to make SVN, Hg,
> or P4 remotes act transparently like Git remotes.  But that's not
> embedding the git:// protocol inside of another protocol, its doing a
> full up conversion from a non-Git set of semantics to Git semantics.

This is not about embedding git:// protol inside another. Its about
carrying the subprotocols. These transports share with git:// as much
as file:// and ssh:// share with git:// (note that service request is
given as command, not inside data pipe).

And IIRC, the only thing this needs from sr/vcs-helper is the patch to
allow selecting helper with URL. The first versions of series did contain
self-standing functionality equivalent to that, but that got dropped as
equivalent functionality appeared in upstream.

-Ilari

Re: [RFC PATCH 6/8] Remove special casing of http, https and ftp

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> HTTP, HTTPS and FTP are no longer special to transport code. Also
> add support for FTPS (curl supports it so it is easy).
...
> diff --git a/Makefile b/Makefile
> index 42744a4..be0be87 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1676,7 +1676,19 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
>  
> -git-remote-curl$X: remote-curl.o http.o http-walker.o $(GITLIBS)
> +git-remote-http$X: remote-curl.o http.o http-walker.o $(GITLIBS)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
> +
> +git-remote-https$X: remote-curl.o http.o http-walker.o $(GITLIBS)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
> +
> +git-remote-ftp$X: remote-curl.o http.o http-walker.o $(GITLIBS)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
> +
> +git-remote-ftps$X: remote-curl.o http.o http-walker.o $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)

These should all be hardlinks to a single executable, not duplicate
relinks of the same object files.
  
-- 
Shawn.

Re: [RFC PATCH 6/8] Remove special casing of http, https and ftp

[Daniel Barkalow]

On Tue, 1 Dec 2009, Ilari Liusvaara wrote:

> HTTP, HTTPS and FTP are no longer special to transport code. Also
> add support for FTPS (curl supports it so it is easy).

We've been through this extensively, and settled on having a special case 
for URLs that specify a pure location. That is, the distinction between 
http and ftp is at the level of how you get to the content for that 
location, not what you do to interact with it. (Even with webdav or the 
git-specific smart server support, we use the same detection method on all 
locations, and ftp simply never has the possibility of having these 
features detected.)

It would be fine to add "ftps" to the list of URL schemes that indicate a 
pure location, except that it's plausible that ftps supports writing, but 
obviously not by webdav, which is what the push support via curl will 
attempt, so it's more likely to be confusing than helpful.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH 4/8] Support remote helpers implementing smart transports

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> index 5cfdc0c..adf815c 100644
> --- a/Documentation/git-remote-helpers.txt
> +++ b/Documentation/git-remote-helpers.txt
> @@ -90,6 +90,28 @@ Supported if the helper has the "push" capability.
>  +
>  Supported if the helper has the "import" capability.
>  
> +'connect-r' <service>::
> +	Connects to given service. Stdin and stdout of helper are
> +	connected to specified service (no git or git- prefixes are used,
> +	so e.g. fetching uses 'upload-pack' as service) on remote side.

This flies against every other convention we have.  git:// uses the
string 'git-upload-pack' and 'git-receive-pack', and so does the
smart-http code.  We should continue to use the git- prefix here,
to be consistent, even though by context its clearly implied.

> +	Valid replies to this command are 'OK' (connection established),

Why 'OK'?  Currently remote-helpers return an empty blank line
to any successful command, not 'OK'.

> +	'FALLBACK' (no smart transport support, fall back to dumb
> +	transports) and 'ERROR' (can't connect, don't bother trying to
> +	fall back).

FALLBACK almost makes sense, but ERROR we don't do in the
the existing helper protocol.  Instead the helper simply
prints its error message(s) to stderr and does exit(128).
aka what die() does.

> +Supported if the helper has the "connect-r" capability. Not used if
> +helper has the "invoke-r" capability, as invoke is preferred to connect.
> +
> +'invoke-r' <cmdlength> <cmd>::
> +	Like connect-r command, but instead of service name, command
> +	line is given. The length of command field is given in command
> +	length field.
> ++
> +Supported if the helper has the "invoke-r" capability.

Why both connect-r and invoke-r?  Why isn't connect-r sufficient
here?  Isn't it sufficient for any service that runs over git:// ?

-- 
Shawn.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> On Tue, Dec 01, 2009 at 08:52:45AM -0800, Shawn O. Pearce wrote:
> 
> > Or better, why this is even necessary?
> 
> I have seen requests for gits:// (and in fact, I have plans to
> implement that protocol).
> 
> For instance, to support new types of authentication for smart transports
> without patching client git binaries (SSH has lots of failure modes that
> are quite nasty to debug) or abusing GIT_PROXY (yuck). 

So the bulk of this series is about making a proxy for git://
easier to tie into git?

Forgive me if I sound stupid, but for gits:// shouldn't that just
be a matter of git_connect() forking a git-remote-gits process
linked against openssl?  Or, maybe it just runs `openssl s_client`?

Why go through all of this effort of making a really generic proxy
protocol system when the long-term plan is to just ship native
gits:// support as part of git-core?
 
-- 
Shawn.

Re: [RFC PATCH 6/8] Remove special casing of http, https and ftp

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 10:24:14AM -0800, Shawn O. Pearce wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> 
> These should all be hardlinks to a single executable, not duplicate
> relinks of the same object files.

Fixed for next round (probably send that out in few days).

-Ilari

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
>> 
>> For instance, to support new types of authentication for smart transports
>> without patching client git binaries (SSH has lots of failure modes that
>> are quite nasty to debug) or abusing GIT_PROXY (yuck). 
>
> So the bulk of this series is about making a proxy for git://
> easier to tie into git?
>
> Forgive me if I sound stupid, but for gits:// shouldn't that just
> be a matter of git_connect() forking a git-remote-gits process
> linked against openssl?  Or, maybe it just runs `openssl s_client`?
>
> Why go through all of this effort of making a really generic proxy
> protocol system when the long-term plan is to just ship native
> gits:// support as part of git-core?

I didn't know what the long-term plan was to be honest, but after skimming
the series, I think your response is a good summary.

It is somewhat unfortunate that a few changes I liked (e.g. the "debug"
bit), even though it was somewhat painful to read them due to coding style
differences, were not at the beginning of the series but instead buried
after changes that are much bigger and controversial (e.g. [6/8]).

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> It is somewhat unfortunate that a few changes I liked (e.g. the "debug"
> bit), even though it was somewhat painful to read them due to coding style
> differences, were not at the beginning of the series but instead buried
> after changes that are much bigger and controversial (e.g. [6/8]).

I agree about that debug patch, I actually thought that was
interesting, and wish I had done more of that sort of work during
smart-http.  It would have helped me to debug it in the early stages.

-- 
Shawn.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 11:30:09AM -0800, Shawn O. Pearce wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> > 
> > For instance, to support new types of authentication for smart transports
> > without patching client git binaries (SSH has lots of failure modes that
> > are quite nasty to debug) or abusing GIT_PROXY (yuck). 
> 
> So the bulk of this series is about making a proxy for git://
> easier to tie into git?

This is making the "layer 5/6" parts of git:// easier to replace, for whatever
reason that replacement may be desired (and the lower layer is just assumed to
be some kind of full-duplex link).

The part about abusing GIT_PROXY is _very_ nasty hack to be able to layer 6
gateway git smart transports.

The git:// protocol stack is: 
- Git smart transport subprotocols (upload-pack, upload-archive and receive-pack)
- git:// (request signaling and data passing)
- TCP/IP (or comparable)

And ssh://:

- Git smart transport subprotocols (upload-pack, upload-archive and receive-pack)
- SSH (request signaling, data passing, encrypt & auth).
- TCP/IP (or comparable)

Smart-HTTP:

- RPC versions of git smart transport subprotocols
- HTTP
- TLS (optional)
- TCP/IP (or comparable)

This is about:

- Git smart transport subprotocols (upload-pack, upload-archive and receive-pack)
- Some prtocol layer(s) (request signaling, data passing, maybe encrypt & auth, etc...)
- TCP/IP (or comparable)

> Forgive me if I sound stupid, but for gits:// shouldn't that just
> be a matter of git_connect() forking a git-remote-gits process
> linked against openssl?  Or, maybe it just runs `openssl s_client`?

gits:// was just an example. There can be other interesting stuff (I don't
even pretend my imagination is the limit). And I would rather link the gits://
handler to GnuTLS than OpenSSL, but that's seperate matter...

As for "other interesting stuff": Smart transport using Kerberos auth (just
throwing ideas, probably not going to implement that)?

> Why go through all of this effort of making a really generic proxy
> protocol system when the long-term plan is to just ship native
> gits:// support as part of git-core?

gits:// is not actual goal of this series. Its just something to build on
top of it.

-Ilari

Re: [RFC PATCH 6/8] Remove special casing of http, https and ftp

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 02:15:17PM -0500, Daniel Barkalow wrote:
> On Tue, 1 Dec 2009, Ilari Liusvaara wrote:
> 
> > HTTP, HTTPS and FTP are no longer special to transport code. Also
> > add support for FTPS (curl supports it so it is easy).
> 
> We've been through this extensively, and settled on having a special case 
> for URLs that specify a pure location. That is, the distinction between 
> http and ftp is at the level of how you get to the content for that 
> location, not what you do to interact with it. (Even with webdav or the 
> git-specific smart server support, we use the same detection method on all 
> locations, and ftp simply never has the possibility of having these 
> features detected.)

Currently the only thing about http:// and co git main executable knows is
to pass them to curl remote helper (and print error if compiled with NO_CURL,
possibly causing problems with version desync). Git main executable does
not know any difference between say http:// and ftp:// (the remote helper must
obiviously know the difference, but remote helper is not part of git main
executable).

> It would be fine to add "ftps" to the list of URL schemes that indicate a 
> pure location, except that it's plausible that ftps supports writing, but 
> obviously not by webdav, which is what the push support via curl will 
> attempt, so it's more likely to be confusing than helpful.

remote-curl.c code doesn't seem to do anything stupid with ftps:// that it
wouldn't try with ftp://, and trying to push counts as "stupid" here (and
remember that many FTP servers do allow unencrypted uploads, especially with
authentication and CURL can AFAIK handle that).

-Ilari

Re: [RFC PATCH 4/8] Support remote helpers implementing smart transports

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 11:22:33AM -0800, Shawn O. Pearce wrote:
> 
> This flies against every other convention we have.  git:// uses the
> string 'git-upload-pack' and 'git-receive-pack', and so does the
> smart-http code.  We should continue to use the git- prefix here,
> to be consistent, even though by context its clearly implied.

Changed for next round (put the git- -prefixes into names).
 
> Why 'OK'?  Currently remote-helpers return an empty blank line
> to any successful command, not 'OK'.

Changed to "" (i.e. blank line) for next round.
 
> FALLBACK almost makes sense, but ERROR we don't do in the
> the existing helper protocol.  Instead the helper simply
> prints its error message(s) to stderr and does exit(128).
> aka what die() does.

ERROR case changed to exit(128) of helper for next round.

> Why both connect-r and invoke-r?  Why isn't connect-r sufficient
> here?  Isn't it sufficient for any service that runs over git:// ?

Invoke supports those --upload-pack &co (a'la ssh://). connect
doesn't (a'la to git://).

-Ilari

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 12:42:29PM -0800, Junio C Hamano wrote:
> 
> It is somewhat unfortunate that a few changes I liked (e.g. the "debug"
> bit), even though it was somewhat painful to read them due to coding style
> differences, were not at the beginning of the series but instead buried
> after changes that are much bigger and controversial (e.g. [6/8]).

Funny, I considered some other stuff in series much more controversial than
the 6/8 one.

And 6/8 large? Its smallest (source code files only) or second smallest (all
files) in number of line changes in the series.

If one looks at 6/8, what it basically does is:
- Alias remote-curl as remote-{http,ftp}{,s} since the special case dispatch
  rules are no more (.gitignore + makefile).
- Remove the special case dispatch rules (transport.c).

Taking diffstat of fixed version of 6/8 (I'll send that later as second round,
possibly with additional fixes):

 .gitignore  |    4 ++++
 Makefile    |   19 +++++++++++++++++++
 transport.c |    8 --------
 3 files changed, 23 insertions(+), 8 deletions(-)

And here's what it does to transport.c:

-       } else if (!prefixcmp(url, "http://")
-               || !prefixcmp(url, "https://")
-               || !prefixcmp(url, "ftp://")) {
-               /* These three are just plain special. */
-               transport_helper_init(ret, "curl");
-#ifdef NO_CURL
-               error("git was compiled without libcurl support.");
-#endif

That's 8 lines killed in transport.c, 4 new binary aliases (yeah, I'm not that
good with makefiles plus this seems to be somewhat nasty case).

-Ilari 

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> On Tue, Dec 01, 2009 at 12:42:29PM -0800, Junio C Hamano wrote:
>> 
>> It is somewhat unfortunate that a few changes I liked (e.g. the "debug"
>> bit), even though it was somewhat painful to read them due to coding style
>> differences, were not at the beginning of the series but instead buried
>> after changes that are much bigger and controversial (e.g. [6/8]).
>
> Funny, I considered some other stuff in series much more controversial than
> the 6/8 one.

I didn't mean the line count by "large".  I was referring to the size of
change at the conceptual level.  As Daniel already explained, it has been
one of the design assumption so far that there are built-in mappings from
some common <scheme>:// to backend "helpers".

I am _not_ saying that that particular design assumption must be cast in
stone (nothing is)---that is a totally different matter to be debated.
But the fact that it needs to be debated means it is not "a trivial 8-line
reduction", but rather a large conceptual change (perhaps improvement).

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Tue, Dec 01, 2009 at 10:35:58PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> I didn't mean the line count by "large".  I was referring to the size of
> change at the conceptual level.  As Daniel already explained, it has been
> one of the design assumption so far that there are built-in mappings from
> some common <scheme>:// to backend "helpers".

No implicit mappings from <scheme>:// to helpers existed before this series
(except for forcing in URL, which are different). Thus, any mapping had to
be explicit and built-in.

And if mappings http -> curl, https -> curl, ftp -> curl are to remain explicit
in main git binary, I would put them into table and build stub remote-curl if
NO_CURL is defined instead of special casing the error in main git binary
(but I consider that worse than just removing the association from main
git binary).

>From file system listing on this computer (note the I-node numbers, this is
on newer version of change than the one sent):

2068945 -rwxr-xr-x 4 Ilari users 1547231 2009-12-02 15:12 /home/Ilari/.local/git-testing/libexec/git-core/git-remote-ftp
2068945 -rwxr-xr-x 4 Ilari users 1547231 2009-12-02 15:12 /home/Ilari/.local/git-testing/libexec/git-core/git-remote-ftps
2068945 -rwxr-xr-x 4 Ilari users 1547231 2009-12-02 15:12 /home/Ilari/.local/git-testing/libexec/git-core/git-remote-http
2068945 -rwxr-xr-x 4 Ilari users 1547231 2009-12-02 15:12 /home/Ilari/.local/git-testing/libexec/git-core/git-remote-https

So instead of mapping explicitly, those are effectively mapped by filesystem
(that's after the fixes for next round that make helpers hardlinked instead
of copied).

-Ilari

Re: [RFC PATCH 4/8] Support remote helpers implementing smart transports

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> On Tue, Dec 01, 2009 at 11:22:33AM -0800, Shawn O. Pearce wrote:
> > Why both connect-r and invoke-r?  Why isn't connect-r sufficient
> > here?  Isn't it sufficient for any service that runs over git:// ?
> 
> Invoke supports those --upload-pack &co (a'la ssh://). connect
> doesn't (a'la to git://).

Drop invoke-r.

Modify transport-helper.c to allow pushing TRANS_OPT_UPLOADPACK and
TRANS_OPT_RECEIVEPACK down into the helper via the option capability.

I'd rename connect-r to just connect.

For the command line:

  $ git fetch --upload-pack='/path to my /git-upload-pack' origin

The conversation with the helper will be:

  > capabilities
  < option
  < connect
  <
  > option uploadpack /path to my /git-upload-pack
  < ok
  > connect git-upload-pack
  <

Which gives the helper the full information it needs to pass along
the --upload-pack command line argument to the remote system.

-- 
Shawn.

Re: [RFC PATCH 4/8] Support remote helpers implementing smart transports

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> On Tue, Dec 01, 2009 at 11:22:33AM -0800, Shawn O. Pearce wrote:
>  
> > Why 'OK'?  Currently remote-helpers return an empty blank line
> > to any successful command, not 'OK'.
> 
> Changed to "" (i.e. blank line) for next round.

Arrrgh.  Just to correct myself... the 'option' command uses 'ok',
'error', 'unsupported' as its response messages.  Which means
'option' breaks the blank-line-is-ok convention I tried to hold
you to above.

I consider this a mistake on my part.  'option' should respond with
a blank line on success just like 'fetch' or 'push' does.

-- 
Shawn.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> On Tue, Dec 01, 2009 at 10:35:58PM -0800, Junio C Hamano wrote:
>> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
>> 
>> I didn't mean the line count by "large".  I was referring to the size of
>> change at the conceptual level.  As Daniel already explained, it has been
>> one of the design assumption so far that there are built-in mappings from
>> some common <scheme>:// to backend "helpers".
>
> No implicit mappings from <scheme>:// to helpers existed before this series
> (except for forcing in URL, which are different). Thus, any mapping had to
> be explicit and built-in.
> ...
> So instead of mapping explicitly, those are effectively mapped by filesystem
> (that's after the fixes for next round that make helpers hardlinked instead
> of copied).

Sure, it may be a good change; didn't I say that in the part you omitted
from your quote?  But it is conceptually a big change nevertheless, and it
is to be debated --- that makes the parts leading to 6/8 rather a large
change, which was my primary point in the message you are responding to,
so I think we are in agreement that it would have been nicer if the other
bits that are independently useful (like the debug one) were earlier in
the series.

The other minor point in my message was that this is to be debated (which
I think you are doing now), but I am not the best person to debate the
design of this part with.  Daniel and Shawn are the guys who have
primarily worked on the helper interface and relationship between the
helpers and the transport layer, and will have much better insights.

By the way, are you the helpful git expert we often see on #git IRC
channel who goes by the same name?  Welcome to the list; I don't remember
seeing you here, and let me thank you for having helped many new to
intermediate git users over there for a long time.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Johannes Schindelin]

Hi,

On Wed, 2 Dec 2009, Ilari Liusvaara wrote:

> And if mappings http -> curl, https -> curl, ftp -> curl are to remain 
> explicit in main git binary, I would put them into table and build stub 
> remote-curl if NO_CURL is defined instead of special casing the error in 
> main git binary (but I consider that worse than just removing the 
> association from main git binary).

This is definitely a good direction, and it would be even better if the 
absence of the remote helper was also handled gracefully.  Just think 
about a (as of now fictious) git-remote-http.rpm relying on git-core.rpm 
and libcurl.rpm.  If you do not want to access http:// URLs, you can 
install just git-core.  Once you encounter an http:// URL you need to 
access, you install git-remote-http.  Keeping git-core.  (I like to call 
this setup "modular".)

Of course, I never understood why the backend should know the 
implementation detail that it is based on cURL, so it would be even more 
modular (at least by my definition) if there was no hard-coded mapping 
(Sverre -- Cc'ed -- seemed to like URLs of the form "svn::http://..." and 
"cvs::pserver..." to trigger looking for a remote helper explicitely).  I 
find the compiled-in mapping rather limiting.

Ciao,
Dscho

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Sverre Rabbelier]

Heya,

On Wed, Dec 2, 2009 at 18:39, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> This is definitely a good direction, and it would be even better if the
> absence of the remote helper was also handled gracefully.

Yes, that is definitely an improvement we can and should make
regardless of how we handle http(s) and ftp(s), since currently "git
clone nonsense::http://...." will error out with the message that
"git-remote-nonsense" cannot be found.

> Of course, I never understood why the backend should know the
> implementation detail that it is based on cURL, so it would be even more
> modular (at least by my definition) if there was no hard-coded mapping.

Agreed.

> Sverre -- Cc'ed -- seemed to like URLs of the form "svn::http://..." and
> "cvs::pserver..." to trigger looking for a remote helper explicitely.  I
> find the compiled-in mapping rather limiting.

Yes, I do think the double-colon syntax is very nice. That is, someone
who sees "git clone svn::http://" is likely to understand that it is a
svn repo over http that git treats specially.

However, I am not convinced that we should do any magic to map
"foo://" to git-remote-foo. On the other hand, I do think it makes
sense to have something modular that allows "git-remote-http" to be
implemented as a separate package that can be installed.

Perhaps instead of the current special case where "git-remote-curl" is
invoked, it would make more sense to instead special case on "http://"
(etc) and invoke "git-remote-http" in that case. So "git clone svn://"
would not work, but "git clone svn::svn://" would (as is the case
now), as well as "git clone http://" being handled by
"git-remote-http".

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

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

> This is definitely a good direction, and it would be even better if the 
> absence of the remote helper was also handled gracefully.  Just think 
> about a (as of now fictious) git-remote-http.rpm relying on git-core.rpm 
> and libcurl.rpm.  If you do not want to access http:// URLs, you can 
> install just git-core.  Once you encounter an http:// URL you need to 
> access, you install git-remote-http.  Keeping git-core.  (I like to call 
> this setup "modular".)

The "modular" setup is a good thing to do, but I do not know how it
relates to the change Ilari did.  Isn't it simply a matter of excluding
git-remote-curl from the current set of binaries to be shipped with
git-core.rpm and making a separate git-remote-http.rpm to contain it, or
does it involve a lot more than that from the packager's side?

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>> Of course, I never understood why the backend should know the
>> implementation detail that it is based on cURL, so it would be even more
>> modular (at least by my definition) if there was no hard-coded mapping.
>
> Agreed.

I don't get this point at all.

Backend is _very_ aware of how it is implemented itself.  Naming one
implementation git-remote-http is to declare that "I am the one and only
implementation of http handler" and forces another implementation of http
handler, perhaps based on different toolkit than libcurl, to forever be a
second class citizen that need to use name other than 'http'.

The "mapping" you two are calling "hard-coded" may be "hard-coded" but is
a better kind of hard-coding than hard-coding "http" to "this particular
implementation" implicitly like you two seem to be advocating.  Think of
it as having one extra layer of indirection.

When the second implementation of http handler proves to be better than
the current one, we can flip the mapping, and anybody who were using
"http://" to access some repository will automatically updated to use the
new backend instead of the old one.  With your scheme, you probably could
change the name of the old "http" backend to "http-deprecated" and the new
one from "second-class-citizen-http" to "http" to achieve a similar
effect, but I do not think it is as nice as having one extra level of
indirection.

> However, I am not convinced that we should do any magic to map
> "foo://" to git-remote-foo. On the other hand, I do think it makes
> sense to have something modular that allows "git-remote-http" to be
> implemented as a separate package that can be installed.

As I said, I do think modular is good, but I think what Dscho is
advocating does not have much to achieve that goal.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Wed, Dec 02, 2009 at 10:07:55AM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> The "modular" setup is a good thing to do, but I do not know how it
> relates to the change Ilari did. 

Removing core dependency on NO_CURL is required for that. 6/8 is one
way to do that given support for dispatching <scheme>:// to remote
helpers (1/8).

> Isn't it simply a matter of excluding
> git-remote-curl from the current set of binaries to be shipped with
> git-core.rpm and making a separate git-remote-http.rpm to contain it, or
> does it involve a lot more than that from the packager's side?

See above. There's also issues with git remote helper execution, namely
inability to properly handle failure before cap exchange.

With properly fixed main git binary, it is just matter of splitting
remote-xxx executable(s) relating to HTTP to seperate package.

-Ilari

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Sverre Rabbelier]

Heya,

On Wed, Dec 2, 2009 at 19:41, Junio C Hamano <gitster@pobox.com> wrote:
> When the second implementation of http handler proves to be better than
> the current one, we can flip the mapping, and anybody who were using
> "http://" to access some repository will automatically updated to use the
> new backend instead of the old one.  With your scheme, you probably could
> change the name of the old "http" backend to "http-deprecated" and the new
> one from "second-class-citizen-http" to "http" to achieve a similar
> effect, but I do not think it is as nice as having one extra level of
> indirection.

I don't see how what you are talking about is any different. With the
mapping the executable of the alternative implementation still needs
to have a different name, no?

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> I don't see how what you are talking about is any different. With the
> mapping the executable of the alternative implementation still needs
> to have a different name, no?

Sure, but please search for "second class citizen" in my message.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Sverre Rabbelier]

Heya,

On Wed, Dec 2, 2009 at 19:52, Junio C Hamano <gitster@pobox.com> wrote:
> Sure, but please search for "second class citizen" in my message.

I read that, but I don't understand how exactly the mapping will make
it a non-second class citizen. How will your mapping include the
alternative implementation? The word mapping suggests a 1:1 relation
between protocol and implementation, so I don't see how the
alternative implementation would become first-class :(.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sverre Rabbelier <srabbelier@gmail.com> writes:
>
>> I don't see how what you are talking about is any different. With the
>> mapping the executable of the alternative implementation still needs
>> to have a different name, no?
>
> Sure, but please search for "second class citizen" in my message.

Also "extra level of indication".

I do not think "remote-curl" was the best name, and hindsight tells me
that "remote-walker" might have been a better name (it tells us how it
does it more clearly).

And I do not at all mind making the current hard-coded mapping from
http:// to remote-walker to an external table look-up, perhaps something
that can be controlled by .git/config, with a built-in default that is
hard-coded like the way we have now.

After all my main objection is against closing the door to others by one
particular implementation squating on "remote-http" name and refusing the
use of that nice, authoritative-sounding name by others.

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Wed, Dec 02, 2009 at 10:41:40AM -0800, Junio C Hamano wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
> 
> >> Of course, I never understood why the backend should know the
> >> implementation detail that it is based on cURL, so it would be even more
> >> modular (at least by my definition) if there was no hard-coded mapping.
> >
> > Agreed.
> 
> I don't get this point at all.
> 
> Backend is _very_ aware of how it is implemented itself.  Naming one
> implementation git-remote-http is to declare that "I am the one and only
> implementation of http handler" and forces another implementation of http
> handler, perhaps based on different toolkit than libcurl, to forever be a
> second class citizen that need to use name other than 'http'.

At least it can be called as 'foo::http://' (That may be tolerable for
alternate implementations but not for primary ones).

> The "mapping" you two are calling "hard-coded" may be "hard-coded" but is
> a better kind of hard-coding than hard-coding "http" to "this particular
> implementation" implicitly like you two seem to be advocating.  Think of
> it as having one extra layer of indirection.

Its already indirected: By filesystem.

> When the second implementation of http handler proves to be better than
> the current one, we can flip the mapping, and anybody who were using
> "http://" to access some repository will automatically updated to use the
> new backend instead of the old one.  With your scheme, you probably could
> change the name of the old "http" backend to "http-deprecated" and the new
> one from "second-class-citizen-http" to "http" to achieve a similar
> effect, but I do not think it is as nice as having one extra level of
> indirection.

The new HTTP support must either be internal or not. And:

- If it is internal, renaming can be done anyway.
- If it is not, change can not be made.

And at package manager level, this is what 'conflicts: ' is about (and
alternates of apt).

> > However, I am not convinced that we should do any magic to map
> > "foo://" to git-remote-foo. On the other hand, I do think it makes
> > sense to have something modular that allows "git-remote-http" to be
> > implemented as a separate package that can be installed.
> 
> As I said, I do think modular is good, but I think what Dscho is
> advocating does not have much to achieve that goal.

Why should adding new git native protocol (that doesn't have so special
capabilities new core support is fundamentially required) require recompiling
git core? Why it should require more than dropping executable handler to
suitable place?[*]

Why should such protocols need to be specified 'foo::foo://'?

Granted, even with current dispatch meachanisms, its possible to hack
together something that accepts 'foo:://', 'foo::/' or 'foo::' but that
breaks user expectations rather badly (the U in URL stands for 'uniform')... 

And currently the URL space to be reassigned just produces fatal error
messages anyway.

[*] Merge strategies have similar issues. IMO, on encountering unknown
merge strategy, git merging code should check if there's handler for it,
if yes, obtain flags from it and then use it.

-Ilari 

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Jeff King]

On Wed, Dec 02, 2009 at 10:58:39AM -0800, Junio C Hamano wrote:

> After all my main objection is against closing the door to others by one
> particular implementation squating on "remote-http" name and refusing the
> use of that nice, authoritative-sounding name by others.

I would think that it would be useful to use the "remote-http" name as
the extra level of indirection (as a symlink, hardlink, or wrapper
script to remote-curl). Then you could have competing first-class
implementations that would be easy for the user (or package manager) to
switch between.

For example, Debian contains versions of curl built against gnutls and
against openssl. Right now the debian git package requires the gnutls
version. But let's say they ship two packages: git-http-curl-openssl and
git-http-curl-gnutls. Then you can install whichever you prefer, and the
package will contain the file "git-remote-http" pointing to
"git-remote-curl-$whatever".

And yes, if you think about it, this particular situation already works
with a hard-coded "git-remote-curl", since both are built on top of
curl, and that makes a reasonable name. But now extend it to "you don't
want to use curl, but rather some other http library". I don't think we
have any interest in providing a non-curl version as part of git itself,
but it provides a hook should somebody want to write their own http
handler (either using a different library, or maybe a wrapper that does
caching, or whatever).

Just my two cents. I don't plan on writing any such third-party remote
handlers, but it seems simple enough to leave the door open.

-Peff

Re: [RFC PATCH 0/8] Git remote helpers to implement smart transports.

[Ilari Liusvaara]

On Wed, Dec 02, 2009 at 06:39:19PM +0100, Johannes Schindelin wrote:
> 
> This is definitely a good direction, and it would be even better if the 
> absence of the remote helper was also handled gracefully.  Just think 
> about a (as of now fictious) git-remote-http.rpm relying on git-core.rpm 
> and libcurl.rpm.  If you do not want to access http:// URLs, you can 
> install just git-core.  Once you encounter an http:// URL you need to 
> access, you install git-remote-http.  Keeping git-core.  (I like to call 
> this setup "modular".)

There are some rather unfortunate details relating to this.

Main git executable currently has no good way to discover what went wrong
with remote helper execution that fails before reaching capabilities
exchange.

It would be ideal if executions failing due to ENOENT would be reported
as remote helper not existing, other exec errors reported as failed execution,
fatal signals as remote helper crashing and other exits rely on remote helper
reporting the problem.

Unfortunately, this can't be done without breaking remote helper interface,
either by requiring initial response from helper or requiring helpers not
to explicitly fail due to bad parameters before reaching capabilities exchange,
since one can't know if execution was successuful without seeing at least
one incoming line.

IIRC, current versions print some rather funky error if you try to use
nonexistent helper: 'remote-foo is not git command' or some such.

> Of course, I never understood why the backend should know the 
> implementation detail that it is based on cURL, so it would be even more 
> modular (at least by my definition) if there was no hard-coded mapping 
> (Sverre -- Cc'ed -- seemed to like URLs of the form "svn::http://..." and 
> "cvs::pserver..." to trigger looking for a remote helper explicitely).  I 
> find the compiled-in mapping rather limiting.

That syntax is rather nice for handling foregin VCSes that may have URL forms
that overlap with native ones. But it sure isn't nice for those remote helpers
that implement git native transports (remote-curl is already a precedent on
doing that). 

The API is already general enough to do both: Git native transports (currently
dumb only without lots of effort, which this patchset is about) and foregin 
VCS bridges.

-Ilari

Re: [RFC PATCH 4/8] Support remote helpers implementing smart transports

[Ilari Liusvaara]

On Wed, Dec 02, 2009 at 09:04:57AM -0800, Shawn O. Pearce wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> 
> Drop invoke-r.

Dropped.

> Modify transport-helper.c to allow pushing TRANS_OPT_UPLOADPACK and
> TRANS_OPT_RECEIVEPACK down into the helper via the option capability.

NAK. Modified _process_connect_or_invoke (now _process_connect) to pass
new option that appiles to connecting all subprotocols (if needed).

It looks like following:

 > capabilities
 < option
 < connect
 <
 > option servpath <blahblah>
 < ok
 > connect git-upload-pack
 < 

And from helper POV, all subprotocols should appear identical from
layer 6 POV so it doesn't make sense to diffrentiate between path
for upload-pack and receive-pack (or upload-archive!).

> I'd rename connect-r to just connect.

Yeah, putting repository in RPC explicit signature is bit ugly (there
isn't probaby ever going to be signature that doesn't contain repo as
argument). That would make it 'connect'.

Renamed.

-Ilari

Re: [RFC PATCH 4/8] Support remote helpers implementing smart transports

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> On Wed, Dec 02, 2009 at 09:04:57AM -0800, Shawn O. Pearce wrote:
> > Modify transport-helper.c to allow pushing TRANS_OPT_UPLOADPACK and
> > TRANS_OPT_RECEIVEPACK down into the helper via the option capability.
> 
> NAK. Modified _process_connect_or_invoke (now _process_connect) to pass
> new option that appiles to connecting all subprotocols (if needed).
...
> And from helper POV, all subprotocols should appear identical from
> layer 6 POV so it doesn't make sense to diffrentiate between path
> for upload-pack and receive-pack (or upload-archive!).

That may be true, but the remote.origin.uploadpack and
remote.origin.receivepack configuration options exist
and are passed through as these option uploadpack and
option receivepack callbacks.

If you want to pass through a single option with the remote program
name, you need to do that immediately before the connect invoke
occurs, and instead buffer the two different configuration options
in the struct transport.  I think that would make the series messier
than it is.
 
-- 
Shawn.