[PATCH/RFC 0/6] fetch with refspec

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
Signed-off-by: David Turner <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by David Turner
The local variable 'options' was shadowing a global of the same name.

Signed-off-by: David Turner <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Stefan Beller-4
In reply to this post by David Turner
On Fri, Apr 15, 2016 at 12:19 PM, David Turner <[hidden email]> 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 [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by David Turner
On Fri, Apr 15, 2016 at 3:19 PM, David Turner <[hidden email]> 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 <[hidden email]>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by David Turner
On Fri, Apr 15, 2016 at 3:19 PM, David Turner <[hidden email]> 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 <[hidden email]>
> ---
> 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 ...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by David Turner
On Fri, Apr 15, 2016 at 3:19 PM, David Turner <[hidden email]> 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 <[hidden email]>
> ---
> 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);
> +       }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by David Turner
David Turner <[hidden email]> writes:

> Signed-off-by: David Turner <[hidden email]>
> ---

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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by David Turner
David Turner <[hidden email]> writes:

> The local variable 'options' was shadowing a global of the same name.
>
> Signed-off-by: David Turner <[hidden email]>
> ---

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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by David Turner
David Turner <[hidden email]> 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 <[hidden email]>
> ---

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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 <[hidden email]>
> > ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Stefan Beller-4
On Tue, Apr 19, 2016 at 12:14 AM, Jeff King <[hidden email]> wrote:

> On Mon, Apr 18, 2016 at 11:45:54AM -0700, Junio C Hamano wrote:
>
>> David Turner <[hidden email]> 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 <[hidden email]>
>> > ---
>>
>> 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 [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by Eric Sunshine
On Sat, 2016-04-16 at 21:51 -0400, Eric Sunshine wrote:

> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> [hidden email]> 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.  
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

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

Good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by Junio C Hamano
On Mon, 2016-04-18 at 11:35 -0700, Junio C Hamano wrote:

> David Turner <[hidden email]> writes:
>
> > The local variable 'options' was shadowing a global of the same
> > name.
> >
> > Signed-off-by: David Turner <[hidden email]>
> > ---
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Turner
In reply to this post by Junio C Hamano
On Mon, 2016-04-18 at 11:45 -0700, Junio C Hamano wrote:

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