[PATCH/RFC 0/6] fetch with refspec

[PATCH/RFC 0/6] fetch with refspec

[David Turner]

We've got a lot of refs, but pretty frequently we only want to fetch
one.  It's silly for the server to send a bunch of refs that the client
is just going to ignore.  Here are some patches that fix that.

Let me know if this seems reasonable.

(and I'll start in on another round of index-helper as soon as this is sent!)

David Turner (6):
  http-backend: use argv_array functions
  remote-curl.c: fix variable shadowing
  http-backend: handle refspec argument
  transport: add refspec list parameters to functions
  fetch: pass refspec to http server
  clone: send refspec for single-branch clones

 Documentation/technical/protocol-capabilities.txt | 23 +++++++
 builtin/clone.c                                   | 16 ++++-
 builtin/fetch.c                                   | 24 ++++++-
 builtin/ls-remote.c                               |  2 +-
 builtin/remote.c                                  |  2 +-
 http-backend.c                                    | 23 +++++--
 remote-curl.c                                     | 25 ++++---
 t/t5552-http-fetch-branch.sh                      | 47 +++++++++++++
 transport-helper.c                                | 44 ++++++++----
 transport.c                                       | 14 ++--
 transport.h                                       |  4 +-
 upload-pack.c                                     | 81 ++++++++++++++++++++++-
 12 files changed, 261 insertions(+), 44 deletions(-)
 create mode 100755 t/t5552-http-fetch-branch.sh

-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH/RFC 1/6] http-backend: use argv_array functions

[David Turner]

Signed-off-by: David Turner <dturner@twopensource.com>
---
 http-backend.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 8870a26..a4f0066 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -450,9 +450,7 @@ static void get_info_refs(char *arg)
 	hdr_nocache();
 
 	if (service_name) {
-		const char *argv[] = {NULL /* service name */,
-			"--stateless-rpc", "--advertise-refs",
-			".", NULL};
+		struct argv_array argv = ARGV_ARRAY_INIT;
 		struct rpc_service *svc = select_service(service_name);
 
 		strbuf_addf(&buf, "application/x-git-%s-advertisement",
@@ -463,9 +461,13 @@ static void get_info_refs(char *arg)
 		packet_write(1, "# service=git-%s\n", svc->name);
 		packet_flush(1);
 
-		argv[0] = svc->name;
-		run_service(argv, 0);
+		argv_array_push(&argv, svc->name);
+		argv_array_push(&argv, "--stateless-rpc");
+		argv_array_push(&argv, "--advertise-refs");
 
+		argv_array_push(&argv, ".");
+		run_service(argv.argv, 0);
+		argv_array_clear(&argv);
 	} else {
 		select_getanyfile();
 		for_each_namespaced_ref(show_text_ref, &buf);
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH/RFC 2/6] remote-curl.c: fix variable shadowing

[David Turner]

The local variable 'options' was shadowing a global of the same name.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 remote-curl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..b9b6a90 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -254,7 +254,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	struct strbuf effective_url = STRBUF_INIT;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
-	struct http_get_options options;
+	struct http_get_options get_options;
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -271,15 +271,15 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		strbuf_addf(&refs_url, "service=%s", service);
 	}
 
-	memset(&options, 0, sizeof(options));
-	options.content_type = &type;
-	options.charset = &charset;
-	options.effective_url = &effective_url;
-	options.base_url = &url;
-	options.no_cache = 1;
-	options.keep_error = 1;
+	memset(&get_options, 0, sizeof(get_options));
+	get_options.content_type = &type;
+	get_options.charset = &charset;
+	get_options.effective_url = &effective_url;
+	get_options.base_url = &url;
+	get_options.no_cache = 1;
+	get_options.keep_error = 1;
 
-	http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
+	http_ret = http_get_strbuf(refs_url.buf, &buffer, &get_options);
 	switch (http_ret) {
 	case HTTP_OK:
 		break;
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH/RFC 3/6] http-backend: handle refspec argument

[David Turner]

