[RFC 0/4] Implementation of fetch-blobs and fetch-refs

[RFC 0/4] Implementation of fetch-blobs and fetch-refs

[Jonathan Tan]

My previous proposal about a possible new server endpoint that serves
blobs and refs [1] didn't seem to garner much interest, so I thought I'd
provide a possible implementation of that proposal as a work in
progress.

In particular, patch 1 demonstrates that a new server endpoint that
serves refs without the initial ref advertisement can be done in 228
lines of code (according to "git diff --stat") while accounting for the
various special cases that upload-pack imposes (stateless RPC, inclusion
of an additional response when depth is requested, handling of "done" in
request, sending a packfile directly after a response containing "ACK
ready" without waiting for "done", and so on).

I'm hoping that these features would be of more interest to more people.
To that end, I'm sending these patches in the hope of showing that these
features are useful (omitting ref advertisements help greatly when
serving large repos, as described in the commit message of patch 1, and
serving blobs is useful for any fetch-blob-on-demand repo scheme) and
that my proposed way of implementing them can be done in a relatively
uncomplicated manner (as seen in these patches).

Patch 1-3 show what serving refs without the advertisement would look
like from the server's and fetch-pack's points of view. Patch 4 is
similar to some of the other blob-serving patches except that this
contains reachability checks and that this bundles the resulting objects
in a packfile.

[1] <ffd92ad9-39fe-c76b-178d-6e3d6a425037@google.com>

Jonathan Tan (4):
  server-endpoint: serve refs without advertisement
  fetch-pack: refactor "want" pkt-line generation
  fetch-pack: support new server endpoint
  server-endpoint: serve blobs by hash

 .gitignore                 |   1 +
 Makefile                   |   3 +
 builtin/fetch-pack.c       |  10 +-
 fetch-pack.c               | 129 +++++++++++------
 fetch-pack.h               |   1 +
 server-endpoint.c          | 347 +++++++++++++++++++++++++++++++++++++++++++++
 t/helper/.gitignore        |   1 +
 t/helper/test-un-pkt.c     |  40 ++++++
 t/t5573-server-endpoint.sh |  60 ++++++++
 t/t9999-mytests.sh         | 242 +++++++++++++++++++++++++++++++
 10 files changed, 790 insertions(+), 44 deletions(-)
 create mode 100644 server-endpoint.c
 create mode 100644 t/helper/test-un-pkt.c
 create mode 100644 t/t5573-server-endpoint.sh
 create mode 100644 t/t9999-mytests.sh

-- 
2.12.2.715.g7642488e1d-goog

[RFC 1/4] server-endpoint: serve refs without advertisement

[Jonathan Tan]

Introduce a new binary that can act as an endpoint to serve refs without
first sending the ref advertisement (a list of all ref names and
associated hashes that the server contains). For very large
repositories, including an internal Android repository with more than
700000 refs, this would save tens of megabytes of network bandwidth
during each fetch.

This endpoint handles ref namespaces and "uploadpack.hiderefs" by
itself, and handles other functionality by invoking upload-pack and
acting as an intermediary (therefore having to know the relatively
minute details of the fetch-pack/upload-pack protocol).

Note: There is still an issue with the handling of "deepen" lines. The
documentation for the pack protocol states that "deepen 0" is the same
as not specifying any depth, but upload-pack seems to not accept "deepen
0". I'm not sure if it's better to change the documentation or change
the code - I generally prefer to change the code in such cases, but
treating "deepen 0" (and similar things like "deepen 000") differently
from other "deepen"s requires multiple components to know about this
special case (upload-pack, fetch-pack, and now server-endpoint) so I'm
inclined to just forbid it (like in the current code).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 .gitignore        |   1 +
 Makefile          |   2 +
 server-endpoint.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+)
 create mode 100644 server-endpoint.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..761e06d2c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -140,6 +140,7 @@
 /git-rm
 /git-send-email
 /git-send-pack
+/git-server-endpoint
 /git-sh-i18n
 /git-sh-i18n--envsubst
 /git-sh-setup
diff --git a/Makefile b/Makefile
index c80fec292..0d3813772 100644
--- a/Makefile
+++ b/Makefile
@@ -603,6 +603,7 @@ PROGRAM_OBJS += shell.o
 PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += remote-testsvn.o
+PROGRAM_OBJS += server-endpoint.o
 
 # Binary suffix, set to .exe for Windows builds
 X =
@@ -673,6 +674,7 @@ BINDIR_PROGRAMS_NEED_X += git-upload-pack
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
 BINDIR_PROGRAMS_NEED_X += git-upload-archive
 BINDIR_PROGRAMS_NEED_X += git-shell
+BINDIR_PROGRAMS_NEED_X += git-server-endpoint
 
 BINDIR_PROGRAMS_NO_X += git-cvsserver
 
