Re: [PATCH v4] fetch-pack: support negotiation tip whitelist

2018-07-22 Thread Duy Nguyen
On Tue, Jul 3, 2018 at 12:41 AM Jonathan Tan  wrote:
> +static void add_negotiation_tips(struct git_transport_options *smart_options)
> +{
> +   struct oid_array *oids = xcalloc(1, sizeof(*oids));
> +   int i;
> +
> +   for (i = 0; i < negotiation_tip.nr; i++) {
> +   const char *s = negotiation_tip.items[i].string;
> +   int old_nr;
> +   if (!has_glob_specials(s)) {
> +   struct object_id oid;
> +   if (get_oid(s, ))
> +   die("%s is not a valid object", s);

Please _() this string and the warning() below

> +   oid_array_append(oids, );
> +   continue;
> +   }
> +   old_nr = oids->nr;
> +   for_each_glob_ref(add_oid, s, oids);
> +   if (old_nr == oids->nr)
> +   warning("Ignoring --negotiation-tip=%s because it 
> does not match any refs",
> +   s);
> +   }
> +   smart_options->negotiation_tips = oids;
> +}
> +
>  static struct transport *prepare_transport(struct remote *remote, int deepen)
>  {
> struct transport *transport;
> @@ -1075,6 +1112,12 @@ static struct transport *prepare_transport(struct 
> remote *remote, int deepen)
>filter_options.filter_spec);
> set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
> }
> +   if (negotiation_tip.nr) {
> +   if (transport->smart_options)
> +   add_negotiation_tips(transport->smart_options);
> +   else
> +   warning("Ignoring --negotiation-tip because the 
> protocol does not support it.");
> +   }
> return transport;
>  }
-- 
Duy


[PATCH v4] fetch-pack: support negotiation tip whitelist

2018-07-02 Thread Jonathan Tan
During negotiation, fetch-pack eventually reports as "have" lines all
commits reachable from all refs. Allow the user to restrict the commits
sent in this way by providing a whitelist of tips; only the tips
themselves and their ancestors will be sent.

Both globs and single objects are supported.

This feature is only supported for protocols that support connect or
stateless-connect (such as HTTP with protocol v2).

This will speed up negotiation when the repository has multiple
relatively independent branches (for example, when a repository
interacts with multiple repositories, such as with linux-next [1] and
torvalds/linux [2]), and the user knows which local branch is likely to
have commits in common with the upstream branch they are fetching.

[1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/
[2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/

Signed-off-by: Jonathan Tan 
---
This is on jt/fetch-pack-negotiator.

I noticed that jt/fetch-nego-tip had a version queued with Brandon's
suggestion (of the closing parenthesis) included, but not any of Junio's
suggestions, so here is one with the suggestions from both.

> "git notes"???
>
> As this is to be used in the context of "git fetch", using glob
> e.g. "refs/heads/*", is sensible and good enough.  I was actually
> wondering if we want the head-match refs/heads/, but as "git fetch
> origin refs/heads/" does not work that way, I think we shouldn't.

OK - the above was not in the commit message, so I think we're fine. In
the commit message and in the user-facing documentation, I describe this
as a "glob".

> This is a tangent, but didn't ref-in-want wanted to use head-match
> refs/heads/ to match everything under refs/heads/?  If the latest
> incarnation wants to do so, we may want to fix that.

I don't recall anything like that - the user still specifies a refspec
(like existing users do), and in the protocol, all wanted refs are
matched by exact name.

> This by itself is not worth a reason to reroll, but please make it a
> habit to have a blank line after the run of decls before the first
> statement, at least while we still forbid decl-after-stmt.  The
> result is easier to read that way.

Thanks - I've done it for add_oid() and all the new functions introduced
in this patch.

> Style: "if test ..."

> broken &&-chaining?

Both done.
---
 Documentation/fetch-options.txt | 16 +++
 builtin/fetch.c | 43 ++
 fetch-pack.c| 20 -
 fetch-pack.h|  7 +++
 t/t5510-fetch.sh| 78 +
 transport-helper.c  |  3 ++
 transport.c |  1 +
 transport.h | 10 +
 8 files changed, 176 insertions(+), 2 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 97d3217df..2d09f87b4 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -42,6 +42,22 @@ the current repository has the same history as the source 
repository.
.git/shallow. This option updates .git/shallow and accept such
refs.
 
+--negotiation-tip=::
+   By default, Git will report, to the server, commits reachable
+   from all local refs to find common commits in an attempt to
+   reduce the size of the to-be-received packfile. If specified,
+   Git will only report commits reachable from the given tips.
+   This is useful to speed up fetches when the user knows which
+   local ref is likely to have commits in common with the
+   upstream ref being fetched.
++
+This option may be specified more than once; if so, Git will report
+commits reachable from any of the given commits.
++
+The argument to this option may be a glob on ref names, a ref, or the (possibly
+abbreviated) SHA-1 of a commit. Specifying a glob is equivalent to specifying
+this option multiple times, one for each matching ref name.
+
 ifndef::git-pull[]
 --dry-run::
Show what would be done, without making any changes.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..49ab6ac06 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -63,6 +63,7 @@ static int shown_url = 0;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options;
 static struct string_list server_options = STRING_LIST_INIT_DUP;
+static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
 {
@@ -174,6 +175,8 @@ static struct option builtin_fetch_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_STRING_LIST(0, "negotiation-tip", _tip, N_("revision"),
+   N_("report that we have only objects reachable from 
this object")),