[RFC PATCH v3 0/8] Remote helpers smart transport extensions

[RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Ilari Liusvaara]

Changes from v2 to v3:

- Rename "FALLBACK" -> "fallback"
- Get rid of magic "layer6 ready" return
- Initialize paths when called as foo::bar://
- Remove dead code as NULL remote can't happen
- Move the stream dupping from unknown protocols patch to taking over
  transports patch (that one needs it)
- Reorder the series so that debugging and mandatory cap patches are
  first.
- Rename virtual_connected to got_remote_heads (that's what it is)
- Coding style fix ups
- Move transport connect code from smart transports to archive patch.

Ilari Liusvaara (8):
  Add remote helper debug mode
  Support mandatory capabilities
  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

 .gitignore                           |    4 +
 Documentation/git-remote-helpers.txt |   30 ++++-
 Makefile                             |   24 +++-
 builtin-archive.c                    |   17 ++-
 transport-helper.c                   |  265 +++++++++++++++++++++++++++++-----
 transport.c                          |  231 +++++++++++++++++++++++-------
 transport.h                          |   28 ++++
 7 files changed, 506 insertions(+), 93 deletions(-)

[RFC PATCH v3 1/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 |   90 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 65 insertions(+), 25 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..914bed0 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;
@@ -22,6 +24,45 @@ struct helper_data
 	int refspec_nr;
 };
 
+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)
+{
+	strbuf_reset(buffer);
+	if (debug)
+		fprintf(stderr, "Debug: Remote helper: Waiting...\n");
+	if (strbuf_getline(buffer, helper->out, '\n') == EOF) {
+		if (debug)
+			fprintf(stderr, "Debug: Remote helper quit.\n");
+		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);
+}
+
+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 *get_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
@@ -48,15 +89,15 @@ static struct child_process *get_helper(struct transport *transport)
 		die("Unable to run helper: git %s", helper->argv[0]);
 	data->helper = helper;
 
-	write_str_in_full(helper->in, "capabilities\n");
+	write_constant(helper->in, "capabilities\n");
 
 	data->out = xfdopen(helper->out, "r");
 	while (1) {
-		if (strbuf_getline(&buf, data->out, '\n') == EOF)
-			exit(128); /* child died, message supplied already */
+		recvline(data, &buf);
 
 		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"))
@@ -82,14 +123,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);
 		fclose(data->out);
 		finish_command(data->helper);
@@ -117,10 +163,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;
 
@@ -143,12 +190,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;
@@ -208,13 +250,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);
 
 		if (!prefixcmp(buf.buf, "lock ")) {
 			const char *name = buf.buf + 5;
@@ -249,12 +288,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");
 
@@ -264,7 +304,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);
@@ -369,17 +409,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);
 		if (!buf.len)
 			break;
 
@@ -471,8 +508,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);
 
 		if (!*buf.buf)
 			break;
@@ -497,6 +533,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)
@@ -510,6 +547,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.rc1.300.gfbc27

[RFC PATCH v3 2/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                   |   26 ++++++++++++++++++++------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 5cfdc0c..20a05fe 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 914bed0..f5c585d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -93,24 +93,38 @@ static struct child_process *get_helper(struct transport *transport)
 
 	data->out = xfdopen(helper->out, "r");
 	while (1) {
+		const char* capname;
+		int mandatory = 0;
 		recvline(data, &buf);
 
 		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 "));
+		} else if (mandatory) {
+			fflush(stderr);
+			die("Unknown madatory capability %s. This remote "
+			    "helper probably needs newer version of Git.\n",
+			    capname);
 		}
 	}
 	if (refspecs) {
-- 
1.6.6.rc1.300.gfbc27

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

[Ilari Liusvaara]

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

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

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport-helper.c |   22 ++++++++++++-
 transport.c        |   87 +++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index f5c585d..95aaa02 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -63,6 +63,26 @@ static void write_constant(int fd, const char *str)
 		die_errno("Full write to remote helper failed");
 }
 