diff --git a/server-endpoint.c b/server-endpoint.c
new file mode 100644
index 000000000..a9c0c7c94
--- /dev/null
+++ b/server-endpoint.c
@@ -0,0 +1,228 @@
+#include "cache.h"
+#include "pkt-line.h"
+#include "refs.h"
+#include "revision.h"
+#include "run-command.h"
+
+static const char * const server_endpoint_usage[] = {
+	N_("git server-endpoint [<options>] <dir>"),
+	NULL
+};
+
+static const char *capabilities = "multi_ack_detailed side-band-64k shallow";
+
+struct handle_want_data {
+	int upload_pack_in_fd;
+	int capabilities_sent;
+	struct string_list sent_namespaced_names;
+};
+
+static int send_want(const char *namespaced_name, const struct object_id *oid,
+		     int flags, void *handle_want_data)
+{
+	struct handle_want_data *data = handle_want_data;
+
+	if (ref_is_hidden(strip_namespace(namespaced_name), namespaced_name))
+		return 0;
+	if (string_list_lookup(&data->sent_namespaced_names, namespaced_name))
+		return 0;
+
+	string_list_insert(&data->sent_namespaced_names, namespaced_name);
+
+	if (data->capabilities_sent) {
+		packet_write_fmt(data->upload_pack_in_fd, "want %s\n",
+				 oid_to_hex(oid));
+	} else {
+		packet_write_fmt(data->upload_pack_in_fd, "want %s%s\n",
+				 oid_to_hex(oid), capabilities);
+		data->capabilities_sent = 1;
+	}
+
+	return 0;
+}
+
+static void handle_want(const char *arg, struct handle_want_data *data) {
+	char *namespaced_name = xstrfmt("%s%s", get_git_namespace(), arg);
+	if (has_glob_specials(arg)) {
+		for_each_glob_ref(send_want, namespaced_name, data);
+	} else {
+		struct object_id oid;
+		if (!read_ref(namespaced_name, oid.hash))
+			send_want(namespaced_name, &oid, 0, data);
+	}
+	free(namespaced_name);
+}
+
+static int fetch_ref(int stateless_rpc)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	static const char *argv[] = {
+		"upload-pack", ".", NULL, NULL
+	};
+	struct handle_want_data handle_want_data = {0, 0, STRING_LIST_INIT_DUP};
+
+	char *line;
+	int size;
+
+	int upload_pack_will_respond = 0;
+	int wanted_refs_sent = 0;
+
+	if (stateless_rpc)
+		argv[2] = "--stateless-rpc";
+	cmd.argv = argv;
+	cmd.git_cmd = 1;
+	cmd.in = -1;
+	cmd.out = -1;
+
+	if (start_command(&cmd))
+		goto error;
+
+	handle_want_data.upload_pack_in_fd = cmd.in;
+
+	if (!stateless_rpc) {
+		/* Drain the initial ref advertisement (until flush-pkt). */
+		while (packet_read_line(cmd.out, NULL))
+			;
+	}
+
+	/* Send the wants. Upload-pack will not respond to this unless a depth
+	 * request is made. */
+	while ((line = packet_read_line(0, NULL))) {
+		const char *arg;
+		if (skip_prefix(line, "want ", &arg)) {
+			handle_want(arg, &handle_want_data);
+		} else if (starts_with(line, "shallow ")) {
+			packet_write_fmt(cmd.in, "%s", line);
+		} else if (starts_with(line, "deepen ") ||
+			   starts_with(line, "deepen-since ") ||
+			   starts_with(line, "deepen-not ")) {
+			packet_write_fmt(cmd.in, "%s", line);
+			upload_pack_will_respond = 1;
+		}
+	}
+	packet_flush(cmd.in);
+
+	if (upload_pack_will_respond) {
+		while ((line = packet_read_line(cmd.out, NULL))) {
+			packet_write_fmt(1, "%s", line);
+		}
+		packet_flush(1);
+	}
+
+	/* Continue to copy the conversation. */
+	do {
+		char buffer[LARGE_PACKET_DATA_MAX];
+		char size_buffer[5]; /* 4 bytes + NUL */
+		int done_received = 0;
+		int ready_received = 0;
+		int options = PACKET_READ_CHOMP_NEWLINE;
+
+		while ((line = packet_read_line(0, NULL))) {
+			packet_write_fmt(cmd.in, "%s", line);
+			if (!strcmp(line, "done")) {
+				done_received = 1;
+				/* "done" also marks the end of the request. */
+				goto after_flush;
+			}
+		}
+		packet_flush(cmd.in);
+after_flush:
+		while ((size = packet_read(cmd.out, NULL, NULL, buffer,
+					   sizeof(buffer), options))) {
+			int send_wanted_refs = 0;
+			if (!wanted_refs_sent) {
+				if ((done_received || ready_received) &&
+				    size == strlen("ACK ") + GIT_SHA1_HEXSZ &&
+				    starts_with(buffer, "ACK "))
+					send_wanted_refs = 1;
+				else if (done_received && !strcmp(buffer, "NAK"))
+					send_wanted_refs = 1;
+				else if (size == strlen("ACK  ready") + GIT_SHA1_HEXSZ &&
+					 starts_with(buffer, "ACK ") &&
+					 !strcmp(buffer + strlen("ACK  ") + GIT_SHA1_HEXSZ, "ready"))
+					ready_received = 1;
+			}
+			if (send_wanted_refs) {
+				struct string_list_item *item;
+				for_each_string_list_item(item,
+							  &handle_want_data.sent_namespaced_names) {
+					struct object_id oid;
+					if (read_ref(item->string, oid.hash))
+						die("something happened");
+					packet_write_fmt(1, "wanted %s %s",
+							 oid_to_hex(&oid),
+							 strip_namespace(item->string));
+				}
+				wanted_refs_sent = 1;
+				/* Do not chomp any more characters because
+				 * binary data (packfile) is about to be sent.
+				 */
+				options = 0;
+			}
+			sprintf(size_buffer, "%04x", size + 4);
+			write_or_die(1, size_buffer, 4);
+			write_or_die(1, buffer, size);
+			if (!wanted_refs_sent && !strcmp(buffer, "NAK")) {
+				/* NAK before we send wanted refs marks the end
+				 * of the response. */
+				goto after_flush_2;
+			}
+		}
+		packet_flush(1);
+after_flush_2:
+		;
+	} while (!stateless_rpc && !wanted_refs_sent);
+
+	close(cmd.in);
+	cmd.in = -1;
+	close(cmd.out);
+	cmd.out = -1;
+
+	if (finish_command(&cmd))
+		return -1;
+
+	return 0;
+
+error:
+
+	if (cmd.in >= 0)
+		close(cmd.in);
+	if (cmd.out >= 0)
+		close(cmd.out);
+	return -1;
+}
+
+static int server_endpoint_config(const char *var, const char *value, void *unused)
+{
+	return parse_hide_refs_config(var, value, "uploadpack");
+}
+
+int cmd_main(int argc, const char **argv)
+{
+	int stateless_rpc = 0;
+
+	struct option options[] = {
+		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
+			 N_("quit after a single request/response exchange")),
+		OPT_END()
+	};
+
+	char *line;
+
+	packet_trace_identity("server-endpoint");
+	check_replace_refs = 0;
+
+	argc = parse_options(argc, argv, NULL, options, server_endpoint_usage, 0);
+
+	if (argc != 1)
+		die("must have 1 arg");
+
+	if (!enter_repo(argv[0], 0))
+		die("does not appear to be a git repository");
+	git_config(server_endpoint_config, NULL);
+
+	line = packet_read_line(0, NULL);
+	if (!strcmp(line, "fetch-refs"))
+		return fetch_ref(stateless_rpc);
+	die("only fetch-refs is supported");
+}
-- 
2.12.2.715.g7642488e1d-goog

