Bundle URIs IV: advertise over protocol v2

This is based on the recent master batch that included ds/bundle-uri-....

Now that git clone --bundle-uri can download a bundle list from a plaintex file in config format, we can use the same set of key-value pairs to advertise a bundle list over protocol v2. At the end of this series:

1. A server can advertise bundles when uploadPack.advertiseBundleURIs is enabled. The bundle list comes from the server's local config, specifically the bundle.* namespace.
2. A client can notice a server's bundle-uri advertisement and request the bundle list if transfer.bundleURI is enabled. The bundles are downloaded as if the list was advertised from the --bundle-uri option.

Many patches in this series were adapted from Ævar's v2 RFC [1]. He is retained as author and I added myself as co-author only if the modifications were significant.

[1]
https://lore.kernel.org/git/[email protected]/

* Patches 1-5 are mostly taken from [1], again with mostly minor updates. The one major difference is the packet line format being a single key=value format instead of a sequence of pairs. This also means that Patch 4 is entirely new since it feeds these pairs directly from the server's config.

* Patches 6-9 finish off the ability for the client to notice the capability, request the values, and download bundles before continuing with the rest of the download.

One thing that is not handled here but could be handled in a future change is to disconnect from the origin Git server while downloading the bundle URIs, then reconnecting afterwards. This does not make any difference for HTTPS, but SSH may benefit from the reduced connection time. The git clone --bundle-uri option did not suffer from this because the bundles are downloaded before the server connection begins.

After this series, there is one more before the original scope of the plan is complete: using creation tokens as a heuristic. See [2] for the RFC version of those patches.

[2] https://github.com/derrickstolee/git/pull/22

Thanks,

* Stolee

Derrick Stolee (5):
bundle-uri: serve bundle.* keys from config
strbuf: reintroduce strbuf_parent_directory()
bundle-uri: allow relative URLs in bundle lists
bundle-uri: download bundles from an advertised list
clone: unbundle the advertised bundles

Ævar Arnfjörð Bjarmason (4):
protocol v2: add server-side "bundle-uri" skeleton bundle-uri client: add minimal NOOP client bundle-uri client: add helper for testing server bundle-uri client: add boolean transfer.bundleURI setting

 Documentation/config/transfer.txt      |   6 +
 Documentation/gitprotocol-v2.txt       | 193 +++++++++++++++++++++
 builtin/clone.c                        |  23 +++
 bundle-uri.c                           |  91 +++++++++-
 bundle-uri.h                           |  27 +++
 connect.c                              |  47 +++++
 remote.h                               |   5 +
 serve.c                                |   6 +
 strbuf.c                               |   9 +
 strbuf.h                               |   7 +
 t/helper/test-bundle-uri.c             |  48 ++++++
 t/lib-t5730-protocol-v2-bundle-uri.sh  | 229 +++++++++++++++++++++++++
 t/t5601-clone.sh                       |  59 +++++++
 t/t5701-git-serve.sh                   |  40 ++++-
 t/t5730-protocol-v2-bundle-uri-file.sh |  36 ++++
 t/t5731-protocol-v2-bundle-uri-git.sh  |  17 ++
 t/t5732-protocol-v2-bundle-uri-http.sh |  17 ++
 t/t5750-bundle-uri-parse.sh            |  54 ++++++
 transport-helper.c                     |  13 ++
 transport-internal.h                   |   7 +
 transport.c                            |  87 ++++++++++
 transport.h                            |  23 +++
 22 files changed, 1042 insertions(+), 2 deletions(-)
 create mode 100644 t/lib-t5730-protocol-v2-bundle-uri.sh
 create mode 100755 t/t5730-protocol-v2-bundle-uri-file.sh
 create mode 100755 t/t5731-protocol-v2-bundle-uri-git.sh
 create mode 100755 t/t5732-protocol-v2-bundle-uri-http.sh

base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1400%2Fderrickstolee%2Fbundle-redo%2Fadvertise-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1400/derrickstolee/bundle-redo/advertise-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1400

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Add a skeleton server-side implementation of a new "bundle-uri" command to protocol v2. This will allow conforming clients to optionally seed their initial clones or incremental fetches from URLs containing "*.bundle" files created with "git bundle create".

This change only performs the basic boilerplate of advertising a new protocol v2 capability. The new 'bundle-uri' capability allows a client to request a list of bundles. Right now, the server only returns a flush packet, which corresponds to an empty advertisement. The bundle.* config namespace describes which key-value pairs will be communicated across this interface in future updates.

The critical bit right now is that the new boolean uploadPack.adverstiseBundleURIs config value signals whether or not this capability should be advertised at all.

An earlier version of this patch [1] used a different transfer format than the "key=value" pairs in the current implementation. The change was made to unify the protocol v2 verb with the bundle lists provided by independent bundle servers. Further, the standard allows for the server to advertise a URI that contains a bundle list. This allows users automatically discovering bundle providers that are loosely associated with the origin server, but without the origin server knowing exactly which bundles are currently available.

[1] https://lore.kernel.org/git/[email protected]/

The very-deep headings needed to be modified to stop at level 4 due to documentation build issues. These were not recognized in earlier builds since the file was previously in the Documentation/technical/ directory and was built in a different way. With its current location, the heavily-nested details were causing build issues and they are now replaced with a bulletted list of details.

---
 Documentation/gitprotocol-v2.txt | 193 +++++++++++++++++++++++++++++++
 bundle-uri.c                     |  36 ++++++
 bundle-uri.h                     |   7 ++
 serve.c                          |   6 +
 t/t5701-git-serve.sh             |  40 ++++++-
 5 files changed, 281 insertions(+), 1 deletion(-)

index 59bf41cefb9..57642b4a415 100644
@@ -578,6 +578,199 @@ and associated requested information, each separated by a single space.
 
 	obj-info = obj-id SP obj-size
 
+bundle-uri
+~~~~~~~~~~
+
+If the 'bundle-uri' capability is advertised, the server supports the
+`bundle-uri' command.
+
+The capability is currently advertised with no value (i.e. not
+"bundle-uri=somevalue"), a value may be added in the future for
+supporting command-wide extensions. Clients MUST ignore any unknown
+capability values and proceed with the 'bundle-uri` dialog they
+support.
+
+The 'bundle-uri' command is intended to be issued before `fetch` to
+get URIs to bundle files (see linkgit:git-bundle[1]) to "seed" and
+inform the subsequent `fetch` command.
+
+The client CAN issue `bundle-uri` before or after any other valid
+command. To be useful to clients it's expected that it'll be issued
+after an `ls-refs` and before `fetch`, but CAN be issued at any time
+in the dialog.
+
+DISCUSSION of bundle-uri
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+The intent of the feature is optimize for server resource consumption
+in the common case by changing the common case of fetching a very
+large PACK during linkgit:git-clone[1] into a smaller incremental
+fetch.
+
+It also allows servers to achieve better caching in combination with
+an `uploadpack.packObjectsHook` (see linkgit:git-config[1]).
+
+By having new clones or fetches be a more predictable and common
+negotiation against the tips of recently produces *.bundle file(s).
+Servers might even pre-generate the results of such negotiations for
+the `uploadpack.packObjectsHook` as new pushes come in.
+
+One way that servers could take advantage of these bundles is that the
+server would anticipate that fresh clones will download a known bundle,
+followed by catching up to the current state of the repository using ref
+tips found in that bundle (or bundles).
+
+PROTOCOL for bundle-uri
+^^^^^^^^^^^^^^^^^^^^^^^
+
+A `bundle-uri` request takes no arguments, and as noted above does not
+currently advertise a capability value. Both may be added in the
+future.
+
+When the client issues a `command=bundle-uri` the response is a list of
+key-value pairs provided as packet lines with value `<key>=<value>`. The
+meaning of these key-value pairs are provided by the config keys in the
+`bundle.*` namespace (see linkgit:git-config[1]).
+
+Clients are still expected to fully parse the line according to the
+above format, lines that do not conform to the format SHOULD be
+discarded. The user MAY be warned in such a case.
+
+bundle-uri CLIENT AND SERVER EXPECTATIONS
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+URI CONTENTS::
+The advertised URIs MUST be in one of two possible formats.
++
+The first possible format is a bundle file that `git bundle verify`
+would accept. I.e. they MUST contain one or more reference tips for
+use by the client, MUST indicate prerequisites (in any) with standard
+"-" prefixes, and MUST indicate their "object-format", if
+applicable. Create "*.bundle" files with `git bundle create`.
++
+The second possible format is a plaintext file that `git config --list`
+would accept (with the `--file` option). The key-value pairs in this list
+are in the `bundle.*` namespace (see linkgit:git-config[1]).
+
+bundle-uri CLIENT ERROR RECOVERY::
+A client MUST above all gracefully degrade on errors, whether that
+error is because of bad missing/data in the bundle URI(s), because
+that client is too dumb to e.g. understand and fully parse out bundle
+headers and their prerequisite relationships, or something else.
++
+Server operators should feel confident in turning on "bundle-uri" and
+not worry if e.g. their CDN goes down that clones or fetches will run
+into hard failures. Even if the server bundle bundle(s) are
+incomplete, or bad in some way the client should still end up with a
+functioning repository, just as if it had chosen not to use this
+protocol extension.
++
+All subsequent discussion on client and server interaction MUST keep
+this in mind.
+
+bundle-uri SERVER TO CLIENT::
+The ordering of the returned bundle uris is not significant. Clients
+MUST parse their headers to discover their contained OIDS and
+prerequisites. A client MUST consider the content of the bundle(s)
+themselves and their header as the ultimate source of truth.
++
+A server MAY even return bundle(s) that don't have any direct
+relationship to the repository being cloned (either through accident,
+or intentional "clever" configuration), and expect a client to sort
+out what data they'd like from the bundle(s), if any.
+
+bundle-uri CLIENT TO SERVER::
+The client SHOULD provide reference tips found in the bundle header(s)
+as 'have' lines in any subsequent `fetch` request. A client MAY also
+ignore the bundle(s) entirely if doing so is deemed worse for some
+reason, e.g. if the bundles can't be downloaded, it doesn't like the
+tips it finds etc.
+
+WHEN ADVERTISED BUNDLE(S) REQUIRE NO FURTHER NEGOTIATION::
+If after issuing `bundle-uri` and `ls-refs`, and getting the header(s)
+of the bundle(s) the client finds that the ref tips it wants can be
+retrieved entirety from advertised bundle(s), it MAY disconnect. The
+results of such a 'clone' or 'fetch' should be indistinguishable from
+the state attained without using bundle-uri.
+
+EARLY CLIENT DISCONNECTIONS AND ERROR RECOVERY::
+A client MAY perform an early disconnect while still downloading the
+bundle(s) (having streamed and parsed their headers). In such a case
+the client MUST gracefully recover from any errors related to
+finishing the download and validation of the bundle(s).
++
+I.e. a client might need to re-connect and issue a 'fetch' command,
+and possibly fall back to not making use of 'bundle-uri' at all.
++
+This "MAY" behavior is specified as such (and not a "SHOULD") on the
+assumption that a server advertising bundle uris is more likely than
+not to be serving up a relatively large repository, and to be pointing
+to URIs that have a good chance of being in working order. A client
+MAY e.g. look at the payload size of the bundles as a heuristic to see
+if an early disconnect is worth it, should falling back on a full
+"fetch" dialog be necessary.
+
+WHEN ADVERTISED BUNDLE(S) REQUIRE FURTHER NEGOTIATION::
+A client SHOULD commence a negotiation of a PACK from the server via
+the "fetch" command using the OID tips found in advertised bundles,
+even if's still in the process of downloading those bundle(s).
++
+This allows for aggressive early disconnects from any interactive
+server dialog. The client blindly trusts that the advertised OID tips
+are relevant, and issues them as 'have' lines, it then requests any
+tips it would like (usually from the "ls-refs" advertisement) via
+'want' lines. The server will then compute a (hopefully small) PACK
+with the expected difference between the tips from the bundle(s) and
+the data requested.
++
+The only connection the client then needs to keep active is to the
+concurrently downloading static bundle(s), when those and the
+incremental PACK are retrieved they should be inflated and
+validated. Any errors at this point should be gracefully recovered
+from, see above.
+
+bundle-uri PROTOCOL FEATURES
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+As noted above the `<key>=<value>` definitions are documented by the
+`bundle.*` config namespace.
+
+In particular, the `bundle.version` key specifies an integer value. The
+only accepted value at the moment is `1`, but if the client sees an
+unexpected value here then the client MUST ignore the bundle list.
+
+As long as `bundle.version` is understood, all other unknown keys MAY be
+ignored by the client. The server will guarantee compatibility with older
+clients, though newer clients may be better able to use the extra keys to
+minimize downloads.
+
+Any backwards-incompatible addition of pre-URI key-value will be
+guarded by a new `bundle.version` value or values in 'bundle-uri'
+capability advertisement itself, and/or by new future `bundle-uri`
+request arguments.
+
+Some example key-value pairs that are not currently implemented but could
+be implemented in the future include:
+
+ * Add a "hash=<val>" or "size=<bytes>" advertise the expected hash or
+   size of the bundle file.
+
+ * Advertise that one or more bundle files are the same (to e.g. have
+   clients round-robin or otherwise choose one of N possible files).
+
+ * A "oid=<OID>" shortcut and "prerequisite=<OID>" shortcut. For
+   expressing the common case of a bundle with one tip and no
+   prerequisites, or one tip and one prerequisite.
++
+This would allow for optimizing the common case of servers who'd like
+to provide one "big bundle" containing only their "main" branch,
+and/or incremental updates thereof.
++
+A client receiving such a a response MAY assume that they can skip
+retrieving the header from a bundle at the indicated URI, and thus
+save themselves and the server(s) the request(s) needed to inspect the
+headers of that bundle or bundles.
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 79a914f961b..32022595964 100644
@@ -563,6 +563,42 @@ cleanup:
 	return result;
 }
 