+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;
@@ -83,7 +103,7 @@ static struct child_process *get_helper(struct transport *transport)
 	strbuf_addf(&buf, "remote-%s", data->name);
 	helper->argv[0] = strbuf_detach(&buf, NULL);
 	helper->argv[1] = transport->remote->name;
-	helper->argv[2] = transport->url;
+	helper->argv[2] = remove_ext_force(transport->url);
 	helper->git_cmd = 1;
 	if (start_command(helper))
 		die("Unable to run helper: git %s", helper->argv[0]);
diff --git a/transport.c b/transport.c
index 3eea836..eadd553 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));
@@ -805,30 +854,23 @@ struct transport *transport_get(struct remote *remote, const char *url)
 
 	if (remote && remote->foreign_vcs) {
 		transport_helper_init(ret, remote->foreign_vcs);
-		return ret;
-	}
-
-	if (!prefixcmp(url, "rsync:")) {
+	} else if (!prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
-
-	} else if (!prefixcmp(url, "http://")
-	        || !prefixcmp(url, "https://")
-	        || !prefixcmp(url, "ftp://")) {
-		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 +887,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.");
+#endif
+	} 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.rc1.300.gfbc27

[RFC PATCH v3 4/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 |   13 ++++++++++
 2 files changed, 60 insertions(+), 31 deletions(-)

diff --git a/transport.c b/transport.c
index eadd553..deaf983 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);
@@ -858,12 +853,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://")
@@ -873,20 +870,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://")) {
@@ -904,14 +895,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..d0192f9 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,10 @@ 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.rc1.300.gfbc27

[RFC PATCH v3 5/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 |   28 ++++++++++++++++++++++++-
 transport.c        |   57 +++++++++++++++++++++++++++++++++++++++++++++++----
 transport.h        |   10 +++++++++
 3 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 95aaa02..3d99fe1 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -22,6 +22,7 @@ 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 void sendline(struct helper_data *helper, struct strbuf *buffer)
@@ -63,6 +64,15 @@ static void write_constant(int fd, const char *str)
 		die_errno("Full write to remote helper failed");
 }
 
+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;
@@ -91,6 +101,7 @@ static struct child_process *get_helper(struct transport *transport)
 	const char **refspecs = NULL;
 	int refspec_nr = 0;
 	int refspec_alloc = 0;
+	int duped;
 
 	if (data->helper)
 		return data->helper;
@@ -109,9 +120,21 @@ static struct child_process *get_helper(struct transport *transport)
 		die("Unable to run helper: git %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.
+	*/
+        duped = dup(helper->out);
+	if (duped < 0)
+		die_errno("Can't dup helper output fd");
+	data->out = xfdopen(duped, "r");
+	setvbuf(data->out, NULL, _IONBF, 0);
+
 	write_constant(helper->in, "capabilities\n");
 
-	data->out = xfdopen(helper->out, "r");
 	while (1) {
 		const char* capname;
 		int mandatory = 0;
@@ -171,6 +194,7 @@ static int disconnect_helper(struct transport *transport)
 		strbuf_addf(&buf, "\n");
 		sendline(data, &buf);
 		close(data->helper->in);
+		close(data->helper->out);
 		fclose(data->out);
 		finish_command(data->helper);
 		free((char *)data->helper->argv[0]);
@@ -590,5 +614,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 deaf983..e9802e3 100644
--- a/transport.c
+++ b/transport.c
@@ -398,6 +398,7 @@ struct git_transport_data {
 	struct git_transport_options options;
 	struct child_process *conn;
 	int fd[2];
+	unsigned got_remote_heads : 1;
 	struct extra_have_objects extra_have;
 };
 
@@ -432,10 +433,15 @@ 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->conn)
+		return 0;
+
 	data->conn = git_connect(data->fd, transport->url,
 				 for_push ? data->options.receivepack :
 				 data->options.uploadpack,
 				 verbose ? CONNECT_VERBOSE : 0);
+
 	return 0;
 }
 
