Re: [PATCH 2/4] builtin/fetch.c: add fetch.serverOption configuration

[Patrick Steinhardt <ps@pks.im>]

On Mon, Sep 02, 2024 at 12:13:54PM +0000, Xing Xin via GitGitGadget wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c297569a473..d49dff8a76a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2231,7 +2241,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			 N_("accept refs that update .git/shallow")),
>  		OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
>  			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
> -		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
> +		OPT_STRING_LIST('o', "server-option", &option_server_options, N_("server-specific"),
> +				N_("option to transmit")),
>  		OPT_IPVERSION(&family),
>  		OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>  				N_("report that we have only objects reachable from this object")),
> @@ -2272,6 +2283,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_fetch_options, builtin_fetch_usage, 0);
>  
> +	server_options = option_server_options.nr ?
> +			 &option_server_options : &config_server_options;
> +
>  	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
>  		config.recurse_submodules = recurse_submodules_cli;
>  

Doesn't this mean that `server_options` will never be `NULL`? Why do we
have the new checks for whether or not `server_options` is set, like
e.g. in the next hunk.

> @@ -2418,8 +2432,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			result = 1;
>  			goto cleanup;
>  		}
> -		if (server_options.nr)
> -			gtransport->server_options = &server_options;
> +		if (server_options && server_options->nr)
> +			gtransport->server_options = server_options;
>  		result = transport_fetch_refs(gtransport, NULL);
>  
>  		oidset_iter_init(&acked_commits, &iter);
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 1ef540f73d3..ae25400010e 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -368,17 +368,45 @@ test_expect_success 'ref advertisement is filtered during fetch using protocol v
>  test_expect_success 'server-options are sent when fetching' '
>  	test_when_finished "rm -f log" &&
>  
> -	test_commit -C file_parent four &&
> -
> +	# Specify server options from command line
>  	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
>  		fetch -o hello -o world origin main &&
> +	test_grep "server-option=hello" log &&
> +	test_grep "server-option=world" log &&
> +	rm -f log &&
>  
> -	git -C file_child log -1 --format=%s origin/main >actual &&
> -	git -C file_parent log -1 --format=%s >expect &&
> -	test_cmp expect actual &&
> +	# Specify server options from fetch.serverOption config
> +	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
> +		-c fetch.serverOption=hello -c fetch.serverOption=world \
> +		fetch origin main &&
> +	test_grep "server-option=hello" log &&
> +	test_grep "server-option=world" log &&
> +	rm -f log &&
>  
> -	grep "server-option=hello" log &&
> -	grep "server-option=world" log
> +	# Cmdline server options take a higher priority
> +	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
> +		-c fetch.serverOption=hello -c fetch.serverOption=world \
> +		fetch -o foo=bar origin main &&
> +	test_grep ! "server-option=hello" log &&
> +	test_grep ! "server-option=world" log &&
> +	test_grep "server-option=foo=bar" log
> +'

I think it makes more sense to check for the new behaviour in new tests
exclusively. Otherwise one is left wondering whether any of the old
behaviour changed.

Patrick