+/**
+ * API for serve.c.
+ */
+
+int bundle_uri_advertise(struct repository *r, struct strbuf *value)
+{
+	static int advertise_bundle_uri = -1;
+
+	if (advertise_bundle_uri != -1)
+		goto cached;
+
+	advertise_bundle_uri = 0;
+	git_config_get_maybe_bool("uploadpack.advertisebundleuris", &advertise_bundle_uri);
+
+cached:
+	return advertise_bundle_uri;
+}
+
+int bundle_uri_command(struct repository *r,
+		       struct packet_reader *request)
+{
+	struct packet_writer writer;
+	packet_writer_init(&writer, 1);
+
+	while (packet_reader_read(request) == PACKET_READ_NORMAL)
+		die(_("bundle-uri: unexpected argument: '%s'"), request->line);
+	if (request->status != PACKET_READ_FLUSH)
+		die(_("bundle-uri: expected flush after arguments"));
+
+	/* TODO: Implement the communication */
+
+	packet_writer_flush(&writer);
+
+	return 0;
+}
+
 /**
  * General API for {transport,connect}.c etc.
  */

index 4dbc269823c..357111ecce8 100644
@@ -4,6 +4,7 @@
 #include "hashmap.h"
 #include "strbuf.h"
 
+struct packet_reader;
 struct repository;
 struct string_list;
 

@@ -92,6 +93,12 @@ int bundle_uri_parse_config_format(const char *uri,
  */
 int fetch_bundle_uri(struct repository *r, const char *uri);
 
+/**
+ * API for serve.c.
+ */
+int bundle_uri_advertise(struct repository *r, struct strbuf *value);
+int bundle_uri_command(struct repository *r, struct packet_reader *request);
+
 /**
  * General API for {transport,connect}.c etc.
  */

index 733347f602a..cbf4a143cfe 100644
@@ -7,6 +7,7 @@
 #include "protocol-caps.h"
 #include "serve.h"
 #include "upload-pack.h"
+#include "bundle-uri.h"
 
 static int advertise_sid = -1;
 static int client_hash_algo = GIT_HASH_SHA1;

@@ -135,6 +136,11 @@ static struct protocol_capability capabilities[] = {
 		.advertise = always_advertise,
 		.command = cap_object_info,
 	},
+	{
+		.name = "bundle-uri",
+		.advertise = bundle_uri_advertise,
+		.command = bundle_uri_command,
+	},
 };
 
 void protocol_v2_advertise_capabilities(void)

index 1896f671cb3..f21e5e9d33d 100755
@@ -13,7 +13,7 @@ test_expect_success 'test capability advertisement' '
 	wrong_algo sha1:sha256
 	wrong_algo sha256:sha1
 	EOF
-	cat >expect <<-EOF &&
+	cat >expect.base <<-EOF &&
 	version 2
 	agent=git/$(git version | cut -d" " -f3)
 	ls-refs=unborn

@@ -21,8 +21,11 @@ test_expect_success 'test capability advertisement' '
 	server-option
 	object-format=$(test_oid algo)
 	object-info
+	EOF
+	cat >expect.trailer <<-EOF &&
 	0000
 	EOF
+	cat expect.base expect.trailer >expect &&
 
 	GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
 		--advertise-capabilities >out &&

@@ -342,4 +345,39 @@ test_expect_success 'basics of object-info' '
 	test_cmp expect actual
 '
 
+test_expect_success 'test capability advertisement with uploadpack.advertiseBundleURIs' '
+	test_config uploadpack.advertiseBundleURIs true &&
+
+	cat >expect.extra <<-EOF &&
+	bundle-uri
+	EOF
+	cat expect.base \
+	    expect.extra \
+	    expect.trailer >expect &&
+
+	GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
+		--advertise-capabilities >out &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'basics of bundle-uri: dies if not enabled' '
+	test-tool pkt-line pack >in <<-EOF &&
+	command=bundle-uri
+	0000
+	EOF
+
+	cat >err.expect <<-\EOF &&
+	fatal: invalid command '"'"'bundle-uri'"'"'
+	EOF
+
+	cat >expect <<-\EOF &&
+	ERR serve: invalid command '"'"'bundle-uri'"'"'
+	EOF
+
+	test_must_fail test-tool serve-v2 --stateless-rpc <in >out 2>err.actual &&
+	test_cmp err.expect err.actual &&
+	test_must_be_empty out
+'
+
 test_done

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Set up all the needed client parts of the "bundle-uri" protocol extension, without actually doing anything with the bundle URIs.

I.e. if the server says it supports "bundle-uri" we'll issue a command=bundle-uri after command=ls-refs when we're cloning. We'll parse the returned output using the code already tested for in t5750-bundle-uri-parse.sh.

What we aren't doing is actually acting on that data, i.e. downloading the bundle(s) before we get to doing the command=fetch, and adjusting our negotiation dialog appropriately. I'll do that in subsequent commits.

There's a question of what level of encapsulation we should use here, I've opted to use connect.h in clone.c, but we could also e.g. make transport_get_remote_refs() invoke this, i.e. make it implicitly get the bundle-uri list for later steps.

This approach means that we don't "support" this in "git fetch" for now. I'm starting with the case of initial clones, although as noted in preceding commits to the protocol documentation nothing about this approach precludes getting bundles on incremental fetches.

For the t5732-protocol-v2-bundle-uri-http.sh it's not easy to set environment variables for git-upload-pack (it's started by Apache), so let's skip the test under T5730_HTTP, and add unused T5730_{FILE,GIT} prerequisites for consistency and future use.

---
 builtin/clone.c                        |   7 ++
 bundle-uri.c                           |   4 +
 connect.c                              |  47 ++++++++
 remote.h                               |   5 +
 t/lib-t5730-protocol-v2-bundle-uri.sh  | 148 +++++++++++++++++++++++++
 t/t5730-protocol-v2-bundle-uri-file.sh |  36 ++++++
 t/t5731-protocol-v2-bundle-uri-git.sh  |  17 +++
 t/t5732-protocol-v2-bundle-uri-http.sh |  17 +++
 transport-helper.c                     |  13 +++
 transport-internal.h                   |   7 ++
 transport.c                            |  55 +++++++++
 transport.h                            |  19 ++++
 12 files changed, 375 insertions(+)
 create mode 100644 t/lib-t5730-protocol-v2-bundle-uri.sh
 create mode 100755 t/t5730-protocol-v2-bundle-uri-file.sh
 create mode 100755 t/t5731-protocol-v2-bundle-uri-git.sh
 create mode 100755 t/t5732-protocol-v2-bundle-uri-http.sh

index 547d6464b3c..edf98295af2 100644
@@ -27,6 +27,7 @@
 #include "iterator.h"
 #include "sigchain.h"
 #include "branch.h"
+#include "connect.h"
 #include "remote.h"
 #include "run-command.h"
 #include "connected.h"

@@ -1266,6 +1267,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (refs)
 		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
 
+	/*
+	 * Populate transport->got_remote_bundle_uri and
+	 * transport->bundle_uri. We might get nothing.
+	 */
+	transport_get_remote_bundle_uri(transport);
+
 	if (mapped_refs) {
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
 

index 32022595964..2201b604b11 100644
@@ -571,6 +571,10 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)
 {
 	static int advertise_bundle_uri = -1;
 
+	if (value &&
+	    git_env_bool("GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE", 0))
+		strbuf_addstr(value, "test-unknown-capability-value");
+
 	if (advertise_bundle_uri != -1)
 		goto cached;
 

index 1c4621b414b..1ebbe42792e 100644
@@ -234,6 +234,11 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 			     const struct string_list *server_options,
 			     int stateless_rpc);
 
+/* Used for protocol v2 in order to retrieve refs from a remote */
+struct bundle_list;
+int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
+			  struct bundle_list *bundles, int stateless_rpc);
+
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 
 /*

new file mode 100755
index 00000000000..89203d3a23c
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description="Test bundle-uri with protocol v2 and 'file://' transport"
+
+TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# Test protocol v2 with 'file://' transport
+#
+T5730_PROTOCOL=file
+. "$TEST_DIRECTORY"/lib-t5730-protocol-v2-bundle-uri.sh
+
+test_expect_success "unknown capability value with $T5730_PROTOCOL:// using protocol v2" '
+	test_when_finished "rm -f log" &&
+
+	test_config -C "$T5730_PARENT" \
+		uploadpack.bundleURI "$T5730_BUNDLE_URI_ESCAPED" &&
+
+	GIT_TRACE_PACKET="$PWD/log" \
+	GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE=true \
+	git \
+		-c protocol.version=2 \
+		ls-remote --symref "$T5730_URI" \
+		>actual 2>err &&
+
+	# Server responded using protocol v2
+	grep "< version 2" log &&
+
+	grep "> bundle-uri=test-unknown-capability-value" log
+'
+
+test_done

new file mode 100755
index 00000000000..282847b311f
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description="Test bundle-uri with protocol v2 and 'git://' transport"
+
+TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# Test protocol v2 with 'git://' transport
+#
+T5730_PROTOCOL=git
+. "$TEST_DIRECTORY"/lib-t5730-protocol-v2-bundle-uri.sh
+
+test_done

new file mode 100755
index 00000000000..fcc1cf3faef
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description="Test bundle-uri with protocol v2 and 'git://' transport"
+
+TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# Test protocol v2 with 'git://' transport
+#
+T5730_PROTOCOL=http
+. "$TEST_DIRECTORY"/lib-t5730-protocol-v2-bundle-uri.sh
+
+test_done

index e95267a4ab5..3ea7c2bb5ad 100644
@@ -1267,9 +1267,22 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
 	return ret;
 }
 
+static int get_bundle_uri(struct transport *transport)
+{
+	get_helper(transport);
+
+	if (process_connect(transport, 0)) {
+		do_take_over(transport);
+		return transport->vtable->get_bundle_uri(transport);
+	}
+
+	return -1;
+}
+
 static struct transport_vtable vtable = {
 	.set_option	= set_helper_option,
 	.get_refs_list	= get_refs_list,
+	.get_bundle_uri = get_bundle_uri,
 	.fetch_refs	= fetch_refs,
 	.push_refs	= push_refs,
 	.connect	= connect_helper,

index c4ca0b733ac..90ea749e5cf 100644
@@ -26,6 +26,13 @@ struct transport_vtable {
 	struct ref *(*get_refs_list)(struct transport *transport, int for_push,
 				     struct transport_ls_refs_options *transport_options);
 
+	/**
+	 * Populates the remote side's bundle-uri under protocol v2,
+	 * if the "bundle-uri" capability was advertised. Returns 0 if
+	 * OK, negative values on error.
+	 */
+	int (*get_bundle_uri)(struct transport *transport);
+
 	/**
 	 * Fetch the objects for the given refs. Note that this gets
 	 * an array, and should ignore the list structure.

index b5bf7b3e704..85150f504fb 100644
@@ -62,6 +62,7 @@ enum transport_family {
 	TRANSPORT_FAMILY_IPV6
 };
 
+struct bundle_list;
 struct transport {
 	const struct transport_vtable *vtable;
 

@@ -76,6 +77,18 @@ struct transport {
 	 */
 	unsigned got_remote_refs : 1;
 
