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