@@ -447,6 +453,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 	connect_setup(transport, for_push, 0);
 	get_remote_heads(data->fd[0], &refs, 0, NULL,
 			 for_push ? REF_NORMAL : 0, &data->extra_have);
+	data->got_remote_heads = 1;
 
 	return refs;
 }
@@ -477,9 +484,10 @@ 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->got_remote_heads) {
 		connect_setup(transport, 0, 0);
 		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
+		data->got_remote_heads = 1;
 	}
 
 	refs = fetch_pack(&args, data->fd, data->conn,
@@ -490,6 +498,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	if (finish_connect(data->conn))
 		refs = NULL;
 	data->conn = NULL;
+	data->got_remote_heads = 0;
 
 	free_refs(refs_tmp);
 
@@ -718,12 +727,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	struct send_pack_args args;
 	int ret;
 
-	if (!data->conn) {
+	if (!data->got_remote_heads) {
 		struct ref *tmp_refs;
 		connect_setup(transport, 1, 0);
 
 		get_remote_heads(data->fd[0], &tmp_refs, 0, NULL, REF_NORMAL,
 				 NULL);
+		data->got_remote_heads = 1;
 	}
 
 	memset(&args, 0, sizeof(args));
@@ -741,6 +751,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->got_remote_heads = 0;
 
 	return ret;
 }
@@ -749,7 +760,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->got_remote_heads)
+			packet_flush(data->fd[1]);
 		close(data->fd[0]);
 		close(data->fd[1]);
 		finish_connect(data->conn);
@@ -759,6 +771,35 @@ static int disconnect_git(struct transport *transport)
 	return 0;
 }
 