+	/**
+	 * Indicates whether we already called get_bundle_uri_list(); set by
+	 * transport.c::transport_get_remote_bundle_uri().
+	 */
+	unsigned got_remote_bundle_uri : 1;
+
+	/*
+	 * The results of "command=bundle-uri", if both sides support
+	 * the "bundle-uri" capability.
+	 */
+	struct bundle_list *bundles;
+
 	/*
 	 * Transports that call take-over destroys the data specific to
 	 * the transport type while doing so, and cannot be reused.

@@ -281,6 +294,12 @@ void transport_ls_refs_options_release(struct transport_ls_refs_options *opts);
 const struct ref *transport_get_remote_refs(struct transport *transport,
 					    struct transport_ls_refs_options *transport_options);
 
+/**
+ * Retrieve bundle URI(s) from a remote. Populates "struct
+ * transport"'s "bundle_uri" and "got_remote_bundle_uri".
+ */
+int transport_get_remote_bundle_uri(struct transport *transport);
+
 /*
  * Fetch the hash algorithm used by a remote.
  *

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Add a 'test-tool bundle-uri ls-remote' command. This is a thin wrapper for issuing protocol v2 "bundle-uri" commands to a server, and to the parsing routines in bundle-uri.c.

Since in the "git clone" case we'll have already done the handshake(), but not here, introduce a "got_advertisement" state along with "got_remote_heads". It seems to me that the "got_remote_heads" is badly named in the first place, and the whole logic of eagerly getting ls-refs on handshake() or not could be refactored somewhat, but let's not do that now, and instead just add another self-documenting state variable.

---
 builtin/clone.c                       |  2 +-
 t/helper/test-bundle-uri.c            | 46 +++++++++++++++++++
 t/lib-t5730-protocol-v2-bundle-uri.sh | 63 ++++++++++++++++++++++-----
 transport.c                           | 43 ++++++++++++++----
 transport.h                           |  6 ++-
 5 files changed, 139 insertions(+), 21 deletions(-)

index edf98295af2..22b1e506452 100644
@@ -1271,7 +1271,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 * Populate transport->got_remote_bundle_uri and
 	 * transport->bundle_uri. We might get nothing.
 	 */
-	transport_get_remote_bundle_uri(transport);
+	transport_get_remote_bundle_uri(transport, 1);
 
 	if (mapped_refs) {
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));

index 85150f504fb..dd0115b83bf 100644
@@ -297,8 +297,12 @@ const struct ref *transport_get_remote_refs(struct transport *transport,
 /**
  * Retrieve bundle URI(s) from a remote. Populates "struct
  * transport"'s "bundle_uri" and "got_remote_bundle_uri".
+ *
+ * With `quiet=1` it will not complain if the serve doesn't support
+ * the protocol, but only if we discover the server uses it, and
+ * encounter issues then.
  */
-int transport_get_remote_bundle_uri(struct transport *transport);
+int transport_get_remote_bundle_uri(struct transport *transport, int quiet);
 
 /*
  * Fetch the hash algorithm used by a remote.

gitgitgadget

From: Derrick Stolee <[email protected]>

Implement the "bundle-uri" protocol v2 capability by populating the key=value packet lines from the local Git config. The list of bundles is provided from the keys beginning with "bundle.".

In the future, we may want to filter this list to be more specific to the exact known keys that the server intends to share, but for flexibility at the moment we will assume that the config values are well-formed.

---
 bundle-uri.c                          | 16 +++++++++++-
 t/lib-t5730-protocol-v2-bundle-uri.sh | 35 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

index 2201b604b11..3469f1aaa98 100644
@@ -585,6 +585,16 @@ cached:
 	return advertise_bundle_uri;
 }
 
+static int config_to_packet_line(const char *key, const char *value, void *data)
+{
+	struct packet_reader *writer = data;
+
+	if (!strncmp(key, "bundle.", 7))
+		packet_write_fmt(writer->fd, "%s=%s", key, value);
+
+	return 0;
+}
+
 int bundle_uri_command(struct repository *r,
 		       struct packet_reader *request)
 {

@@ -596,7 +606,11 @@ int bundle_uri_command(struct repository *r,
 	if (request->status != PACKET_READ_FLUSH)
 		die(_("bundle-uri: expected flush after arguments"));
 
-	/* TODO: Implement the communication */
+	/*
+	 * Read all "bundle.*" config lines to the client as key=value
+	 * packet lines.
+	 */
+	git_config(config_to_packet_line, &writer);
 
 	packet_writer_flush(&writer);
 

index c327544641b..000fcc5e20b 100644
@@ -158,6 +158,8 @@ test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2" '
 	[bundle]
 		version = 1
 		mode = all
+	[bundle "only"]
+		uri = $T5730_BUNDLE_URI_ESCAPED
 	EOF
 	GIT_TRACE_PACKET="$PWD/log" \
 	test-tool bundle-uri \

@@ -181,6 +183,39 @@ test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2 and ext
 	[bundle]
 		version = 1
 		mode = all
+	[bundle "only"]
+		uri = $T5730_BUNDLE_URI_ESCAPED
+	EOF
+	GIT_TRACE_PACKET="$PWD/log" \
+	test-tool bundle-uri \
+		ls-remote \
+		"$T5730_URI" \
+		>actual &&
+	test_cmp_config_output expect actual
+'
+
+
+test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2 with list" '
+	test_when_finished "rm -f log" &&
+
+	test_config -C "$T5730_PARENT" \
+		bundle.bundle1.uri "$T5730_BUNDLE_URI_ESCAPED-1.bdl" &&
+	test_config -C "$T5730_PARENT" \
+		bundle.bundle2.uri "$T5730_BUNDLE_URI_ESCAPED-2.bdl" &&
+	test_config -C "$T5730_PARENT" \
+		bundle.bundle3.uri "$T5730_BUNDLE_URI_ESCAPED-3.bdl" &&
+
+	# All data about bundle URIs
+	cat >expect <<-EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	[bundle "bundle1"]
+		uri = $T5730_BUNDLE_URI_ESCAPED-1.bdl
+	[bundle "bundle2"]
+		uri = $T5730_BUNDLE_URI_ESCAPED-2.bdl
+	[bundle "bundle3"]
+		uri = $T5730_BUNDLE_URI_ESCAPED-3.bdl
 	EOF
 	GIT_TRACE_PACKET="$PWD/log" \
 	test-tool bundle-uri \

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

The yet-to-be introduced client support for bundle-uri will always fall back on a full clone, but we'd still like to be able to ignore a server's bundle-uri advertisement entirely.

The new transfer.bundleURI config option defaults to 'false', but a user can set it to 'true' to enable checking for bundle URIs from the origin Git server using protocol v2.

To enable this setting by default in the correct tests, add a GIT_TEST_BUNDLE_URI environment variable.

---
 Documentation/config/transfer.txt     |  6 ++++++
 t/lib-t5730-protocol-v2-bundle-uri.sh |  3 +++
 transport.c                           | 10 +++++++---
 3 files changed, 16 insertions(+), 3 deletions(-)

index 264812cca4d..c3ac767d1e4 100644
@@ -115,3 +115,9 @@ transfer.unpackLimit::
 transfer.advertiseSID::
 	Boolean. When true, client and server processes will advertise their
 	unique session IDs to their remote counterpart. Defaults to false.
+
+transfer.bundleURI::
+	When `true`, local `git clone` commands will request bundle
+	information from the remote server (if advertised) and download
+	bundles before continuing the clone through the Git protocol.
+	Defaults to `false`.

index 000fcc5e20b..872bc39ad1b 100644
@@ -1,5 +1,8 @@
 # Included from t573*-protocol-v2-bundle-uri-*.sh
 
+GIT_TEST_BUNDLE_URI=1
+export GIT_TEST_BUNDLE_URI
+
 T5730_PARENT=
 T5730_URI=
 T5730_BUNDLE_URI=

index 86460f5be28..b33180226ae 100644
@@ -1538,6 +1538,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
 
 int transport_get_remote_bundle_uri(struct transport *transport, int quiet)
 {
+	int value = 0;
 	const struct transport_vtable *vtable = transport->vtable;
 
 	/* Check config only once. */

@@ -1545,10 +1546,13 @@ int transport_get_remote_bundle_uri(struct transport *transport, int quiet)
 		return 0;
 
 	/*
-	 * This is intentionally below the transport.injectBundleURI,
-	 * we want to be able to inject into protocol v0, or into the
-	 * dialog of a server who doesn't support this.
+	 * Don't use bundle-uri at all, if configured not to. Only proceed
+	 * if GIT_TEST_BUNDLE_URI=1 or transfer.bundleURI=true.
 	 */
+	if (!git_env_bool("GIT_TEST_BUNDLE_URI", 0) &&
+	    (git_config_get_bool("transfer.bundleuri", &value) || !value))
+		return 0;
+
 	if (!vtable->get_bundle_uri) {
 		if (quiet)
 			return -1;

gitgitgadget

From: Derrick Stolee <[email protected]>

The strbuf_parent_directory() method was added as a static method in contrib/scalar by d0feac4e8c0 (scalar: 'register' sets recommended config and starts maintenance, 2021-12-03) and then removed in 65f6a9eb0b9 (scalar: constrain enlistment search, 2022-08-18), but now there is a need for a similar method in the bundle URI feature.

Re-add the method, this time in strbuf.c. The method requirements are slightly modified to allow a trailing slash, in which case nothing is done. The return value is the number of byte removed.

---
 strbuf.c | 9 +++++++++
 strbuf.h | 7 +++++++
 2 files changed, 16 insertions(+)

index 0890b1405c5..b5cb324c431 100644
@@ -1200,3 +1200,12 @@ int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 	free(path2);
 	return res;
 }
+
+size_t strbuf_parent_directory(struct strbuf *buf)
+{
+	size_t len = buf->len;
+	size_t offset = offset_1st_component(buf->buf);
+	char *path_sep = find_last_dir_sep(buf->buf + offset);
+	strbuf_setlen(buf, path_sep ? path_sep - buf->buf : offset);
+	return len - buf->len;
+}

index 76965a17d44..8a964a08c31 100644
@@ -664,6 +664,13 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
 int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 			      const char *const *env);
 
+/*
+ * Remove the deepest subdirectory in the provided path string. If path
+ * contains a trailing separator, then the path is considered a directory
+ * and nothing is modified.
+ */
+size_t strbuf_parent_directory(struct strbuf *buf);
+
 void strbuf_add_lines(struct strbuf *sb,
 		      const char *prefix,
 		      const char *buf,

gitgitgadget

From: Derrick Stolee <[email protected]>

The logic in fetch_bundle_uri() is useful for the --bundle-uri option of 'git clone', but is not helpful when the clone operation discovers a list of URIs from the bundle-uri protocol v2 verb. To actually download and unbundle the advertised bundles, we need a different mechanism.

Create the new fetch_bundle_list() method which is very similar to fetch_bundle_uri() except that it relies on download_bundle_list() instead of fetch_bundle_uri_internal(). The download_bundle_list() method will recursively call fetch_bundle_uri_internal() if any of the advertised URIs serve a bundle list instead of a bundle. This will also follow the bundle.list.mode setting from the input list: "any" will download only one such URI while "all" will download data from all of the URIs.

In an identical way to fetch_bundle_uri(), the bundles are unbundled after all of the bundle lists have been expanded and all necessary URIs.

---
 bundle-uri.c | 21 +++++++++++++++++++++
 bundle-uri.h | 11 +++++++++++
 2 files changed, 32 insertions(+)

index 0f3902bbd2b..0b0081ed269 100644
@@ -577,6 +577,27 @@ cleanup:
 	return result;
 }
 
+int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)
+{
+	int result;
+	struct bundle_list global_list;
+
+	init_bundle_list(&global_list);
+
+	/* If a bundle is added to this global list, then it is required. */
+	global_list.mode = BUNDLE_MODE_ALL;
+
+	if ((result = download_bundle_list(r, list, &global_list, 0)))
+		goto cleanup;
+
+	result = unbundle_all_bundles(r, &global_list);
+
+cleanup:
+	for_all_bundles_in_list(&global_list, unlink_bundle, NULL);
+	clear_bundle_list(&global_list);
+	return result;
+}
+
 /**
  * API for serve.c.
  */

