Re: [PATCH 2/4] builtin/fetch.c: add fetch.serverOption configuration
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 3, 2024, 10:09 UTC
- Message-ID
- <ZtbgZiVNrWVMBIWr@pks.im>
- In-Reply-To
- <7e2d5c504d762bb9cdb9ca008b92d349d81c194b.1725279236.git.gitgitgadget@gmail.com>
On Mon, Sep 02, 2024 at 12:13:54PM +0000, Xing Xin via GitGitGadget wrote:
Show 24 quoted lines
> 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.
Show 49 quoted lines
> @@ -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