| Message ID | 20190405204413.93900-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clone: send server options when using protocol v2 | expand |
Jonathan Tan wrote: > Commit 5e3548ef16 ("fetch: send server options when using protocol v2", > 2018-04-24) taught "fetch" the ability to send server options when using > protocol v2, but not "clone". This ability is triggered by "-o" or > "--server-option". > > Teach "clone" the same ability, except that because "clone" already > has "-o" for another parameter, teach "clone" only to receive > "--server-option". Can you give an example of what this would be used for? An example I can think of might be git clone --server-option=priority=batch <url> > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Documentation/git-clone.txt | 7 +++++++ > builtin/clone.c | 6 ++++++ > t/t5702-protocol-v2.sh | 11 +++++++++++ > 3 files changed, 24 insertions(+) Thanks. [...] > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -66,6 +66,7 @@ static int option_dissociate; > static int max_jobs = -1; > static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP; > static struct list_objects_filter_options filter_options; > +static struct string_list server_options = STRING_LIST_INIT_DUP; The other string-list options in this file all use NODUP. Is there a reason this one uses DUP instead? (Just curious --- I suspect either would work fine, since nothing here does tricks with modifying argv entries after option parsing.) The same question applies to the corresponding option in builtin/fetch.c, so while it is not likely to matter in practice, it would be nice for readability to find out. > > static int recurse_submodules_cb(const struct option *opt, > const char *arg, int unset) > @@ -137,6 +138,8 @@ static struct option builtin_clone_options[] = { > N_("separate git dir from working tree")), > OPT_STRING_LIST('c', "config", &option_config, N_("key=value"), > N_("set config inside the new repository")), > + OPT_STRING_LIST(0, "server-option", &server_options, N_("server-specific"), nit: long line > + N_("option to transmit")), [...] > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -251,6 +251,17 @@ test_expect_success 'server-options are sent when fetching' ' > grep "server-option=world" log > ' > > +test_expect_success 'server-options are sent when cloning' ' > + test_when_finished "rm -rf log myclone" && > + > + GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \ > + clone --server-option=hello --server-option=world \ > + "file://$(pwd)/file_parent" myclone && > + > + grep "server-option=hello" log && > + grep "server-option=world" log > +' Nice. Thanks for including this. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Thanks for a pleasant read.
> > Teach "clone" the same ability, except that because "clone" already > > has "-o" for another parameter, teach "clone" only to receive > > "--server-option". > > Can you give an example of what this would be used for? An example I > can think of might be > > git clone --server-option=priority=batch <url> protocol-v2.txt says that it is server-specific, so I don't think I can give any meaningful examples here. > > --- a/builtin/clone.c > > +++ b/builtin/clone.c > > @@ -66,6 +66,7 @@ static int option_dissociate; > > static int max_jobs = -1; > > static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP; > > static struct list_objects_filter_options filter_options; > > +static struct string_list server_options = STRING_LIST_INIT_DUP; > > The other string-list options in this file all use NODUP. Is there a > reason this one uses DUP instead? (Just curious --- I suspect either > would work fine, since nothing here does tricks with modifying argv > entries after option parsing.) > > The same question applies to the corresponding option in > builtin/fetch.c, so while it is not likely to matter in practice, it > would be nice for readability to find out. I guess it could be either. It's true that I just copied it from builtin/fetch.c. I'll change it to NODUP if a reroll is needed for another reason. > > + OPT_STRING_LIST(0, "server-option", &server_options, N_("server-specific"), > > nit: long line I'll change this if a reroll is needed for another reason. > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > Thanks for a pleasant read. Thanks for your review.
Jonathan Tan <jonathantanmy@google.com> writes: >> > Teach "clone" the same ability, except that because "clone" already >> > has "-o" for another parameter, teach "clone" only to receive >> > "--server-option". >> >> Can you give an example of what this would be used for? An example I >> can think of might be >> >> git clone --server-option=priority=batch <url> > > protocol-v2.txt says that it is server-specific, so I don't think I can > give any meaningful examples here. Does the code behave sensibly when the --server-option=... option is given and (a) the given option is not understood by the other side that talks protocol v2? Or (b) it turns out that the other side does not talk protocol v2? In the former case, I would expect that there would be a way to respond with a failure for the other side and "git clone" should notice and respond by die()-ing at the end. In the latter case, I would expect at least that a warning about accepted but ignored server-option to be given, or "git clone" to fail with an error "cannot honor the server-option". If the code does not match the above expectations, at least that should be documented, I think. If "git fetch" shares the same issue, this would be a good time to address it, too. Thanks.
> Does the code behave sensibly when the --server-option=... option is > given and > > (a) the given option is not understood by the other side that talks > protocol v2? Or > > (b) it turns out that the other side does not talk protocol v2? > > In the former case, I would expect that there would be a way to > respond with a failure for the other side and "git clone" should > notice and respond by die()-ing at the end. Right now, the server ignores it (it collects them as "keys" in process_request() in serve.c and then passes them to the individual commands, but they don't do anything about it). Right now it's documented as: -o <option>:: --server-option=<option>:: Transmit the given string to the server when communicating using protocol version 2. The given string must not contain a NUL or LF character. When multiple `--server-option=<option>` are given, they are all sent to the other side in the order listed on the command line. I'm inclined to add: The server's handling of server options, including unknown ones, is server-specific. > In the latter case, I would expect at least that a warning about > accepted but ignored server-option to be given, or "git clone" to > fail with an error "cannot honor the server-option". Right now there's no warning, but I'll add one in the next reroll to both "fetch" and "clone". > If the code does not match the above expectations, at least that > should be documented, I think. If "git fetch" shares the same > issue, this would be a good time to address it, too.
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index 2fd12524f9..5b79da359c 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -131,6 +131,13 @@ objects from the source repository into a pack in the cloned repository. is specified. This flag forces progress status even if the standard error stream is not directed to a terminal. +--server-option=<option>:: + Transmit the given string to the server when communicating using + protocol version 2. The given string must not contain a NUL or LF + character. + When multiple `--server-option=<option>` are given, they are all + sent to the other side in the order listed on the command line. + --no-checkout:: -n:: No checkout of HEAD is performed after the clone is complete. diff --git a/builtin/clone.c b/builtin/clone.c index 50bde99618..625d47c1f4 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -66,6 +66,7 @@ static int option_dissociate; static int max_jobs = -1; static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP; static struct list_objects_filter_options filter_options; +static struct string_list server_options = STRING_LIST_INIT_DUP; static int recurse_submodules_cb(const struct option *opt, const char *arg, int unset) @@ -137,6 +138,8 @@ static struct option builtin_clone_options[] = { N_("separate git dir from working tree")), OPT_STRING_LIST('c', "config", &option_config, N_("key=value"), N_("set config inside the new repository")), + OPT_STRING_LIST(0, "server-option", &server_options, N_("server-specific"), + N_("option to transmit")), OPT_SET_INT('4', "ipv4", &family, N_("use IPv4 addresses only"), TRANSPORT_FAMILY_IPV4), OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"), @@ -1136,6 +1139,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) transport_set_option(transport, TRANS_OPT_UPLOADPACK, option_upload_pack); + if (server_options.nr) + transport->server_options = &server_options; + if (filter_options.choice) { struct strbuf expanded_filter_spec = STRBUF_INIT; expand_list_objects_filter_spec(&filter_options, diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index e112b6086c..5b28d88401 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -251,6 +251,17 @@ test_expect_success 'server-options are sent when fetching' ' grep "server-option=world" log ' +test_expect_success 'server-options are sent when cloning' ' + test_when_finished "rm -rf log myclone" && + + GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \ + clone --server-option=hello --server-option=world \ + "file://$(pwd)/file_parent" myclone && + + grep "server-option=hello" log && + grep "server-option=world" log +' + test_expect_success 'upload-pack respects config using protocol v2' ' git init server && write_script server/.git/hook <<-\EOF &&
Commit 5e3548ef16 ("fetch: send server options when using protocol v2", 2018-04-24) taught "fetch" the ability to send server options when using protocol v2, but not "clone". This ability is triggered by "-o" or "--server-option". Teach "clone" the same ability, except that because "clone" already has "-o" for another parameter, teach "clone" only to receive "--server-option". Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/git-clone.txt | 7 +++++++ builtin/clone.c | 6 ++++++ t/t5702-protocol-v2.sh | 11 +++++++++++ 3 files changed, 24 insertions(+)