[RFC 2/4] fetch-pack: refactor "want" pkt-line generation

[Jonathan Tan]

In fetch-pack, refactor the generation of the initial request
(containing the "want" lines) into its own function. This cleans up the
code slightly in that the scopes of certain variables are reduced, but
this commit mainly is in preparation for a subsequent one.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 58 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index d07d85ce3..74771a283 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -314,29 +314,14 @@ static int next_flush(struct fetch_pack_args *args, int count)
 	return count;
 }
 
-static int find_common(struct fetch_pack_args *args,
-		       int fd[2], unsigned char *result_sha1,
-		       struct ref *refs)
+/*
+ * Generate the "want" lines to be sent. If there are no "want" lines to be
+ * sent, return NULL.
+ */
+static char *get_wants(const struct fetch_pack_args *args, struct ref *refs)
 {
-	int fetching;
-	int count = 0, flushes = 0, flush_at = INITIAL_FLUSH, retval;
-	const unsigned char *sha1;
-	unsigned in_vain = 0;
-	int got_continue = 0;
-	int got_ready = 0;
 	struct strbuf req_buf = STRBUF_INIT;
-	size_t state_len = 0;
 
-	if (args->stateless_rpc && multi_ack == 1)
-		die(_("--stateless-rpc requires multi_ack_detailed"));
-	if (marked)
-		for_each_ref(clear_marks, NULL);
-	marked = 1;
-
-	for_each_ref(rev_list_insert_ref_oid, NULL);
-	for_each_cached_alternate(insert_one_alternate_object);
-
-	fetching = 0;
 	for ( ; refs ; refs = refs->next) {
 		unsigned char *remote = refs->old_oid.hash;
 		const char *remote_hex;
@@ -358,7 +343,7 @@ static int find_common(struct fetch_pack_args *args,
 		}
 
 		remote_hex = sha1_to_hex(remote);
-		if (!fetching) {
+		if (!req_buf.len) {
 			struct strbuf c = STRBUF_INIT;
 			if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
 			if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
@@ -378,14 +363,37 @@ static int find_common(struct fetch_pack_args *args,
 			strbuf_release(&c);
 		} else
 			packet_buf_write(&req_buf, "want %s\n", remote_hex);
-		fetching++;
 	}
 
-	if (!fetching) {
-		strbuf_release(&req_buf);
+	return req_buf.len ? strbuf_detach(&req_buf, NULL) : NULL;
+}
+
+static int find_common(struct fetch_pack_args *args,
+		       int fd[2], unsigned char *result_sha1,
+		       char *wants)
+{
+	int count = 0, flushes = 0, flush_at = INITIAL_FLUSH, retval;
+	const unsigned char *sha1;
+	unsigned in_vain = 0;
+	int got_continue = 0;
+	int got_ready = 0;
+	struct strbuf req_buf = STRBUF_INIT;
+	size_t state_len = 0;
+
+	if (args->stateless_rpc && multi_ack == 1)
+		die(_("--stateless-rpc requires multi_ack_detailed"));
+	if (marked)
+		for_each_ref(clear_marks, NULL);
+	marked = 1;
+
+	for_each_ref(rev_list_insert_ref_oid, NULL);
+	for_each_cached_alternate(insert_one_alternate_object);
+
+	if (!wants) {
 		packet_flush(fd[1]);
 		return 1;
 	}
+	strbuf_attach(&req_buf, wants, strlen(wants), strlen(wants));
 
 	if (is_repository_shallow())
 		write_shallow_commits(&req_buf, 1, NULL);
@@ -943,7 +951,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		packet_flush(fd[1]);
 		goto all_done;
 	}
-	if (find_common(args, fd, sha1, ref) < 0)
+	if (find_common(args, fd, sha1, get_wants(args, ref)) < 0)
 		if (!args->keep_pack)
 			/* When cloning, it is not unusual to have
 			 * no common commit.
-- 
2.12.2.715.g7642488e1d-goog

[RFC 3/4] fetch-pack: support new server endpoint

[Jonathan Tan]

This is a patch to demonstrate that fetch-pack only requires a small
amount of changes to support the new server endpoint, and that things
generally work in various situations, including both stateless RPC and
non-stateless RPC (as can be seen from the tests).

Some minor issues remain:
 - Names of refs should be treated as strings (when interfacing with the
   new endpoint is requested) instead of being parsed into struct ref
   unconditionally.
 - Capability management could probably be done better instead of
   checking for "--new-way" everywhere.
 - I'm not sure how to test the upload-pack code path that sends "ACK
   ready".

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Makefile               |   1 +
 builtin/fetch-pack.c   |  10 +-
 fetch-pack.c           |  75 +++++++++++----
 fetch-pack.h           |   1 +
 t/helper/.gitignore    |   1 +
 t/helper/test-un-pkt.c |  40 ++++++++
 t/t9999-mytests.sh     | 242 +++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 349 insertions(+), 21 deletions(-)
 create mode 100644 t/helper/test-un-pkt.c
 create mode 100644 t/t9999-mytests.sh

diff --git a/Makefile b/Makefile
index 0d3813772..0b4510d3f 100644
--- a/Makefile
+++ b/Makefile
@@ -641,6 +641,7 @@ TEST_PROGRAMS_NEED_X += test-string-list
 TEST_PROGRAMS_NEED_X += test-submodule-config
 TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
+TEST_PROGRAMS_NEED_X += test-un-pkt
 TEST_PROGRAMS_NEED_X += test-urlmatch-normalization
 TEST_PROGRAMS_NEED_X += test-wildmatch
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 2a1c1c213..4bd83c4ff 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -143,6 +143,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.update_shallow = 1;
 			continue;
 		}
+		if (!strcmp("--new-way", arg)) {
+			args.new_way = 1;
+			continue;
+		}
 		usage(fetch_pack_usage);
 	}
 	if (deepen_not.nr)
@@ -193,7 +197,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		if (!conn)
 			return args.diag_url ? 0 : 1;
 	}
-	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+	if (!args.new_way)
+		get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
 			 &shallow, pack_lockfile_ptr);
@@ -219,7 +224,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	 * remote no-such-ref' would silently succeed without issuing
 	 * an error.
 	 */
-	ret |= report_unmatched_refs(sought, nr_sought);
+	if (!args.new_way)
+		ret |= report_unmatched_refs(sought, nr_sought);
 
 	while (ref) {
 		printf("%s %s\n",
diff --git a/fetch-pack.c b/fetch-pack.c
index 74771a283..4cbdada7b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -251,7 +251,7 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)
 	}
 }
 
-static enum ack_type get_ack(int fd, unsigned char *result_sha1)
+static enum ack_type get_ack(int fd, unsigned char *result_sha1, struct ref **ref, struct sha1_array *shallow)
 {
 	int len;
 	char *line = packet_read_line(fd, &len);
@@ -261,6 +261,23 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 		die(_("git fetch-pack: expected ACK/NAK, got EOF"));
 	if (!strcmp(line, "NAK"))
 		return NAK;
+	if (ref && skip_prefix(line, "wanted ", &arg)) {
+		struct object_id oid;
+		if (!get_sha1_hex(arg, oid.hash) && arg[40] == ' ' && arg[41]) {
+			struct ref *new_ref = alloc_ref(arg + 41);
+			oidcpy(&new_ref->old_oid, &oid);
+			new_ref->next = *ref;
+			*ref = new_ref;
+			return get_ack(fd, result_sha1, ref, shallow);
+		}
+	}
+	if (shallow && skip_prefix(line, "shallow ", &arg)) {
+		struct object_id oid;
+		if (!get_sha1_hex(arg, oid.hash)) {
+			sha1_array_append(shallow, oid.hash);
+			return get_ack(fd, result_sha1, ref, shallow);
+		}
+	}
 	if (skip_prefix(line, "ACK ", &arg)) {
 		if (!get_sha1_hex(arg, result_sha1)) {
 			arg += 40;
@@ -370,7 +387,7 @@ static char *get_wants(const struct fetch_pack_args *args, struct ref *refs)
 
 static int find_common(struct fetch_pack_args *args,
 		       int fd[2], unsigned char *result_sha1,
-		       char *wants)
+		       char *wants, struct ref **ref, struct sha1_array *shallow)
 {
 	int count = 0, flushes = 0, flush_at = INITIAL_FLUSH, retval;
 	const unsigned char *sha1;
@@ -475,7 +492,7 @@ static int find_common(struct fetch_pack_args *args,
 
 			consume_shallow_list(args, fd[0]);
 			do {
-				ack = get_ack(fd[0], result_sha1);
+				ack = get_ack(fd[0], result_sha1, NULL, NULL);
 				if (ack)
 					print_verbose(args, _("got %s %d %s"), "ack",
 						      ack, sha1_to_hex(result_sha1));
@@ -544,7 +561,7 @@ static int find_common(struct fetch_pack_args *args,
 	if (!got_ready || !no_done)
 		consume_shallow_list(args, fd[0]);
 	while (flushes || multi_ack) {
-		int ack = get_ack(fd[0], result_sha1);
+		int ack = get_ack(fd[0], result_sha1, ref, shallow);
 		if (ack) {
 			print_verbose(args, _("got %s (%d) %s"), "ack",
 				      ack, sha1_to_hex(result_sha1));
@@ -878,22 +895,22 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 				 struct shallow_info *si,
 				 char **pack_lockfile)
 {
-	struct ref *ref = copy_ref_list(orig_ref);
+	struct ref *ref;
 	unsigned char sha1[20];
 	const char *agent_feature;
 	int agent_len;
+	int find_common_result;
 
-	sort_ref_list(&ref, ref_compare_name);
-	QSORT(sought, nr_sought, cmp_ref_by_name);
-
-	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
-		die(_("Server does not support shallow clients"));
+	if (!args->new_way) {
+		if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
+			die(_("Server does not support shallow clients"));
+	}
 	if (args->depth > 0 || args->deepen_since || args->deepen_not)
 		args->deepen = 1;
-	if (server_supports("multi_ack_detailed")) {
+	if (server_supports("multi_ack_detailed") || args->new_way) {
 		print_verbose(args, _("Server supports multi_ack_detailed"));
 		multi_ack = 2;
-		if (server_supports("no-done")) {
+		if (server_supports("no-done") || args->new_way) {
 			print_verbose(args, _("Server supports no-done"));
 			if (args->stateless_rpc)
 				no_done = 1;
@@ -936,22 +953,42 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 			print_verbose(args, _("Server version is %.*s"),
 				      agent_len, agent_feature);
 	}
-	if (server_supports("deepen-since"))
+	if (server_supports("deepen-since") || args->new_way)
 		deepen_since_ok = 1;
 	else if (args->deepen_since)
 		die(_("Server does not support --shallow-since"));
-	if (server_supports("deepen-not"))
+	if (server_supports("deepen-not") || args->new_way)
 		deepen_not_ok = 1;
 	else if (args->deepen_not)
 		die(_("Server does not support --shallow-exclude"));
 	if (!server_supports("deepen-relative") && args->deepen_relative)
 		die(_("Server does not support --deepen"));
 
-	if (everything_local(args, &ref, sought, nr_sought)) {
-		packet_flush(fd[1]);
-		goto all_done;
+	if (args->new_way) {
+		struct strbuf wants = STRBUF_INIT;
+		int i;
+		struct sha1_array shallow = SHA1_ARRAY_INIT;
+
+		ref = NULL;
+		multi_ack = 2;
+		use_sideband = 2;
+
+		packet_buf_write(&wants, "fetch-refs\n");
+		for (i = 0; i < nr_sought; i++)
+			packet_buf_write(&wants, "want %s\n", sought[i]->name);
+		find_common_result = find_common(args, fd, sha1, strbuf_detach(&wants, NULL), &ref, &shallow);
+		prepare_shallow_info(si, &shallow);
+	} else {
+		ref = copy_ref_list(orig_ref);
+		sort_ref_list(&ref, ref_compare_name);
+		QSORT(sought, nr_sought, cmp_ref_by_name);
+		if (everything_local(args, &ref, sought, nr_sought)) {
+			packet_flush(fd[1]);
+			goto all_done;
+		}
+		find_common_result = find_common(args, fd, sha1, get_wants(args, ref), NULL, NULL);
 	}
-	if (find_common(args, fd, sha1, get_wants(args, ref)) < 0)
+	if (find_common_result < 0)
 		if (!args->keep_pack)
 			/* When cloning, it is not unusual to have
 			 * no common commit.
@@ -1128,7 +1165,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 	if (nr_sought)
 		nr_sought = remove_duplicates_in_refs(sought, nr_sought);
 
-	if (!ref) {
+	if (!args->new_way && !ref) {
 		packet_flush(fd[1]);
 		die(_("no matching remote head"));
 	}
diff --git a/fetch-pack.h b/fetch-pack.h
index a2d46e6e7..ab7a80696 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -29,6 +29,7 @@ struct fetch_pack_args {
 	unsigned cloning:1;
 	unsigned update_shallow:1;
 	unsigned deepen:1;
+	unsigned new_way:1;
 };
 
 /*
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index d6e8b3679..cce5069cb 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -29,5 +29,6 @@
 /test-submodule-config
 /test-subprocess
 /test-svn-fe
+/test-un-pkt
 /test-urlmatch-normalization
 /test-wildmatch
diff --git a/t/helper/test-un-pkt.c b/t/helper/test-un-pkt.c
new file mode 100644
index 000000000..6dcf87980
--- /dev/null
+++ b/t/helper/test-un-pkt.c
@@ -0,0 +1,40 @@
+/*
+ * This program takes payloads in pkt-line format (one or more pkt-lines
+ * followed by a flush pkt) and runs the given command once per payload.
+ */
+#include "cache.h"
+#include "pkt-line.h"
+#include "run-command.h"
+
+int cmd_main(int argc, const char **argv)
+{
+	struct child_process *cmd = NULL;
+	char buffer[LARGE_PACKET_MAX];
+	int size;
+
+	while ((size = packet_read(0, NULL, NULL, buffer, sizeof(buffer),
+				   PACKET_READ_GENTLE_ON_EOF)) >= 0) {
+		if (size > 0) {
+			if (!cmd) {
+				cmd = xmalloc(sizeof(*cmd));
+				child_process_init(cmd);
+				cmd->argv = argv + 1;
+				cmd->git_cmd = 1;
+				cmd->in = -1;
+				cmd->out = 0;
+				if (start_command(cmd))
+					die("could not start command");
+			}
+			write_in_full(cmd->in, buffer, size);
+		} else if (cmd) {
+			close(cmd->in);
+			if (finish_command(cmd))
+				die("could not finish command");
+			free(cmd);
+			cmd = NULL;
+			if (size < 0)
+				break;
+		}
+	}
+	return 0;
+}
diff --git a/t/t9999-mytests.sh b/t/t9999-mytests.sh
new file mode 100644
index 000000000..9bb4e85e3
--- /dev/null
+++ b/t/t9999-mytests.sh
@@ -0,0 +1,242 @@
+#!/bin/sh
+
+test_description='my tests'
+
+. ./test-lib.sh
+
+test_expect_success 'fetch-pack --new-way basic' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+
+		git checkout -b one
+		test_commit 1 &&
+		git checkout master &&
+
+		git checkout -b two
+		test_commit 2 &&
+		git checkout master &&
+
+		git checkout -b dont_fetch_this
+		test_commit 3 &&
+		git checkout master
+	) &&
+	git fetch-pack --new-way --exec=git-server-endpoint server refs/heads/one refs/heads/two\* >actual &&
+
+	grep "$(printf "%s refs/heads/one" $(git -C server rev-parse --verify one))" actual &&
+	grep "$(printf "%s refs/heads/two" $(git -C server rev-parse --verify two))" actual &&
+	! grep dont_fetch_this actual
+'
+
+test_expect_success 'fetch-pack --new-way hideRefs' '
+	rm -rf server &&
+	git init server &&
+	test_config -C server transfer.hideRefs refs/heads/b2 &&
+	(
+		cd server &&
+		test_commit 0 &&
+
+		git checkout -b b1
+		test_commit 1 &&
+		git checkout master &&
+
+		git checkout -b b2
+		test_commit 2 &&
+		git checkout master
+	) &&
+	git fetch-pack --new-way --exec=git-server-endpoint server refs/heads/b\* >actual &&
+
+	grep "$(printf "%s refs/heads/b1" $(git -C server rev-parse --verify 1))" actual &&
+	! grep refs/heads/b2 actual
+'
+
+test_expect_success 'fetch-pack --new-way long negotiation' '
+	rm -rf server client &&
+	git init server &&
+	test_commit -C server 0 &&
+
+	git clone server client &&
+
+	test_commit -C server 1 &&
+
+	git -C client checkout -b sidebranch &&
+	for i in $(seq 2 32)
+	do
+		test_commit -C client $i
+	done &&
+
+	git -C client fetch-pack --new-way --exec=git-server-endpoint ../server refs/heads/master >actual &&
+
+	grep "$(printf "%s refs/heads/master" $(git -C server rev-parse --verify 1))" actual
+'
+
+test_expect_success 'fetch-pack --new-way with shallow client' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+		test_commit 1
+	) &&
+	rm -rf client &&
+	git clone --depth=1 "file://$(pwd)/server" client &&
+	(
+		cd server &&
+		git checkout -b two 0 &&
+		test_commit 2
+	) &&
+
+	# check that the shallow clone does not include this parent commit
+	test_must_fail git -C client cat-file -e $(git -C server rev-parse 0) &&
+
+	git -C client fetch-pack --new-way --exec=git-server-endpoint ../server refs/heads/two >actual &&
+	# 0 is the parent of 2, so it must be included now
+	git -C client cat-file -e $(git -C server rev-parse 0)
+'
+
+test_expect_success 'fetch-pack --new-way --depth' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0
+	) &&
+	rm -rf client &&
+	git clone server client &&
+	(
+		cd server &&
+		test_commit 1 &&
+		test_commit 2
+	) &&
+
+	git -C client fetch-pack --new-way --exec=git-server-endpoint --depth=1 ../server refs/heads/master >actual &&
+	git -C client cat-file -e $(git -C server rev-parse 2) &&
+	test_must_fail git -C client cat-file -e $(git -C server rev-parse 1)
+'
+
+test_expect_success 'fetch-pack --new-way --shallow-since' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0
+	) &&
+	rm -rf client &&
+	git clone server client &&
+	(
+		cd server &&
+		test_commit 1 &&
+		test_commit 2
+	) &&
+	DATE=$(git -C server log --format="format:%at" --no-walk 2) &&
+
+	git -C client fetch-pack --new-way --exec=git-server-endpoint --shallow-since=$DATE ../server refs/heads/master >actual &&
+	git -C client cat-file -e $(git -C server rev-parse 2) &&
+	test_must_fail git -C client cat-file -e $(git -C server rev-parse 1)
+'
+
+test_expect_success 'fetch-pack --new-way --shallow-exclude' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0
+	) &&
+	rm -rf client &&
+	git clone server client &&
+	(
+		cd server &&
+		test_commit 1 &&
+		test_commit 2
+	) &&
+
+	git -C client fetch-pack --new-way --exec=git-server-endpoint --shallow-exclude=1 ../server refs/heads/master >actual &&
+	git -C client cat-file -e $(git -C server rev-parse 2) &&
+	test_must_fail git -C client cat-file -e $(git -C server rev-parse 1)
+'
+
+test_expect_success PIPE 'fetch-pack --new-way --stateless-rpc' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+
+		git checkout -b one
+		test_commit 1 &&
+		git checkout master &&
+
+		git checkout -b two
+		test_commit 2 &&
+		git checkout master &&
+
+		git checkout -b dont_fetch_this
+		test_commit 3 &&
+		git checkout master
+	) &&
+	rm -rf client &&
+	git init client &&
+
+	mkfifo se-out &&
+
+	git -C client fetch-pack --new-way --stateless-rpc ../server refs/heads/one refs/heads/two\* <se-out | test-un-pkt server-endpoint server --stateless-rpc >se-out &&
+
+	git -C client cat-file -e $(git -C server rev-parse 1) &&
+	git -C client cat-file -e $(git -C server rev-parse 2) &&
+	test_must_fail git -C client cat-file -e $(git -C server rev-parse 3)
+'
+
+test_expect_success 'fetch-pack --new-way --stateless-rpc long negotiation' '
+	rm -rf server client &&
+	git init server &&
+	test_commit -C server 0 &&
+
+	git clone server client &&
+
+	test_commit -C server 1 &&
+
+	git -C client checkout -b sidebranch &&
+	for i in $(seq 2 32)
+	do
+		test_commit -C client $i
+	done &&
+
+	rm -f se-out &&
+	mkfifo se-out &&
+
+	git -C client fetch-pack --new-way --stateless-rpc ../server refs/heads/master <se-out | test-un-pkt server-endpoint server --stateless-rpc >se-out &&
+
+	git -C client cat-file -e $(git -C server rev-parse 1)
+'
+
+test_expect_success 'fetch-pack --new-way --stateless-rpc namespaces' '
+	rm -rf server &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+
+		git checkout -b mybranch
+		test_commit 1 &&
+		git checkout master &&
+
+		git checkout -b mybranch_ns
+		test_commit 2 &&
+		git checkout master &&
+		git update-ref refs/namespaces/ns/refs/heads/mybranch mybranch_ns
+	) &&
+	rm -rf client &&
+	git init client &&
+
+	rm -f se-out &&
+	mkfifo se-out &&
+
+	git -C client fetch-pack --new-way --stateless-rpc ../server refs/heads/mybranch <se-out | GIT_NAMESPACE=ns test-un-pkt server-endpoint server --stateless-rpc >se-out &&
+
+	git -C client cat-file -e $(git -C server rev-parse 2) &&
+	test_must_fail git -C client cat-file -e $(git -C server rev-parse 1)
+'
+
+test_done
-- 
2.12.2.715.g7642488e1d-goog

[RFC 4/4] server-endpoint: serve blobs by hash

[Jonathan Tan]

Upgrade server-endpoint to also serve blobs in a packfile given their
hashes.  Reachability checks are performed before the packfile is sent -
both an absent blob and an unreachable blob are reported to the user in
the same way ("not our blob").

Due to a bug in "rev-list" in the absence of bitmaps (discussed here
[1]), the server repositories in tests all have bitmaps.

[1] <20170309003547.6930-1-jonathantanmy@google.com>

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 server-endpoint.c          | 121 ++++++++++++++++++++++++++++++++++++++++++++-
 t/t5573-server-endpoint.sh |  60 ++++++++++++++++++++++
 2 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 t/t5573-server-endpoint.sh

diff --git a/server-endpoint.c b/server-endpoint.c
index a9c0c7c94..870b853a6 100644
--- a/server-endpoint.c
+++ b/server-endpoint.c
@@ -192,6 +192,123 @@ static int fetch_ref(int stateless_rpc)
 	return -1;
 }
 
+/*
+ * Returns 1 if all blobs are reachable. If not, returns 0 and stores the hash
+ * of one of the unreachable blobs in unreachable.
+ */
+static int are_all_reachable(const struct object_array *blobs, struct object_id *unreachable)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	static const char *argv[] = {
+		"rev-list", "--objects", "--use-bitmap-index", "--stdin", "--not", "--all", "--not", NULL,
+	};
+	int i;
+	char buf[41] = {0};
+
+	cmd.argv = argv;
+	cmd.git_cmd = 1;
+	cmd.in = -1;
+	cmd.out = -1;
+
+	if (start_command(&cmd))
+		goto error;
+	
+	for (i = 0; i < blobs->nr; i++) {
+		write_in_full(cmd.in, sha1_to_hex(blobs->objects[i].item->oid.hash), 40);
+		write_in_full(cmd.in, "\n", 1);
+	}
+	close(cmd.in);
+	cmd.in = -1;
+
+	i = read_in_full(cmd.out, buf, 40);
+	close(cmd.out);
+	cmd.out = -1;
+
+	if (finish_command(&cmd))
+		goto error;
+
+	if (i) {
+		if (get_oid_hex(buf, unreachable))
+			goto error;
+		return 0;
+	}
+
+	return 1;
+
+error:
+	if (cmd.out >= 0)
+		close(cmd.out);
+	die("problem with running rev-list");
+}
+
+static void send_blobs(const struct object_array *blobs)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	static const char *argv[] = {
+		"pack-objects", "--stdout", NULL
+	};
+	int i;
+
+	cmd.argv = argv;
+	cmd.git_cmd = 1;
+	cmd.in = -1;
+	cmd.out = 0;
+
+	if (start_command(&cmd))
+		goto error;
+	
+	for (i = 0; i < blobs->nr; i++) {
+		write_in_full(cmd.in, sha1_to_hex(blobs->objects[i].item->oid.hash), 40);
+		write_in_full(cmd.in, "\n", 1);
+	}
+	close(cmd.in);
+	cmd.in = -1;
+
+	if (finish_command(&cmd))
+		goto error;
+
+	return;
+
+error:
+	die("problem with running pack-objects");
+}
+
+static int fetch_blob(void)
+{
+	char *line;
+
+	struct object_array wanted_blobs = OBJECT_ARRAY_INIT;
+	struct object_id unreachable;
+
+	while ((line = packet_read_line(0, NULL))) {
+		const char *arg;
+		if (skip_prefix(line, "want ", &arg)) {
+			struct object_id oid;
+			struct object *obj;
+			if (get_oid_hex(arg, &oid)) {
+				packet_write_fmt(1, "ERR invalid object ID <%s>", arg);
+				return 0;
+			}
+			obj = parse_object(oid.hash);
+			if (!obj || obj->type != OBJ_BLOB) {
+				packet_write_fmt(1, "ERR not our blob <%s>", arg);
+				return 0;
+			}
+			add_object_array(obj, NULL, &wanted_blobs);
+		}
+	}
+
+	if (!are_all_reachable(&wanted_blobs, &unreachable)) {
+		packet_write_fmt(1, "ERR not our blob <%s>", oid_to_hex(&unreachable));
+		return 0;
+	}
+
+	packet_write_fmt(1, "ACK\n");
+	send_blobs(&wanted_blobs);
+
+	return 0;
+}
+
 static int server_endpoint_config(const char *var, const char *value, void *unused)
 {
 	return parse_hide_refs_config(var, value, "uploadpack");
@@ -224,5 +341,7 @@ int cmd_main(int argc, const char **argv)
 	line = packet_read_line(0, NULL);
 	if (!strcmp(line, "fetch-refs"))
 		return fetch_ref(stateless_rpc);
-	die("only fetch-refs is supported");
+	if (!strcmp(line, "fetch-blobs"))
+		return fetch_blob();
+	die("only fetch-refs and fetch-blobs are supported");
 }
diff --git a/t/t5573-server-endpoint.sh b/t/t5573-server-endpoint.sh
new file mode 100644
index 000000000..48f052851
--- /dev/null
+++ b/t/t5573-server-endpoint.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='server-endpoint'
+
+. ./test-lib.sh
+
+test_expect_success 'fetch-blobs basic' '
+	rm -rf server client &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+		test_commit 1 &&
+		git repack -a -d --write-bitmap-index
+	) &&
+	BLOB0=$(git hash-object server/0.t) &&
+	BLOB1=$(git hash-object server/1.t) &&
+	printf "000ffetch-blobs0031want %s0031want %s0000" "$BLOB0" "$BLOB1" | git server-endpoint server >out &&
+
+	test "$(head -1 out)" = "0008ACK" &&
+
+	git init client &&
+	sed 1d out | git -C client unpack-objects &&
+	git -C client cat-file -e "$BLOB0" &&
+	git -C client cat-file -e "$BLOB1"
+'
+
+test_expect_success 'fetch-blobs no such object' '
+	rm -rf server client &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+		git repack -a -d --write-bitmap-index
+	) &&
+	BLOB0=$(git hash-object server/0.t) &&
+	echo myblob >myblob &&
+	MYBLOB=$(git hash-object myblob) &&
+	printf "000ffetch-blobs0031want %s0031want %s0000" "$BLOB0" "$MYBLOB" | git server-endpoint server >out &&
+
+	test_i18ngrep "$(printf "ERR not our blob.*%s" "$MYBLOB")" out
+'
+
+test_expect_success 'fetch-blobs unreachable' '
+	rm -rf server client &&
+	git init server &&
+	(
+		cd server &&
+		test_commit 0 &&
+		git repack -a -d --write-bitmap-index
+	) &&
+	BLOB0=$(git hash-object server/0.t) &&
+	echo myblob >myblob &&
+	MYBLOB=$(git -C server hash-object -w ../myblob) &&
+	printf "000ffetch-blobs0031want %s0031want %s0000" "$BLOB0" "$MYBLOB" | git server-endpoint server >out &&
+
+	test_i18ngrep "$(printf "ERR not our blob.*%s" "$MYBLOB")" out
+'
+
+test_done
-- 
2.12.2.715.g7642488e1d-goog

Re: [RFC 0/4] Implementation of fetch-blobs and fetch-refs

[Stefan Beller]

Sorry for not paying attention to this patch set earlier. I missed
(and might still
miss) the big picture(tm) here. This is because
http://public-inbox.org/git/ffd92ad9-39fe-c76b-178d-6e3d6a425037@google.com/
seemed to be specific for big binary files, but ...

On Mon, Apr 10, 2017 at 1:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

> In particular, patch 1 demonstrates that a new server endpoint that
> serves refs without the initial ref advertisement can be done in 228
> lines of code (according to "git diff --stat") while accounting for the
> various special cases that upload-pack imposes (stateless RPC, inclusion
> of an additional response when depth is requested, handling of "done" in
> request, sending a packfile directly after a response containing "ACK
> ready" without waiting for "done", and so on).

... from looking at the patches, this actually implements the idea
that was thrown around on the mailing list as "client speaks first",
as this essentially omits the refs advertisement, and then continues the
conversation by running upload-pack as normal.

That seems very exciting to me!

> To that end, I'm sending these patches in the hope of showing that these
> features are useful (omitting ref advertisements help greatly when
> serving large repos, as described in the commit message of patch 1, and
> serving blobs is useful for any fetch-blob-on-demand repo scheme) and
> that my proposed way of implementing them can be done in a relatively
> uncomplicated manner (as seen in these patches).

Yes, if we encapsulate v1 protocol on (both?) sides, we seem to need
very little additional code, but get the benefit of using lots of well
tested code.

Thanks,
Stefan