[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v4 06/11] bundle-uri client: add helper for testing server
From:       Jeff King <peff () peff ! net>
Date:       2022-12-30 16:31:47
Message-ID: Y68Sc3V5x6xSucZW () coredump ! intra ! peff ! net
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]