+void transport_take_over(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->got_remote_heads = 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, ':');
@@ -854,6 +895,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;
@@ -861,6 +903,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://")
@@ -876,8 +919,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->got_remote_heads = 0;
 	} else if (!prefixcmp(url, "http://")
 		|| !prefixcmp(url, "https://")
 		|| !prefixcmp(url, "ftp://")) {
@@ -938,9 +983,9 @@ int transport_push(struct transport *transport,
 	*nonfastforward = 0;
 	verify_remote_names(refspec_nr, refspec);
 
-	if (transport->push)
+	if (transport->push) {
 		return transport->push(transport, refspec_nr, refspec, flags);
-	if (transport->push_refs) {
+	} else if (transport->push_refs) {
 		struct ref *remote_refs =
 			transport->get_refs_list(transport, 1);
 		struct ref *local_refs = get_local_heads();
@@ -984,6 +1029,7 @@ const struct ref *transport_get_remote_refs(struct transport *transport)
 {
 	if (!transport->remote_refs)
 		transport->remote_refs = transport->get_refs_list(transport, 0);
+
 	return transport->remote_refs;
 }
 
@@ -1018,6 +1064,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
 	}
 
 	rc = transport->fetch(transport, nr_heads, heads);
+
 	free(heads);
 	return rc;
 }
diff --git a/transport.h b/transport.h
index d0192f9..499962d 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.
@@ -128,6 +137,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
 int transport_disconnect(struct transport *transport);
 char *transport_anonymize_url(const char *url);
+void transport_take_over(struct transport *transport);
 
 /* Transport methods defined outside transport.c */
 int transport_helper_init(struct transport *transport, const char *name);
-- 
1.6.6.rc1.300.gfbc27

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

[Ilari Liusvaara]

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

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 20a05fe..b957813 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -93,6 +93,20 @@ Supported if the helper has the "push" capability.
 +
 Supported if the helper has the "import" capability.
 
+'connect' <service>::
+	Connects to given service. Stdin and stdout of helper are
+	connected to specified service (git prefix is included in service
+	name so e.g. fetching uses 'git-upload-pack' as service) on
+	remote side. Valid replies to this command are empty line
+	(connection established), 'fallback' (no smart transport support,
+	fall back to dumb transports) and just exiting with error message
+	printed (can't connect, don't bother trying to fall back). After
+	line feed terminating the positive (empty) response, the output
+	of service starts. After the connection ends, the remote
+	helper exits.
++
+Supported if the helper has the "connect" 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
@@ -126,6 +140,9 @@ CAPABILITIES
 	all, it must cover all refs reported by the list command; if
 	it is not used, it is effectively "*:*"
 
+'connect'::
+	This helper supports the 'connect' command.
+
 REF LIST ATTRIBUTES
 -------------------
 
@@ -168,9 +185,15 @@ OPTIONS
 	but don't actually change any repository data.	For most
 	helpers this only applies to the 'push', if supported.
 
+'option servpath <c-style-quoted-path>'::
+	Set service path (--upload-pack, --receive-pack etc.) for
+	next connect. Remote helper MAY support this option. Remote
+	helper MUST NOT rely on this option being set before
+	connect request occurs.
+
 Documentation
 -------------
-Documentation by Daniel Barkalow.
+Documentation by Daniel Barkalow and Ilari Liusvaara
 
 GIT
 ---
diff --git a/transport-helper.c b/transport-helper.c
index 3d99fe1..e0254bc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -18,7 +18,8 @@ struct helper_data
 	unsigned fetch : 1,
 		import : 1,
 		option : 1,
-		push : 1;
+		push : 1,
+		connect : 1;
 	/* These go from remote name (as in "list") to private name */
 	struct refspec *refspecs;
 	int refspec_nr;
@@ -163,7 +164,10 @@ static struct child_process *get_helper(struct transport *transport)
 				   refspec_nr + 1,
 				   refspec_alloc);
 			refspecs[refspec_nr++] = strdup(buf.buf + strlen("refspec "));
-		} else if (mandatory) {
+		}
+		else if (!strcmp(capname, "connect"))
+			data->connect = 1;
+		else if (mandatory) {
 			fflush(stderr);
 			die("Unknown madatory capability %s. This remote "
 			    "helper probably needs newer version of Git.\n",
@@ -386,12 +390,78 @@ static int fetch_with_import(struct transport *transport,
 	return 0;
 }
 
+static int _process_connect(struct transport *transport,
+				      const char *name, const char *exec)
+{
+	struct helper_data *data = transport->data;
+	struct strbuf cmdbuf = STRBUF_INIT;
+	struct child_process *helper;
+	int r;
+
+	helper = get_helper(transport);
+
+	/* Handle --upload-pack and friends. This is fire and forget...
+	   just warn if it fails. */
+	if (strcmp(name, exec)) {
+		r = set_helper_option(transport, "servpath", exec);
+		if (r > 0)
+			fprintf(stderr, "Warning: Setting remote service path "
+				"not supported by protocol.\n");
+		else if (r < 0)
+			fprintf(stderr, "Warning: Invalid remote service "
+				"path.\n");
+	}
+
+	if (data->connect)
+		strbuf_addf(&cmdbuf, "connect %s\n", name);
+	else
+		return 0;
+
+	xchgline(data, &cmdbuf);
+	if (!strcmp(cmdbuf.buf, "")) {
+		if (debug)
+			fprintf(stderr, "Debug: Smart transport connection "
+				"ready.\n");
+		return 1;
+	} else if (!strcmp(cmdbuf.buf, "fallback")) {
+		if (debug)
+			fprintf(stderr, "Debug: Falling back to dumb "
+				"transport.\n");
+		return 0;
+	} else
+		die("Unknown response to connect: %s",
+			cmdbuf.buf);
+
+	return 0;	/* Shouldn't be here. */
+}
+
+static int process_connect(struct transport* transport,
+				     int for_push)
+{
+	struct helper_data *data = transport->data;
+	const char *name;
+	const char *exec;
+
+	name = for_push ? "git-receive-pack" : "git-upload-pack";
+	if (for_push)
+		exec = data->gitoptions.receivepack;
+	else
+		exec = data->gitoptions.uploadpack;
+
+	return _process_connect(transport, name, exec);
+}
+
 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(transport, 0)) {
+		transport_take_over(transport);
+		return transport->fetch(transport, nr_heads, to_fetch);
+	}
+
 	count = 0;
 	for (i = 0; i < nr_heads; i++)
 		if (!(to_fetch[i]->status & REF_STATUS_UPTODATE))
@@ -419,6 +489,11 @@ static int push_refs(struct transport *transport,
 	struct child_process *helper;
 	struct ref *ref;
 
+	if (process_connect(transport, 1)) {
+		transport_take_over(transport);
+		return transport->push_refs(transport, remote_refs, flags);
+	}
+
 	if (!remote_refs)
 		return 0;
 
@@ -559,6 +634,11 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	helper = get_helper(transport);
 
+	if (process_connect(transport, for_push)) {
+		transport_take_over(transport);
+		return transport->get_refs_list(transport, for_push);
+	}
+
 	if (data->push && for_push)
 		write_str_in_full(helper->in, "list for-push\n");
 	else
-- 
1.6.6.rc1.300.gfbc27

[RFC PATCH v3 7/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 ++++++++++-------
 transport-helper.c |   19 +++++++++++++++++++
 transport.c        |   21 +++++++++++++++++++++
 transport.h        |    5 +++++
 4 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 12351e9..d34b3fd 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, "git-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;
 }
diff --git a/transport-helper.c b/transport-helper.c
index e0254bc..a49be55 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -451,6 +451,24 @@ static int process_connect(struct transport* transport,
 	return _process_connect(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 connect is inited. */
+	get_helper(transport);
+	if (!data->connect)
+		die("Operation not supported by protocol.");
+
+	if (!_process_connect(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)
 {
@@ -694,6 +712,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 e9802e3..b418889 100644
--- a/transport.c
+++ b/transport.c
@@ -756,6 +756,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;
@@ -917,6 +928,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;
@@ -1078,6 +1090,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 499962d..d702e31 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
@@ -139,6 +141,9 @@ int transport_disconnect(struct transport *transport);
 char *transport_anonymize_url(const char *url);
 void transport_take_over(struct transport *transport);
 
+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.rc1.300.gfbc27

[RFC PATCH v3 8/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  |    4 ++++
 Makefile    |   24 ++++++++++++++++++++++--
 transport.c |    8 --------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7cc54b4..7c79f97 100644
--- a/.gitignore
+++ b/.gitignore
@@ -107,6 +107,10 @@
 /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..1332225 100644
--- a/Makefile
+++ b/Makefile
@@ -424,6 +424,13 @@ BUILT_INS += git-stage$X
 BUILT_INS += git-status$X
 BUILT_INS += git-whatchanged$X
 
+#ifdef NO_CURL
+REMOTE_CURL_NAMES =
+#else
+# Yes, this is missing git-remote-http intentionally!
+REMOTE_CURL_NAMES = git-remote-https git-remote-ftp git-remote-ftps
+#endif
+
 # what 'all' will build and 'install' will install in gitexecdir,
 # excluding programs for built-in commands
 ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
@@ -1097,7 +1104,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 +1683,13 @@ 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)
+$(REMOTE_CURL_NAMES): git-remote-http$X
+	$(QUIET_LNCP)$(RM) $@ && \
+	ln $< $@ 2>/dev/null || \
+	ln -s $< $@ 2>/dev/null || \
+	cp $< $@
+
+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)
 
@@ -1853,6 +1866,7 @@ endif
 ifneq (,$X)
 	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
 endif
+
 	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
 	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
 	{ test "$$bindir/" = "$$execdir/" || \
@@ -1866,6 +1880,12 @@ endif
 		ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
 	  done; } && \
+	{ for p in $(REMOTE_CURL_NAMES); do \
+		$(RM) "$$execdir/$$p" && \
+		ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
+		ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
+		cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
+	  done; } && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
 install-doc:
diff --git a/transport.c b/transport.c
index b418889..c6684ac 100644
--- a/transport.c
+++ b/transport.c
@@ -935,14 +935,6 @@ struct transport *transport_get(struct remote *remote, const char *url)
 
 		data->conn = NULL;
 		data->got_remote_heads = 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.");
-#endif
 	} else {
 		/* Unknown protocol in URL. Pass to external handler. */
 		int len = external_specification_len(url);
-- 
1.6.6.rc1.300.gfbc27

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Junio C Hamano]

I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
while doing so.  Please sanity check the result.

Thanks.

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
> while doing so.  Please sanity check the result.

I see that you changed many "char* variable" to "char *variable", but
what is the reason for these changes?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Erik Faye-Lund]

On Mon, Dec 7, 2009 at 1:06 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> Quoting Junio C Hamano <gitster@pobox.com>
>
>> I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
>> while doing so.  Please sanity check the result.
>
> I see that you changed many "char* variable" to "char *variable", but
> what is the reason for these changes?
>

Documentation/CodingGuidelines:

 - When declaring pointers, the star sides with the variable
   name, i.e. "char *string", not "char* string" or
   "char * string".  This makes it easier to understand code
   like "char *string, c;".

-- 
Erik "kusma" Faye-Lund

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Nicolas Pitre]

On Mon, 7 Dec 2009, Nanako Shiraishi wrote:

> Quoting Junio C Hamano <gitster@pobox.com>
> 
> > I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
> > while doing so.  Please sanity check the result.
> 
> I see that you changed many "char* variable" to "char *variable", but
> what is the reason for these changes?

The * is an attribute of the variable and not of the type.  This makes 
for clearer code.


Nicolas

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Ilari Liusvaara]

On Sun, Dec 06, 2009 at 11:36:08PM -0800, Junio C Hamano wrote:
> I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
> while doing so.  Please sanity check the result.

The conflict resolution seems sane. 

Sorry about leaving lots of codingstyle stuff unfixed.

-Ilari

Re: [RFC PATCH v3 5/8] Support taking over transports

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> @@ -109,9 +120,21 @@ static struct child_process *get_helper(struct transport *transport)
>  		die("Unable to run helper: git %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.
> +	*/
> +        duped = dup(helper->out);

Formatting error here, the line is indented wrong.

> +	if (duped < 0)
> +		die_errno("Can't dup helper output fd");
> +	data->out = xfdopen(duped, "r");
> +	setvbuf(data->out, NULL, _IONBF, 0);

I wonder if this is really a good idea.  Most helpers actually
use a lot of text based IO to communicate.  Disabling buffering
for those helpers to avoid overreading the advertisement from a
connect is a problem.

Maybe we could leave buffering on, but use a handshake protocol
with the helper during connect:

  (1) > "connect git-upload-pack\n"
  (2) < "\n"
  (3) > "begin\n"

During 2 we are still buffered, but the only content on the pipe
should be the single blank line, so we pull that in and the FILE*
buffer should be empty.

After writing message 2 the remote helper blocks reading for the
"begin\n" message 3.  This gives the transport-helper.c code time
to switch off the FILE* and start using raw IO off the pipe before
any data starts coming down the line.

It does mean that the helper may need to run unbuffered IO, but
if the helper is only a smart helper supporting connect then this
isn't really a problem.  It can run buffered IO until connect is
received, switch to unbuffered, and use unbuffered for the single
"begin\n" message it has to consume before it starts writing output
or reading input.

-- 
Shawn.

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

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> diff --git a/transport-helper.c b/transport-helper.c
> index 3d99fe1..e0254bc 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
>  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(transport, 0)) {
> +		transport_take_over(transport);
> +		return transport->fetch(transport, nr_heads, to_fetch);
> +	}

We should already be connected because of the prior call into
get_refs_list().  If I read your code correctly we'd try to open
a new connection right here, which makes no sense.  But its also
possible for us to be in a different transport, so we do code with
the assumption that we didn't get invoked through get_refs_list()
first and therefore need to open the connection ourselves.

Also, given the above invocation pattern, I see no reason why you
need the disown virtual function on struct transport*.  Just pass
the #@!**! struct child* into transport_take_over() from the 3
call sites here and get rid of that unnecessary indirection.

-- 
Shawn.

Re: [RFC PATCH v3 7/8] Support remote archive from external protocol helpers

[Shawn O. Pearce]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> Helpers which support invoke/connect also should support remote archive

There is no such thing as invoke anymore.

-- 
Shawn.

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Junio C Hamano]

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