index 7905e56732c..a75b68d2f5a 100644
@@ -102,6 +102,17 @@ int bundle_uri_parse_config_format(const char *uri,
  */
 int fetch_bundle_uri(struct repository *r, const char *uri);
 
+/**
+ * Given a bundle list that was already advertised (likely by the
+ * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
+ * bundles according to the bundle strategy of that list.
+ *
+ * Returns non-zero if no bundle information is found at the given 'uri'.
+ */
+int fetch_bundle_list(struct repository *r,
+		      const char *uri,
+		      struct bundle_list *list);
+
 /**
  * API for serve.c.
  */

gitgitgadget

From: Derrick Stolee <[email protected]>

Bundle providers may want to distribute that data across multiple CDNs. This might require a change in the base URI, all the way to the domain name. If all bundles require an absolute URI in their 'uri' value, then every push to a CDN would require altering the table of contents to match the expected domain and exact location within it.

Allow a bundle list to specify a relative URI for the bundles. This allows easier distribution of bundle data.

---
 bundle-uri.c                | 16 ++++++++++-
 bundle-uri.h                |  9 +++++++
 t/helper/test-bundle-uri.c  |  2 ++
 t/t5750-bundle-uri-parse.sh | 54 +++++++++++++++++++++++++++++++++++++
 transport.c                 |  3 +++
 5 files changed, 83 insertions(+), 1 deletion(-)

index 3469f1aaa98..0f3902bbd2b 100644
@@ -7,6 +7,7 @@
 #include "hashmap.h"
 #include "pkt-line.h"
 #include "config.h"
+#include "remote.h"
 
 static int compare_bundles(const void *hashmap_cmp_fn_data,
 			   const struct hashmap_entry *he1,

@@ -49,6 +50,7 @@ void clear_bundle_list(struct bundle_list *list)
 
 	for_all_bundles_in_list(list, clear_remote_bundle_info, NULL);
 	hashmap_clear_and_free(&list->bundles, struct remote_bundle_info, ent);
+	free(list->baseURI);
 }
 
 int for_all_bundles_in_list(struct bundle_list *list,

@@ -163,7 +165,7 @@ static int bundle_list_update(const char *key, const char *value,
 	if (!strcmp(subkey, "uri")) {
 		if (bundle->uri)
 			return -1;
-		bundle->uri = xstrdup(value);
+		bundle->uri = relative_url(list->baseURI, value, NULL);
 		return 0;
 	}
 

@@ -190,6 +192,18 @@ int bundle_uri_parse_config_format(const char *uri,
 		.error_action = CONFIG_ERROR_ERROR,
 	};
 
+	if (!list->baseURI) {
+		struct strbuf baseURI = STRBUF_INIT;
+		strbuf_addstr(&baseURI, uri);
+
+		/*
+		 * If the URI does not end with a trailing slash, then
+		 * remove the filename portion of the path. This is
+		 * important for relative URIs.
+		 */
+		strbuf_parent_directory(&baseURI);
+		list->baseURI = strbuf_detach(&baseURI, NULL);
+	}
 	result = git_config_from_file_with_options(config_to_bundle_list,
 						   filename, list,
 						   &opts);

index 357111ecce8..7905e56732c 100644
@@ -61,6 +61,15 @@ struct bundle_list {
 	int version;
 	enum bundle_list_mode mode;
 	struct hashmap bundles;
+
+	/**
+	 * The baseURI of a bundle_list is used as the base for any
+	 * relative URIs advertised by the bundle list at that location.
+	 *
+	 * When the list is generated from a Git server, then use that
+	 * server's location.
+	 */
+	char *baseURI;
 };
 
 void init_bundle_list(struct bundle_list *list);

index ffb975b7b4f..5aa0b494ce3 100644
@@ -40,6 +40,8 @@ static int cmd__bundle_uri_parse(int argc, const char **argv, enum input_mode mo
 
 	init_bundle_list(&list);
 
+	list.baseURI = xstrdup("<uri>");
+
 	switch (mode) {
 	case KEY_VALUE_PAIRS:
 		if (argc != 1)

index b33180226ae..2c4ff0c2023 100644
@@ -1553,6 +1553,9 @@ int transport_get_remote_bundle_uri(struct transport *transport, int quiet)
 	    (git_config_get_bool("transfer.bundleuri", &value) || !value))
 		return 0;
 
+	if (!transport->bundles->baseURI)
+		transport->bundles->baseURI = xstrdup(transport->url);
+
 	if (!vtable->get_bundle_uri) {
 		if (quiet)
 			return -1;

gitgitgadget

From: Derrick Stolee <[email protected]>

A previous change introduced the transport methods to acquire a bundle list from the 'bundle-uri' protocol v2 verb, when advertised _and_ when the client has chosen to enable the feature.

Teach Git to download and unbundle the data advertised by those bundles during 'git clone'.

Also, since the --bundle-uri option exists, we do not want to mix the advertised bundles with the user-specified bundles.

---
 builtin/clone.c  | 26 +++++++++++++++++----
 t/t5601-clone.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 5 deletions(-)

index 22b1e506452..09f10477ed6 100644
@@ -1267,11 +1267,27 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (refs)
 		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
 
-	/*
-	 * Populate transport->got_remote_bundle_uri and
-	 * transport->bundle_uri. We might get nothing.
-	 */
-	transport_get_remote_bundle_uri(transport, 1);
+	if (!bundle_uri) {
+		/*
+		* Populate transport->got_remote_bundle_uri and
+		* transport->bundle_uri. We might get nothing.
+		*/
+		transport_get_remote_bundle_uri(transport, 1);
+
+		if (transport->bundles &&
+		    hashmap_get_size(&transport->bundles->bundles)) {
+			/* At this point, we need the_repository to match the cloned repo. */
+			if (repo_init(the_repository, git_dir, work_tree))
+				warning(_("failed to initialize the repo, skipping bundle URI"));
+			if (fetch_bundle_list(the_repository,
+					      remote->url[0],
+					      transport->bundles))
+				warning(_("failed to fetch advertised bundles"));
+		} else {
+			clear_bundle_list(transport->bundles);
+			FREE_AND_NULL(transport->bundles);
+		}
+	}
 
 	if (mapped_refs) {
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));

gitgitgadget

Hi Stolee

Please use a "/**" opening comment in strbuf.h, i.e. an API-doc comment.

I see just above what you're adding we got it wrong :(, but most docs for functions in that file correctly use that.

Some recent background at:
https://lore.kernel.org/git/[email protected]/

s/verb/command/ here, and in a couple of later commit messages and an in-code comment as well.

As mentioned before on a previous bundle-uri patch series, protocol v2 has commands, not verbs.

I think this should be

When the client issues a command=bundle-uri request, the response is...

I can elaborate more here, but the intention is that the protocol defines only how these key-value pairs are delivered, and how the client assigns meaning to those values and acts upon them is defined elsewhere.

"still" is not needed here.

Using MUST simplifies things a lot.

How about...

"The content at the advertised URIs MUST be one of two types" ?

The files do not need that extension. This sentence can be removed.

Thanks.

The main repository server, since the bundle server is not speaking the Git protocol, but there is definitely room to clarify here.

Will do.

This is an interesting idea, although the examples of "good" and "bad" are probably best left as the test cases. Looking through the rest of this document, this section is already much more verbose than the others, so I hesitate to add these examples at this point. Perhaps there is room to improve the whole document with such examples as a follow-up.

Thanks,
-Stolee

This is based on the recent master batch that included ds/bundle-uri-....

Now that git clone --bundle-uri can download a bundle list from a plaintex file in config format, we can use the same set of key-value pairs to advertise a bundle list over protocol v2. At the end of this series:

1. A server can advertise bundles when uploadPack.advertiseBundleURIs is enabled. The bundle list comes from the server's local config, specifically the bundle.* namespace.
2. A client can notice a server's bundle-uri advertisement and request the bundle list if transfer.bundleURI is enabled. The bundles are downloaded as if the list was advertised from the --bundle-uri option.

Many patches in this series were adapted from Ævar's v2 RFC [1]. He is retained as author and I added myself as co-author only if the modifications were significant.

[1]
https://lore.kernel.org/git/[email protected]/

* Patches 1-5 are mostly taken from [1], again with mostly minor updates. The one major difference is the packet line format being a single key=value format instead of a sequence of pairs. This also means that Patch 4 is entirely new since it feeds these pairs directly from the server's config.

* Patches 6-9 finish off the ability for the client to notice the capability, request the values, and download bundles before continuing with the rest of the download.

One thing that is not handled here but could be handled in a future change is to disconnect from the origin Git server while downloading the bundle URIs, then reconnecting afterwards. This does not make any difference for HTTPS, but SSH may benefit from the reduced connection time. The git clone --bundle-uri option did not suffer from this because the bundles are downloaded before the server connection begins.

After this series, there is one more before the original scope of the plan is complete: using creation tokens as a heuristic. See [2] for the RFC version of those patches.

[2] https://github.com/derrickstolee/git/pull/22

Updates in v2
=============

* Commit messages now refer to protocol v2 "commands" not "verbs".
* Several edits were made to gitprotocol-v2.txt thanks to Victoria's thorough review.
* strbuf_parent_directory() is renamed strbuf_strip_file_from_path() to make it more clear how it behaves when ending with a slash.

Thanks,

* Stolee

Derrick Stolee (5):
bundle-uri: serve bundle.* keys from config
strbuf: introduce strbuf_strip_file_from_path()
bundle-uri: allow relative URLs in bundle lists
bundle-uri: download bundles from an advertised list
clone: unbundle the advertised bundles

Ævar Arnfjörð Bjarmason (4):
protocol v2: add server-side "bundle-uri" skeleton bundle-uri client: add minimal NOOP client bundle-uri client: add helper for testing server bundle-uri client: add boolean transfer.bundleURI setting

 Documentation/config/transfer.txt      |   6 +
 Documentation/gitprotocol-v2.txt       | 201 ++++++++++++++++++++++
 builtin/clone.c                        |  23 +++
 bundle-uri.c                           |  91 +++++++++-
 bundle-uri.h                           |  27 +++
 connect.c                              |  47 +++++
 remote.h                               |   5 +
 serve.c                                |   6 +
 strbuf.c                               |   9 +
 strbuf.h                               |  12 ++
 t/helper/test-bundle-uri.c             |  48 ++++++
 t/lib-t5730-protocol-v2-bundle-uri.sh  | 229 +++++++++++++++++++++++++
 t/t5601-clone.sh                       |  59 +++++++
 t/t5701-git-serve.sh                   |  40 ++++-
 t/t5730-protocol-v2-bundle-uri-file.sh |  36 ++++
 t/t5731-protocol-v2-bundle-uri-git.sh  |  17 ++
 t/t5732-protocol-v2-bundle-uri-http.sh |  17 ++
 t/t5750-bundle-uri-parse.sh            |  54 ++++++
 transport-helper.c                     |  13 ++
 transport-internal.h                   |   7 +
 transport.c                            |  87 ++++++++++
 transport.h                            |  23 +++
 22 files changed, 1055 insertions(+), 2 deletions(-)
 create mode 100644 t/lib-t5730-protocol-v2-bundle-uri.sh
 create mode 100755 t/t5730-protocol-v2-bundle-uri-file.sh
 create mode 100755 t/t5731-protocol-v2-bundle-uri-git.sh
 create mode 100755 t/t5732-protocol-v2-bundle-uri-http.sh

base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1400%2Fderrickstolee%2Fbundle-redo%2Fadvertise-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1400/derrickstolee/bundle-redo/advertise-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1400

Range-diff vs v1:

1: a02eee98318 ! 1: beae335b855 protocol v2: add server-side "bundle-uri" skeleton
@@ Commit message

An earlier version of this patch [1] used a different transfer format than the "key=value" pairs in the current implementation. The change was
- made to unify the protocol v2 verb with the bundle lists provided by
+ made to unify the protocol v2 command with the bundle lists provided by
independent bundle servers. Further, the standard allows for the server to advertise a URI that contains a bundle list. This allows users automatically discovering bundle providers that are loosely associated
@@ Documentation/gitprotocol-v2.txt: and associated requested information, each sep
+currently advertise a capability value. Both may be added in the +future.
+
-+When the client issues a command=bundle-uri the response is a list of -+key-value pairs provided as packet lines with value <key>=<value>. The -+meaning of these key-value pairs are provided by the config keys in the -+bundle.* namespace (see linkgit:git-config[1]).
++When the client issues a command=bundle-uri request, the response is a ++list of key-value pairs provided as packet lines with value ++<key>=<value>. Each <key> should be interpreted as a config key from ++the bundle.* namespace to construct a list of bundles. These keys are ++grouped by a bundle.<id>. subsection, where each key corresponding to a ++given <id> contributes attributes to the bundle defined by that <id>. ++See linkgit:git-config[1] for the specific details of these keys and how ++the Git client will interpret their values.
+
-+Clients are still expected to fully parse the line according to the -+above format, lines that do not conform to the format SHOULD be -+discarded. The user MAY be warned in such a case.
++Clients MUST parse the line according to the above format, lines that do ++not conform to the format SHOULD be discarded. The user MAY be warned in ++such a case.
+
+bundle-uri CLIENT AND SERVER EXPECTATIONS +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+URI CONTENTS::
-+The advertised URIs MUST be in one of two possible formats.
++The content at the advertised URIs MUST be one of two types.
++
-+The first possible format is a bundle file that git bundle verify ++The advertised URI may contain a bundle file that git bundle verify
+would accept. I.e. they MUST contain one or more reference tips for +use by the client, MUST indicate prerequisites (in any) with standard +"-" prefixes, and MUST indicate their "object-format", if
-+applicable. Create "*.bundle" files with git bundle create. ++applicable.
++
-+The second possible format is a plaintext file that git config --list -+would accept (with the --file option). The key-value pairs in this list -+are in the bundle.* namespace (see linkgit:git-config[1]).
++The advertised URI may alternatively contain a plaintext file that git ++config --list would accept (with the --file option). The key-value ++pairs in this list are in the bundle.* namespace (see ++linkgit:git-config[1]).
+
+bundle-uri CLIENT ERROR RECOVERY::
+A client MUST above all gracefully degrade on errors, whether that
@@ Documentation/gitprotocol-v2.txt: and associated requested information, each sep
+WHEN ADVERTISED BUNDLE(S) REQUIRE NO FURTHER NEGOTIATION::
+If after issuing bundle-uri and ls-refs, and getting the header(s) +of the bundle(s) the client finds that the ref tips it wants can be
-+retrieved entirety from advertised bundle(s), it MAY disconnect. The -+results of such a 'clone' or 'fetch' should be indistinguishable from -+the state attained without using bundle-uri.
++retrieved entirely from advertised bundle(s), the client MAY disconnect ++from the Git server. The results of such a 'clone' or 'fetch' should be ++indistinguishable from the state attained without using bundle-uri.
+
+EARLY CLIENT DISCONNECTIONS AND ERROR RECOVERY::
+A client MAY perform an early disconnect while still downloading the
@@ Documentation/gitprotocol-v2.txt: and associated requested information, each sep
+bundle-uri PROTOCOL FEATURES
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
-+As noted above the <key>=<value> definitions are documented by the -+bundle.* config namespace.
++The client constructs a bundle list from the <key>=<value> pairs ++provided by the server. These pairs are part of the bundle.* namespace ++as documented in linkgit:git-config[1]. In this section, we discuss some ++of these keys and describe the actions the client will do in response to ++this information.
+
+In particular, the bundle.version key specifies an integer value. The +only accepted value at the moment is 1, but if the client sees an

  2:  64dd9bf41de =  2:  0d85aef965d bundle-uri client: add minimal NOOP client
  3:  ae0003bb39b =  3:  c3269a24b57 bundle-uri client: add helper for testing server
  4:  431cd585184 =  4:  cd906f6d981 bundle-uri: serve bundle.* keys from config
  5:  c877f7c033d =  5:  93397468931 bundle-uri client: add boolean transfer.bundleURI setting
  6:  2200a70d279 !  6:  7d86852c015 strbuf: reintroduce strbuf_parent_directory()

@@ Metadata
Author: Derrick Stolee <[email protected]>

## Commit message ##
- strbuf: reintroduce strbuf_parent_directory()
+ strbuf: introduce strbuf_strip_file_from_path()

The strbuf_parent_directory() method was added as a static method in contrib/scalar by d0feac4e8c0 (scalar: 'register' sets recommended
@@ Commit message
65f6a9eb0b9 (scalar: constrain enlistment search, 2022-08-18), but now there is a need for a similar method in the bundle URI feature.

- Re-add the method, this time in strbuf.c. The method requirements are
- slightly modified to allow a trailing slash, in which case nothing is
- done. The return value is the number of byte removed.
+ Re-add the method, this time in strbuf.c, but with a new name:
+ strbuf_strip_file_from_path(). The method requirements are slightly
+ modified to allow a trailing slash, in which case nothing is done, which
+ makes the name change valuable. The return value is the number of bytes
+ removed.

@@ strbuf.c: int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
return res;
}
+
-+size_t strbuf_parent_directory(struct strbuf *buf)
++size_t strbuf_strip_file_from_path(struct strbuf *buf)
+{
+ size_t len = buf->len;
+ size_t offset = offset_1st_component(buf->buf);
+ char *path_sep = find_last_dir_sep(buf->buf + offset);
-+ strbuf_setlen(buf, path_sep ? path_sep - buf->buf : offset);
++ strbuf_setlen(buf, path_sep ? path_sep - buf->buf + 1 : offset);
+ return len - buf->len;
+}

@@ strbuf.h: int launch_sequence_editor(const char *path, struct strbuf *buffer,
const char *const *env);

+/*
-+ * Remove the deepest subdirectory in the provided path string. If path ++ * Remove the filename from the provided path string. If the path
+ * contains a trailing separator, then the path is considered a directory
-+ * and nothing is modified.
++ * and nothing is modified. Returns the number of characters removed from ++ * the path.
++ *
++ * Examples:
++ * - "/path/to/file" -> "/path/to/" (returns: 4)
++ * - "/path/to/dir/" -> "/path/to/dir/" (returns: 0)
+ */
-+size_t strbuf_parent_directory(struct strbuf *buf);
++size_t strbuf_strip_file_from_path(struct strbuf *buf);
+
void strbuf_add_lines(struct strbuf *sb,
const char *prefix,
7: 3550f6fb91b ! 7: 186e112d821 bundle-uri: allow relative URLs in bundle lists

     @@ bundle-uri.c: int bundle_uri_parse_config_format(const char *uri,
      +		 * remove the filename portion of the path. This is
      +		 * important for relative URIs.
      +		 */
     -+		strbuf_parent_directory(&baseURI);
     ++		strbuf_strip_file_from_path(&baseURI);
      +		list->baseURI = strbuf_detach(&baseURI, NULL);
      +	}
       	result = git_config_from_file_with_options(config_to_bundle_list,

8: e002affe4bf ! 8: f254da46a2c bundle-uri: download bundles from an advertised list
@@ Commit message

The logic in fetch_bundle_uri() is useful for the --bundle-uri option of 'git clone', but is not helpful when the clone operation discovers a
- list of URIs from the bundle-uri protocol v2 verb. To actually download
- and unbundle the advertised bundles, we need a different mechanism.
+ list of URIs from the bundle-uri protocol v2 command. To actually
+ download and unbundle the advertised bundles, we need a different
+ mechanism.

Create the new fetch_bundle_list() method which is very similar to fetch_bundle_uri() except that it relies on download_bundle_list()
9: 1c034bba744 ! 9: b62b4b17481 clone: unbundle the advertised bundles
@@ Commit message
clone: unbundle the advertised bundles

A previous change introduced the transport methods to acquire a bundle
- list from the 'bundle-uri' protocol v2 verb, when advertised _and_ when
- the client has chosen to enable the feature.
+ list from the 'bundle-uri' protocol v2 command, when advertised _and_
+ when the client has chosen to enable the feature.

Teach Git to download and unbundle the data advertised by those bundles during 'git clone'.

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Add a skeleton server-side implementation of a new "bundle-uri" command to protocol v2. This will allow conforming clients to optionally seed their initial clones or incremental fetches from URLs containing "*.bundle" files created with "git bundle create".

This change only performs the basic boilerplate of advertising a new protocol v2 capability. The new 'bundle-uri' capability allows a client to request a list of bundles. Right now, the server only returns a flush packet, which corresponds to an empty advertisement. The bundle.* config namespace describes which key-value pairs will be communicated across this interface in future updates.

The critical bit right now is that the new boolean uploadPack.adverstiseBundleURIs config value signals whether or not this capability should be advertised at all.

An earlier version of this patch [1] used a different transfer format than the "key=value" pairs in the current implementation. The change was made to unify the protocol v2 command with the bundle lists provided by independent bundle servers. Further, the standard allows for the server to advertise a URI that contains a bundle list. This allows users automatically discovering bundle providers that are loosely associated with the origin server, but without the origin server knowing exactly which bundles are currently available.

[1] https://lore.kernel.org/git/[email protected]/

The very-deep headings needed to be modified to stop at level 4 due to documentation build issues. These were not recognized in earlier builds since the file was previously in the Documentation/technical/ directory and was built in a different way. With its current location, the heavily-nested details were causing build issues and they are now replaced with a bulletted list of details.

---
 Documentation/gitprotocol-v2.txt | 201 +++++++++++++++++++++++++++++++
 bundle-uri.c                     |  36 ++++++
 bundle-uri.h                     |   7 ++
 serve.c                          |   6 +
 t/t5701-git-serve.sh             |  40 +++++-
 5 files changed, 289 insertions(+), 1 deletion(-)

index 59bf41cefb9..10bd2d40cec 100644
@@ -578,6 +578,207 @@ and associated requested information, each separated by a single space.
 
 	obj-info = obj-id SP obj-size
 
+bundle-uri
+~~~~~~~~~~
+
+If the 'bundle-uri' capability is advertised, the server supports the
+`bundle-uri' command.
+
+The capability is currently advertised with no value (i.e. not
+"bundle-uri=somevalue"), a value may be added in the future for
+supporting command-wide extensions. Clients MUST ignore any unknown
+capability values and proceed with the 'bundle-uri` dialog they
+support.
+
+The 'bundle-uri' command is intended to be issued before `fetch` to
+get URIs to bundle files (see linkgit:git-bundle[1]) to "seed" and
+inform the subsequent `fetch` command.
+
+The client CAN issue `bundle-uri` before or after any other valid
+command. To be useful to clients it's expected that it'll be issued
+after an `ls-refs` and before `fetch`, but CAN be issued at any time
+in the dialog.
+
+DISCUSSION of bundle-uri
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+The intent of the feature is optimize for server resource consumption
+in the common case by changing the common case of fetching a very
+large PACK during linkgit:git-clone[1] into a smaller incremental
+fetch.
+
+It also allows servers to achieve better caching in combination with
+an `uploadpack.packObjectsHook` (see linkgit:git-config[1]).
+
+By having new clones or fetches be a more predictable and common
+negotiation against the tips of recently produces *.bundle file(s).
+Servers might even pre-generate the results of such negotiations for
+the `uploadpack.packObjectsHook` as new pushes come in.
+
+One way that servers could take advantage of these bundles is that the
+server would anticipate that fresh clones will download a known bundle,
+followed by catching up to the current state of the repository using ref
+tips found in that bundle (or bundles).
+
+PROTOCOL for bundle-uri
+^^^^^^^^^^^^^^^^^^^^^^^
+
+A `bundle-uri` request takes no arguments, and as noted above does not
+currently advertise a capability value. Both may be added in the
+future.
+
+When the client issues a `command=bundle-uri` request, the response is a
+list of key-value pairs provided as packet lines with value
+`<key>=<value>`. Each `<key>` should be interpreted as a config key from
+the `bundle.*` namespace to construct a list of bundles. These keys are
+grouped by a `bundle.<id>.` subsection, where each key corresponding to a
+given `<id>` contributes attributes to the bundle defined by that `<id>`.
+See linkgit:git-config[1] for the specific details of these keys and how
+the Git client will interpret their values.
+
+Clients MUST parse the line according to the above format, lines that do
+not conform to the format SHOULD be discarded. The user MAY be warned in
+such a case.
+
+bundle-uri CLIENT AND SERVER EXPECTATIONS
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+URI CONTENTS::
+The content at the advertised URIs MUST be one of two types.
++
+The advertised URI may contain a bundle file that `git bundle verify`
+would accept. I.e. they MUST contain one or more reference tips for
+use by the client, MUST indicate prerequisites (in any) with standard
+"-" prefixes, and MUST indicate their "object-format", if
+applicable.
++
+The advertised URI may alternatively contain a plaintext file that `git
+config --list` would accept (with the `--file` option). The key-value
+pairs in this list are in the `bundle.*` namespace (see
+linkgit:git-config[1]).
+
+bundle-uri CLIENT ERROR RECOVERY::
+A client MUST above all gracefully degrade on errors, whether that
+error is because of bad missing/data in the bundle URI(s), because
+that client is too dumb to e.g. understand and fully parse out bundle
+headers and their prerequisite relationships, or something else.
++
+Server operators should feel confident in turning on "bundle-uri" and
+not worry if e.g. their CDN goes down that clones or fetches will run
+into hard failures. Even if the server bundle bundle(s) are
+incomplete, or bad in some way the client should still end up with a
+functioning repository, just as if it had chosen not to use this
+protocol extension.
++
+All subsequent discussion on client and server interaction MUST keep
+this in mind.
+
+bundle-uri SERVER TO CLIENT::
+The ordering of the returned bundle uris is not significant. Clients
+MUST parse their headers to discover their contained OIDS and
+prerequisites. A client MUST consider the content of the bundle(s)
+themselves and their header as the ultimate source of truth.
++
+A server MAY even return bundle(s) that don't have any direct
+relationship to the repository being cloned (either through accident,
+or intentional "clever" configuration), and expect a client to sort
+out what data they'd like from the bundle(s), if any.
+
+bundle-uri CLIENT TO SERVER::
+The client SHOULD provide reference tips found in the bundle header(s)
+as 'have' lines in any subsequent `fetch` request. A client MAY also
+ignore the bundle(s) entirely if doing so is deemed worse for some
+reason, e.g. if the bundles can't be downloaded, it doesn't like the
+tips it finds etc.
+
+WHEN ADVERTISED BUNDLE(S) REQUIRE NO FURTHER NEGOTIATION::
+If after issuing `bundle-uri` and `ls-refs`, and getting the header(s)
+of the bundle(s) the client finds that the ref tips it wants can be
+retrieved entirely from advertised bundle(s), the client MAY disconnect
+from the Git server. The results of such a 'clone' or 'fetch' should be
+indistinguishable from the state attained without using bundle-uri.
+
+EARLY CLIENT DISCONNECTIONS AND ERROR RECOVERY::
+A client MAY perform an early disconnect while still downloading the
+bundle(s) (having streamed and parsed their headers). In such a case
+the client MUST gracefully recover from any errors related to
+finishing the download and validation of the bundle(s).
++
+I.e. a client might need to re-connect and issue a 'fetch' command,
+and possibly fall back to not making use of 'bundle-uri' at all.
++
+This "MAY" behavior is specified as such (and not a "SHOULD") on the
+assumption that a server advertising bundle uris is more likely than
+not to be serving up a relatively large repository, and to be pointing
+to URIs that have a good chance of being in working order. A client
+MAY e.g. look at the payload size of the bundles as a heuristic to see
+if an early disconnect is worth it, should falling back on a full
+"fetch" dialog be necessary.
+
+WHEN ADVERTISED BUNDLE(S) REQUIRE FURTHER NEGOTIATION::
+A client SHOULD commence a negotiation of a PACK from the server via
+the "fetch" command using the OID tips found in advertised bundles,
+even if's still in the process of downloading those bundle(s).
++
+This allows for aggressive early disconnects from any interactive
+server dialog. The client blindly trusts that the advertised OID tips
+are relevant, and issues them as 'have' lines, it then requests any
+tips it would like (usually from the "ls-refs" advertisement) via
+'want' lines. The server will then compute a (hopefully small) PACK
+with the expected difference between the tips from the bundle(s) and
+the data requested.
++
+The only connection the client then needs to keep active is to the
+concurrently downloading static bundle(s), when those and the
+incremental PACK are retrieved they should be inflated and
+validated. Any errors at this point should be gracefully recovered
+from, see above.
+
+bundle-uri PROTOCOL FEATURES
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The client constructs a bundle list from the `<key>=<value>` pairs
+provided by the server. These pairs are part of the `bundle.*` namespace
+as documented in linkgit:git-config[1]. In this section, we discuss some
+of these keys and describe the actions the client will do in response to
+this information.
+
+In particular, the `bundle.version` key specifies an integer value. The
+only accepted value at the moment is `1`, but if the client sees an
+unexpected value here then the client MUST ignore the bundle list.
+
+As long as `bundle.version` is understood, all other unknown keys MAY be
+ignored by the client. The server will guarantee compatibility with older
+clients, though newer clients may be better able to use the extra keys to
+minimize downloads.
+
+Any backwards-incompatible addition of pre-URI key-value will be
+guarded by a new `bundle.version` value or values in 'bundle-uri'
+capability advertisement itself, and/or by new future `bundle-uri`
+request arguments.
+
+Some example key-value pairs that are not currently implemented but could
+be implemented in the future include:
+
+ * Add a "hash=<val>" or "size=<bytes>" advertise the expected hash or
+   size of the bundle file.
+
+ * Advertise that one or more bundle files are the same (to e.g. have
+   clients round-robin or otherwise choose one of N possible files).
+
+ * A "oid=<OID>" shortcut and "prerequisite=<OID>" shortcut. For
+   expressing the common case of a bundle with one tip and no
+   prerequisites, or one tip and one prerequisite.
++
+This would allow for optimizing the common case of servers who'd like
+to provide one "big bundle" containing only their "main" branch,
+and/or incremental updates thereof.
++
+A client receiving such a a response MAY assume that they can skip
+retrieving the header from a bundle at the indicated URI, and thus
+save themselves and the server(s) the request(s) needed to inspect the
+headers of that bundle or bundles.
+
 GIT
 ---
 Part of the linkgit:git[1] suite

index 79a914f961b..32022595964 100644
@@ -563,6 +563,42 @@ cleanup:
 	return result;
 }
 
+/**
+ * API for serve.c.
+ */
+
+int bundle_uri_advertise(struct repository *r, struct strbuf *value)
+{
+	static int advertise_bundle_uri = -1;
+
+	if (advertise_bundle_uri != -1)
+		goto cached;
+
+	advertise_bundle_uri = 0;
+	git_config_get_maybe_bool("uploadpack.advertisebundleuris", &advertise_bundle_uri);
+
+cached:
+	return advertise_bundle_uri;
+}
+
+int bundle_uri_command(struct repository *r,
+		       struct packet_reader *request)
+{
+	struct packet_writer writer;
+	packet_writer_init(&writer, 1);
+
+	while (packet_reader_read(request) == PACKET_READ_NORMAL)
+		die(_("bundle-uri: unexpected argument: '%s'"), request->line);
+	if (request->status != PACKET_READ_FLUSH)
+		die(_("bundle-uri: expected flush after arguments"));
+
+	/* TODO: Implement the communication */
+
+	packet_writer_flush(&writer);
+
+	return 0;
+}
+
 /**
  * General API for {transport,connect}.c etc.
  */

index 4dbc269823c..357111ecce8 100644
@@ -4,6 +4,7 @@
 #include "hashmap.h"
 #include "strbuf.h"
 
+struct packet_reader;
 struct repository;
 struct string_list;
 

@@ -92,6 +93,12 @@ int bundle_uri_parse_config_format(const char *uri,
  */
 int fetch_bundle_uri(struct repository *r, const char *uri);
 
+/**
+ * API for serve.c.
+ */
+int bundle_uri_advertise(struct repository *r, struct strbuf *value);
+int bundle_uri_command(struct repository *r, struct packet_reader *request);
+
 /**
  * General API for {transport,connect}.c etc.
  */

index 733347f602a..cbf4a143cfe 100644
@@ -7,6 +7,7 @@
 #include "protocol-caps.h"
 #include "serve.h"
 #include "upload-pack.h"
+#include "bundle-uri.h"
 
 static int advertise_sid = -1;
 static int client_hash_algo = GIT_HASH_SHA1;

@@ -135,6 +136,11 @@ static struct protocol_capability capabilities[] = {
 		.advertise = always_advertise,
 		.command = cap_object_info,
 	},
+	{
+		.name = "bundle-uri",
+		.advertise = bundle_uri_advertise,
+		.command = bundle_uri_command,
+	},
 };
 
 void protocol_v2_advertise_capabilities(void)

index 1896f671cb3..f21e5e9d33d 100755
@@ -13,7 +13,7 @@ test_expect_success 'test capability advertisement' '
 	wrong_algo sha1:sha256
 	wrong_algo sha256:sha1
 	EOF
-	cat >expect <<-EOF &&
+	cat >expect.base <<-EOF &&
 	version 2
 	agent=git/$(git version | cut -d" " -f3)
 	ls-refs=unborn

@@ -21,8 +21,11 @@ test_expect_success 'test capability advertisement' '
 	server-option
 	object-format=$(test_oid algo)
 	object-info
+	EOF
+	cat >expect.trailer <<-EOF &&
 	0000
 	EOF
+	cat expect.base expect.trailer >expect &&
 
 	GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
 		--advertise-capabilities >out &&

@@ -342,4 +345,39 @@ test_expect_success 'basics of object-info' '
 	test_cmp expect actual
 '
 
+test_expect_success 'test capability advertisement with uploadpack.advertiseBundleURIs' '
+	test_config uploadpack.advertiseBundleURIs true &&
+
+	cat >expect.extra <<-EOF &&
+	bundle-uri
+	EOF
+	cat expect.base \
+	    expect.extra \
+	    expect.trailer >expect &&
+
+	GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
+		--advertise-capabilities >out &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'basics of bundle-uri: dies if not enabled' '
+	test-tool pkt-line pack >in <<-EOF &&
+	command=bundle-uri
+	0000
+	EOF
+
+	cat >err.expect <<-\EOF &&
+	fatal: invalid command '"'"'bundle-uri'"'"'
+	EOF
+
+	cat >expect <<-\EOF &&
+	ERR serve: invalid command '"'"'bundle-uri'"'"'
+	EOF
+
+	test_must_fail test-tool serve-v2 --stateless-rpc <in >out 2>err.actual &&
+	test_cmp err.expect err.actual &&
+	test_must_be_empty out
+'
+
 test_done

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Set up all the needed client parts of the "bundle-uri" protocol extension, without actually doing anything with the bundle URIs.

I.e. if the server says it supports "bundle-uri" we'll issue a command=bundle-uri after command=ls-refs when we're cloning. We'll parse the returned output using the code already tested for in t5750-bundle-uri-parse.sh.

What we aren't doing is actually acting on that data, i.e. downloading the bundle(s) before we get to doing the command=fetch, and adjusting our negotiation dialog appropriately. I'll do that in subsequent commits.

There's a question of what level of encapsulation we should use here, I've opted to use connect.h in clone.c, but we could also e.g. make transport_get_remote_refs() invoke this, i.e. make it implicitly get the bundle-uri list for later steps.

This approach means that we don't "support" this in "git fetch" for now. I'm starting with the case of initial clones, although as noted in preceding commits to the protocol documentation nothing about this approach precludes getting bundles on incremental fetches.

For the t5732-protocol-v2-bundle-uri-http.sh it's not easy to set environment variables for git-upload-pack (it's started by Apache), so let's skip the test under T5730_HTTP, and add unused T5730_{FILE,GIT} prerequisites for consistency and future use.

---
 builtin/clone.c                        |   7 ++
 bundle-uri.c                           |   4 +
 connect.c                              |  47 ++++++++
 remote.h                               |   5 +
 t/lib-t5730-protocol-v2-bundle-uri.sh  | 148 +++++++++++++++++++++++++
 t/t5730-protocol-v2-bundle-uri-file.sh |  36 ++++++
 t/t5731-protocol-v2-bundle-uri-git.sh  |  17 +++
 t/t5732-protocol-v2-bundle-uri-http.sh |  17 +++
 transport-helper.c                     |  13 +++
 transport-internal.h                   |   7 ++
 transport.c                            |  55 +++++++++
 transport.h                            |  19 ++++
 12 files changed, 375 insertions(+)
 create mode 100644 t/lib-t5730-protocol-v2-bundle-uri.sh
 create mode 100755 t/t5730-protocol-v2-bundle-uri-file.sh
 create mode 100755 t/t5731-protocol-v2-bundle-uri-git.sh
 create mode 100755 t/t5732-protocol-v2-bundle-uri-http.sh

index 547d6464b3c..edf98295af2 100644
@@ -27,6 +27,7 @@
 #include "iterator.h"
 #include "sigchain.h"
 #include "branch.h"
+#include "connect.h"
 #include "remote.h"
 #include "run-command.h"
 #include "connected.h"

@@ -1266,6 +1267,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (refs)
 		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
 
+	/*
+	 * Populate transport->got_remote_bundle_uri and
+	 * transport->bundle_uri. We might get nothing.
+	 */
+	transport_get_remote_bundle_uri(transport);
+
 	if (mapped_refs) {
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
 

index 32022595964..2201b604b11 100644
@@ -571,6 +571,10 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)
 {
 	static int advertise_bundle_uri = -1;
 
+	if (value &&
+	    git_env_bool("GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE", 0))
+		strbuf_addstr(value, "test-unknown-capability-value");
+
 	if (advertise_bundle_uri != -1)
 		goto cached;
 

index 1c4621b414b..1ebbe42792e 100644
@@ -234,6 +234,11 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 			     const struct string_list *server_options,
 			     int stateless_rpc);
 
+/* Used for protocol v2 in order to retrieve refs from a remote */
+struct bundle_list;
+int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
+			  struct bundle_list *bundles, int stateless_rpc);
+
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 
 /*

new file mode 100755
index 00000000000..89203d3a23c
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description="Test bundle-uri with protocol v2 and 'file://' transport"
+
+TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# Test protocol v2 with 'file://' transport
+#
+T5730_PROTOCOL=file
+. "$TEST_DIRECTORY"/lib-t5730-protocol-v2-bundle-uri.sh
+
+test_expect_success "unknown capability value with $T5730_PROTOCOL:// using protocol v2" '
+	test_when_finished "rm -f log" &&
+
+	test_config -C "$T5730_PARENT" \
+		uploadpack.bundleURI "$T5730_BUNDLE_URI_ESCAPED" &&
+
+	GIT_TRACE_PACKET="$PWD/log" \
+	GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE=true \
+	git \
+		-c protocol.version=2 \
+		ls-remote --symref "$T5730_URI" \
+		>actual 2>err &&
+
+	# Server responded using protocol v2
+	grep "< version 2" log &&
+
+	grep "> bundle-uri=test-unknown-capability-value" log
+'
+
+test_done

new file mode 100755
index 00000000000..282847b311f
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description="Test bundle-uri with protocol v2 and 'git://' transport"
+
+TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# Test protocol v2 with 'git://' transport
+#
+T5730_PROTOCOL=git
+. "$TEST_DIRECTORY"/lib-t5730-protocol-v2-bundle-uri.sh
+
+test_done

new file mode 100755
index 00000000000..fcc1cf3faef
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description="Test bundle-uri with protocol v2 and 'git://' transport"
+
+TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# Test protocol v2 with 'git://' transport
+#
+T5730_PROTOCOL=http
+. "$TEST_DIRECTORY"/lib-t5730-protocol-v2-bundle-uri.sh
+
+test_done

index e95267a4ab5..3ea7c2bb5ad 100644
@@ -1267,9 +1267,22 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
 	return ret;
 }
 
+static int get_bundle_uri(struct transport *transport)
+{
+	get_helper(transport);
+
+	if (process_connect(transport, 0)) {
+		do_take_over(transport);
+		return transport->vtable->get_bundle_uri(transport);
+	}
+
+	return -1;
+}
+
 static struct transport_vtable vtable = {
 	.set_option	= set_helper_option,
 	.get_refs_list	= get_refs_list,
+	.get_bundle_uri = get_bundle_uri,
 	.fetch_refs	= fetch_refs,
 	.push_refs	= push_refs,
 	.connect	= connect_helper,

index c4ca0b733ac..90ea749e5cf 100644
@@ -26,6 +26,13 @@ struct transport_vtable {
 	struct ref *(*get_refs_list)(struct transport *transport, int for_push,
 				     struct transport_ls_refs_options *transport_options);
 
+	/**
+	 * Populates the remote side's bundle-uri under protocol v2,
+	 * if the "bundle-uri" capability was advertised. Returns 0 if
+	 * OK, negative values on error.
+	 */
+	int (*get_bundle_uri)(struct transport *transport);
+
 	/**
 	 * Fetch the objects for the given refs. Note that this gets
 	 * an array, and should ignore the list structure.

index b5bf7b3e704..85150f504fb 100644
@@ -62,6 +62,7 @@ enum transport_family {
 	TRANSPORT_FAMILY_IPV6
 };
 
+struct bundle_list;
 struct transport {
 	const struct transport_vtable *vtable;
 

@@ -76,6 +77,18 @@ struct transport {
 	 */
 	unsigned got_remote_refs : 1;
 
+	/**
+	 * Indicates whether we already called get_bundle_uri_list(); set by
+	 * transport.c::transport_get_remote_bundle_uri().
+	 */
+	unsigned got_remote_bundle_uri : 1;
+
+	/*
+	 * The results of "command=bundle-uri", if both sides support
+	 * the "bundle-uri" capability.
+	 */
+	struct bundle_list *bundles;
+
 	/*
 	 * Transports that call take-over destroys the data specific to
 	 * the transport type while doing so, and cannot be reused.

@@ -281,6 +294,12 @@ void transport_ls_refs_options_release(struct transport_ls_refs_options *opts);
 const struct ref *transport_get_remote_refs(struct transport *transport,
 					    struct transport_ls_refs_options *transport_options);
 
+/**
+ * Retrieve bundle URI(s) from a remote. Populates "struct
+ * transport"'s "bundle_uri" and "got_remote_bundle_uri".
+ */
+int transport_get_remote_bundle_uri(struct transport *transport);
+
 /*
  * Fetch the hash algorithm used by a remote.
  *

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

Add a 'test-tool bundle-uri ls-remote' command. This is a thin wrapper for issuing protocol v2 "bundle-uri" commands to a server, and to the parsing routines in bundle-uri.c.

Since in the "git clone" case we'll have already done the handshake(), but not here, introduce a "got_advertisement" state along with "got_remote_heads". It seems to me that the "got_remote_heads" is badly named in the first place, and the whole logic of eagerly getting ls-refs on handshake() or not could be refactored somewhat, but let's not do that now, and instead just add another self-documenting state variable.

---
 builtin/clone.c                       |  2 +-
 t/helper/test-bundle-uri.c            | 46 +++++++++++++++++++
 t/lib-t5730-protocol-v2-bundle-uri.sh | 63 ++++++++++++++++++++++-----
 transport.c                           | 43 ++++++++++++++----
 transport.h                           |  6 ++-
 5 files changed, 139 insertions(+), 21 deletions(-)

index edf98295af2..22b1e506452 100644
@@ -1271,7 +1271,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 * Populate transport->got_remote_bundle_uri and
 	 * transport->bundle_uri. We might get nothing.
 	 */
-	transport_get_remote_bundle_uri(transport);
+	transport_get_remote_bundle_uri(transport, 1);
 
 	if (mapped_refs) {
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));

index 85150f504fb..dd0115b83bf 100644
@@ -297,8 +297,12 @@ const struct ref *transport_get_remote_refs(struct transport *transport,
 /**
  * Retrieve bundle URI(s) from a remote. Populates "struct
  * transport"'s "bundle_uri" and "got_remote_bundle_uri".
+ *
+ * With `quiet=1` it will not complain if the serve doesn't support
+ * the protocol, but only if we discover the server uses it, and
+ * encounter issues then.
  */
-int transport_get_remote_bundle_uri(struct transport *transport);
+int transport_get_remote_bundle_uri(struct transport *transport, int quiet);
 
 /*
  * Fetch the hash algorithm used by a remote.

gitgitgadget

From: Derrick Stolee <[email protected]>

The strbuf_parent_directory() method was added as a static method in contrib/scalar by d0feac4e8c0 (scalar: 'register' sets recommended config and starts maintenance, 2021-12-03) and then removed in 65f6a9eb0b9 (scalar: constrain enlistment search, 2022-08-18), but now there is a need for a similar method in the bundle URI feature.

Re-add the method, this time in strbuf.c, but with a new name:
strbuf_strip_file_from_path(). The method requirements are slightly modified to allow a trailing slash, in which case nothing is done, which makes the name change valuable. The return value is the number of bytes removed.

---
 strbuf.c |  9 +++++++++
 strbuf.h | 12 ++++++++++++
 2 files changed, 21 insertions(+)

index 0890b1405c5..8d1e2e8bb61 100644
@@ -1200,3 +1200,12 @@ int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 	free(path2);
 	return res;
 }
+
+size_t strbuf_strip_file_from_path(struct strbuf *buf)
+{
+	size_t len = buf->len;
+	size_t offset = offset_1st_component(buf->buf);
+	char *path_sep = find_last_dir_sep(buf->buf + offset);
+	strbuf_setlen(buf, path_sep ? path_sep - buf->buf + 1 : offset);
+	return len - buf->len;
+}

index 76965a17d44..4822b713786 100644
@@ -664,6 +664,18 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
 int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 			      const char *const *env);
 
+/*
+ * Remove the filename from the provided path string. If the path
+ * contains a trailing separator, then the path is considered a directory
+ * and nothing is modified. Returns the number of characters removed from
+ * the path.
+ *
+ * Examples:
+ * - "/path/to/file" -> "/path/to/" (returns: 4)
+ * - "/path/to/dir/" -> "/path/to/dir/" (returns: 0)
+ */
+size_t strbuf_strip_file_from_path(struct strbuf *buf);
+
 void strbuf_add_lines(struct strbuf *sb,
 		      const char *prefix,
 		      const char *buf,

gitgitgadget

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>

The yet-to-be introduced client support for bundle-uri will always fall back on a full clone, but we'd still like to be able to ignore a server's bundle-uri advertisement entirely.

The new transfer.bundleURI config option defaults to 'false', but a user can set it to 'true' to enable checking for bundle URIs from the origin Git server using protocol v2.

To enable this setting by default in the correct tests, add a GIT_TEST_BUNDLE_URI environment variable.

---
 Documentation/config/transfer.txt     |  6 ++++++
 t/lib-t5730-protocol-v2-bundle-uri.sh |  3 +++
 transport.c                           | 10 +++++++---
 3 files changed, 16 insertions(+), 3 deletions(-)

index 264812cca4d..c3ac767d1e4 100644
@@ -115,3 +115,9 @@ transfer.unpackLimit::
 transfer.advertiseSID::
 	Boolean. When true, client and server processes will advertise their
 	unique session IDs to their remote counterpart. Defaults to false.
+
+transfer.bundleURI::
+	When `true`, local `git clone` commands will request bundle
+	information from the remote server (if advertised) and download
+	bundles before continuing the clone through the Git protocol.
+	Defaults to `false`.

index 000fcc5e20b..872bc39ad1b 100644
@@ -1,5 +1,8 @@
 # Included from t573*-protocol-v2-bundle-uri-*.sh
 
+GIT_TEST_BUNDLE_URI=1
+export GIT_TEST_BUNDLE_URI
+
 T5730_PARENT=
 T5730_URI=
 T5730_BUNDLE_URI=

index 86460f5be28..b33180226ae 100644
@@ -1538,6 +1538,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
 
 int transport_get_remote_bundle_uri(struct transport *transport, int quiet)
 {
+	int value = 0;
 	const struct transport_vtable *vtable = transport->vtable;
 
 	/* Check config only once. */

@@ -1545,10 +1546,13 @@ int transport_get_remote_bundle_uri(struct transport *transport, int quiet)
 		return 0;
 
 	/*
-	 * This is intentionally below the transport.injectBundleURI,
-	 * we want to be able to inject into protocol v0, or into the
-	 * dialog of a server who doesn't support this.
+	 * Don't use bundle-uri at all, if configured not to. Only proceed
+	 * if GIT_TEST_BUNDLE_URI=1 or transfer.bundleURI=true.
 	 */
+	if (!git_env_bool("GIT_TEST_BUNDLE_URI", 0) &&
+	    (git_config_get_bool("transfer.bundleuri", &value) || !value))
+		return 0;
+
 	if (!vtable->get_bundle_uri) {
 		if (quiet)
 			return -1;

gitgitgadget

From: Derrick Stolee <[email protected]>

The logic in fetch_bundle_uri() is useful for the --bundle-uri option of 'git clone', but is not helpful when the clone operation discovers a list of URIs from the bundle-uri protocol v2 command. To actually download and unbundle the advertised bundles, we need a different mechanism.

Create the new fetch_bundle_list() method which is very similar to fetch_bundle_uri() except that it relies on download_bundle_list() instead of fetch_bundle_uri_internal(). The download_bundle_list() method will recursively call fetch_bundle_uri_internal() if any of the advertised URIs serve a bundle list instead of a bundle. This will also follow the bundle.list.mode setting from the input list: "any" will download only one such URI while "all" will download data from all of the URIs.

In an identical way to fetch_bundle_uri(), the bundles are unbundled after all of the bundle lists have been expanded and all necessary URIs.

---
 bundle-uri.c | 21 +++++++++++++++++++++
 bundle-uri.h | 11 +++++++++++
 2 files changed, 32 insertions(+)

index ab91bb32e9b..5914d220c43 100644
@@ -577,6 +577,27 @@ cleanup:
 	return result;
 }
 
+int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)
+{
+	int result;
+	struct bundle_list global_list;
+
+	init_bundle_list(&global_list);
+
+	/* If a bundle is added to this global list, then it is required. */
+	global_list.mode = BUNDLE_MODE_ALL;
+
+	if ((result = download_bundle_list(r, list, &global_list, 0)))
+		goto cleanup;
+
+	result = unbundle_all_bundles(r, &global_list);
+
+cleanup:
+	for_all_bundles_in_list(&global_list, unlink_bundle, NULL);
+	clear_bundle_list(&global_list);
+	return result;
+}
+
 /**
  * API for serve.c.
  */

index 7905e56732c..a75b68d2f5a 100644
@@ -102,6 +102,17 @@ int bundle_uri_parse_config_format(const char *uri,
  */
 int fetch_bundle_uri(struct repository *r, const char *uri);
 
+/**
+ * Given a bundle list that was already advertised (likely by the
+ * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
+ * bundles according to the bundle strategy of that list.
+ *
+ * Returns non-zero if no bundle information is found at the given 'uri'.
+ */
+int fetch_bundle_list(struct repository *r,
+		      const char *uri,
+		      struct bundle_list *list);
+
 /**
  * API for serve.c.
  */

gitgitgadget

From: Derrick Stolee <[email protected]>

Bundle providers may want to distribute that data across multiple CDNs. This might require a change in the base URI, all the way to the domain name. If all bundles require an absolute URI in their 'uri' value, then every push to a CDN would require altering the table of contents to match the expected domain and exact location within it.

Allow a bundle list to specify a relative URI for the bundles. This allows easier distribution of bundle data.

---
 bundle-uri.c                | 16 ++++++++++-
 bundle-uri.h                |  9 +++++++
 t/helper/test-bundle-uri.c  |  2 ++
 t/t5750-bundle-uri-parse.sh | 54 +++++++++++++++++++++++++++++++++++++
 transport.c                 |  3 +++
 5 files changed, 83 insertions(+), 1 deletion(-)

index 3469f1aaa98..ab91bb32e9b 100644
@@ -7,6 +7,7 @@
 #include "hashmap.h"
 #include "pkt-line.h"
 #include "config.h"
+#include "remote.h"
 
 static int compare_bundles(const void *hashmap_cmp_fn_data,
 			   const struct hashmap_entry *he1,

@@ -49,6 +50,7 @@ void clear_bundle_list(struct bundle_list *list)
 
 	for_all_bundles_in_list(list, clear_remote_bundle_info, NULL);
 	hashmap_clear_and_free(&list->bundles, struct remote_bundle_info, ent);
+	free(list->baseURI);
 }
 
 int for_all_bundles_in_list(struct bundle_list *list,

@@ -163,7 +165,7 @@ static int bundle_list_update(const char *key, const char *value,
 	if (!strcmp(subkey, "uri")) {
 		if (bundle->uri)
 			return -1;
-		bundle->uri = xstrdup(value);
+		bundle->uri = relative_url(list->baseURI, value, NULL);
 		return 0;
 	}
 

@@ -190,6 +192,18 @@ int bundle_uri_parse_config_format(const char *uri,
 		.error_action = CONFIG_ERROR_ERROR,
 	};
 
+	if (!list->baseURI) {
+		struct strbuf baseURI = STRBUF_INIT;
+		strbuf_addstr(&baseURI, uri);
+
+		/*
+		 * If the URI does not end with a trailing slash, then
+		 * remove the filename portion of the path. This is
+		 * important for relative URIs.
+		 */
+		strbuf_strip_file_from_path(&baseURI);
+		list->baseURI = strbuf_detach(&baseURI, NULL);
+	}
 	result = git_config_from_file_with_options(config_to_bundle_list,
 						   filename, list,
 						   &opts);

index 357111ecce8..7905e56732c 100644
@@ -61,6 +61,15 @@ struct bundle_list {
 	int version;
 	enum bundle_list_mode mode;
 	struct hashmap bundles;
+
+	/**
+	 * The baseURI of a bundle_list is used as the base for any
+	 * relative URIs advertised by the bundle list at that location.
+	 *
+	 * When the list is generated from a Git server, then use that
+	 * server's location.
+	 */
+	char *baseURI;
 };
 
 void init_bundle_list(struct bundle_list *list);

index ffb975b7b4f..5aa0b494ce3 100644
@@ -40,6 +40,8 @@ static int cmd__bundle_uri_parse(int argc, const char **argv, enum input_mode mo
 
 	init_bundle_list(&list);
 
+	list.baseURI = xstrdup("<uri>");
+
 	switch (mode) {
 	case KEY_VALUE_PAIRS:
 		if (argc != 1)

index b33180226ae..2c4ff0c2023 100644
@@ -1553,6 +1553,9 @@ int transport_get_remote_bundle_uri(struct transport *transport, int quiet)
 	    (git_config_get_bool("transfer.bundleuri", &value) || !value))
 		return 0;
 
+	if (!transport->bundles->baseURI)
+		transport->bundles->baseURI = xstrdup(transport->url);
+
 	if (!vtable->get_bundle_uri) {
 		if (quiet)
 			return -1;

gitgitgadget

From: Derrick Stolee <[email protected]>

Implement the "bundle-uri" protocol v2 capability by populating the key=value packet lines from the local Git config. The list of bundles is provided from the keys beginning with "bundle.".

In the future, we may want to filter this list to be more specific to the exact known keys that the server intends to share, but for flexibility at the moment we will assume that the config values are well-formed.

---
 bundle-uri.c                          | 16 +++++++++++-
 t/lib-t5730-protocol-v2-bundle-uri.sh | 35 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

index 2201b604b11..3469f1aaa98 100644
@@ -585,6 +585,16 @@ cached:
 	return advertise_bundle_uri;
 }
 
+static int config_to_packet_line(const char *key, const char *value, void *data)
+{
+	struct packet_reader *writer = data;
+
+	if (!strncmp(key, "bundle.", 7))
+		packet_write_fmt(writer->fd, "%s=%s", key, value);
+
+	return 0;
+}
+
 int bundle_uri_command(struct repository *r,
 		       struct packet_reader *request)
 {

@@ -596,7 +606,11 @@ int bundle_uri_command(struct repository *r,
 	if (request->status != PACKET_READ_FLUSH)
 		die(_("bundle-uri: expected flush after arguments"));
 
-	/* TODO: Implement the communication */
+	/*
+	 * Read all "bundle.*" config lines to the client as key=value
+	 * packet lines.
+	 */
+	git_config(config_to_packet_line, &writer);
 
 	packet_writer_flush(&writer);
 

index c327544641b..000fcc5e20b 100644
@@ -158,6 +158,8 @@ test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2" '
 	[bundle]
 		version = 1
 		mode = all
+	[bundle "only"]
+		uri = $T5730_BUNDLE_URI_ESCAPED
 	EOF
 	GIT_TRACE_PACKET="$PWD/log" \
 	test-tool bundle-uri \

@@ -181,6 +183,39 @@ test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2 and ext
 	[bundle]
 		version = 1
 		mode = all
+	[bundle "only"]
+		uri = $T5730_BUNDLE_URI_ESCAPED
+	EOF
+	GIT_TRACE_PACKET="$PWD/log" \
+	test-tool bundle-uri \
+		ls-remote \
+		"$T5730_URI" \
+		>actual &&
+	test_cmp_config_output expect actual
+'
+
+
+test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2 with list" '
+	test_when_finished "rm -f log" &&
+
+	test_config -C "$T5730_PARENT" \
+		bundle.bundle1.uri "$T5730_BUNDLE_URI_ESCAPED-1.bdl" &&
+	test_config -C "$T5730_PARENT" \
+		bundle.bundle2.uri "$T5730_BUNDLE_URI_ESCAPED-2.bdl" &&
+	test_config -C "$T5730_PARENT" \
+		bundle.bundle3.uri "$T5730_BUNDLE_URI_ESCAPED-3.bdl" &&
+
+	# All data about bundle URIs
+	cat >expect <<-EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	[bundle "bundle1"]
+		uri = $T5730_BUNDLE_URI_ESCAPED-1.bdl
+	[bundle "bundle2"]
+		uri = $T5730_BUNDLE_URI_ESCAPED-2.bdl
+	[bundle "bundle3"]
+		uri = $T5730_BUNDLE_URI_ESCAPED-3.bdl
 	EOF
 	GIT_TRACE_PACKET="$PWD/log" \
 	test-tool bundle-uri \

gitgitgadget

From: Derrick Stolee <[email protected]>

A previous change introduced the transport methods to acquire a bundle list from the 'bundle-uri' protocol v2 command, when advertised _and_ when the client has chosen to enable the feature.

Teach Git to download and unbundle the data advertised by those bundles during 'git clone'.

Also, since the --bundle-uri option exists, we do not want to mix the advertised bundles with the user-specified bundles.

---
 builtin/clone.c  | 26 +++++++++++++++++----
 t/t5601-clone.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 5 deletions(-)

index 22b1e506452..09f10477ed6 100644
@@ -1267,11 +1267,27 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (refs)
 		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
 
-	/*
-	 * Populate transport->got_remote_bundle_uri and
-	 * transport->bundle_uri. We might get nothing.
-	 */
-	transport_get_remote_bundle_uri(transport, 1);
+	if (!bundle_uri) {
+		/*
+		* Populate transport->got_remote_bundle_uri and
+		* transport->bundle_uri. We might get nothing.
+		*/
+		transport_get_remote_bundle_uri(transport, 1);
+
+		if (transport->bundles &&
+		    hashmap_get_size(&transport->bundles->bundles)) {
+			/* At this point, we need the_repository to match the cloned repo. */
+			if (repo_init(the_repository, git_dir, work_tree))
+				warning(_("failed to initialize the repo, skipping bundle URI"));
+			if (fetch_bundle_list(the_repository,
+					      remote->url[0],
+					      transport->bundles))
+				warning(_("failed to fetch advertised bundles"));
+		} else {
+			clear_bundle_list(transport->bundles);
+			FREE_AND_NULL(transport->bundles);
+		}
+	}
 
 	if (mapped_refs) {
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));

gitgitgadget

Makes sense.

I'm not sure I follow what this sentence is saying. Looking at the implementation below [1], you've added a call to 'transport_get_remote_bundle_uri()' in 'clone.c', but that's defined in 'transport.h' (which is already included in 'clone.c'). Why is 'connect.h' needed at all?

This explanation seems more complicated than necessary. I think it's sufficient to say "The no-op client is initially used only in 'clone' to test the basic functionality. The bundle URI client will be integrated into fetch, pull, etc. in later patches".

"skip the test" doesn't explain *which* test is skipped (and it doesn't look like you skip all of them). I think you're referring to "bad client with $T5730_PROTOCOL:// using protocol v2" specifically?

Maybe I'm missing something, but why not just rename 'got_remote_heads' to something like 'finished_handshake' rather than adding 'got_advertisement' (since, AFAICT, it's always identical in value to 'got_remote_heads').

This commit also introduces the 'quiet' flag to 'transport_get_remote_bundle_uri()', but there's no mention in the commit message. The message also doesn't explain the changes to existing tests (adding 'bundle.*' settings, swapping out 'git ls-remote' for the new 'test-tool bundle-uri ls-remote' in existing tests, etc.). I think these are all relevant to fully understanding the patch, so could you mention them in your next reroll?

This patch looks good - the implementation is pretty straightforward ("send a config value if its key matches 'bundle.*'), and the tests cover both single and multiple bundle URIs returned by the server.

Thanks for adding this, an "opt-in" approach seems reasonable for introducing this feature.

This makes sense. I'm less concerned with this environment variable than those in patch 2 [1], since it's in line with the test variables that enable/disable whole features ('GIT_TEST_SPLIT_INDEX', 'GIT_TEST_COMMIT_GRAPH', etc.).

The only thing feedback I can think of would be that this patch could be moved to earlier in the series (that is, immediately after creating 'transport_get_remote_bundle_uri()'). That said, I don't feel strongly either way.

[1] https://lore.kernel.org/git/[email protected]/

The implementation and documentation below align with the commit message. Looks good!

*Extremely* minor point, but why return anything at all? The call in the next patch doesn't use the return value, and some similar-in-spirit 'strbuf' functions (like 'strbuf_trim()') return nothing.

I don't think this is worth changing if you can imagine using that return value for something eventually; just wanted to point it out as something to (optionally) consider if you re-roll for something else anyway.

This explanation is clear and matches the implementation below. I'll admit it's a bit difficult to wrap my head around what's going on but, from what I understand, it does what it needs to do to set up for the next patch.

There's no way to test this change in this patch (since 'fetch_bundle_list()' isn't called anywhere yet), but I think that's fine; making it testable would probably make the patch too long/complicated to follow.

Could you add a test verifying that '--bundle-uri' causes 'clone' to skip bundle URI auto-discovery? It's clear from the implementation above that 'clone' is currently doing that as-expected, but it might be nice to have the test for regression testing purposes.

It's not. Good catch!

Definitely can be reworded. Specifically, fetches need more functionality (coming in part V) before there is value in that integration.

Will make the specific test to skip more clear.

I'm going to investigate how we can test similar functionality within the test-tool code instead, if possible.

I'll definitely split them out, making this change much more about the test boilerplate. In addition, most of the test boilerplate actually works without the 'git clone' update, so this can be split into three commits:

1. Create the test infrastructure to check that the server advertises the 'bundle-uri' command appropriately.

2. Implement the basic client that issues and parses the 'bundle-uri' command. Add the request to 'git clone' and add a test that verifies that the client makes the request.

3. Add the extra error condition tests.

I agree. I think 'lib-bundle-uri-protocol.sh' is a clearer name.

Will move, although I was not able to find the use in t9120.

I think this is one of the tests that doesn't work in HTTP, but could be skipped using a prereq if it is placed in the common test script.

I will rethink this test coverage to see if there is a different way to check similar behavior without as much insertion into the client/server code.

Yes, although it suffers from a mistake of this form I've seen before:
got_remote_bundle_uri is a single bit, so this only works every other time. I will fix this.

Thanks for the detailed review!
-Stolee

I think that is a reasonable recommendation.

I will think more on this as I get further into your review and figure out a way to do the error case tests. At minimum, I've split out some things so they might be easier to rearrange, but the 'git clone' integration is (currently) still paired with the implementation in transport.c.

This can be delayed until the next change that actually reads that config.

You're right. This is essentially inserting test code into the product (although the assert()s would be compiled out, I assume). The only differnce here is that after the handshake, protocol v2 has not executed the 'ls-refs' command, while the other protocol versions start with a ref advertisement in the initial response.

Thanks,
-Stolee