[v4,06/11] bundle-uri client: add helper for testing server

Commit Message

Ævar Arnfjörð Bjarmason Dec. 22, 2022, 3:14 p.m. UTC

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

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.

In the "git clone" case we'll have already done the handshake(),
but not here. Add an extra case to check for this handshake in
get_bundle_uri() for ease of use for future callers.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 t/helper/test-bundle-uri.c   | 46 ++++++++++++++++++++++++++++++++++++
 t/lib-bundle-uri-protocol.sh | 46 ++++++++++++++++++++++++++++++++++++
 transport.c                  |  7 ++++++
 3 files changed, 99 insertions(+)

Comments

Jeff King Dec. 30, 2022, 4:31 p.m. UTC | #1

On Thu, Dec 22, 2022 at 03:14:12PM +0000, Ævar Arnfjörð Bjarmason via GitGitGadget wrote:

> +static int cmd_ls_remote(int argc, const char **argv)
> +{
> +	const char *uploadpack = NULL;
> +	struct string_list server_options = STRING_LIST_INIT_DUP;

These two variables are initialized to NULL and empty respectively, and
then not accessed until here:

> +	transport = transport_get(remote, NULL);
> +	if (uploadpack)
> +		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
> +	if (server_options.nr)
> +		transport->server_options = &server_options;

where neither conditional will trigger, since they will still be NULL
and empty.

Is this function missing some argv parsing that would affect these? Or
if it's not needed, would we want to remove them, like:

diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c
index 5df5bc3b89..b18e760310 100644
--- a/t/helper/test-bundle-uri.c
+++ b/t/helper/test-bundle-uri.c
@@ -76,8 +76,6 @@ static int cmd__bundle_uri_parse(int argc, const char **argv, enum input_mode mo
 
 static int cmd_ls_remote(int argc, const char **argv)
 {
-	const char *uploadpack = NULL;
-	struct string_list server_options = STRING_LIST_INIT_DUP;
 	const char *dest;
 	struct remote *remote;
 	struct transport *transport;
@@ -95,11 +93,6 @@ static int cmd_ls_remote(int argc, const char **argv)
 		die(_("remote '%s' has no configured URL"), dest);
 
 	transport = transport_get(remote, NULL);
-	if (uploadpack)
-		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
-	if (server_options.nr)
-		transport->server_options = &server_options;
-
 	if (transport_get_remote_bundle_uri(transport) < 0) {
 		error(_("could not get the bundle-uri list"));
 		status = 1;

Not a huge deal, but I noticed that Coverity complained about the
uploadpack one because this hit 'next' (the server_options one I found
manually, but it was kind of obvious when looking at the other).

-Peff

Derrick Stolee Jan. 5, 2023, 7:09 p.m. UTC | #2

On 12/30/22 11:31 AM, Jeff King wrote:
> On Thu, Dec 22, 2022 at 03:14:12PM +0000, Ævar Arnfjörð Bjarmason via GitGitGadget wrote:
> 
>> +static int cmd_ls_remote(int argc, const char **argv)
>> +{
>> +	const char *uploadpack = NULL;
>> +	struct string_list server_options = STRING_LIST_INIT_DUP;
> 
> These two variables are initialized to NULL and empty respectively, and
> then not accessed until here:
> 
>> +	transport = transport_get(remote, NULL);
>> +	if (uploadpack)
>> +		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
>> +	if (server_options.nr)
>> +		transport->server_options = &server_options;
> 
> where neither conditional will trigger, since they will still be NULL
> and empty.
> 
> Is this function missing some argv parsing that would affect these? Or
> if it's not needed, would we want to remove them, like:

...

> Not a huge deal, but I noticed that Coverity complained about the
> uploadpack one because this hit 'next' (the server_options one I found
> manually, but it was kind of obvious when looking at the other).

Yes, removing these lines would be fine. Perhaps there were
uses for these in an earlier version that were dropped. But
we can remove them now and then add them back when they
actually connect to functionality.

Thanks,
-Stolee

Patch

diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c
index 25afd393428..f8159187014 100644
--- a/t/helper/test-bundle-uri.c
+++ b/t/helper/test-bundle-uri.c
@@ -3,6 +3,10 @@ 
 #include "bundle-uri.h"
 #include "strbuf.h"
 #include "string-list.h"
+#include "transport.h"
+#include "ref-filter.h"
+#include "remote.h"
+#include "refs.h"
 
 enum input_mode {
 	KEY_VALUE_PAIRS,
@@ -68,6 +72,46 @@  usage:
 	usage_with_options(usage, options);
 }
 
+static int cmd_ls_remote(int argc, const char **argv)
+{
+	const char *uploadpack = NULL;
+	struct string_list server_options = STRING_LIST_INIT_DUP;
+	const char *dest;
+	struct remote *remote;
+	struct transport *transport;
+	int status = 0;
+
+	dest = argc > 1 ? argv[1] : NULL;
+
+	remote = remote_get(dest);
+	if (!remote) {
+		if (dest)
+			die(_("bad repository '%s'"), dest);
+		die(_("no remote configured to get bundle URIs from"));
+	}
+	if (!remote->url_nr)
+		die(_("remote '%s' has no configured URL"), dest);
+
+	transport = transport_get(remote, NULL);
+	if (uploadpack)
+		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
+	if (server_options.nr)
+		transport->server_options = &server_options;
+
+	if (transport_get_remote_bundle_uri(transport) < 0) {
+		error(_("could not get the bundle-uri list"));
+		status = 1;
+		goto cleanup;
+	}
+
+	print_bundle_list(stdout, transport->bundles);
+
+cleanup:
+	if (transport_disconnect(transport))
+		return 1;
+	return status;
+}
+
 int cmd__bundle_uri(int argc, const char **argv)
 {
 	const char *usage[] = {
@@ -88,6 +132,8 @@  int cmd__bundle_uri(int argc, const char **argv)
 		return cmd__bundle_uri_parse(argc - 1, argv + 1, KEY_VALUE_PAIRS);
 	if (!strcmp(argv[1], "parse-config"))
 		return cmd__bundle_uri_parse(argc - 1, argv + 1, CONFIG_FILE);
+	if (!strcmp(argv[1], "ls-remote"))
+		return cmd_ls_remote(argc - 1, argv + 1);
 	error("there is no test-tool bundle-uri tool '%s'", argv[1]);
 
 usage:
diff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh
index 75ea8c4418f..5620e230387 100644
--- a/t/lib-bundle-uri-protocol.sh
+++ b/t/lib-bundle-uri-protocol.sh
@@ -119,3 +119,49 @@  test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque
 	# Client issued bundle-uri command
 	grep "> command=bundle-uri" log
 '
+
+# The remaining tests will all assume transfer.bundleURI=true
+#
+# This test can be removed when transfer.bundleURI is enabled by default.
+test_expect_success 'enable transfer.bundleURI for remaining tests' '
+	git config --global transfer.bundleURI true
+'
+
+test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol v2" '
+	test_config -C "$BUNDLE_URI_PARENT" \
+		bundle.only.uri "$BUNDLE_URI_BUNDLE_URI_ESCAPED" &&
+
+	# All data about bundle URIs
+	cat >expect <<-EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	EOF
+
+	test-tool bundle-uri \
+		ls-remote \
+		"$BUNDLE_URI_REPO_URI" \
+		>actual &&
+	test_cmp_config_output expect actual
+'
+
+test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol v2 and extra data" '
+	test_config -C "$BUNDLE_URI_PARENT" \
+		bundle.only.uri "$BUNDLE_URI_BUNDLE_URI_ESCAPED" &&
+
+	# Extra data should be ignored
+	test_config -C "$BUNDLE_URI_PARENT" bundle.only.extra bogus &&
+
+	# All data about bundle URIs
+	cat >expect <<-EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	EOF
+
+	test-tool bundle-uri \
+		ls-remote \
+		"$BUNDLE_URI_REPO_URI" \
+		>actual &&
+	test_cmp_config_output expect actual
+'
diff --git a/transport.c b/transport.c
index 757ad552bf3..0f35114a13e 100644
--- a/transport.c
+++ b/transport.c
@@ -371,6 +371,13 @@  static int get_bundle_uri(struct transport *transport)
 		init_bundle_list(transport->bundles);
 	}
 
+	if (!data->finished_handshake) {
+		struct ref *refs = handshake(transport, 0, NULL, 0);
+
+		if (refs)
+			free_refs(refs);
+	}
+
 	/*
 	 * "Support" protocol v0 and v2 without bundle-uri support by
 	 * silently degrading to a NOOP.