> On Sun, Dec 06, 2009 at 11:36:08PM -0800, Junio C Hamano wrote:
>> I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
>> while doing so.  Please sanity check the result.
>
> The conflict resolution seems sane. 
>
> Sorry about leaving lots of codingstyle stuff unfixed.

No problem at all, and thanks.

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>
>
>> I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
>> while doing so.  Please sanity check the result.
>
> I see that you changed many "char* variable" to "char *variable", but
> what is the reason for these changes?

Nico already gave you correct and more concise version; this more verbose
explanation is primarily for you who said a few times that you are not
fluent in C.  Others can skip this message without missing anything.

I haven't asked people why they choose to write like this:

	char* string;

beyond "that is how we were taught and what we are used to".

But if I have to guess, it is because it makes it appear as if you are
being consistent between basic types like "int" and pointer types like
"char *".  E.g. these both look like "<type> <variable>;"

	int     variable;
        char*   variable;

The example in CodingGuidelines gives a clear illustration that this
however is not the way how C language works.  IOW, the syntax for
declaration is not "<type> <var1>, <var2>,...;".  E.g.

	char*	var1, var2;

declares var1, which is a pointer to a char, and var2, which is a char.
That is what Nico means by '* is an attribute of the variable and not of
the type'.

The only sane way to read declaration in C is to read from inside to
outside, starting from a variable name.  When you read "char **argv", for
example, you treat it as "char (*(*argv))", and read it from the variable
name:

    - It declares argv; what's the type of it I wonder...

    - It is a pointer, because the asterisk in front of it tells us that
      we can dereference it. Now, what's the type of the result of
      dereferencing argv, i.e. "*argv", I wonder...

    - That "*argv" is a pointer, because the asterisk in front of it again
      tells us that we can dereference it. Now, what's the type of
      dereferencing that "*argv", i.e. "**argv", I wonder...

    - Ah, it is a "char", that is what this declaration says.

