[PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

This is part 3 of 3 for partial clone.
It assumes that part 1 [1] and part 2 [2] are in place.

Part 3 is concerned with the commands: clone, fetch, upload-pack, fetch-pack,
remote-curl, index-pack, and the pack-protocol.

Jonathan and I independently started on this task.  This is a first
pass at merging those efforts.  So there are several places that need
refactoring and cleanup.  In particular, the test cases should be
squashed and new tests added.

[1] https://public-inbox.org/git/20171102124445.fbffd43521cd35f6a71e1851@google.com/T/
[2] TODO


Jeff Hostetler (5):
  upload-pack: add object filtering for partial clone
  clone, fetch-pack, index-pack, transport: partial clone
  fetch: add object filtering for partial fetch
  remote-curl: add object filtering for partial clone
  index-pack: silently assume missing objects are promisor

Jonathan Tan (9):
  fetch: refactor calculation of remote list
  pack-objects: test support for blob filtering
  fetch-pack: test support excluding large blobs
  fetch: add from_promisor and exclude-promisor-objects parameters
  t5500: add fetch-pack tests for partial clone
  t5601: test for partial clone
  t5500: more tests for partial clone and fetch
  unpack-trees: batch fetching of missing blobs
  fetch-pack: restore save_commit_buffer after use

 Documentation/config.txt                          |   4 +
 Documentation/gitremote-helpers.txt               |   4 +
 Documentation/technical/pack-protocol.txt         |   8 ++
 Documentation/technical/protocol-capabilities.txt |   8 ++
 builtin/clone.c                                   |  24 ++++-
 builtin/fetch-pack.c                              |   4 +
 builtin/fetch.c                                   |  83 ++++++++++++++--
 builtin/index-pack.c                              |  14 +++
 connected.c                                       |   3 +
 fetch-object.c                                    |  27 ++++-
 fetch-object.h                                    |   5 +
 fetch-pack.c                                      |  17 ++++
 fetch-pack.h                                      |   2 +
 remote-curl.c                                     |  10 +-
 t/t5300-pack-object.sh                            |  45 +++++++++
 t/t5500-fetch-pack.sh                             | 115 ++++++++++++++++++++++
 t/t5601-clone.sh                                  | 101 +++++++++++++++++++
 t/test-lib-functions.sh                           |  12 +++
 transport-helper.c                                |   5 +
 transport.c                                       |   4 +
 transport.h                                       |   5 +
 unpack-trees.c                                    |  22 +++++
 upload-pack.c                                     |  20 +++-
 23 files changed, 526 insertions(+), 16 deletions(-)

-- 
2.9.3

[PATCH 01/14] upload-pack: add object filtering for partial clone

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach upload-pack to negotiate object filtering over the protocol and
to send filter parameters to pack-objects.  This is intended for partial
clone and fetch.

The idea to make upload-pack configurable using uploadpack.allowFilter
comes from Jonathan Tan's work in [1].

[1] https://public-inbox.org/git/f211093280b422c32cc1b7034130072f35c5ed51.1506714999.git.jonathantanmy@google.com/

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/config.txt                          |  4 ++++
 Documentation/technical/pack-protocol.txt         |  8 ++++++++
 Documentation/technical/protocol-capabilities.txt |  8 ++++++++
 upload-pack.c                                     | 20 +++++++++++++++++++-
 4 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6..e528210 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3268,6 +3268,10 @@ uploadpack.packObjectsHook::
 	was run. I.e., `upload-pack` will feed input intended for
 	`pack-objects` to the hook, and expects a completed packfile on
 	stdout.
+
+uploadpack.allowFilter::
+	If this option is set, `upload-pack` will advertise partial
+	clone and partial fetch object filtering.
 +
 Note that this configuration variable is ignored if it is seen in the
 repository-level config (this is a safety measure against fetching from
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index ed1eae8..a43a113 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -212,6 +212,7 @@ out of what the server said it could do with the first 'want' line.
   upload-request    =  want-list
 		       *shallow-line
 		       *1depth-request
+		       [filter-request]
 		       flush-pkt
 
   want-list         =  first-want
@@ -227,6 +228,8 @@ out of what the server said it could do with the first 'want' line.
   additional-want   =  PKT-LINE("want" SP obj-id)
 
   depth             =  1*DIGIT
+
+  filter-request    =  PKT-LINE("filter" SP filter-spec)
 ----
 
 Clients MUST send all the obj-ids it wants from the reference
@@ -249,6 +252,11 @@ complete those commits. Commits whose parents are not received as a
 result are defined as shallow and marked as such in the server. This
 information is sent back to the client in the next step.
 
+The client can optionally request that pack-objects omit various
+objects from the packfile using one of several filtering techniques.
+These are intended for use with partial clone and partial fetch
+operations.  See `rev-list` for possible "filter-spec" values.
+
 Once all the 'want's and 'shallow's (and optional 'deepen') are
 transferred, clients MUST send a flush-pkt, to tell the server side
 that it is done sending the list.
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 26dcc6f..332d209 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -309,3 +309,11 @@ 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.
+
+filter
+------
+
+If the upload-pack server advertises the 'filter' capability,
+fetch-pack may send "filter" commands to request a partial clone
+or partial fetch and request that the server omit various objects
+from the packfile.
diff --git a/upload-pack.c b/upload-pack.c
index e25f725..64a57a4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -10,6 +10,8 @@
 #include "diff.h"
 #include "revision.h"
 #include "list-objects.h"
+#include "list-objects-filter.h"
+#include "list-objects-filter-options.h"
 #include "run-command.h"
 #include "connect.h"
 #include "sigchain.h"
@@ -64,6 +66,10 @@ static int advertise_refs;
 static int stateless_rpc;
 static const char *pack_objects_hook;
 
+static int filter_capability_requested;
+static int filter_advertise;
+static struct list_objects_filter_options filter_options;
+
 static void reset_timeout(void)
 {
 	alarm(timeout);
@@ -131,6 +137,7 @@ static void create_pack_file(void)
 		argv_array_push(&pack_objects.args, "--delta-base-offset");
 	if (use_include_tag)
 		argv_array_push(&pack_objects.args, "--include-tag");
+	arg_format_list_objects_filter(&pack_objects.args, &filter_options);
 
 	pack_objects.in = -1;
 	pack_objects.out = -1;
@@ -794,6 +801,12 @@ static void receive_needs(void)
 			deepen_rev_list = 1;
 			continue;
 		}
+		if (skip_prefix(line, "filter ", &arg)) {
+			if (!filter_capability_requested)
+				die("git upload-pack: filtering capability not negotiated");
+			parse_list_objects_filter(&filter_options, arg);
+			continue;
+		}
 		if (!skip_prefix(line, "want ", &arg) ||
 		    get_oid_hex(arg, &oid_buf))
 			die("git upload-pack: protocol error, "
@@ -821,6 +834,8 @@ static void receive_needs(void)
 			no_progress = 1;
 		if (parse_feature_request(features, "include-tag"))
 			use_include_tag = 1;
+		if (parse_feature_request(features, "filter"))
+			filter_capability_requested = 1;
 
 		o = parse_object(&oid_buf);
 		if (!o) {
@@ -940,7 +955,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 		struct strbuf symref_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, cb_data);
-		packet_write_fmt(1, "%s %s%c%s%s%s%s%s agent=%s\n",
+		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s agent=%s\n",
 			     oid_to_hex(oid), refname_nons,
 			     0, capabilities,
 			     (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
@@ -949,6 +964,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 				     " allow-reachable-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
+			     filter_advertise ? " filter" : "",
 			     git_user_agent_sanitized());
 		strbuf_release(&symref_info);
 	} else {
@@ -1027,6 +1043,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 	} else if (current_config_scope() != CONFIG_SCOPE_REPO) {
 		if (!strcmp("uploadpack.packobjectshook", var))
 			return git_config_string(&pack_objects_hook, var, value);
+	} else if (!strcmp("uploadpack.allowfilter", var)) {
+		filter_advertise = git_config_bool(var, value);
 	}
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
-- 
2.9.3

[PATCH 02/14] clone, fetch-pack, index-pack, transport: partial clone

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/clone.c      |  9 +++++++++
 builtin/fetch-pack.c |  4 ++++
 builtin/index-pack.c | 10 ++++++++++
 fetch-pack.c         | 13 +++++++++++++
 fetch-pack.h         |  2 ++
 transport-helper.c   |  5 +++++
 transport.c          |  4 ++++
 transport.h          |  5 +++++
 8 files changed, 52 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index dbddd98..fceb9e7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -26,6 +26,7 @@
 #include "run-command.h"
 #include "connected.h"
 #include "packfile.h"
+#include "list-objects-filter-options.h"
 
 /*
  * Overall FIXMEs:
@@ -60,6 +61,7 @@ static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
+static struct list_objects_filter_options filter_options;
 
 static int recurse_submodules_cb(const struct option *opt,
 				 const char *arg, int unset)
@@ -135,6 +137,7 @@ static struct option builtin_clone_options[] = {
 			TRANSPORT_FAMILY_IPV4),
 	OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
 			TRANSPORT_FAMILY_IPV6),
+	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
 	OPT_END()
 };
 
@@ -1073,6 +1076,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			warning(_("--shallow-since is ignored in local clones; use file:// instead."));
 		if (option_not.nr)
 			warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
+		if (filter_options.choice)
+			warning(_("--filter is ignored in local clones; use file:// instead."));
 		if (!access(mkpath("%s/shallow", path), F_OK)) {
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
@@ -1104,6 +1109,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 				     option_upload_pack);
 
+	if (filter_options.choice)
+		transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
+				     filter_options.raw_value);
+
 	if (transport->smart_options && !deepen)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 9a7ebf6..d0fdaa8 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -153,6 +153,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.no_haves = 1;
 			continue;
 		}
+		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
+			parse_list_objects_filter(&args.filter_options, arg);
+			continue;
+		}
 		usage(fetch_pack_usage);
 	}
 	if (deepen_not.nr)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a0a35e6..31cd5ba 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -222,6 +222,16 @@ static unsigned check_object(struct object *obj)
 	if (!(obj->flags & FLAG_CHECKED)) {
 		unsigned long size;
 		int type = sha1_object_info(obj->oid.hash, &size);
+
+		if (type <= 0) {
+			/*
+			 * TODO Use the promisor code to conditionally
+			 * try to fetch this object -or- assume it is ok.
+			 */
+			obj->flags |= FLAG_CHECKED;
+			return 0;
+		}
+
 		if (type <= 0)
 			die(_("did not receive expected object %s"),
 			      oid_to_hex(&obj->oid));
diff --git a/fetch-pack.c b/fetch-pack.c
index 4640b4e..895e8f9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -29,6 +29,7 @@ static int deepen_not_ok;
 static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
+static int server_supports_filtering;
 static struct lock_file shallow_lock;
 static const char *alternate_shallow_file;
 
@@ -379,6 +380,8 @@ static int find_common(struct fetch_pack_args *args,
 			if (deepen_not_ok)      strbuf_addstr(&c, " deepen-not");
 			if (agent_supported)    strbuf_addf(&c, " agent=%s",
 							    git_user_agent_sanitized());
+			if (args->filter_options.choice)
+				strbuf_addstr(&c, " filter");
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
 			strbuf_release(&c);
 		} else
@@ -407,6 +410,9 @@ static int find_common(struct fetch_pack_args *args,
 			packet_buf_write(&req_buf, "deepen-not %s", s->string);
 		}
 	}
+	if (server_supports_filtering && args->filter_options.choice)
+		packet_buf_write(&req_buf, "filter %s",
+				 args->filter_options.raw_value);
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
@@ -967,6 +973,13 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	else
 		prefer_ofs_delta = 0;
 
+	if (server_supports("filter")) {
+		server_supports_filtering = 1;
+		print_verbose(args, _("Server supports filter"));
+	} else if (args->filter_options.choice) {
+		warning("filtering not recognized by server, ignoring");
+	}
+
 	if ((agent_feature = server_feature_value("agent", &agent_len))) {
 		agent_supported = 1;
 		if (agent_len)
diff --git a/fetch-pack.h b/fetch-pack.h
index 84904c3..64661b6 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -3,6 +3,7 @@
 
 #include "string-list.h"
 #include "run-command.h"
+#include "list-objects-filter-options.h"
 
 struct oid_array;
 
@@ -12,6 +13,7 @@ struct fetch_pack_args {
 	int depth;
 	const char *deepen_since;
 	const struct string_list *deepen_not;
+	struct list_objects_filter_options filter_options;
 	unsigned deepen_relative:1;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
diff --git a/transport-helper.c b/transport-helper.c
index c948d52..96823c7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -671,6 +671,11 @@ static int fetch(struct transport *transport,
 	if (data->transport_options.update_shallow)
 		set_helper_option(transport, "update-shallow", "true");
 
+	if (data->transport_options.filter_options.choice)
+		set_helper_option(
+			transport, "filter",
+			data->transport_options.filter_options.raw_value);
+
 	if (data->fetch)
 		return fetch_with_fetch(transport, nr_heads, to_fetch);
 
diff --git a/transport.c b/transport.c
index 8211f82..d50c73b 100644
--- a/transport.c
+++ b/transport.c
@@ -166,6 +166,9 @@ static int set_git_option(struct git_transport_options *opts,
 	} else if (!strcmp(name, TRANS_OPT_NO_HAVES)) {
 		opts->no_haves = !!value;
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_LIST_OBJECTS_FILTER)) {
+		parse_list_objects_filter(&opts->filter_options, value);
+		return 0;
 	}
 	return 1;
 }
@@ -236,6 +239,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.update_shallow = data->options.update_shallow;
 	args.from_promisor = data->options.from_promisor;
 	args.no_haves = data->options.no_haves;
+	args.filter_options = data->options.filter_options;
 
 	if (!data->got_remote_heads) {
 		connect_setup(transport, 0);
diff --git a/transport.h b/transport.h
index 67428f6..f64aa3a 100644
--- a/transport.h
+++ b/transport.h
@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "run-command.h"
 #include "remote.h"
+#include "list-objects-filter-options.h"
 
 struct string_list;
 
@@ -23,6 +24,7 @@ struct git_transport_options {
 	const char *uploadpack;
 	const char *receivepack;
 	struct push_cas_option *cas;
+	struct list_objects_filter_options filter_options;
 };
 
 enum transport_family {
@@ -218,6 +220,9 @@ void transport_check_allowed(const char *type);
 /* Do not send "have" lines */
 #define TRANS_OPT_NO_HAVES "no-haves"
 
+/* Filter objects for partial clone and fetch */
+#define TRANS_OPT_LIST_OBJECTS_FILTER "filter"
+
 /**
  * Returns 0 if the option was used, non-zero otherwise. Prints a
  * message to stderr if the option is not used.
-- 
2.9.3

Re: [PATCH 02/14] clone, fetch-pack, index-pack, transport: partial clone

[Jonathan Tan]

On Thu,  2 Nov 2017 20:31:17 +0000
Jeff Hostetler <git@jeffhostetler.com> wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  builtin/clone.c      |  9 +++++++++
>  builtin/fetch-pack.c |  4 ++++
>  builtin/index-pack.c | 10 ++++++++++
>  fetch-pack.c         | 13 +++++++++++++
>  fetch-pack.h         |  2 ++
>  transport-helper.c   |  5 +++++
>  transport.c          |  4 ++++
>  transport.h          |  5 +++++
>  8 files changed, 52 insertions(+)

I managed to take a look at some of these patches. Firstly, consider
separating out the clone part, since it will not be tested until a few
patches later.

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a0a35e6..31cd5ba 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -222,6 +222,16 @@ static unsigned check_object(struct object *obj)
>  	if (!(obj->flags & FLAG_CHECKED)) {
>  		unsigned long size;
>  		int type = sha1_object_info(obj->oid.hash, &size);
> +
> +		if (type <= 0) {
> +			/*
> +			 * TODO Use the promisor code to conditionally
> +			 * try to fetch this object -or- assume it is ok.
> +			 */
> +			obj->flags |= FLAG_CHECKED;
> +			return 0;
> +		}
> +
>  		if (type <= 0)
>  			die(_("did not receive expected object %s"),
>  			      oid_to_hex(&obj->oid));

This causes some repo corruption tests to fail.

If I remove this and rebase the fetch-pack tests on top [1], the tests
pass, so this might not be necessary (for now, at least).

[1] https://github.com/jonathantanmy/git/commits/pc20171103

Re: [PATCH 02/14] clone, fetch-pack, index-pack, transport: partial clone

[Adam Dinwoodie]

On Friday 03 November 2017 at 01:32 pm -0700, Jonathan Tan wrote:
> On Thu,  2 Nov 2017 20:31:17 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> > index a0a35e6..31cd5ba 100644
> > --- a/builtin/index-pack.c
> > +++ b/builtin/index-pack.c
> > @@ -222,6 +222,16 @@ static unsigned check_object(struct object *obj)
> >  	if (!(obj->flags & FLAG_CHECKED)) {
> >  		unsigned long size;
> >  		int type = sha1_object_info(obj->oid.hash, &size);
> > +
> > +		if (type <= 0) {
> > +			/*
> > +			 * TODO Use the promisor code to conditionally
> > +			 * try to fetch this object -or- assume it is ok.
> > +			 */
> > +			obj->flags |= FLAG_CHECKED;
> > +			return 0;
> > +		}
> > +
> >  		if (type <= 0)
> >  			die(_("did not receive expected object %s"),
> >  			      oid_to_hex(&obj->oid));
> 
> This causes some repo corruption tests to fail.

Confirmed: I see this patch, or at least f7e0dbc38 ("clone, fetch-pack,
index-pack, transport: partial clone", 2017-11-02), causing t5300.26 to
fail on 64-bit Cygwin.

For the sake of anyone trying to reproduce this, I needed to cherry pick
66d4c7a58 ("fixup! upload-pack: add object filtering for partial clone",
2017-11-08) onto that commit before I was able to get it to compile.

Adam

Re: [PATCH 02/14] clone, fetch-pack, index-pack, transport: partial clone

[Jeff Hostetler]

On 11/8/2017 1:01 PM, Adam Dinwoodie wrote:
> On Friday 03 November 2017 at 01:32 pm -0700, Jonathan Tan wrote:
>> On Thu,  2 Nov 2017 20:31:17 +0000
>> Jeff Hostetler <git@jeffhostetler.com> wrote:
>>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>>> index a0a35e6..31cd5ba 100644
>>> --- a/builtin/index-pack.c
>>> +++ b/builtin/index-pack.c
>>> @@ -222,6 +222,16 @@ static unsigned check_object(struct object *obj)
>>>   	if (!(obj->flags & FLAG_CHECKED)) {
>>>   		unsigned long size;
>>>   		int type = sha1_object_info(obj->oid.hash, &size);
>>> +
>>> +		if (type <= 0) {
>>> +			/*
>>> +			 * TODO Use the promisor code to conditionally
>>> +			 * try to fetch this object -or- assume it is ok.
>>> +			 */
>>> +			obj->flags |= FLAG_CHECKED;
>>> +			return 0;
>>> +		}
>>> +
>>>   		if (type <= 0)
>>>   			die(_("did not receive expected object %s"),
>>>   			      oid_to_hex(&obj->oid));
>>
>> This causes some repo corruption tests to fail.
> 
> Confirmed: I see this patch, or at least f7e0dbc38 ("clone, fetch-pack,
> index-pack, transport: partial clone", 2017-11-02), causing t5300.26 to
> fail on 64-bit Cygwin.
> 
> For the sake of anyone trying to reproduce this, I needed to cherry pick
> 66d4c7a58 ("fixup! upload-pack: add object filtering for partial clone",
> 2017-11-08) onto that commit before I was able to get it to compile.
> 
> Adam
> 

Thanks.  I've removed this from my next version.  I think it was
left over from a pre-promisor version.

Jeff

[PATCH 03/14] fetch: refactor calculation of remote list

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

Separate out the calculation of remotes to be fetched from and the
actual fetching. This will allow us to include an additional step before
the actual fetching in a subsequent commit.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/fetch.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 225c734..1b1f039 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1322,7 +1322,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	struct string_list list = STRING_LIST_INIT_DUP;
-	struct remote *remote;
+	struct remote *remote = NULL;
 	int result = 0;
 	struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
 
@@ -1367,17 +1367,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		else if (argc > 1)
 			die(_("fetch --all does not make sense with refspecs"));
 		(void) for_each_remote(get_one_remote_for_fetch, &list);
-		result = fetch_multiple(&list);
 	} else if (argc == 0) {
 		/* No arguments -- use default remote */
 		remote = remote_get(NULL);
-		result = fetch_one(remote, argc, argv);
 	} else if (multiple) {
 		/* All arguments are assumed to be remotes or groups */
 		for (i = 0; i < argc; i++)
 			if (!add_remote_or_group(argv[i], &list))
 				die(_("No such remote or remote group: %s"), argv[i]);
-		result = fetch_multiple(&list);
 	} else {
 		/* Single remote or group */
 		(void) add_remote_or_group(argv[0], &list);
@@ -1385,14 +1382,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			/* More than one remote */
 			if (argc > 1)
 				die(_("Fetching a group and specifying refspecs does not make sense"));
-			result = fetch_multiple(&list);
 		} else {
 			/* Zero or one remotes */
 			remote = remote_get(argv[0]);
-			result = fetch_one(remote, argc-1, argv+1);
+			argc--;
+			argv++;
 		}
 	}
 
+	if (remote)
+		result = fetch_one(remote, argc, argv);
+	else
+		result = fetch_multiple(&list);
+
 	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
 		struct argv_array options = ARGV_ARRAY_INIT;
 
-- 
2.9.3

[PATCH 04/14] fetch: add object filtering for partial fetch

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach fetch to use the list-objects filtering parameters
to allow a "partial fetch" following a "partial clone".

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fetch.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1b1f039..150ca0a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -18,6 +18,7 @@
 #include "argv-array.h"
 #include "utf8.h"
 #include "packfile.h"
+#include "list-objects-filter-options.h"
 
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@ -55,6 +56,7 @@ static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
 static int shown_url = 0;
 static int refmap_alloc, refmap_nr;
 static const char **refmap_array;
+static struct list_objects_filter_options filter_options;
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
 {
@@ -160,6 +162,7 @@ static struct option builtin_fetch_options[] = {
 			TRANSPORT_FAMILY_IPV4),
 	OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
 			TRANSPORT_FAMILY_IPV6),
+	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
 	OPT_END()
 };
 
@@ -754,6 +757,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
 	int want_status;
 	int summary_width = transport_summary_width(ref_map);
+	struct check_connected_options opt = CHECK_CONNECTED_INIT;
 
 	fp = fopen(filename, "a");
 	if (!fp)
@@ -765,7 +769,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		url = xstrdup("foreign");
 
 	rm = ref_map;
-	if (check_connected(iterate_ref_map, &rm, NULL)) {
+	if (check_connected(iterate_ref_map, &rm, &opt)) {
 		rc = error(_("%s did not send all necessary objects\n"), url);
 		goto abort;
 	}
@@ -1044,6 +1048,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 		set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
+	if (filter_options.choice)
+		set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
+			   filter_options.raw_value);
 	return transport;
 }
 
@@ -1242,6 +1249,20 @@ static int fetch_multiple(struct string_list *list)
 	int i, result = 0;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 
+	if (filter_options.choice) {
+		/*
+		 * We currently only support partial-fetches to the remote
+		 * used for the partial-clone because we only support 1
+		 * promisor remote, so we DO NOT allow explicit command
+		 * line filter arguments.
+		 *
+		 * Note that the loop below will spawn background fetches
+		 * for each remote and one of them MAY INHERIT the proper
+		 * partial-fetch settings, so everything is consistent.
+		 */
+		die(_("partial-fetch is not supported on multiple remotes"));
+	}
+
 	if (!append && !dry_run) {
 		int errcode = truncate_fetch_head();
 		if (errcode)
@@ -1267,6 +1288,46 @@ static int fetch_multiple(struct string_list *list)
 	return result;
 }
 
+static inline void partial_fetch_one_setup(struct remote *remote)
+{
+#if 0 /* TODO */
+	if (filter_options.choice) {
+		/*
+		 * A partial-fetch was explicitly requested.
+		 *
+		 * If this is the first partial-* command on
+		 * this repo, we must register the partial
+		 * settings in the repository extension.
+		 *
+		 * If this follows a previous partial-* command
+		 * we must ensure the args are consistent with
+		 * the existing registration (because we don't
+		 * currently support mixing-and-matching).
+		 */
+		partial_clone_utils_register(&filter_options,
+					     remote->name, "fetch");
+		return;
+	}
+
+	if (is_partial_clone_registered() &&
+	    !strcmp(remote->name, repository_format_partial_clone_remote)) {
+		/*
+		 * If a partial-* command has already been used on
+		 * this repo and it was to this remote, we should
+		 * inherit the filter settings used previously.
+		 * That is, if clone omitted very large blobs, then
+		 * fetch should too.
+		 *
+		 * Use the cached filter-spec and create the filter
+		 * settings.
+		 */
+		parse_list_objects_filter(
+			&filter_options,
+			repository_format_partial_clone_filter);
+	}
+#endif
+}
+
 static int fetch_one(struct remote *remote, int argc, const char **argv)
 {
 	static const char **refs = NULL;
@@ -1278,6 +1339,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 		die(_("No remote repository specified.  Please, specify either a URL or a\n"
 		    "remote name from which new revisions should be fetched."));
 
+	partial_fetch_one_setup(remote);
+
+
 	gtransport = prepare_transport(remote, 1);
 
 	if (prune < 0) {
-- 
2.9.3

Re: [PATCH 04/14] fetch: add object filtering for partial fetch

[Jonathan Tan]

I did some of my own investigation and have a working (i.e. passing
tests) version of this patch here:

https://github.com/jonathantanmy/git/commits/pc20171103

If you want, you can use that, or incorporate the changes therein here.
I'll also remark on my findings inline.

On Thu,  2 Nov 2017 20:31:19 +0000
Jeff Hostetler <git@jeffhostetler.com> wrote:

> @@ -754,6 +757,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
>  	int want_status;
>  	int summary_width = transport_summary_width(ref_map);
> +	struct check_connected_options opt = CHECK_CONNECTED_INIT;
>  
>  	fp = fopen(filename, "a");
>  	if (!fp)
> @@ -765,7 +769,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  		url = xstrdup("foreign");
>  
>  	rm = ref_map;
> -	if (check_connected(iterate_ref_map, &rm, NULL)) {
> +	if (check_connected(iterate_ref_map, &rm, &opt)) {

opt here is unchanged from CHECK_CONNECTED_INIT, so this change is unnecessary.

>  		rc = error(_("%s did not send all necessary objects\n"), url);
>  		goto abort;
>  	}
> @@ -1044,6 +1048,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
>  		set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
>  	if (update_shallow)
>  		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
> +	if (filter_options.choice)
> +		set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
> +			   filter_options.raw_value);

You'll also need to set TRANS_OPT_FROM_PROMISOR.

> @@ -1242,6 +1249,20 @@ static int fetch_multiple(struct string_list *list)
>  	int i, result = 0;
>  	struct argv_array argv = ARGV_ARRAY_INIT;
>  
> +	if (filter_options.choice) {
> +		/*
> +		 * We currently only support partial-fetches to the remote
> +		 * used for the partial-clone because we only support 1
> +		 * promisor remote, so we DO NOT allow explicit command
> +		 * line filter arguments.
> +		 *
> +		 * Note that the loop below will spawn background fetches
> +		 * for each remote and one of them MAY INHERIT the proper
> +		 * partial-fetch settings, so everything is consistent.
> +		 */
> +		die(_("partial-fetch is not supported on multiple remotes"));
> +	}
> +
>  	if (!append && !dry_run) {
>  		int errcode = truncate_fetch_head();
>  		if (errcode)

My intention in doing the "fetch: refactor calculation of remote list"
patch is so that the interaction between the provided list of remotes
and the specification of the filter can be handled using the following
diff:

    -	if (remote)
    +	if (remote) {
    +		if (filter_options.choice &&
    +		    strcmp(remote->name, repository_format_partial_clone_remote))
    +			die(_("--blob-max-bytes can only be used with the remote configured in core.partialClone"));
     		result = fetch_one(remote, argc, argv);
    -	else
    +	} else {
    +		if (filter_options.choice)
    +			die(_("--blob-max-bytes can only be used with the remote configured in core.partialClone"));
     		result = fetch_multiple(&list);
    +	}

(Ignore the "blob-max-bytes" in the error message - that needs to be
updated.)

The GitHub link I provided above has this diff, and it seems to work.

Re: [PATCH 04/14] fetch: add object filtering for partial fetch

[Jeff Hostetler]

On 11/3/2017 4:38 PM, Jonathan Tan wrote:
>> @@ -1242,6 +1249,20 @@ static int fetch_multiple(struct string_list *list)
>>   	int i, result = 0;
>>   	struct argv_array argv = ARGV_ARRAY_INIT;
>>   
>> +	if (filter_options.choice) {
>> +		/*
>> +		 * We currently only support partial-fetches to the remote
>> +		 * used for the partial-clone because we only support 1
>> +		 * promisor remote, so we DO NOT allow explicit command
>> +		 * line filter arguments.
>> +		 *
>> +		 * Note that the loop below will spawn background fetches
>> +		 * for each remote and one of them MAY INHERIT the proper
>> +		 * partial-fetch settings, so everything is consistent.
>> +		 */
>> +		die(_("partial-fetch is not supported on multiple remotes"));
>> +	}
>> +
>>   	if (!append && !dry_run) {
>>   		int errcode = truncate_fetch_head();
>>   		if (errcode)
> 
> My intention in doing the "fetch: refactor calculation of remote list"
> patch is so that the interaction between the provided list of remotes
> and the specification of the filter can be handled using the following
> diff:
> 
>      -	if (remote)
>      +	if (remote) {
>      +		if (filter_options.choice &&
>      +		    strcmp(remote->name, repository_format_partial_clone_remote))
>      +			die(_("--blob-max-bytes can only be used with the remote configured in core.partialClone"));
>       		result = fetch_one(remote, argc, argv);
>      -	else
>      +	} else {
>      +		if (filter_options.choice)
>      +			die(_("--blob-max-bytes can only be used with the remote configured in core.partialClone"));
>       		result = fetch_multiple(&list);
>      +	}
> 
> (Ignore the "blob-max-bytes" in the error message - that needs to be
> updated.)
> 
> The GitHub link I provided above has this diff, and it seems to work.
> 

I put the filter_options.choice tests inside the fetch_{one,multiple}
routines because the former needs to be able to register partial clone
with the config and/or inherit the default filter-spec for the
promisor remote and that took more code that what can neatly fit inline
here.  This will be more apparent in my next patch series.

Jeff

[PATCH 05/14] remote-curl: add object filtering for partial clone

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/gitremote-helpers.txt |  4 ++++
 remote-curl.c                       | 10 ++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 6da3f41..d46d561 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -468,6 +468,10 @@ set by Git if the remote helper has the 'option' capability.
 
 TODO document 'option from-promisor' and 'option no-haves' ?
 
+'option filter <filter-spec>'::
+	An object filter specification for partial clone or fetch
+	as described in rev-list.
+
 SEE ALSO
 --------
 linkgit:git-remote[1]
diff --git a/remote-curl.c b/remote-curl.c
index 41e8a42..840f3ce 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -13,6 +13,7 @@
 #include "credential.h"
 #include "sha1-array.h"
 #include "send-pack.h"
+#include "list-objects-filter-options.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -22,6 +23,7 @@ struct options {
 	int verbosity;
 	unsigned long depth;
 	char *deepen_since;
+	char *partial_clone_filter;
 	struct string_list deepen_not;
 	struct string_list push_options;
 	unsigned progress : 1,
@@ -163,11 +165,12 @@ static int set_option(const char *name, const char *value)
 	} else if (!strcmp(name, "from-promisor")) {
 		options.from_promisor = 1;
 		return 0;
-
 	} else if (!strcmp(name, "no-haves")) {
 		options.no_haves = 1;
 		return 0;
-
+	} else if (!strcmp(name, "filter")) {
+		options.partial_clone_filter = xstrdup(value);
+		return 0;
 	} else {
 		return 1 /* unsupported */;
 	}
@@ -837,6 +840,9 @@ static int fetch_git(struct discovery *heads,
 		argv_array_push(&args, "--from-promisor");
 	if (options.no_haves)
 		argv_array_push(&args, "--no-haves");
+	if (options.partial_clone_filter)
+		argv_array_pushf(&args, "--%s=%s",
+				 CL_ARG__FILTER, options.partial_clone_filter);
 	argv_array_push(&args, url.buf);
 
 	for (i = 0; i < nr_heads; i++) {
-- 
2.9.3

[PATCH 06/14] pack-objects: test support for blob filtering

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

As part of an effort to improve Git support for very large repositories
in which clients typically have only a subset of all version-controlled
blobs, test pack-objects support for --filter=blobs:limit=<n>, packing only
blobs not exceeding that size unless the blob corresponds to a file
whose name starts with ".git". upload-pack will eventually be taught to
use this new parameter if needed to exclude certain blobs during a fetch
or clone, potentially drastically reducing network consumption when
serving these very large repositories.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t5300-pack-object.sh  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh | 12 ++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 9c68b99..0739a07 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -457,6 +457,51 @@ test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'pack-objects --threads=N or pack.
 	grep -F "no threads support, ignoring pack.threads" err
 '
 
+lcut () {
+	perl -e '$/ = undef; $_ = <>; s/^.{'$1'}//s; print $_'
+}
+
+test_expect_success 'filtering by size works with multiple excluded' '
+	rm -rf server &&
+	git init server &&
+	printf a > server/a &&
+	printf b > server/b &&
+	printf c-very-long-file > server/c &&
+	printf d-very-long-file > server/d &&
+	git -C server add a b c d &&
+	git -C server commit -m x &&
+
+	git -C server rev-parse HEAD >objects &&
+	git -C server pack-objects --revs --stdout --filter=blobs:limit=10 <objects >my.pack &&
+
+	# Ensure that only the small blobs are in the packfile
+	git index-pack my.pack &&
+	git verify-pack -v my.idx >objectlist &&
+	grep $(git hash-object server/a) objectlist &&
+	grep $(git hash-object server/b) objectlist &&
+	! grep $(git hash-object server/c) objectlist &&
+	! grep $(git hash-object server/d) objectlist
+'
+
+test_expect_success 'filtering by size never excludes special files' '
+	rm -rf server &&
+	git init server &&
+	printf a-very-long-file > server/a &&
+	printf a-very-long-file > server/.git-a &&
+	printf b-very-long-file > server/b &&
+	git -C server add a .git-a b &&
+	git -C server commit -m x &&
+
+	git -C server rev-parse HEAD >objects &&
+	git -C server pack-objects --revs --stdout --filter=blobs:limit=10 <objects >my.pack &&
+
+	# Ensure that the .git-a blob is in the packfile, despite also
+	# appearing as a non-.git file
+	git index-pack my.pack &&
+	git verify-pack -v my.idx >objectlist &&
+	grep $(git hash-object server/a) objectlist
+'
+
 #
 # WARNING!
 #
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2..07b79c7 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1020,3 +1020,15 @@ nongit () {
 		"$@"
 	)
 }
+
+# Converts big-endian pairs of hexadecimal digits into bytes. For example,
+# "printf 61620d0a | hex_pack" results in "ab\r\n".
+hex_pack () {
+	perl -e '$/ = undef; $input = <>; print pack("H*", $input)'
+}
+
+# Converts bytes into big-endian pairs of hexadecimal digits. For example,
+# "printf 'ab\r\n' | hex_unpack" results in "61620d0a".
+hex_unpack () {
+	perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)'
+}
-- 
2.9.3

[PATCH 07/14] fetch-pack: test support excluding large blobs

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

Created tests to verify fetch-pack and upload-pack support
for excluding large blobs using --filter=blobs:limit=<n>
parameter.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t5500-fetch-pack.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 80a1a32..fdb98a8 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -755,4 +755,31 @@ test_expect_success 'fetching deepen' '
 	)
 '
 
+test_expect_success 'filtering by size' '
+	rm -rf server client &&
+	test_create_repo server &&
+	test_commit -C server one &&
+	test_config -C server uploadpack.allowfilter 1 &&
+
+	test_create_repo client &&
+	git -C client fetch-pack --filter=blobs:limit=0 ../server HEAD &&
+
+	# Ensure that object is not inadvertently fetched
+	test_must_fail git -C client cat-file -e $(git hash-object server/one.t)
+'
+
+test_expect_success 'filtering by size has no effect if support for it is not advertised' '
+	rm -rf server client &&
+	test_create_repo server &&
+	test_commit -C server one &&
+
+	test_create_repo client &&
+	git -C client fetch-pack --filter=blobs:limit=0 ../server HEAD 2> err &&
+
+	# Ensure that object is fetched
+	git -C client cat-file -e $(git hash-object server/one.t) &&
+
+	test_i18ngrep "filtering not recognized by server" err
+'
+
 test_done
-- 
2.9.3

[PATCH 08/14] fetch: add from_promisor and exclude-promisor-objects parameters

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

Teach fetch to use from-promisor and exclude-promisor-objects
parameters with sub-commands.  Initialize fetch_if_missing
global variable.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fetch.c | 9 ++++++---
 connected.c     | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 150ca0a..ab53df3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -19,6 +19,7 @@
 #include "utf8.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "partial-clone-utils.h"
 
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@ -1048,9 +1049,11 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 		set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
-	if (filter_options.choice)
+	if (filter_options.choice) {
 		set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
 			   filter_options.raw_value);
+		set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
+	}
 	return transport;
 }
 
@@ -1290,7 +1293,6 @@ static int fetch_multiple(struct string_list *list)
 
 static inline void partial_fetch_one_setup(struct remote *remote)
 {
-#if 0 /* TODO */
 	if (filter_options.choice) {
 		/*
 		 * A partial-fetch was explicitly requested.
@@ -1325,7 +1327,6 @@ static inline void partial_fetch_one_setup(struct remote *remote)
 			&filter_options,
 			repository_format_partial_clone_filter);
 	}
-#endif
 }
 
 static int fetch_one(struct remote *remote, int argc, const char **argv)
@@ -1392,6 +1393,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("fetch");
 
+	fetch_if_missing = 0;
+
 	/* Record the command line for the reflog */
 	strbuf_addstr(&default_rla, "fetch");
 	for (i = 1; i < argc; i++)
diff --git a/connected.c b/connected.c
index f416b05..6015316 100644
--- a/connected.c
+++ b/connected.c
@@ -4,6 +4,7 @@
 #include "connected.h"
 #include "transport.h"
 #include "packfile.h"
+#include "partial-clone-utils.h"
 
 /*
  * If we feed all the commits we want to verify to this command
@@ -56,6 +57,8 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,
 	argv_array_push(&rev_list.args,"rev-list");
 	argv_array_push(&rev_list.args, "--objects");
 	argv_array_push(&rev_list.args, "--stdin");
+	if (is_partial_clone_registered())
+		argv_array_push(&rev_list.args, "--exclude-promisor-objects");
 	argv_array_push(&rev_list.args, "--not");
 	argv_array_push(&rev_list.args, "--all");
 	argv_array_push(&rev_list.args, "--quiet");
-- 
2.9.3

[PATCH 09/14] t5500: add fetch-pack tests for partial clone

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t5500-fetch-pack.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index fdb98a8..7c8339f 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -782,4 +782,40 @@ test_expect_success 'filtering by size has no effect if support for it is not ad
 	test_i18ngrep "filtering not recognized by server" err
 '
 
+fetch_blob_max_bytes () {
+		      SERVER="$1"
+		      URL="$2"
+
+	rm -rf "$SERVER" client &&
+	test_create_repo "$SERVER" &&
+	test_commit -C "$SERVER" one &&
+	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
+
+	git clone "$URL" client &&
+	test_config -C client extensions.partialcloneremote origin &&
+
+	test_commit -C "$SERVER" two &&
+
+	git -C client fetch --filter=blobs:limit=0 origin HEAD:somewhere &&
+
+	# Ensure that commit is fetched, but blob is not
+	test_config -C client extensions.partialcloneremote "arbitrary string" &&
+	git -C client cat-file -e $(git -C "$SERVER" rev-parse two) &&
+	test_must_fail git -C client cat-file -e $(git hash-object "$SERVER/two.t")
+}
+
+test_expect_success 'fetch with filtering' '
+		     fetch_blob_max_bytes server server
+'
+
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'fetch with filtering and HTTP' '
+		     fetch_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
+'
+
+stop_httpd
+
+
 test_done
-- 
2.9.3

[PATCH 10/14] t5601: test for partial clone

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/clone.c  | 17 ++++++++++++++---
 t/t5601-clone.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index fceb9e7..08315d8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -27,6 +27,7 @@
 #include "connected.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "partial-clone-utils.h"
 
 /*
  * Overall FIXMEs:
@@ -889,6 +890,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct refspec *refspec;
 	const char *fetch_pattern;
 
+	fetch_if_missing = 0;
+
 	packet_trace_identity("clone");
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
@@ -1109,11 +1112,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 				     option_upload_pack);
 
-	if (filter_options.choice)
+	if (filter_options.choice) {
 		transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
 				     filter_options.raw_value);
+		transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
+	}
 
-	if (transport->smart_options && !deepen)
+	if (transport->smart_options && !deepen && !filter_options.choice)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
 	refs = transport_get_remote_refs(transport);
@@ -1173,13 +1178,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	write_refspec_config(src_ref_prefix, our_head_points_at,
 			remote_head_points_at, &branch_top);
 
+	if (filter_options.choice)
+		partial_clone_utils_register(&filter_options, "origin",
+					     "clone");
+
 	if (is_local)
 		clone_local(path, git_dir);
 	else if (refs && complete_refs_before_fetch)
 		transport_fetch_refs(transport, mapped_refs);
 
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
-			   branch_top.buf, reflog_msg.buf, transport, !is_local);
+			   branch_top.buf, reflog_msg.buf, transport,
+			   !is_local && !filter_options.choice);
 
 	update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
@@ -1200,6 +1210,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	junk_mode = JUNK_LEAVE_REPO;
+	fetch_if_missing = 1;
 	err = checkout(submodule_progress);
 
 	strbuf_release(&reflog_msg);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9c56f77..567161e 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -571,4 +571,53 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a usable pack' '
 	git -C replay.git index-pack -v --stdin <tmp.pack
 '
 
+partial_clone () {
+	       SERVER="$1" &&
+	       URL="$2" &&
+
+	rm -rf "$SERVER" client &&
+	test_create_repo "$SERVER" &&
+	test_commit -C "$SERVER" one &&
+	HASH1=$(git hash-object "$SERVER/one.t") &&
+	git -C "$SERVER" revert HEAD &&
+	test_commit -C "$SERVER" two &&
+	HASH2=$(git hash-object "$SERVER/two.t") &&
+	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
+	test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
+
+	git clone --filter=blobs:limit=0 "$URL" client &&
+
+	git -C client fsck &&
+
+	# Ensure that unneeded blobs are not inadvertently fetched.
+	test_config -C client extensions.partialcloneremote "not a remote" &&
+	test_must_fail git -C client cat-file -e "$HASH1" &&
+
+	# But this blob was fetched, because clone performs an initial checkout
+	git -C client cat-file -e "$HASH2"
+}
+
+test_expect_success 'partial clone' '
+		     partial_clone server "file://$(pwd)/server"
+'
+
+test_expect_success 'partial clone: warn if server does not support object filtering' '
+		     rm -rf server client &&
+		     test_create_repo server &&
+		     test_commit -C server one &&
+
+	git clone --filter=blobs:limit=0 "file://$(pwd)/server" client 2> err &&
+
+	test_i18ngrep "filtering not recognized by server" err
+'
+
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'partial clone using HTTP' '
+		     partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
+'
+
+stop_httpd
+
 test_done
-- 
2.9.3

[PATCH 11/14] t5500: more tests for partial clone and fetch

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t5500-fetch-pack.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 7c8339f..86cf653 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -782,7 +782,7 @@ test_expect_success 'filtering by size has no effect if support for it is not ad
 	test_i18ngrep "filtering not recognized by server" err
 '
 
-fetch_blob_max_bytes () {
+setup_blob_max_bytes () {
 		      SERVER="$1"
 		      URL="$2"
 
@@ -794,7 +794,11 @@ fetch_blob_max_bytes () {
 	git clone "$URL" client &&
 	test_config -C client extensions.partialcloneremote origin &&
 
-	test_commit -C "$SERVER" two &&
+	test_commit -C "$SERVER" two
+}
+
+do_blob_max_bytes() {
+	SERVER="$1" &&
 
 	git -C client fetch --filter=blobs:limit=0 origin HEAD:somewhere &&
 
@@ -805,14 +809,62 @@ fetch_blob_max_bytes () {
 }
 
 test_expect_success 'fetch with filtering' '
-		     fetch_blob_max_bytes server server
+	setup_blob_max_bytes server server &&
+	do_blob_max_bytes server
+'
+
+test_expect_success 'fetch respects configured filtering' '
+	setup_blob_max_bytes server server &&
+
+	test_config -C client extensions.partialclonefilter blobs:limit=0 &&
+
+	git -C client fetch origin HEAD:somewhere &&
+
+	# Ensure that commit is fetched, but blob is not
+	test_config -C client extensions.partialcloneremote "arbitrary string" &&
+	git -C client cat-file -e $(git -C server rev-parse two) &&
+	test_must_fail git -C client cat-file -e $(git hash-object server/two.t)
+'
+
+test_expect_success 'pull respects configured filtering' '
+	setup_blob_max_bytes server server &&
+
+	# Hide two.t from tip so that client does not load it upon the
+	# automatic checkout that pull performs
+	git -C server rm two.t &&
+	test_commit -C server three &&
+
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+	test_config -C client extensions.partialclonefilter blobs:limit=0 &&
+
+	git -C client pull origin &&
+
+	# Ensure that commit is fetched, but blob is not
+	test_config -C client extensions.partialcloneremote "arbitrary string" &&
+	git -C client cat-file -e $(git -C server rev-parse two) &&
+	test_must_fail git -C client cat-file -e $(git hash-object server/two.t)
+'
+
+test_expect_success 'clone configures filtering' '
+	rm -rf server client &&
+	test_create_repo server &&
+	test_commit -C server one &&
+	test_commit -C server two &&
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+
+	git clone --filter=blobs:limit=12345 server client &&
+
+	# Ensure that we can, for example, checkout HEAD^
+	rm -rf client/.git/objects/* &&
+	git -C client checkout HEAD^
 '
 
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
 test_expect_success 'fetch with filtering and HTTP' '
-		     fetch_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
+	setup_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server" &&
+	do_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server"
 '
 
 stop_httpd
-- 
2.9.3

[PATCH 12/14] unpack-trees: batch fetching of missing blobs

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

When running checkout, first prefetch all blobs that are to be updated
but are missing. This means that only one pack is downloaded during such
operations, instead of one per missing blob.

This operates only on the blob level - if a repository has a missing
tree, they are still fetched one at a time.

This does not use the delayed checkout mechanism introduced in commit
2841e8f ("convert: add "status=delayed" to filter process protocol",
2017-06-30) due to significant conceptual differences - in particular,
for partial clones, we already know what needs to be fetched based on
the contents of the local repo alone, whereas for status=delayed, it is
the filter process that tells us what needs to be checked in the end.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 fetch-object.c   | 27 +++++++++++++++++++++++----
 fetch-object.h   |  5 +++++
 t/t5601-clone.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c   | 22 ++++++++++++++++++++++
 4 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/fetch-object.c b/fetch-object.c
index 369b61c..21b4dfa 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -3,12 +3,12 @@
 #include "pkt-line.h"
 #include "strbuf.h"
 #include "transport.h"
+#include "fetch-object.h"
 
-void fetch_object(const char *remote_name, const unsigned char *sha1)
+static void fetch_refs(const char *remote_name, struct ref *ref)
 {
 	struct remote *remote;
 	struct transport *transport;
-	struct ref *ref;
 	int original_fetch_if_missing = fetch_if_missing;
 
 	fetch_if_missing = 0;
@@ -17,10 +17,29 @@ void fetch_object(const char *remote_name, const unsigned char *sha1)
 		die(_("Remote with no URL"));
 	transport = transport_get(remote, remote->url[0]);
 
-	ref = alloc_ref(sha1_to_hex(sha1));
-	hashcpy(ref->old_oid.hash, sha1);
 	transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
 	transport_set_option(transport, TRANS_OPT_NO_HAVES, "1");
 	transport_fetch_refs(transport, ref);
 	fetch_if_missing = original_fetch_if_missing;
 }
+
+void fetch_object(const char *remote_name, const unsigned char *sha1)
+{
+	struct ref *ref = alloc_ref(sha1_to_hex(sha1));
+	hashcpy(ref->old_oid.hash, sha1);
+	fetch_refs(remote_name, ref);
+}
+
+void fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
+{
+	struct ref *ref = NULL;
+	int i;
+
+	for (i = 0; i < to_fetch->nr; i++) {
+		struct ref *new_ref = alloc_ref(oid_to_hex(&to_fetch->oid[i]));
+		oidcpy(&new_ref->old_oid, &to_fetch->oid[i]);
+		new_ref->next = ref;
+		ref = new_ref;
+	}
+	fetch_refs(remote_name, ref);
+}
diff --git a/fetch-object.h b/fetch-object.h
index f371300..4b269d0 100644
--- a/fetch-object.h
+++ b/fetch-object.h
@@ -1,6 +1,11 @@
 #ifndef FETCH_OBJECT_H
 #define FETCH_OBJECT_H
 
+#include "sha1-array.h"
+
 extern void fetch_object(const char *remote_name, const unsigned char *sha1);
 
+extern void fetch_objects(const char *remote_name,
+			  const struct oid_array *to_fetch);
+
 #endif
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 567161e..3211f86 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -611,6 +611,58 @@ test_expect_success 'partial clone: warn if server does not support object filte
 	test_i18ngrep "filtering not recognized by server" err
 '
 
+test_expect_success 'batch missing blob request during checkout' '
+	rm -rf server client &&
+
+	test_create_repo server &&
+	echo a >server/a &&
+	echo b >server/b &&
+	git -C server add a b &&
+
+	git -C server commit -m x &&
+	echo aa >server/a &&
+	echo bb >server/b &&
+	git -C server add a b &&
+	git -C server commit -m x &&
+
+	test_config -C server uploadpack.allowfilter 1 &&
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+
+	git clone --filter=blobs:limit=0 "file://$(pwd)/server" client &&
+
+	# Ensure that there is only one negotiation by checking that there is
+	# only "done" line sent. ("done" marks the end of negotiation.)
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client checkout HEAD^ &&
+	grep "git> done" trace >done_lines &&
+	test_line_count = 1 done_lines
+'
+
+test_expect_success 'batch missing blob request does not inadvertently try to fetch gitlinks' '
+	rm -rf server client &&
+
+	test_create_repo repo_for_submodule &&
+	test_commit -C repo_for_submodule x &&
+
+	test_create_repo server &&
+	echo a >server/a &&
+	echo b >server/b &&
+	git -C server add a b &&
+	git -C server commit -m x &&
+
+	echo aa >server/a &&
+	echo bb >server/b &&
+	# Also add a gitlink pointing to an arbitrary repository
+	git -C server submodule add "$(pwd)/repo_for_submodule" c &&
+	git -C server add a b c &&
+	git -C server commit -m x &&
+
+	test_config -C server uploadpack.allowfilter 1 &&
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+
+	# Make sure that it succeeds
+	git clone --filter=blobs:limit=0 "file://$(pwd)/server" client
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 71b70cc..c18c21a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -14,6 +14,7 @@
 #include "dir.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "fetch-object.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -369,6 +370,27 @@ static int check_updates(struct unpack_trees_options *o)
 		load_gitmodules_file(index, &state);
 
 	enable_delayed_checkout(&state);
+	if (repository_format_partial_clone_remote && o->update && !o->dry_run) {
+		/*
+		 * Prefetch the objects that are to be checked out in the loop
+		 * below.
+		 */
+		struct oid_array to_fetch = OID_ARRAY_INIT;
+		int fetch_if_missing_store = fetch_if_missing;
+		fetch_if_missing = 0;
+		for (i = 0; i < index->cache_nr; i++) {
+			struct cache_entry *ce = index->cache[i];
+			if ((ce->ce_flags & CE_UPDATE) &&
+			    !S_ISGITLINK(ce->ce_mode)) {
+				if (!has_object_file(&ce->oid))
+					oid_array_append(&to_fetch, &ce->oid);
+			}
+		}
+		if (to_fetch.nr)
+			fetch_objects(repository_format_partial_clone_remote,
+				      &to_fetch);
+		fetch_if_missing = fetch_if_missing_store;
+	}
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
-- 
2.9.3

[PATCH 13/14] fetch-pack: restore save_commit_buffer after use

[Jeff Hostetler]

From: Jonathan Tan <jonathantanmy@google.com>

In fetch-pack, the global variable save_commit_buffer is set to 0, but
not restored to its original value after use.

In particular, if show_log() (in log-tree.c) is invoked after
fetch_pack() in the same process, show_log() will return before printing
out the commit message (because the invocation to
get_cached_commit_buffer() returns NULL, because the commit buffer was
not saved). I discovered this when attempting to run "git log -S" in a
partial clone, triggering the case where revision walking lazily loads
missing objects.

Therefore, restore save_commit_buffer to its original value after use.

An alternative to solve the problem I had is to replace
get_cached_commit_buffer() with get_commit_buffer(). That invocation was
introduced in commit a97934d ("use get_cached_commit_buffer where
appropriate", 2014-06-13) to replace "commit->buffer" introduced in
commit 3131b71 ("Add "--show-all" revision walker flag for debugging",
2008-02-13). In the latter commit, the commit author seems to be
deciding between not showing an unparsed commit at all and showing an
unparsed commit without the message (which is what the commit does), and
did not mention parsing the unparsed commit, so I prefer to preserve the
existing behavior.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 fetch-pack.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index 895e8f9..121f03e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -717,6 +717,7 @@ static int everything_local(struct fetch_pack_args *args,
 {
 	struct ref *ref;
 	int retval;
+	int old_save_commit_buffer = save_commit_buffer;
 	timestamp_t cutoff = 0;
 
 	save_commit_buffer = 0;
@@ -784,6 +785,9 @@ static int everything_local(struct fetch_pack_args *args,
 		print_verbose(args, _("already have %s (%s)"), oid_to_hex(remote),
 			      ref->name);
 	}
+
+	save_commit_buffer = old_save_commit_buffer;
+
 	return retval;
 }
 
-- 
2.9.3

[PATCH 14/14] index-pack: silently assume missing objects are promisor

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach index-pack to not complain about missing objects
when the --promisor flag is given.  The assumption is that
index-pack is currently building the idx and promisor data
and the is_promisor_object() query would fail anyway.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/index-pack.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 31cd5ba..51693dc 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -82,6 +82,7 @@ static int verbose;
 static int show_resolving_progress;
 static int show_stat;
 static int check_self_contained_and_connected;
+static int arg_promisor_given;
 
 static struct progress *progress;
 
@@ -223,10 +224,11 @@ static unsigned check_object(struct object *obj)
 		unsigned long size;
 		int type = sha1_object_info(obj->oid.hash, &size);
 
-		if (type <= 0) {
+		if (type <= 0 && arg_promisor_given) {
 			/*
-			 * TODO Use the promisor code to conditionally
-			 * try to fetch this object -or- assume it is ok.
+			 * Assume this missing object is promised.  We can't
+			 * confirm it because we are indexing the packfile
+			 * that omitted it.
 			 */
 			obj->flags |= FLAG_CHECKED;
 			return 0;
@@ -1717,8 +1719,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				keep_msg = arg + 7;
 			} else if (!strcmp(arg, "--promisor")) {
 				promisor_msg = "";
+				arg_promisor_given = 1;
 			} else if (starts_with(arg, "--promisor=")) {
 				promisor_msg = arg + strlen("--promisor=");
+				arg_promisor_given = 1;
 			} else if (starts_with(arg, "--threads=")) {
 				char *end;
 				nr_threads = strtoul(arg+10, &end, 0);
-- 
2.9.3

Re: [PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Jonathan Tan]

On Thu,  2 Nov 2017 20:31:15 +0000
Jeff Hostetler <git@jeffhostetler.com> wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> This is part 3 of 3 for partial clone.
> It assumes that part 1 [1] and part 2 [2] are in place.
> 
> Part 3 is concerned with the commands: clone, fetch, upload-pack, fetch-pack,
> remote-curl, index-pack, and the pack-protocol.
> 
> Jonathan and I independently started on this task.  This is a first
> pass at merging those efforts.  So there are several places that need
> refactoring and cleanup.  In particular, the test cases should be
> squashed and new tests added.

Thanks. What are your future plans with this patch set? In particular, the
tests don't pass at HEAD^.

I took a quick glance to see if there were any issues that I could
immediately spot, but couldn't find any. I thought of fetch_if_missing,
but it seems that it is indeed used in this patch set (as expected).

I'll look at it more thorougly, and feel free to let me know if there is
anything in particular you would like comments on.

Re: [PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Jeff Hostetler]

On 11/2/2017 7:41 PM, Jonathan Tan wrote:
> On Thu,  2 Nov 2017 20:31:15 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> This is part 3 of 3 for partial clone.
>> It assumes that part 1 [1] and part 2 [2] are in place.
>>
>> Part 3 is concerned with the commands: clone, fetch, upload-pack, fetch-pack,
>> remote-curl, index-pack, and the pack-protocol.
>>
>> Jonathan and I independently started on this task.  This is a first
>> pass at merging those efforts.  So there are several places that need
>> refactoring and cleanup.  In particular, the test cases should be
>> squashed and new tests added.
> 
> Thanks. What are your future plans with this patch set? In particular, the
> tests don't pass at HEAD^.
> 
Patch 14/14 fixed 2 existing tests.  I think I want to merge that with
patch 2/14 as part of the cleanup.

Bigger picture, I would like squash all this down.  But first I wanted
you to see if there was anything I missed during the merge.

> I took a quick glance to see if there were any issues that I could
> immediately spot, but couldn't find any. I thought of fetch_if_missing,
> but it seems that it is indeed used in this patch set (as expected).
> 
> I'll look at it more thorougly, and feel free to let me know if there is
> anything in particular you would like comments on.
> 

Thanks, will do.
Jeff

Re: [PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> This is part 3 of 3 for partial clone.
> It assumes that part 1 [1] and part 2 [2] are in place.

Thanks.  As planned these replaced the partial clone with size
filter thing from Jonathan.  The resulting integration passed the
tests locally so I pushed it out.

By the way, the enhancement in this series made to list-objects.c
had a bit of interaction with the last round of Stefan's "describe
blob" topic when both were merged to 'pu'.  I think I resolved it
correctly, but the merge resolution can use extra sets of eyes.

diff --cc list-objects.c
index 5390a7440d,07a92f35fe..e05de01af1
--- a/list-objects.c
+++ b/list-objects.c
@@@ -220,32 -183,20 +220,22 @@@ static void add_pending_tree(struct rev
  	add_pending_object(revs, &tree->object, "");
  }
  
- static void do_traverse(struct rev_info *revs,
- 			show_commit_fn show_commit,
- 			show_object_fn show_object,
- 			void *show_data,
- 			filter_object_fn filter_fn,
- 			void *filter_data)
+ static void traverse_trees_and_blobs(struct rev_info *revs,
+ 				     struct strbuf *base,
+ 				     show_object_fn show_object,
 -				     void *data)
++				     void *show_data,
++				     filter_object_fn filter_fn,
++				     void *filter_data)
  {
  	int i;
- 	struct commit *commit;
- 	struct strbuf base;
  
- 	strbuf_init(&base, PATH_MAX);
- 	while ((commit = get_revision(revs)) != NULL) {
- 		/*
- 		 * an uninteresting boundary commit may not have its tree
- 		 * parsed yet, but we are not going to show them anyway
- 		 */
- 		if (commit->tree)
- 			add_pending_tree(revs, commit->tree);
- 		show_commit(commit, show_data);
- 	}
 -	assert(base->len == 0);
 -
++	assert(!base->len);
  	for (i = 0; i < revs->pending.nr; i++) {
  		struct object_array_entry *pending = revs->pending.objects + i;
  		struct object *obj = pending->item;
  		const char *name = pending->name;
  		const char *path = pending->path;
++
  		if (obj->flags & (UNINTERESTING | SEEN))
  			continue;
  		if (obj->type == OBJ_TAG) {
@@@ -257,47 -208,41 +247,76 @@@
  			path = "";
  		if (obj->type == OBJ_TREE) {
  			process_tree(revs, (struct tree *)obj, show_object,
- 				     &base, path, show_data,
 -				     base, path, data);
++				     base, path, show_data,
 +				     filter_fn, filter_data);
  			continue;
  		}
  		if (obj->type == OBJ_BLOB) {
  			process_blob(revs, (struct blob *)obj, show_object,
- 				     &base, path, show_data,
 -				     base, path, data);
++				     base, path, show_data,
 +				     filter_fn, filter_data);
  			continue;
  		}
  		die("unknown pending object %s (%s)",
  		    oid_to_hex(&obj->oid), name);
  	}
  	object_array_clear(&revs->pending);
- 	strbuf_release(&base);
+ }
+ 
 -void traverse_commit_list(struct rev_info *revs,
 -			  show_commit_fn show_commit,
 -			  show_object_fn show_object,
 -			  void *data)
++static void do_traverse(struct rev_info *revs,
++			show_commit_fn show_commit,
++			show_object_fn show_object,
++			void *show_data,
++			filter_object_fn filter_fn,
++			void *filter_data)
+ {
+ 	struct commit *commit;
+ 	struct strbuf csp; /* callee's scratch pad */
 -	strbuf_init(&csp, PATH_MAX);
+ 
++	strbuf_init(&csp, PATH_MAX);
+ 	while ((commit = get_revision(revs)) != NULL) {
+ 		/*
+ 		 * an uninteresting boundary commit may not have its tree
+ 		 * parsed yet, but we are not going to show them anyway
+ 		 */
+ 		if (commit->tree)
+ 			add_pending_tree(revs, commit->tree);
 -		show_commit(commit, data);
++		show_commit(commit, show_data);
+ 		if (revs->tree_blobs_in_commit_order)
 -			traverse_trees_and_blobs(revs, &csp, show_object, data);
++			traverse_trees_and_blobs(revs, &csp,
++						 show_object, show_data,
++						 filter_fn, filter_data);
+ 	}
 -	traverse_trees_and_blobs(revs, &csp, show_object, data);
 -
++	traverse_trees_and_blobs(revs, &csp,
++				 show_object, show_data,
++				 filter_fn, filter_data);
+ 	strbuf_release(&csp);
  }
 +
 +void traverse_commit_list(struct rev_info *revs,
 +			  show_commit_fn show_commit,
 +			  show_object_fn show_object,
 +			  void *show_data)
 +{
 +	do_traverse(revs, show_commit, show_object, show_data, NULL, NULL);
 +}
 +
 +void traverse_commit_list_filtered(
 +	struct list_objects_filter_options *filter_options,
 +	struct rev_info *revs,
 +	show_commit_fn show_commit,
 +	show_object_fn show_object,
 +	void *show_data,
 +	struct oidset *omitted)
 +{
 +	filter_object_fn filter_fn = NULL;
 +	filter_free_fn filter_free_fn = NULL;
 +	void *filter_data = NULL;
 +
 +	filter_data = list_objects_filter__init(omitted, filter_options,
 +						&filter_fn, &filter_free_fn);
 +	do_traverse(revs, show_commit, show_object, show_data,
 +		    filter_fn, filter_data);
 +	if (filter_data && filter_free_fn)
 +		filter_free_fn(filter_data);
 +}