Allow clients to pass a "refspec" parameter through to upload-pack;
upload-pack will only advertise refs which match that refspec.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 http-backend.c |  9 +++++++
 upload-pack.c  | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index a4f0066..9731391 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -452,6 +452,7 @@ static void get_info_refs(char *arg)
 	if (service_name) {
 		struct argv_array argv = ARGV_ARRAY_INIT;
 		struct rpc_service *svc = select_service(service_name);
+		const char *refspec;
 
 		strbuf_addf(&buf, "application/x-git-%s-advertisement",
 			svc->name);
@@ -465,6 +466,14 @@ static void get_info_refs(char *arg)
 		argv_array_push(&argv, "--stateless-rpc");
 		argv_array_push(&argv, "--advertise-refs");
 
+		refspec = get_parameter("refspec");
+		if (refspec) {
+			struct strbuf interesting_refs = STRBUF_INIT;
+			strbuf_addstr(&interesting_refs, "--interesting-refs=");
+			strbuf_addstr(&interesting_refs, refspec);
+			argv_array_push(&argv, interesting_refs.buf);
+			strbuf_release(&interesting_refs);
+		}
 		argv_array_push(&argv, ".");
 		run_service(argv.argv, 0);
 		argv_array_clear(&argv);
diff --git a/upload-pack.c b/upload-pack.c
index b3f6653..da140c2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -52,6 +52,7 @@ static int keepalive = 5;
 static int use_sideband;
 static int advertise_refs;
 static int stateless_rpc;
+static struct string_list interesting_refspecs = STRING_LIST_INIT_DUP;
 
 static void reset_timeout(void)
 {
@@ -687,16 +688,61 @@ static void receive_needs(void)
 	free(shallows.objects);
 }
 
-/* return non-zero if the ref is hidden, otherwise 0 */
+struct refspec_data {
+	int has_star;
+	size_t prefixlen;
+	size_t suffixlen;
+};
+
+static int matches_refspec(const char *refspec, struct refspec_data *data,
+		    const char *ref)
+{
+	size_t len;
+
+	if (!data->has_star)
+		return !strcmp(refspec, ref);
+
+	if (strncmp(refspec, ref, data->prefixlen))
+		return -1;
+
+	len = strlen(refspec);
+	if (len < data->prefixlen + data->suffixlen)
+		return -1;
+
+	return strcmp(ref + (len - data->suffixlen),
+		      refspec + data->prefixlen + 1);
+}
+
+/*
+ * return non-zero if the ref is hidden or outside the provided
+ * refspecs, otherwise 0
+*/
 static int mark_our_ref(const char *refname, const char *refname_full,
 			const struct object_id *oid)
 {
 	struct object *o = lookup_unknown_object(oid->hash);
+	struct string_list_item *item;
 
 	if (ref_is_hidden(refname, refname_full)) {
 		o->flags |= HIDDEN_REF;
 		return 1;
 	}
+
+	if (interesting_refspecs.nr) {
+		int found = 0;
+		/*
+		 * TODO: this could be faster for large numbers of
+		 * refspecs by using tries or a DFA.
+		 */
+		for_each_string_list_item(item, &interesting_refspecs)
+			if (matches_refspec(item->string, item->util, refname)) {
+				found = 1;
+				break;
+			}
+		if (!found)
+			return 1;
+
+	}
 	o->flags |= OUR_REF;
 	return 0;
 }
@@ -725,7 +771,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
-		" include-tag multi_ack_detailed";
+		" include-tag multi_ack_detailed interesting-refs";
 	const char *refname_nons = strip_namespace(refname);
 	struct object_id peeled;
 
@@ -820,6 +866,24 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
+static struct refspec_data *make_refspec_data(const char *refspec)
+{
+	struct refspec_data *data;
+	const char *star;
+
+	data = xmalloc(sizeof(struct refspec_data));
+	star = strchr(refspec, '*');
+	if (star) {
+		data->has_star = 1;
+		data->prefixlen = star - refspec;
+		data->suffixlen = strlen(refspec) - (data->prefixlen + 1);
+	} else {
+		data->has_star = 0;
+	}
+
+	return data;
+}
+
 int main(int argc, char **argv)
 {
 	char *dir;
@@ -841,6 +905,19 @@ int main(int argc, char **argv)
 			advertise_refs = 1;
 			continue;
 		}
+		if (starts_with(arg, "--interesting-refs=")) {
+			struct string_list_item *item;
+
+			string_list_split(&interesting_refspecs, arg + 19,
+					  ' ', -1);
+			for_each_string_list_item(item, &interesting_refspecs) {
+				if (check_refname_format(item->string,
+							 REFNAME_REFSPEC_PATTERN))
+					die("invalid refspec %s", item->string);
+				item->util = make_refspec_data(item->string);
+			}
+			continue;
+		}
 		if (!strcmp(arg, "--stateless-rpc")) {
 			stateless_rpc = 1;
 			continue;
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

Add parameters for a list of refspecs to transport_get_remote_refs and
get_refs_list.  These parameters are presently unused -- soon, we will
use them to implement fetches which only learn about a subset of refs.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 builtin/clone.c     |  2 +-
 builtin/fetch.c     |  6 ++++--
 builtin/ls-remote.c |  2 +-
 builtin/remote.c    |  2 +-
 transport-helper.c  | 24 ++++++++++++------------
 transport.c         | 14 ++++++++------
 transport.h         |  4 ++--
 7 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 6616392..91f668c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1010,7 +1010,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (transport->smart_options && !option_depth)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
-	refs = transport_get_remote_refs(transport);
+	refs = transport_get_remote_refs(transport, NULL, 0);
 
 	if (refs) {
 		mapped_refs = wanted_peer_refs(refs, refspec);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e4639d8..cafab37 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -221,7 +221,8 @@ static void find_non_local_tags(struct transport *transport,
 	struct string_list_item *item = NULL;
 
 	for_each_ref(add_existing, &existing_refs);
-	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+	for (ref = transport_get_remote_refs(transport, NULL, 0);
+	     ref; ref = ref->next) {
 		if (!starts_with(ref->name, "refs/tags/"))
 			continue;
 
@@ -301,8 +302,9 @@ static struct ref *get_ref_map(struct transport *transport,
 
 	/* opportunistically-updated references: */
 	struct ref *orefs = NULL, **oref_tail = &orefs;
+	const struct ref *remote_refs;
 
-	const struct ref *remote_refs = transport_get_remote_refs(transport);
+	remote_refs = transport_get_remote_refs(transport, NULL, 0);
 
 	if (refspec_count) {
 		struct refspec *fetch_refspec;
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 66cdd45..bce706e 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -94,7 +94,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
-	ref = transport_get_remote_refs(transport);
+	ref = transport_get_remote_refs(transport, NULL, 0);
 	if (transport_disconnect(transport))
 		return 1;
 
diff --git a/builtin/remote.c b/builtin/remote.c
index fda5c2e..5745e8b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -862,7 +862,7 @@ static int get_remote_ref_states(const char *name,
 	if (query) {
 		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
-		remote_refs = transport_get_remote_refs(transport);
+		remote_refs = transport_get_remote_refs(transport, NULL, 0);
 		transport_disconnect(transport);
 
 		states->queried = 1;
diff --git a/transport-helper.c b/transport-helper.c
index b934183..b5c91d2 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -99,7 +99,7 @@ static void do_take_over(struct transport *transport)
 
 static void standard_options(struct transport *t);
 
-static struct child_process *get_helper(struct transport *transport)
+static struct child_process *get_helper(struct transport *transport, const struct refspec *req_refspecs, int req_refspec_nr)
 {
 	struct helper_data *data = transport->data;
 	struct strbuf buf = STRBUF_INIT;
@@ -267,7 +267,7 @@ static int set_helper_option(struct transport *transport,
 	struct strbuf buf = STRBUF_INIT;
 	int i, ret, is_bool = 0;
 
-	get_helper(transport);
+	get_helper(transport, NULL, 0);
 
 	if (!data->option)
 		return 1;
@@ -395,7 +395,7 @@ static int fetch_with_fetch(struct transport *transport,
 
 static int get_importer(struct transport *transport, struct child_process *fastimport)
 {
-	struct child_process *helper = get_helper(transport);
+	struct child_process *helper = get_helper(transport, NULL, 0);
 	struct helper_data *data = transport->data;
 	int cat_blob_fd, code;
 	child_process_init(fastimport);
@@ -418,7 +418,7 @@ static int get_exporter(struct transport *transport,
 			struct string_list *revlist_args)
 {
 	struct helper_data *data = transport->data;
-	struct child_process *helper = get_helper(transport);
+	struct child_process *helper = get_helper(transport, NULL, 0);
 	int i;
 
 	child_process_init(fastexport);
@@ -451,7 +451,7 @@ static int fetch_with_import(struct transport *transport,
 	struct ref *posn;
 	struct strbuf buf = STRBUF_INIT;
 
-	get_helper(transport);
+	get_helper(transport, NULL, 0);
 
 	if (get_importer(transport, &fastimport))
 		die("Couldn't run fast-import");
@@ -523,7 +523,7 @@ static int process_connect_service(struct transport *transport,
 	int r, duped, ret = 0;
 	FILE *input;
 
-	helper = get_helper(transport);
+	helper = get_helper(transport, NULL, 0);
 
 	/*
 	 * Yes, dup the pipe another time, as we need unbuffered version
@@ -599,7 +599,7 @@ static int connect_helper(struct transport *transport, const char *name,
 	struct helper_data *data = transport->data;
 
 	/* Get_helper so connect is inited. */
-	get_helper(transport);
+	get_helper(transport, NULL, 0);
 	if (!data->connect)
 		die("Operation not supported by protocol.");
 
@@ -805,7 +805,7 @@ static int push_refs_with_push(struct transport *transport,
 	struct string_list cas_options = STRING_LIST_INIT_DUP;
 	struct string_list_item *cas_option;
 
-	get_helper(transport);
+	get_helper(transport, NULL, 0);
 	if (!data->push)
 		return 1;
 
@@ -888,7 +888,7 @@ static int push_refs_with_export(struct transport *transport,
 			warning("helper %s does not support 'force'", data->name);
 	}
 
-	helper = get_helper(transport);
+	helper = get_helper(transport, NULL, 0);
 
 	write_constant(helper->in, "export\n");
 
@@ -992,7 +992,7 @@ static int has_attribute(const char *attrs, const char *attr) {
 	}
 }
 
-static struct ref *get_refs_list(struct transport *transport, int for_push)
+static struct ref *get_refs_list(struct transport *transport, const struct refspec *refspecs, int refspec_count, int for_push)
 {
 	struct helper_data *data = transport->data;
 	struct child_process *helper;
@@ -1001,11 +1001,11 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 	struct ref *posn;
 	struct strbuf buf = STRBUF_INIT;
 
-	helper = get_helper(transport);
+	helper = get_helper(transport, refspecs, refspec_count);
 
 	if (process_connect(transport, for_push)) {
 		do_take_over(transport);
-		return transport->get_refs_list(transport, for_push);
+		return transport->get_refs_list(transport, refspecs, refspec_count, for_push);
 	}
 
 	if (data->push && for_push)
diff --git a/transport.c b/transport.c
index 095e61f..e241e42 100644
--- a/transport.c
+++ b/transport.c
@@ -70,7 +70,9 @@ struct bundle_transport_data {
 	struct bundle_header header;
 };
 
-static struct ref *get_refs_from_bundle(struct transport *transport, int for_push)
+static struct ref *get_refs_from_bundle(struct transport *transport,
+					const struct refspec *refspecs,
+					int refspec_count, int for_push)
 {
 	struct bundle_transport_data *data = transport->data;
 	struct ref *result = NULL;
@@ -177,7 +179,7 @@ static int connect_setup(struct transport *transport, int for_push)
 	return 0;
 }
 
-static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
+static struct ref *get_refs_via_connect(struct transport *transport, const struct refspec *refspecs, int refspec_count, int for_push)
 {
 	struct git_transport_data *data = transport->data;
 	struct ref *refs;
@@ -870,7 +872,7 @@ int transport_push(struct transport *transport,
 		if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
 			return -1;
 
-		remote_refs = transport->get_refs_list(transport, 1);
+		remote_refs = transport->get_refs_list(transport, NULL, 0, 1);
 
 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
@@ -949,10 +951,10 @@ int transport_push(struct transport *transport,
 	return 1;
 }
 
-const struct ref *transport_get_remote_refs(struct transport *transport)
+const struct ref *transport_get_remote_refs(struct transport *transport, const struct refspec *refspecs, int refspec_count)
 {
 	if (!transport->got_remote_refs) {
-		transport->remote_refs = transport->get_refs_list(transport, 0);
+		transport->remote_refs = transport->get_refs_list(transport, refspecs, refspec_count, 0);
 		transport->got_remote_refs = 1;
 	}
 
@@ -1099,7 +1101,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
 	other[len - 8] = '\0';
 	remote = remote_get(other);
 	transport = transport_get(remote, other);
-	for (extra = transport_get_remote_refs(transport);
+	for (extra = transport_get_remote_refs(transport, NULL, 0);
 	     extra;
 	     extra = extra->next)
 		cb->fn(extra, cb->data);
diff --git a/transport.h b/transport.h
index c681408..e53d860 100644
--- a/transport.h
+++ b/transport.h
@@ -65,7 +65,7 @@ struct transport {
 	 * the ref without a huge amount of effort, it should store it
 	 * in the ref's old_sha1 field; otherwise it should be all 0.
 	 **/
-	struct ref *(*get_refs_list)(struct transport *transport, int for_push);
+	struct ref *(*get_refs_list)(struct transport *transport, const struct refspec *refspecs, int refspec_count, int for_push);
 
 	/**
 	 * Fetch the objects for the given refs. Note that this gets
@@ -207,7 +207,7 @@ int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
 		   unsigned int * reject_reasons);
 
-const struct ref *transport_get_remote_refs(struct transport *transport);
+const struct ref *transport_get_remote_refs(struct transport *transport, const struct refspec *refspecs, int refspec_count);
 
 int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH/RFC 5/6] fetch: pass refspec to http server

[David Turner]

When fetching over http, send the requested refspec to the server.
The server will then only send refs matching that refspec.  It is
permitted for old servers to ignore that parameter, and the client
will automatically handle this.

When the server has many refs, and the client only wants a few, this
can save bandwidth and reduce fetch latency.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/technical/protocol-capabilities.txt | 23 +++++++++++++
 builtin/fetch.c                                   | 20 ++++++++++-
 remote-curl.c                                     |  7 ++++
 t/t5552-http-fetch-branch.sh                      | 42 +++++++++++++++++++++++
 transport-helper.c                                |  8 ++++-
 5 files changed, 98 insertions(+), 2 deletions(-)
 create mode 100755 t/t5552-http-fetch-branch.sh

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index eaab6b4..8c4a0b9 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -275,3 +275,26 @@ to accept a signed push certificate, and asks the <nonce> to be
 included in the push certificate.  A send-pack client MUST NOT
 send a push-cert packet unless the receive-pack server advertises
 this capability.
+
+interesting-refs
+----------------
+
+For HTTP(S) servers only:
+
+Whether or not the upload-pack server advertises this capability,
+fetch-pack may send a "refspec" parameter in the query string of a
+fetch request.  This capability indicates that such a parameter will
+be respected, in case the client cares.
+
+Whenever the receive-pack server gets that parameter, it will not
+advertise all refs and will instead only advertise refs that match
+those listed in the header. The parameter is a space-separated list of
+refs.  A ref may optionally contain up to one wildcard.
+Advertisements will still respect hideRefs.
+
+The presence or absence of the "refspec" parameter does not affect
+what refs a client is permitted to fetch; this is still controlled in
+the normal fashion.
+
+This saves time in the presence of a large number of refs, because the
+client need not wait for the server to send the complete list of refs.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index cafab37..c22a92f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -302,9 +302,27 @@ static struct ref *get_ref_map(struct transport *transport,
 
 	/* opportunistically-updated references: */
 	struct ref *orefs = NULL, **oref_tail = &orefs;
+	struct refspec *qualified_refspecs;
 	const struct ref *remote_refs;
 
-	remote_refs = transport_get_remote_refs(transport, NULL, 0);
+	qualified_refspecs = xcalloc(refspec_count, sizeof(*qualified_refspecs));
+	for (i = 0; i < refspec_count; i++) {
+		if (starts_with(refspecs[i].src, "refs/")) {
+			qualified_refspecs[i].src = xstrdup(refspecs[i].src);
+		} else {
+			struct strbuf buf = STRBUF_INIT;
+			strbuf_addf(&buf, "refs/heads/%s", refspecs[i].src);
+			qualified_refspecs[i].src = strbuf_detach(&buf, NULL);
+		}
+	}
+
+	remote_refs = transport_get_remote_refs(transport, qualified_refspecs,
+						refspec_count);
+
+	for (i = 0; i < refspec_count; i++) {
+		free(qualified_refspecs[i].src);
+	}
+	free(qualified_refspecs);
 
 	if (refspec_count) {
 		struct refspec *fetch_refspec;
diff --git a/remote-curl.c b/remote-curl.c
index b9b6a90..e914d3f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -20,6 +20,7 @@ static struct strbuf url = STRBUF_INIT;
 struct options {
 	int verbosity;
 	unsigned long depth;
+	char *refspec;
 	unsigned progress : 1,
 		check_self_contained_and_connected : 1,
 		cloning : 1,
@@ -43,6 +44,10 @@ static int set_option(const char *name, const char *value)
 		options.verbosity = v;
 		return 0;
 	}
+	else if (!strcmp(name, "refspec")) {
+		options.refspec = xstrdup(value);
+		return 0;
+	}
 	else if (!strcmp(name, "progress")) {
 		if (!strcmp(value, "true"))
 			options.progress = 1;
@@ -269,6 +274,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		else
 			strbuf_addch(&refs_url, '&');
 		strbuf_addf(&refs_url, "service=%s", service);
+		if (options.refspec)
+			strbuf_addf(&refs_url, "&refspec=%s", options.refspec);
 	}
 
 	memset(&get_options, 0, sizeof(get_options));
diff --git a/t/t5552-http-fetch-branch.sh b/t/t5552-http-fetch-branch.sh
new file mode 100755
index 0000000..0e905d9
--- /dev/null
+++ b/t/t5552-http-fetch-branch.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='fetch just one branch'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repo' '
+	git init "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	(
+		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+		test_commit 1
+	)
+'
+
+test_expect_success 'clone http repository' '
+	git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_expect_success 'make some more commits' '
+	(
+		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+		test_commit 2 &&
+		git checkout -b another_branch &&
+		test_commit 3
+		git checkout -b a_third_branch &&
+		test_commit 4
+	)
+'
+
+test_expect_success 'fetch with refspec only fetches requested branch' '
+	test_when_finished "rm trace" &&
+	(
+		cd clone &&
+		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch origin another_branch &&
+		! grep "refs/heads/master" ../trace
+	)
+'
+
+stop_httpd
+test_done
diff --git a/transport-helper.c b/transport-helper.c
index b5c91d2..7d75d64 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -40,6 +40,9 @@ struct helper_data {
 	struct git_transport_options transport_options;
 };
 
+static int set_helper_option(struct transport *transport,
+			     const char *name, const char *value);
+
 static void sendline(struct helper_data *helper, struct strbuf *buffer)
 {
 	if (debug)
@@ -109,6 +112,7 @@ static struct child_process *get_helper(struct transport *transport, const struc
 	int refspec_alloc = 0;
 	int duped;
 	int code;
+	int i;
 
 	if (data->helper)
 		return data->helper;
@@ -202,7 +206,6 @@ static struct child_process *get_helper(struct transport *transport, const struc
 		}
 	}
 	if (refspecs) {
-		int i;
 		data->refspec_nr = refspec_nr;
 		data->refspecs = parse_fetch_refspec(refspec_nr, refspecs);
 		for (i = 0; i < refspec_nr; i++)
@@ -214,6 +217,9 @@ static struct child_process *get_helper(struct transport *transport, const struc
 	strbuf_release(&buf);
 	if (debug)
 		fprintf(stderr, "Debug: Capabilities complete.\n");
+
+	for (i = 0; i < req_refspec_nr; i++)
+		set_helper_option(transport, "refspec", req_refspecs[i].src);
 	standard_options(transport);
 	return data->helper;
 }
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH/RFC 6/6] clone: send refspec for single-branch clones

[David Turner]

For single-branch clones (when we know in advance what the remote
branch name will be), send a refspec so that the server doesn't
tell us about any other refs.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 builtin/clone.c              | 16 +++++++++++++++-
 t/t5552-http-fetch-branch.sh |  5 +++++
 transport-helper.c           | 12 +++++++++++-
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 91f668c..9a0291d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1010,7 +1010,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (transport->smart_options && !option_depth)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
-	refs = transport_get_remote_refs(transport, NULL, 0);
+	if (option_single_branch && option_branch) {
+		struct refspec branch_refspec = {0};
+
+		if (starts_with(option_branch, "refs/")) {
+			branch_refspec.src = xstrdup(option_branch);
+		} else {
+			struct strbuf buf = STRBUF_INIT;
+			strbuf_addf(&buf, "refs/heads/%s", option_branch);
+			branch_refspec.src = strbuf_detach(&buf, NULL);
+		}
+		refs = transport_get_remote_refs(transport, &branch_refspec, 1);
+		free(branch_refspec.src);
+	} else {
+		refs = transport_get_remote_refs(transport, NULL, 0);
+	}
 
 	if (refs) {
 		mapped_refs = wanted_peer_refs(refs, refspec);
diff --git a/t/t5552-http-fetch-branch.sh b/t/t5552-http-fetch-branch.sh
index 0e905d9..8a8e218 100755
--- a/t/t5552-http-fetch-branch.sh
+++ b/t/t5552-http-fetch-branch.sh
@@ -38,5 +38,10 @@ test_expect_success 'fetch with refspec only fetches requested branch' '
 	)
 '
 
+test_expect_success 'single-branch clone only fetches requested branch' '
+	GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git clone --single-branch -b master $HTTPD_URL/smart/repo.git sbc &&
+	! grep "refs/heads/another_branch" trace
+'
+
 stop_httpd
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 7d75d64..3775d63 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -28,6 +28,7 @@ struct helper_data {
 		signed_tags : 1,
 		check_connectivity : 1,
 		no_disconnect_req : 1,
+		refspec : 1,
 		no_private_update : 1;
 	char *export_marks;
 	char *import_marks;
@@ -114,8 +115,15 @@ static struct child_process *get_helper(struct transport *transport, const struc
 	int code;
 	int i;
 
-	if (data->helper)
+	if (data->helper) {
+		if (!data->refspec && req_refspec_nr) {
+			for (i = 0; i < req_refspec_nr; i++)
+				set_helper_option(transport, "refspec",
+						  req_refspecs[i].src);
+			data->refspec = 1;
+		}
 		return data->helper;
+	}
 
 	helper = xmalloc(sizeof(*helper));
 	child_process_init(helper);
@@ -220,6 +228,8 @@ static struct child_process *get_helper(struct transport *transport, const struc
 
 	for (i = 0; i < req_refspec_nr; i++)
 		set_helper_option(transport, "refspec", req_refspecs[i].src);
+	if (req_refspec_nr)
+		data->refspec = 1;
 	standard_options(transport);
 	return data->helper;
 }
-- 
2.4.2.767.g62658d5-twtrsrc

Re: [PATCH/RFC 0/6] fetch with refspec

[Stefan Beller]

On Fri, Apr 15, 2016 at 12:19 PM, David Turner <dturner@twopensource.com> wrote:
> We've got a lot of refs, but pretty frequently we only want to fetch
> one.  It's silly for the server to send a bunch of refs that the client
> is just going to ignore.  Here are some patches that fix that.
>
> Let me know if this seems reasonable.

Thanks for working on this!

I had a similar goal back then for non-http traffic and that series
exploded in size[1]
The issue at my attempt was non http traffic would require a protocol
update such that
the client speaks first to transport the refspec to the server. To
make "client speaks first"
happen, we'd need to have a protocol v2. So that attempt of mine
stalled as it seemed like
a huge thing.

[1] WIP at https://github.com/stefanbeller/git/tree/protocol2-10

This series looks small and reasonable from a cursory read.

Thanks,
Stefan

>
> (and I'll start in on another round of index-helper as soon as this is sent!)
>
> David Turner (6):
>   http-backend: use argv_array functions
>   remote-curl.c: fix variable shadowing
>   http-backend: handle refspec argument
>   transport: add refspec list parameters to functions
>   fetch: pass refspec to http server
>   clone: send refspec for single-branch clones
>
>  Documentation/technical/protocol-capabilities.txt | 23 +++++++
>  builtin/clone.c                                   | 16 ++++-
>  builtin/fetch.c                                   | 24 ++++++-
>  builtin/ls-remote.c                               |  2 +-
>  builtin/remote.c                                  |  2 +-
>  http-backend.c                                    | 23 +++++--
>  remote-curl.c                                     | 25 ++++---
>  t/t5552-http-fetch-branch.sh                      | 47 +++++++++++++
>  transport-helper.c                                | 44 ++++++++----
>  transport.c                                       | 14 ++--
>  transport.h                                       |  4 +-
>  upload-pack.c                                     | 81 ++++++++++++++++++++++-
>  12 files changed, 261 insertions(+), 44 deletions(-)
>  create mode 100755 t/t5552-http-fetch-branch.sh
>
> --
> 2.4.2.767.g62658d5-twtrsrc
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/6] http-backend: handle refspec argument

[Eric Sunshine]

On Fri, Apr 15, 2016 at 3:19 PM, David Turner <dturner@twopensource.com> wrote:
> Allow clients to pass a "refspec" parameter through to upload-pack;
> upload-pack will only advertise refs which match that refspec.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/http-backend.c b/http-backend.c
> @@ -465,6 +466,14 @@ static void get_info_refs(char *arg)
>                 argv_array_push(&argv, "--stateless-rpc");
>                 argv_array_push(&argv, "--advertise-refs");
>
> +               refspec = get_parameter("refspec");
> +               if (refspec) {
> +                       struct strbuf interesting_refs = STRBUF_INIT;
> +                       strbuf_addstr(&interesting_refs, "--interesting-refs=");
> +                       strbuf_addstr(&interesting_refs, refspec);
> +                       argv_array_push(&argv, interesting_refs.buf);
> +                       strbuf_release(&interesting_refs);
> +               }

    if (refspec)
        argv_array_pushf(&interesting_refs,
            "--interesting-refs=%s", refspec);

>                 argv_array_push(&argv, ".");
>                 run_service(argv.argv, 0);
>                 argv_array_clear(&argv);
> @@ -841,6 +905,19 @@ int main(int argc, char **argv)
> +               if (starts_with(arg, "--interesting-refs=")) {
> +                       struct string_list_item *item;
> +
> +                       string_list_split(&interesting_refspecs, arg + 19,
> +                                         ' ', -1);
> +                       for_each_string_list_item(item, &interesting_refspecs) {
> +                               if (check_refname_format(item->string,
> +                                                        REFNAME_REFSPEC_PATTERN))
> +                                       die("invalid refspec %s", item->string);
> +                               item->util = make_refspec_data(item->string);
> +                       }
> +                       continue;
> +               }

Is this leaking the string list?

>                 if (!strcmp(arg, "--stateless-rpc")) {
>                         stateless_rpc = 1;
>                         continue;
> --
> 2.4.2.767.g62658d5-twtrsrc

Re: [PATCH/RFC 5/6] fetch: pass refspec to http server

[Eric Sunshine]

On Fri, Apr 15, 2016 at 3:19 PM, David Turner <dturner@twopensource.com> wrote:
> When fetching over http, send the requested refspec to the server.
> The server will then only send refs matching that refspec.  It is
> permitted for old servers to ignore that parameter, and the client
> will automatically handle this.
>
> When the server has many refs, and the client only wants a few, this
> can save bandwidth and reduce fetch latency.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> @@ -302,9 +302,27 @@ static struct ref *get_ref_map(struct transport *transport,
> -       remote_refs = transport_get_remote_refs(transport, NULL, 0);
> +       qualified_refspecs = xcalloc(refspec_count, sizeof(*qualified_refspecs));
> +       for (i = 0; i < refspec_count; i++) {
> +               if (starts_with(refspecs[i].src, "refs/")) {
> +                       qualified_refspecs[i].src = xstrdup(refspecs[i].src);
> +               } else {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       strbuf_addf(&buf, "refs/heads/%s", refspecs[i].src);
> +                       qualified_refspecs[i].src = strbuf_detach(&buf, NULL);

Alternately, replace these three lines with:

    qualified_refspecs[i].src = xstrfmt("refs/heads/%s", refspecs[i].src);

and drop the braces.

> +               }
> +       }
> +
> +       remote_refs = transport_get_remote_refs(transport, qualified_refspecs,
> +                                               refspec_count);
> +
> +       for (i = 0; i < refspec_count; i++) {
> +               free(qualified_refspecs[i].src);
> +       }
> +       free(qualified_refspecs);
> diff --git a/t/t5552-http-fetch-branch.sh b/t/t5552-http-fetch-branch.sh
> @@ -0,0 +1,42 @@
> +test_expect_success 'make some more commits' '
> +       (
> +               cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +               test_commit 2 &&
> +               git checkout -b another_branch &&
> +               test_commit 3

Broken &&-chain.

> +               git checkout -b a_third_branch &&
> +               test_commit 4
> +       )
> +'
> +
> +test_expect_success 'fetch with refspec only fetches requested branch' '
> +       test_when_finished "rm trace" &&
> +       (
> +               cd clone &&
> +               GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch origin another_branch &&
> +               ! grep "refs/heads/master" ../trace
> +       )
> +'

This could be done without the subshell, perhaps?

    GIT_TRACE_PACKET=blah git -C clone fetch ...

Re: [PATCH/RFC 6/6] clone: send refspec for single-branch clones

[Eric Sunshine]

On Fri, Apr 15, 2016 at 3:19 PM, David Turner <dturner@twopensource.com> wrote:
> For single-branch clones (when we know in advance what the remote
> branch name will be), send a refspec so that the server doesn't
> tell us about any other refs.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/builtin/clone.c b/builtin/clone.c
> @@ -1010,7 +1010,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> +       if (option_single_branch && option_branch) {
> +               struct refspec branch_refspec = {0};
> +
> +               if (starts_with(option_branch, "refs/")) {
> +                       branch_refspec.src = xstrdup(option_branch);
> +               } else {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       strbuf_addf(&buf, "refs/heads/%s", option_branch);
> +                       branch_refspec.src = strbuf_detach(&buf, NULL);

branch_refspec.src = xstrfmt("refs/heads/%s", option_branch);

> +               }
> +               refs = transport_get_remote_refs(transport, &branch_refspec, 1);
> +               free(branch_refspec.src);
> +       } else {
> +               refs = transport_get_remote_refs(transport, NULL, 0);
> +       }

Re: [PATCH/RFC 1/6] http-backend: use argv_array functions

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

OK (it might be easier to read if you used the pushl form for the
"fixed initial segment" like these calls, though).

>  http-backend.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/http-backend.c b/http-backend.c
> index 8870a26..a4f0066 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -450,9 +450,7 @@ static void get_info_refs(char *arg)
>  	hdr_nocache();
>  
>  	if (service_name) {
> -		const char *argv[] = {NULL /* service name */,
> -			"--stateless-rpc", "--advertise-refs",
> -			".", NULL};
> +		struct argv_array argv = ARGV_ARRAY_INIT;
>  		struct rpc_service *svc = select_service(service_name);
>  
>  		strbuf_addf(&buf, "application/x-git-%s-advertisement",
> @@ -463,9 +461,13 @@ static void get_info_refs(char *arg)
>  		packet_write(1, "# service=git-%s\n", svc->name);
>  		packet_flush(1);
>  
> -		argv[0] = svc->name;
> -		run_service(argv, 0);
> +		argv_array_push(&argv, svc->name);
> +		argv_array_push(&argv, "--stateless-rpc");
> +		argv_array_push(&argv, "--advertise-refs");
>  
> +		argv_array_push(&argv, ".");
> +		run_service(argv.argv, 0);
> +		argv_array_clear(&argv);
>  	} else {
>  		select_getanyfile();
>  		for_each_namespaced_ref(show_text_ref, &buf);

Re: [PATCH/RFC 2/6] remote-curl.c: fix variable shadowing

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> The local variable 'options' was shadowing a global of the same name.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

OK.  In general, giving a longer and more descriptive name to the
global would be a direction to lead to more readable code, though.

>  remote-curl.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 15e48e2..b9b6a90 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -254,7 +254,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
>  	struct strbuf effective_url = STRBUF_INIT;
>  	struct discovery *last = last_discovery;
>  	int http_ret, maybe_smart = 0;
> -	struct http_get_options options;
> +	struct http_get_options get_options;
>  
>  	if (last && !strcmp(service, last->service))
>  		return last;
> @@ -271,15 +271,15 @@ static struct discovery *discover_refs(const char *service, int for_push)
>  		strbuf_addf(&refs_url, "service=%s", service);
>  	}
>  
> -	memset(&options, 0, sizeof(options));
> -	options.content_type = &type;
> -	options.charset = &charset;
> -	options.effective_url = &effective_url;
> -	options.base_url = &url;
> -	options.no_cache = 1;
> -	options.keep_error = 1;
> +	memset(&get_options, 0, sizeof(get_options));
> +	get_options.content_type = &type;
> +	get_options.charset = &charset;
> +	get_options.effective_url = &effective_url;
> +	get_options.base_url = &url;
> +	get_options.no_cache = 1;
> +	get_options.keep_error = 1;
>  
> -	http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
> +	http_ret = http_get_strbuf(refs_url.buf, &buffer, &get_options);
>  	switch (http_ret) {
>  	case HTTP_OK:
>  		break;

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Add parameters for a list of refspecs to transport_get_remote_refs and
> get_refs_list.  These parameters are presently unused -- soon, we will
> use them to implement fetches which only learn about a subset of refs.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

What the code tries to do I am more than halfway happy.  It is
unfortunate that we cannot do this natively without upgrading the
protocol in a fundamental way, but this is a nice way to work it
around only for Git-over-HTTP transport without having to break the
protocol.
 
As a POC it is OK, but I am moderately unhappy with the use of
"refspec" here.

At the transport layer, we shouldn't care what the receiving end
intends to do with the objects that sits at the tip of the refs at
the other end, so sending "refspecs" down feels somewhat wrong for
this feature.  At one layer up in the next patch, you do use
"interesting refs" which makes it clear that only the left-hand-side
of a refspec, i.e. what they call it, matters, and I think that is a
much better phrasing of the concept (and the passed data should only
be the left-hand-side of refspecs).

Thanks.

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Mon, Apr 18, 2016 at 11:45:54AM -0700, Junio C Hamano wrote:

> David Turner <dturner@twopensource.com> writes:
> 
> > Add parameters for a list of refspecs to transport_get_remote_refs and
> > get_refs_list.  These parameters are presently unused -- soon, we will
> > use them to implement fetches which only learn about a subset of refs.
> >
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> 
> What the code tries to do I am more than halfway happy.  It is
> unfortunate that we cannot do this natively without upgrading the
> protocol in a fundamental way, but this is a nice way to work it
> around only for Git-over-HTTP transport without having to break the
> protocol.

I dunno, I am a bit negative on bringing new features to Git-over-HTTP
(which is already less efficient than the other protocols!) without any
plan for supporting them in the other protocols.

I thought Stefan's v2 protocol work looked quite good, but it seems to
have stalled. The hardest part of that topic is figuring out the upgrade
path. But for git-over-http, we can solve that in the same way that
David is passing in the extra refspecs.

So I'd rather see something like:

  1. Support for v2 "capabilities only" initial negotiation, followed
     by ref advertisement.

  2. Support for refspec-limiting capability.

  3. HTTP-only option from client to trigger v2 on the server.

That's still HTTP-specific, but it has a clear path for converging with
the ssh and git protocols eventually, rather than having to support
magic out-of-band capabilities forever.

It does require an extra round of HTTP request/response, though.

-Peff

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Stefan Beller]

On Tue, Apr 19, 2016 at 12:14 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Apr 18, 2016 at 11:45:54AM -0700, Junio C Hamano wrote:
>
>> David Turner <dturner@twopensource.com> writes:
>>
>> > Add parameters for a list of refspecs to transport_get_remote_refs and
>> > get_refs_list.  These parameters are presently unused -- soon, we will
>> > use them to implement fetches which only learn about a subset of refs.
>> >
>> > Signed-off-by: David Turner <dturner@twopensource.com>
>> > ---
>>
>> What the code tries to do I am more than halfway happy.  It is
>> unfortunate that we cannot do this natively without upgrading the
>> protocol in a fundamental way, but this is a nice way to work it
>> around only for Git-over-HTTP transport without having to break the
>> protocol.
>
> I dunno, I am a bit negative on bringing new features to Git-over-HTTP
> (which is already less efficient than the other protocols!) without any
> plan for supporting them in the other protocols.
>
> I thought Stefan's v2 protocol work looked quite good, but it seems to
> have stalled. The hardest part of that topic is figuring out the upgrade
> path. But for git-over-http, we can solve that in the same way that
> David is passing in the extra refspecs.

Yeah it stalled, though I hope to revive it eventually.

I was positive about these changes for that same reason: If http and native
protocol move apart even more, it will be easier to make the native only
v2 protocol without needing to fiddle with http, i.e. this series would reduce
scope of the v2 series drastically?

>
> So I'd rather see something like:
>
>   1. Support for v2 "capabilities only" initial negotiation, followed
>      by ref advertisement.

And the client needs to talk in between capabilities and ref advertisement.
Even if it is just a flush for now. That can be extended later to the actual
desired capabilities, but the server needs to at least wait for a client packet
in here.

Note that the server side for v2 capabilites only first is done, the client side
is missing as I found that to be the hard part.


>
>   2. Support for refspec-limiting capability.
>
>   3. HTTP-only option from client to trigger v2 on the server.
>
> That's still HTTP-specific, but it has a clear path for converging with
> the ssh and git protocols eventually, rather than having to support
> magic out-of-band capabilities forever.
>
> It does require an extra round of HTTP request/response, though.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/6] http-backend: handle refspec argument

[David Turner]

On Sat, 2016-04-16 at 21:51 -0400, Eric Sunshine wrote:
> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> dturner@twopensource.com> wrote:
> > +               if (refspec) {
> > +                       struct strbuf interesting_refs =
> > STRBUF_INIT;
> > +                       strbuf_addstr(&interesting_refs, "-
> > -interesting-refs=");
> > +                       strbuf_addstr(&interesting_refs, refspec);
> > +                       argv_array_push(&argv,
> > interesting_refs.buf);
> > +                       strbuf_release(&interesting_refs);
> > +               }
> 
>     if (refspec)
>         argv_array_pushf(&interesting_refs,
>             "--interesting-refs=%s", refspec);


Will fix, thanks.

> >                 argv_array_push(&argv, ".");
> >                 run_service(argv.argv, 0);
> >                 argv_array_clear(&argv);
> > @@ -841,6 +905,19 @@ int main(int argc, char **argv)
> > +               if (starts_with(arg, "--interesting-refs=")) {
> > ...
> > +                       continue;
> > +               }
> 
> Is this leaking the string list?

Yes, intentionally.  interesting_refspec is a global that we look at
later.  

Re: [PATCH/RFC 1/6] http-backend: use argv_array functions

[David Turner]

On Mon, 2016-04-18 at 11:34 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> 
> OK (it might be easier to read if you used the pushl form for the
> "fixed initial segment" like these calls, though).

Good idea.

Re: [PATCH/RFC 2/6] remote-curl.c: fix variable shadowing

[David Turner]

On Mon, 2016-04-18 at 11:35 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > The local variable 'options' was shadowing a global of the same
> > name.
> > 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> 
> OK.  In general, giving a longer and more descriptive name to the
> global would be a direction to lead to more readable code, though.

OK, will do that instead.

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Mon, 2016-04-18 at 11:45 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > Add parameters for a list of refspecs to transport_get_remote_refs
> > and
> > get_refs_list.  These parameters are presently unused -- soon, we
> > will
> > use them to implement fetches which only learn about a subset of
> > refs.
> > 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> 
> What the code tries to do I am more than halfway happy.  It is
> unfortunate that we cannot do this natively without upgrading the
> protocol in a fundamental way, but this is a nice way to work it
> around only for Git-over-HTTP transport without having to break the
> protocol.
>  
> As a POC it is OK, but I am moderately unhappy with the use of
> "refspec" here.
> 
> At the transport layer, we shouldn't care what the receiving end
> intends to do with the objects that sits at the tip of the refs at
> the other end, so sending "refspecs" down feels somewhat wrong for
> this feature.  At one layer up in the next patch, you do use
> "interesting refs" which makes it clear that only the left-hand-side
> of a refspec, i.e. what they call it, matters, and I think that is a
> much better phrasing of the concept (and the passed data should only
> be the left-hand-side of refspecs).

I will rename the parameter to "interesting_refs".

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> What the code tries to do I am more than halfway happy.  It is
>> unfortunate that we cannot do this natively without upgrading the
>> protocol in a fundamental way, but this is a nice way to work it
>> around only for Git-over-HTTP transport without having to break the
>> protocol.
>
> I dunno, I am a bit negative on bringing new features to Git-over-HTTP
> (which is already less efficient than the other protocols!) without any
> plan for supporting them in the other protocols.
>
> I thought Stefan's v2 protocol work looked quite good, but it seems to
> have stalled. The hardest part of that topic is figuring out the upgrade
> path. But for git-over-http, we can solve that in the same way that
> David is passing in the extra refspecs.

Yeah, I had the same feeling.

> So I'd rather see something like:
>
>   1. Support for v2 "capabilities only" initial negotiation, followed
>      by ref advertisement.
>
>   2. Support for refspec-limiting capability.
>
>   3. HTTP-only option from client to trigger v2 on the server.

I like that; reducing the initial scope of v2 so that we can do this
new feature as its first use case would be a good way to move things
forward.

> That's still HTTP-specific, but it has a clear path for converging with
> the ssh and git protocols eventually, rather than having to support
> magic out-of-band capabilities forever.
>
> It does require an extra round of HTTP request/response, though.

Yes.  And as we discussed before, we can do "upload-pack" that
advertises "by the way, upload-pack-v2 is available next to me" and
a separate "upload-pack-v2" that talks v2 (i.e. its initial message
is limited to capabilities and nothing else) would probably be a
sufficient migration path for native protocol.

        

Re: [PATCH/RFC 6/6] clone: send refspec for single-branch clones

[David Turner]

On Sat, 2016-04-16 at 22:36 -0400, Eric Sunshine wrote:
> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> dturner@twopensource.com> wrote:
> > For single-branch clones (when we know in advance what the remote
> > branch name will be), send a refspec so that the server doesn't
> > tell us about any other refs.
> > 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > @@ -1010,7 +1010,21 @@ int cmd_clone(int argc, const char **argv,
> > const char *prefix)
> > +       if (option_single_branch && option_branch) {
> > +               struct refspec branch_refspec = {0};
> > +
> > +               if (starts_with(option_branch, "refs/")) {
> > +                       branch_refspec.src =
> > xstrdup(option_branch);
> > +               } else {
> > +                       struct strbuf buf = STRBUF_INIT;
> > +                       strbuf_addf(&buf, "refs/heads/%s",
> > option_branch);
> > +                       branch_refspec.src = strbuf_detach(&buf,
> > NULL);
> 
> branch_refspec.src = xstrfmt("refs/heads/%s", option_branch);


Will fix, thanks.

Re: [PATCH/RFC 5/6] fetch: pass refspec to http server

[David Turner]

On Sat, 2016-04-16 at 22:33 -0400, Eric Sunshine wrote:
> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> dturner@twopensource.com> wrote:
> > When fetching over http, send the requested refspec to the server.
> > The server will then only send refs matching that refspec.  It is
> > permitted for old servers to ignore that parameter, and the client
> > will automatically handle this.
> > 
> > When the server has many refs, and the client only wants a few,
> > this
> > can save bandwidth and reduce fetch latency.
> > 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > @@ -302,9 +302,27 @@ static struct ref *get_ref_map(struct
> > transport *transport,
> > -       remote_refs = transport_get_remote_refs(transport, NULL,
> > 0);
> > +       qualified_refspecs = xcalloc(refspec_count,
> > sizeof(*qualified_refspecs));
> > +       for (i = 0; i < refspec_count; i++) {
> > +               if (starts_with(refspecs[i].src, "refs/")) {
> > +                       qualified_refspecs[i].src =
> > xstrdup(refspecs[i].src);
> > +               } else {
> > +                       struct strbuf buf = STRBUF_INIT;
> > +                       strbuf_addf(&buf, "refs/heads/%s",
> > refspecs[i].src);
> > +                       qualified_refspecs[i].src =
> > strbuf_detach(&buf, NULL);
> 
> Alternately, replace these three lines with:
> 
>     qualified_refspecs[i].src = xstrfmt("refs/heads/%s",
> refspecs[i].src);
> 
> and drop the braces.

Yep, good point.

> > +               }
> > +       }
> > +
> > +       remote_refs = transport_get_remote_refs(transport,
> > qualified_refspecs,
> > +                                               refspec_count);
> > +
> > +       for (i = 0; i < refspec_count; i++) {
> > +               free(qualified_refspecs[i].src);
> > +       }
> > +       free(qualified_refspecs);
> > diff --git a/t/t5552-http-fetch-branch.sh b/t/t5552-http-fetch
> > -branch.sh
> > @@ -0,0 +1,42 @@
> > +test_expect_success 'make some more commits' '
> > +       (
> > +               cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +               test_commit 2 &&
> > +               git checkout -b another_branch &&
> > +               test_commit 3
> 
> Broken &&-chain.

Thanks.

> > +               git checkout -b a_third_branch &&
> > +               test_commit 4
> > +       )
> > +'
> > +
> > +test_expect_success 'fetch with refspec only fetches requested
> > branch' '
> > +       test_when_finished "rm trace" &&
> > +       (
> > +               cd clone &&
> > +               GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch
> > origin another_branch &&
> > +               ! grep "refs/heads/master" ../trace
> > +       )
> > +'
> 
> This could be done without the subshell, perhaps?
> 
>     GIT_TRACE_PACKET=blah git -C clone fetch ...


Will fix these, thanks.

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Tue, 2016-04-19 at 03:14 -0400, Jeff King wrote:
> On Mon, Apr 18, 2016 at 11:45:54AM -0700, Junio C Hamano wrote:
> 
> > David Turner <dturner@twopensource.com> writes:
> > 
> > > Add parameters for a list of refspecs to
> > > transport_get_remote_refs and
> > > get_refs_list.  These parameters are presently unused -- soon, we
> > > will
> > > use them to implement fetches which only learn about a subset of
> > > refs.
> > > 
> > > Signed-off-by: David Turner <dturner@twopensource.com>
> > > ---
> > 
> > What the code tries to do I am more than halfway happy.  It is
> > unfortunate that we cannot do this natively without upgrading the
> > protocol in a fundamental way, but this is a nice way to work it
> > around only for Git-over-HTTP transport without having to break the
> > protocol.
> 
> I dunno, I am a bit negative on bringing new features to Git-over
> -HTTP
> (which is already less efficient than the other protocols!) without
> any
> plan for supporting them in the other protocols.

Interesting -- can you expand on git-over-http being less efficient?
This is the first I'd heard of it.  Is it documented somewhere?

> I thought Stefan's v2 protocol work looked quite good, but it seems
> to
> have stalled. The hardest part of that topic is figuring out the
> upgrade
> path. But for git-over-http, we can solve that in the same way that
> David is passing in the extra refspecs.
> 
> So I'd rather see something like:
> 
>   1. Support for v2 "capabilities only" initial negotiation, followed
>      by ref advertisement.
> 
>   2. Support for refspec-limiting capability.
> 
>   3. HTTP-only option from client to trigger v2 on the server.
> 
> That's still HTTP-specific, but it has a clear path for converging
> with
> the ssh and git protocols eventually, rather than having to support
> magic out-of-band capabilities forever.
> 
> It does require an extra round of HTTP request/response, though.

This seems way more complicated to me, and not necessarily super
-efficient.  That is, it seems like rather a lot of work to add a whole
round of negotiation and a new protocol, when all we really need is one
little tweak.

I do think a fetch v2 protocol is potentially interesting for other
reasons, but I don't know that I have the time to fully implement it.  

I wonder if it would be possible to just add these tweaks to v1, and
save the v2 work for when someone has the time to implement it?

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Tue, Apr 19, 2016 at 05:40:01PM -0400, David Turner wrote:

> > I dunno, I am a bit negative on bringing new features to Git-over
> > -HTTP
> > (which is already less efficient than the other protocols!) without
> > any
> > plan for supporting them in the other protocols.
> 
> Interesting -- can you expand on git-over-http being less efficient?
> This is the first I'd heard of it.  Is it documented somewhere?

I don't know offhand of thorough discussion I can link to. But
basically, the issue is the tip negotiation that happens during a fetch.
In the normal git-over-ssh and git-over-tcp protocols, we're
full-duplex, and both sides remember all of the state. So the client is
spewing "want" and "have" lines at the server, which is responding
asynchronously with acks or naks until they reach a shared point to
generate the pack from.

In the HTTP protocol, this negotiation has to happen via synchronous
request/response pairs. So the client says "here are some haves; what do
you think?" and gets back the response. Then it prepares another of
haves, and so on, until the server says "OK, I've seen enough; here's
the pack". But because the server is stateless, each request has to
summarize the findings of the prior request. And so each request gets
slightly bigger as we iterate.

There are some tunable parameters there (e.g., how many haves to send in
the first batch?), and the current settings are meant to be a mix of not
wasting too much time preparing a request, but also putting enough into
it that common requests can complete with only a single round trip.

I don't have numbers on how often we have to fall back multiple
requests, or how big they can grow. I know I have very occasionally seen
pathological cases where we outgrew the HTTP buffer sizes, and re-trying
the fetch via ssh just worked.

I'm cc-ing Shawn, who designed all of this, and can probably give more
details (and may also have opinions on new http-only protocol features,
as he'd probably end up implementing them in JGit, too).

It would be nice if we could do a true full-duplex conversation over
HTTP. I looked into Websockets at one point, but IIRC there wasn't
libcurl support for them.

> > So I'd rather see something like:
> > 
> >   1. Support for v2 "capabilities only" initial negotiation, followed
> >      by ref advertisement.
> > 
> >   2. Support for refspec-limiting capability.
> > 
> >   3. HTTP-only option from client to trigger v2 on the server.
> > 
> > That's still HTTP-specific, but it has a clear path for converging
> > with
> > the ssh and git protocols eventually, rather than having to support
> > magic out-of-band capabilities forever.
> > 
> > It does require an extra round of HTTP request/response, though.
> 
> This seems way more complicated to me, and not necessarily super
> -efficient.  That is, it seems like rather a lot of work to add a whole
> round of negotiation and a new protocol, when all we really need is one
> little tweak.

It is less efficient because of the extra round. If the new protocol
were truly client-speaks-first, we could drop that round (which is
essentially what your proposal is doing; you're just sticking the
first-speak part into HTTP parameters).

I don't know how much that round costs if it's part of the same TCP
session, or part of the same pipelined HTTP connection.

> I wonder if it would be possible to just add these tweaks to v1, and
> save the v2 work for when someone has the time to implement it?

I don't think it's possible for the non-HTTP protocols. The single
change in v2 is to add a phase before the ref advertisement starts.
Without that, the server is going to start spewing advertisements.

You can find previous discussion on the list, but I think the options
basically are:

  1. Something like v2, where the client gets a chance to speak before
     the advertisement.

  2. Some out-of-band way of getting values from the client to the
     server (so maybe extra command-line arguments for git-over-ssh, and
     maybe shoving something after the "\0" for git-daemon, and of
     course extra parameters for HTTP).

  3. The client saying "stop spewing refs at me, I want to give you a
     ref filter" asynchronously, and accepting a little spew at the
     beginning of each conversation. That obviously only works for the
     full-duplex transports, so you'd probably fall back to (2) for
     http.

-Peff

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Tue, 2016-04-19 at 19:22 -0400, Jeff King wrote:
> You can find previous discussion on the list, but I think the options
> basically are:
> 
>   1. Something like v2, where the client gets a chance to speak
> before
>      the advertisement.
> 
>   2. Some out-of-band way of getting values from the client to the
>      server (so maybe extra command-line arguments for git-over-ssh,
> and
>      maybe shoving something after the "\0" for git-daemon, and of
>      course extra parameters for HTTP).
> 
>   3. The client saying "stop spewing refs at me, I want to give you a
>      ref filter" asynchronously, and accepting a little spew at the
>      beginning of each conversation. That obviously only works for
> the
>      full-duplex transports, so you'd probably fall back to (2) for
>      http.

OK, so (2) seems like what I'm doing -- it just happens that I only
implemented it for one protocol. 

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Tue, Apr 19, 2016 at 07:43:11PM -0400, David Turner wrote:

> On Tue, 2016-04-19 at 19:22 -0400, Jeff King wrote:
> > You can find previous discussion on the list, but I think the options
> > basically are:
> > 
> >   1. Something like v2, where the client gets a chance to speak
> > before
> >      the advertisement.
> > 
> >   2. Some out-of-band way of getting values from the client to the
> >      server (so maybe extra command-line arguments for git-over-ssh,
> > and
> >      maybe shoving something after the "\0" for git-daemon, and of
> >      course extra parameters for HTTP).
> > 
> >   3. The client saying "stop spewing refs at me, I want to give you a
> >      ref filter" asynchronously, and accepting a little spew at the
> >      beginning of each conversation. That obviously only works for
> > the
> >      full-duplex transports, so you'd probably fall back to (2) for
> >      http.
> 
> OK, so (2) seems like what I'm doing -- it just happens that I only
> implemented it for one protocol.

Right. And I don't mind that approach _if_ we can figure out a way to do
it for all protocols. But I think there are some complications with the
other ones, which means that HTTP will have the ability to grow features
the other protocols do not.

-Peff

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Tue, 2016-04-19 at 21:17 -0400, Jeff King wrote:
> On Tue, Apr 19, 2016 at 07:43:11PM -0400, David Turner wrote:
> 
> > On Tue, 2016-04-19 at 19:22 -0400, Jeff King wrote:
> > > You can find previous discussion on the list, but I think the
> > > options
> > > basically are:
> > > 
> > >   1. Something like v2, where the client gets a chance to speak
> > > before
> > >      the advertisement.
> > > 
> > >   2. Some out-of-band way of getting values from the client to
> > > the
> > >      server (so maybe extra command-line arguments for git-over
> > > -ssh,
> > > and
> > >      maybe shoving something after the "\0" for git-daemon, and
> > > of
> > >      course extra parameters for HTTP).
> > > 
> > >   3. The client saying "stop spewing refs at me, I want to give
> > > you a
> > >      ref filter" asynchronously, and accepting a little spew at
> > > the
> > >      beginning of each conversation. That obviously only works
> > > for
> > > the
> > >      full-duplex transports, so you'd probably fall back to (2)
> > > for
> > >      http.
> > 
> > OK, so (2) seems like what I'm doing -- it just happens that I only
> > implemented it for one protocol.
> 
> Right. And I don't mind that approach _if_ we can figure out a way to
> do
> it for all protocols. But I think there are some complications with
> the
> other ones, which means that HTTP will have the ability to grow
> features
> the other protocols do not.

As you note, it appears that git-daemon does sort-of have support for
extra args -- see parse_host_arg.  So it wouldn't be hard to add
something here. Unfortunately, current versions of git die on unknown
args.  So this change would not be backwards-compatible.  We could put
a decider on it so that clients would only try it when explicitly
enabled.  Or we could have clients try it with, and in the event of an
error, retry without.  Neither is ideal, but both are possible.

If I read this code correctly, git-over-ssh will pass through arbitrary
arguments.  So this should be trivial.

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:

> As you note, it appears that git-daemon does sort-of have support for
> extra args -- see parse_host_arg.  So it wouldn't be hard to add
> something here. Unfortunately, current versions of git die on unknown
> args.  So this change would not be backwards-compatible.  We could put
> a decider on it so that clients would only try it when explicitly
> enabled.  Or we could have clients try it with, and in the event of an
> error, retry without.  Neither is ideal, but both are possible.

Right. This ends up being the same difficulty that the v2 protocol
encountered; how do you figure out what you can speak without resorting
to expensive fallbacks, when do you flip the switch, do you remember the
protocol you used last time with this server, etc.

Which isn't to say it's necessarily a bad thing. Maybe the path forward
instead of v2 is to shoe-horn this data into the pre-protocol
conversation, and go from there. The protocol accepts that "somehow" it
got some extra data from the transport layer, and acts on its uniformly.

> If I read this code correctly, git-over-ssh will pass through arbitrary
> arguments.  So this should be trivial.

It does if you are ssh-ing to a real shell-level account on the server,
but if you are using git-shell or some other wrapper to restrict clients
from running arbitrary commands, it will likely reject it.

-Peff

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Wed, 2016-04-20 at 16:57 -0400, Jeff King wrote:
> On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:
> 
> > As you note, it appears that git-daemon does sort-of have support
> > for
> > extra args -- see parse_host_arg.  So it wouldn't be hard to add
> > something here. Unfortunately, current versions of git die on
> > unknown
> > args.  So this change would not be backwards-compatible.  We could
> > put
> > a decider on it so that clients would only try it when explicitly
> > enabled.  Or we could have clients try it with, and in the event of
> > an
> > error, retry without.  Neither is ideal, but both are possible.
> 
> Right. This ends up being the same difficulty that the v2 protocol
> encountered; how do you figure out what you can speak without
> resorting
> to expensive fallbacks, when do you flip the switch, do you remember
> the
> protocol you used last time with this server, etc.

Right.

[moved]
> > If I read this code correctly, git-over-ssh will pass through
> > arbitrary
> > arguments.  So this should be trivial.
> 
> It does if you are ssh-ing to a real shell-level account on the
> server,
> but if you are using git-shell or some other wrapper to restrict
> clients
> from running arbitrary commands, it will likely reject it.

Oh, I see how I was mis-reading shell.c.  Oops.
[/moved]


> Which isn't to say it's necessarily a bad thing. Maybe the path
> forward
> instead of v2 is to shoe-horn this data into the pre-protocol
> conversation, and go from there. The protocol accepts that "somehow"
> it
> got some extra data from the transport layer, and acts on its
> uniformly.

OK, so it seems like only HTTP (and non-git-shell-git://) allow backwar
ds-compatible optional pre-protocol messages.  So we don't have good
options; we only have bad ones.  We have to decide which particular
kind of badness we're willing to accept, and to what degree we care
about extensibility.  As I see it, the badness options are (in no
particular order):

1. Nothing changes.
2. HTTP grows more extensions; other protocols stagnate.
3. HTTP grows extensions; other protocols get extensions but:
   a. only use them on explicit client configuration or
   b. try/fail/remember per-remote
4. A backwards-incompatible protocol v2 is introduced, which
   hits alternate endpoints (with the same a/b as above).  This is
   different from 3. in that protocol v2 is explicitly designed around
   a capabilities negotiation phase rather than unilateral client-side
   decisions.
5. Think of another way to make fetch performant with many refs, and 	
    defer the extension decision.

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Stefan Beller]

On Mon, Apr 25, 2016 at 9:44 AM, David Turner <dturner@twopensource.com> wrote:
> On Wed, 2016-04-20 at 16:57 -0400, Jeff King wrote:
>> On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:
>>
>> > As you note, it appears that git-daemon does sort-of have support
>> > for
>> > extra args -- see parse_host_arg.  So it wouldn't be hard to add
>> > something here. Unfortunately, current versions of git die on
>> > unknown
>> > args.  So this change would not be backwards-compatible.  We could
>> > put
>> > a decider on it so that clients would only try it when explicitly
>> > enabled.  Or we could have clients try it with, and in the event of
>> > an
>> > error, retry without.  Neither is ideal, but both are possible.
>>
>> Right. This ends up being the same difficulty that the v2 protocol
>> encountered; how do you figure out what you can speak without
>> resorting
>> to expensive fallbacks, when do you flip the switch, do you remember
>> the
>> protocol you used last time with this server, etc.
>
> Right.
>
> [moved]
>> > If I read this code correctly, git-over-ssh will pass through
>> > arbitrary
>> > arguments.  So this should be trivial.
>>
>> It does if you are ssh-ing to a real shell-level account on the
>> server,
>> but if you are using git-shell or some other wrapper to restrict
>> clients
>> from running arbitrary commands, it will likely reject it.
>
> Oh, I see how I was mis-reading shell.c.  Oops.
> [/moved]
>
>
>> Which isn't to say it's necessarily a bad thing. Maybe the path
>> forward
>> instead of v2 is to shoe-horn this data into the pre-protocol
>> conversation, and go from there. The protocol accepts that "somehow"
>> it
>> got some extra data from the transport layer, and acts on its
>> uniformly.
>
> OK, so it seems like only HTTP (and non-git-shell-git://) allow backwar
> ds-compatible optional pre-protocol messages.  So we don't have good
> options; we only have bad ones.  We have to decide which particular
> kind of badness we're willing to accept, and to what degree we care
> about extensibility.  As I see it, the badness options are (in no
> particular order):
>
> 1. Nothing changes.
> 2. HTTP grows more extensions; other protocols stagnate.
> 3. HTTP grows extensions; other protocols get extensions but:
>    a. only use them on explicit client configuration or
>    b. try/fail/remember per-remote
> 4. A backwards-incompatible protocol v2 is introduced, which
>    hits alternate endpoints (with the same a/b as above).  This is
>    different from 3. in that protocol v2 is explicitly designed around
>    a capabilities negotiation phase rather than unilateral client-side
>    decisions.
> 5. Think of another way to make fetch performant with many refs, and
>     defer the extension decision.

I'd prefer 2,3,4 over 1,5.

Speaking about 2,3,4:

Maybe we can do a mix of 2 and 4:

   1) HTTP grows more extensions; other protocols stagnate for now.
   2) Come up with a backwards-incompatible protocol v2, foccussed on
       capabilities negotiation phase, hitting alternative end points
       (non http only, or rather a subset of protocols only)
    3) if HTTP sees the benefits of the native protocol v2, we may switch
        HTTP, too

(in time order of execution. Each point is decoupled from the others and may
be done by different people at different times.)


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

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Stefan Beller]

On Mon, Apr 25, 2016 at 3:10 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Apr 25, 2016 at 9:44 AM, David Turner <dturner@twopensource.com> wrote:
>> On Wed, 2016-04-20 at 16:57 -0400, Jeff King wrote:
>>> On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:
>>>
>>> > As you note, it appears that git-daemon does sort-of have support
>>> > for
>>> > extra args -- see parse_host_arg.  So it wouldn't be hard to add
>>> > something here. Unfortunately, current versions of git die on
>>> > unknown
>>> > args.  So this change would not be backwards-compatible.  We could
>>> > put
>>> > a decider on it so that clients would only try it when explicitly
>>> > enabled.  Or we could have clients try it with, and in the event of
>>> > an
>>> > error, retry without.  Neither is ideal, but both are possible.
>>>
>>> Right. This ends up being the same difficulty that the v2 protocol
>>> encountered; how do you figure out what you can speak without
>>> resorting
>>> to expensive fallbacks, when do you flip the switch, do you remember
>>> the
>>> protocol you used last time with this server, etc.
>>
>> Right.
>>
>> [moved]
>>> > If I read this code correctly, git-over-ssh will pass through
>>> > arbitrary
>>> > arguments.  So this should be trivial.
>>>
>>> It does if you are ssh-ing to a real shell-level account on the
>>> server,
>>> but if you are using git-shell or some other wrapper to restrict
>>> clients
>>> from running arbitrary commands, it will likely reject it.
>>
>> Oh, I see how I was mis-reading shell.c.  Oops.
>> [/moved]
>>
>>
>>> Which isn't to say it's necessarily a bad thing. Maybe the path
>>> forward
>>> instead of v2 is to shoe-horn this data into the pre-protocol
>>> conversation, and go from there. The protocol accepts that "somehow"
>>> it
>>> got some extra data from the transport layer, and acts on its
>>> uniformly.
>>
>> OK, so it seems like only HTTP (and non-git-shell-git://) allow backwar
>> ds-compatible optional pre-protocol messages.  So we don't have good
>> options; we only have bad ones.  We have to decide which particular
>> kind of badness we're willing to accept, and to what degree we care
>> about extensibility.  As I see it, the badness options are (in no
>> particular order):
>>
>> 1. Nothing changes.
>> 2. HTTP grows more extensions; other protocols stagnate.
>> 3. HTTP grows extensions; other protocols get extensions but:
>>    a. only use them on explicit client configuration or
>>    b. try/fail/remember per-remote
>> 4. A backwards-incompatible protocol v2 is introduced, which
>>    hits alternate endpoints (with the same a/b as above).  This is
>>    different from 3. in that protocol v2 is explicitly designed around
>>    a capabilities negotiation phase rather than unilateral client-side
>>    decisions.
>> 5. Think of another way to make fetch performant with many refs, and
>>     defer the extension decision.
>
> I'd prefer 2,3,4 over 1,5.
>
> Speaking about 2,3,4:
>
> Maybe we can do a mix of 2 and 4:
>
>    1) HTTP grows more extensions; other protocols stagnate for now.
>    2) Come up with a backwards-incompatible protocol v2, foccussed on
>        capabilities negotiation phase, hitting alternative end points
>        (non http only, or rather a subset of protocols only)
>     3) if HTTP sees the benefits of the native protocol v2, we may switch
>         HTTP, too
>
> (in time order of execution. Each point is decoupled from the others and may
> be done by different people at different times.)
>

Today I rebased protocol-v2[1] and it was fewer conflicts than expected.
I am surprised by myself that there is even a test case for v2 already,
so I think it is more progressed that I had in mind. Maybe we can do 1)
for now and hope that the non http catches up eventually?


[1] https://github.com/stefanbeller/git/tree/protocol-v2

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Tue, Apr 26, 2016 at 08:59:22PM -0700, Stefan Beller wrote:

> > Maybe we can do a mix of 2 and 4:
> >
> >    1) HTTP grows more extensions; other protocols stagnate for now.
> >    2) Come up with a backwards-incompatible protocol v2, foccussed on
> >        capabilities negotiation phase, hitting alternative end points
> >        (non http only, or rather a subset of protocols only)
> >     3) if HTTP sees the benefits of the native protocol v2, we may switch
> >         HTTP, too
> >
> > (in time order of execution. Each point is decoupled from the others and may
> > be done by different people at different times.)
> >
> 
> Today I rebased protocol-v2[1] and it was fewer conflicts than expected.
> I am surprised by myself that there is even a test case for v2 already,
> so I think it is more progressed that I had in mind. Maybe we can do 1)
> for now and hope that the non http catches up eventually?

If the plan is something like:

  1. v2 exists, but client/server don't know when they should use it.

  2. smart-http grows an extra parameter for the client to say "hey, I
     know v2"

  3. Other protocols get some manual v2 support (e.g., client asks for
     "upload-pack2" if instructed by the user, server either speaks v2
     then or says "eh, what?").

I like that very much. It lets us "cheat" on the hard part of the
problem for http, which is what David's series is doing, but it provides
a clear path forward for the protocols eventually reaching feature
parity (namely that eventually all servers speak v2, and the client can
start asking for v2 by default).

-Peff

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If the plan is something like:
>
>   1. v2 exists, but client/server don't know when they should use it.
>
>   2. smart-http grows an extra parameter for the client to say "hey, I
>      know v2"
>
>   3. Other protocols get some manual v2 support (e.g., client asks for
>      "upload-pack2" if instructed by the user, server either speaks v2
>      then or says "eh, what?").
>
> I like that very much. It lets us "cheat" on the hard part of the
> problem for http, which is what David's series is doing, but it provides
> a clear path forward for the protocols eventually reaching feature
> parity (namely that eventually all servers speak v2, and the client can
> start asking for v2 by default).

Yup, thanks for clearly writing it down. I like the above very much,
too.

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Tue, 2016-04-26 at 20:59 -0700, Stefan Beller wrote:
> On Mon, Apr 25, 2016 at 3:10 PM, Stefan Beller <sbeller@google.com>
> wrote:
> > On Mon, Apr 25, 2016 at 9:44 AM, David Turner <
> > dturner@twopensource.com> wrote:
> > > On Wed, 2016-04-20 at 16:57 -0400, Jeff King wrote:
> > > > On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:
> > > > 
> > > > > As you note, it appears that git-daemon does sort-of have
> > > > > support
> > > > > for
> > > > > extra args -- see parse_host_arg.  So it wouldn't be hard to
> > > > > add
> > > > > something here. Unfortunately, current versions of git die on
> > > > > unknown
> > > > > args.  So this change would not be backwards-compatible.  We
> > > > > could
> > > > > put
> > > > > a decider on it so that clients would only try it when
> > > > > explicitly
> > > > > enabled.  Or we could have clients try it with, and in the
> > > > > event of
> > > > > an
> > > > > error, retry without.  Neither is ideal, but both are
> > > > > possible.
> > > > 
> > > > Right. This ends up being the same difficulty that the v2
> > > > protocol
> > > > encountered; how do you figure out what you can speak without
> > > > resorting
> > > > to expensive fallbacks, when do you flip the switch, do you
> > > > remember
> > > > the
> > > > protocol you used last time with this server, etc.
> > > 
> > > Right.
> > > 
> > > [moved]
> > > > > If I read this code correctly, git-over-ssh will pass through
> > > > > arbitrary
> > > > > arguments.  So this should be trivial.
> > > > 
> > > > It does if you are ssh-ing to a real shell-level account on the
> > > > server,
> > > > but if you are using git-shell or some other wrapper to
> > > > restrict
> > > > clients
> > > > from running arbitrary commands, it will likely reject it.
> > > 
> > > Oh, I see how I was mis-reading shell.c.  Oops.
> > > [/moved]
> > > 
> > > 
> > > > Which isn't to say it's necessarily a bad thing. Maybe the path
> > > > forward
> > > > instead of v2 is to shoe-horn this data into the pre-protocol
> > > > conversation, and go from there. The protocol accepts that
> > > > "somehow"
> > > > it
> > > > got some extra data from the transport layer, and acts on its
> > > > uniformly.
> > > 
> > > OK, so it seems like only HTTP (and non-git-shell-git://) allow
> > > backwar
> > > ds-compatible optional pre-protocol messages.  So we don't have
> > > good
> > > options; we only have bad ones.  We have to decide which
> > > particular
> > > kind of badness we're willing to accept, and to what degree we
> > > care
> > > about extensibility.  As I see it, the badness options are (in no
> > > particular order):
> > > 
> > > 1. Nothing changes.
> > > 2. HTTP grows more extensions; other protocols stagnate.
> > > 3. HTTP grows extensions; other protocols get extensions but:
> > >    a. only use them on explicit client configuration or
> > >    b. try/fail/remember per-remote
> > > 4. A backwards-incompatible protocol v2 is introduced, which
> > >    hits alternate endpoints (with the same a/b as above).  This
> > > is
> > >    different from 3. in that protocol v2 is explicitly designed
> > > around
> > >    a capabilities negotiation phase rather than unilateral client
> > > -side
> > >    decisions.
> > > 5. Think of another way to make fetch performant with many refs,
> > > and
> > >     defer the extension decision.
> > 
> > I'd prefer 2,3,4 over 1,5.
> > 
> > Speaking about 2,3,4:
> > 
> > Maybe we can do a mix of 2 and 4:
> > 
> >    1) HTTP grows more extensions; other protocols stagnate for now.
> >    2) Come up with a backwards-incompatible protocol v2, foccussed
> > on
> >        capabilities negotiation phase, hitting alternative end
> > points
> >        (non http only, or rather a subset of protocols only)
> >     3) if HTTP sees the benefits of the native protocol v2, we may
> > switch
> >         HTTP, too
> > 
> > (in time order of execution. Each point is decoupled from the
> > others and may
> > be done by different people at different times.)
> > 
> 
> Today I rebased protocol-v2[1] and it was fewer conflicts than
> expected.
> I am surprised by myself that there is even a test case for v2
> already,
> so I think it is more progressed that I had in mind. Maybe we can do
> 1)
> for now and hope that the non http catches up eventually?
> 
> 
> [1] https://github.com/stefanbeller/git/tree/protocol-v2


Do you plan to send these patches to the mailing list?  What's the next
step here?

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Stefan Beller]

On Fri, Apr 29, 2016 at 4:05 PM, David Turner <dturner@twopensource.com> wrote:
> On Tue, 2016-04-26 at 20:59 -0700, Stefan Beller wrote:
>> On Mon, Apr 25, 2016 at 3:10 PM, Stefan Beller <sbeller@google.com>
>> wrote:
>> > On Mon, Apr 25, 2016 at 9:44 AM, David Turner <
>> > dturner@twopensource.com> wrote:
>> > > On Wed, 2016-04-20 at 16:57 -0400, Jeff King wrote:
>> > > > On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:
>> > > >
>> > > > > As you note, it appears that git-daemon does sort-of have
>> > > > > support
>> > > > > for
>> > > > > extra args -- see parse_host_arg.  So it wouldn't be hard to
>> > > > > add
>> > > > > something here. Unfortunately, current versions of git die on
>> > > > > unknown
>> > > > > args.  So this change would not be backwards-compatible.  We
>> > > > > could
>> > > > > put
>> > > > > a decider on it so that clients would only try it when
>> > > > > explicitly
>> > > > > enabled.  Or we could have clients try it with, and in the
>> > > > > event of
>> > > > > an
>> > > > > error, retry without.  Neither is ideal, but both are
>> > > > > possible.
>> > > >
>> > > > Right. This ends up being the same difficulty that the v2
>> > > > protocol
>> > > > encountered; how do you figure out what you can speak without
>> > > > resorting
>> > > > to expensive fallbacks, when do you flip the switch, do you
>> > > > remember
>> > > > the
>> > > > protocol you used last time with this server, etc.
>> > >
>> > > Right.
>> > >
>> > > [moved]
>> > > > > If I read this code correctly, git-over-ssh will pass through
>> > > > > arbitrary
>> > > > > arguments.  So this should be trivial.
>> > > >
>> > > > It does if you are ssh-ing to a real shell-level account on the
>> > > > server,
>> > > > but if you are using git-shell or some other wrapper to
>> > > > restrict
>> > > > clients
>> > > > from running arbitrary commands, it will likely reject it.
>> > >
>> > > Oh, I see how I was mis-reading shell.c.  Oops.
>> > > [/moved]
>> > >
>> > >
>> > > > Which isn't to say it's necessarily a bad thing. Maybe the path
>> > > > forward
>> > > > instead of v2 is to shoe-horn this data into the pre-protocol
>> > > > conversation, and go from there. The protocol accepts that
>> > > > "somehow"
>> > > > it
>> > > > got some extra data from the transport layer, and acts on its
>> > > > uniformly.
>> > >
>> > > OK, so it seems like only HTTP (and non-git-shell-git://) allow
>> > > backwar
>> > > ds-compatible optional pre-protocol messages.  So we don't have
>> > > good
>> > > options; we only have bad ones.  We have to decide which
>> > > particular
>> > > kind of badness we're willing to accept, and to what degree we
>> > > care
>> > > about extensibility.  As I see it, the badness options are (in no
>> > > particular order):
>> > >
>> > > 1. Nothing changes.
>> > > 2. HTTP grows more extensions; other protocols stagnate.
>> > > 3. HTTP grows extensions; other protocols get extensions but:
>> > >    a. only use them on explicit client configuration or
>> > >    b. try/fail/remember per-remote
>> > > 4. A backwards-incompatible protocol v2 is introduced, which
>> > >    hits alternate endpoints (with the same a/b as above).  This
>> > > is
>> > >    different from 3. in that protocol v2 is explicitly designed
>> > > around
>> > >    a capabilities negotiation phase rather than unilateral client
>> > > -side
>> > >    decisions.
>> > > 5. Think of another way to make fetch performant with many refs,
>> > > and
>> > >     defer the extension decision.
>> >
>> > I'd prefer 2,3,4 over 1,5.
>> >
>> > Speaking about 2,3,4:
>> >
>> > Maybe we can do a mix of 2 and 4:
>> >
>> >    1) HTTP grows more extensions; other protocols stagnate for now.
>> >    2) Come up with a backwards-incompatible protocol v2, foccussed
>> > on
>> >        capabilities negotiation phase, hitting alternative end
>> > points
>> >        (non http only, or rather a subset of protocols only)
>> >     3) if HTTP sees the benefits of the native protocol v2, we may
>> > switch
>> >         HTTP, too
>> >
>> > (in time order of execution. Each point is decoupled from the
>> > others and may
>> > be done by different people at different times.)
>> >
>>
>> Today I rebased protocol-v2[1] and it was fewer conflicts than
>> expected.
>> I am surprised by myself that there is even a test case for v2
>> already,
>> so I think it is more progressed that I had in mind. Maybe we can do
>> 1)
>> for now and hope that the non http catches up eventually?
>>
>>
>> [1] https://github.com/stefanbeller/git/tree/protocol-v2
>
>
> Do you plan to send these patches to the mailing list?  What's the next
> step here?

I can send them out if you want to. As I flip flop between
"they are perfect" and "they are horrible, nobody should see them",
I haven't done it so far (also time constraints).

Will send them later today, in case it's urgent you can fetch them from
my github account.

What's the next step?
I don't know. I guess we can either collaborate on a large
series (do-it-all) or rather work on many smaller series'
that solve it partially one by one.

I'll send the patches out later today.

Thanks,
Stefan