And the style we use (which is the same as Linux kernel style) naturally
matches the way how you read this declaration.

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

[Ilari Liusvaara]

On Mon, Dec 07, 2009 at 10:11:48AM -0800, Shawn O. Pearce wrote:
> 
> We should already be connected because of the prior call into
> get_refs_list().  If I read your code correctly we'd try to open
> a new connection right here, which makes no sense. 

The have prior connection case can't happen since take_over_transport()
overwrites the method pointers.

> But its also
> possible for us to be in a different transport, so we do code with
> the assumption that we didn't get invoked through get_refs_list()
> first and therefore need to open the connection ourselves.

Right. The reason why the code is there is in case somebody invokes
fetch() first.

The same things apply to push function too.

> Also, given the above invocation pattern, I see no reason why you
> need the disown virtual function on struct transport*.  Just pass
> the #@!**! struct child* into transport_take_over() from the 3
> call sites here and get rid of that unnecessary indirection.

Fixed.

-Ilari

Re: [RFC PATCH v3 7/8] Support remote archive from external protocol helpers

[Ilari Liusvaara]

On Mon, Dec 07, 2009 at 10:12:27AM -0800, Shawn O. Pearce wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:

> There is no such thing as invoke anymore.

Fixed.

-Ilari

Re: [RFC PATCH v3 5/8] Support taking over transports

[Ilari Liusvaara]

On Mon, Dec 07, 2009 at 09:49:47AM -0800, Shawn O. Pearce wrote:
> 
> > +	if (duped < 0)
> > +		die_errno("Can't dup helper output fd");
> > +	data->out = xfdopen(duped, "r");
> > +	setvbuf(data->out, NULL, _IONBF, 0);
> 
> I wonder if this is really a good idea.  Most helpers actually
> use a lot of text based IO to communicate.  Disabling buffering
> for those helpers to avoid overreading the advertisement from a
> connect is a problem.

I dropped the buffering change. 
 
> Maybe we could leave buffering on, but use a handshake protocol
> with the helper during connect:
> 
>   (1) > "connect git-upload-pack\n"
>   (2) < "\n"
>   (3) > "begin\n"
> 
> During 2 we are still buffered, but the only content on the pipe
> should be the single blank line, so we pull that in and the FILE*
> buffer should be empty.

Doesn't work. Stream buffering can only be changed before first
I/O.
 
I figured out a solution. Dup the file descriptor second time and
make another FILE* for it. Then send the connect and read the
response over the new stream. This avoids overreading and allows
other I/O to be buffered. After connect attempt, the new streaam
can be closed.

This way most of the I/O can be buffered, only reading responses
to connect commands can't, and that is at most 1 byte if fallbacks
aren't required. And the thing fits nicely inside _process_connect().
Oh, and no protocol change needed.

-Ilari

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> Quoting Junio C Hamano <gitster@pobox.com>
>>
>>> I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
>>> while doing so.  Please sanity check the result.
>>
>> I see that you changed many "char* variable" to "char *variable", but
>> what is the reason for these changes?
>
> Nico already gave you correct and more concise version; this more verbose
> explanation is primarily for you who said a few times that you are not
> fluent in C.  Others can skip this message without missing anything.

Thanks everybody for quick answers and thank you, Junio, 
for a detailed explanation. Now I understand.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Jeff King]

On Mon, Dec 07, 2009 at 12:07:24PM -0800, Junio C Hamano wrote:

> I haven't asked people why they choose to write like this:
> 
> 	char* string;
> 
> beyond "that is how we were taught and what we are used to".

I have seen it in C++ code and recommended many years ago on
comp.lang.c++. The argument was something along the lines of:

  1. It's good to keep type information together, especially in C++
     where you are often doing things like using types as template
     parameters.

  2. The fact that "char* foo, bar" doesn't do what you want isn't
     relevant if you have a style guideline not to declare two variables
     on the same line (because it's easier to notice both if they each
     get their own line, and because in C++ you can declare closer to
     the point of use).

But that is me paraphrasing an argument I read on usenet almost 10 years
ago, so I may be entirely misremembering (and please don't flame me; I
am presenting it for anthropological curiosity, not because I believe we
should use that style).

-Peff

Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions

[Jeff King]

On Tue, Dec 08, 2009 at 12:57:35AM -0500, Jeff King wrote:

> On Mon, Dec 07, 2009 at 12:07:24PM -0800, Junio C Hamano wrote:
> 
> > I haven't asked people why they choose to write like this:
> > 
> > 	char* string;
> > 
> > beyond "that is how we were taught and what we are used to".
> 
> I have seen it in C++ code and recommended many years ago on
> comp.lang.c++. The argument was something along the lines of:

Perhaps I should have simply used google:

  http://www2.research.att.com/~bs/bs_faq2.html#whitespace

-Peff