[PATCH] clone: send server options when using protocol v2

[PATCH] clone: send server options when using protocol v2

[Jonathan Tan]

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(+)

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 &&
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH] clone: send server options when using protocol v2

[Jonathan Nieder]

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.

Re: [PATCH] clone: send server options when using protocol v2

[Jonathan Tan]

> > 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.

Re: [PATCH] clone: send server options when using protocol v2

[Junio C Hamano]

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.

Re: [PATCH] clone: send server options when using protocol v2

[Jonathan Tan]

> 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.

[PATCH v2 0/2] Server options when cloning

[Jonathan Tan]

I created v2 by rewriting jt/clone-server-option, so you'll see some
Reviewed-by lines and Signed-off-by lines with another name than mine.

Changes from v1:
 - Broke long line. (DUP->NODUP was already done by Junio in
   jt/clone-server-option, so you don't see it in the range-diff below.)
 - Warn if server-option is specified but not sent to the server (in
   fetch and clone).
 - Document that server's handling of server-option is server-specific.

These patches are on maint, matching jt/clone-server-option, but should
apply cleanly on master too.

Jonathan Tan (2):
  transport: warn if server options are unsupported
  clone: send server options when using protocol v2

 Documentation/fetch-options.txt |  3 ++-
 Documentation/git-clone.txt     |  8 +++++++
 builtin/clone.c                 |  6 ++++++
 t/t5702-protocol-v2.sh          | 38 +++++++++++++++++++++++++++++++++
 transport.c                     |  8 +++++++
 5 files changed, 62 insertions(+), 1 deletion(-)

Range-diff against v1:
-:  ---------- > 1:  af3cc05324 transport: warn if server options are unsupported
1:  90ce94e039 ! 2:  142c25abd2 clone: send server options when using protocol v2
    @@ -11,10 +11,27 @@
         has "-o" for another parameter, teach "clone" only to receive
         "--server-option".
     
    +    Explain in the documentation, both for clone and for fetch, that server
    +    handling of server options are server-specific.
    +
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
         Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    + diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
    + --- a/Documentation/fetch-options.txt
    + +++ b/Documentation/fetch-options.txt
    +@@
    + --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.
    ++	character.  The server's handling of server options, including
    ++	unknown ones, is server-specific.
    + 	When multiple `--server-option=<option>` are given, they are all
    + 	sent to the other side in the order listed on the command line.
    + 
    +
      diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
      --- a/Documentation/git-clone.txt
      +++ b/Documentation/git-clone.txt
    @@ -25,7 +42,8 @@
     +--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.
    ++	character.  The server's handling of server options, including
    ++	unknown ones, is server-specific.
     +	When multiple `--server-option=<option>` are given, they are all
     +	sent to the other side in the order listed on the command line.
     +
    @@ -48,8 +66,8 @@
      		   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_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"),
    @@ -68,7 +86,7 @@
      --- a/t/t5702-protocol-v2.sh
      +++ b/t/t5702-protocol-v2.sh
     @@
    - 	grep "server-option=world" log
    + 	grep "Ignoring server options" err
      '
      
     +test_expect_success 'server-options are sent when cloning' '
    @@ -81,6 +99,16 @@
     +	grep "server-option=hello" log &&
     +	grep "server-option=world" log
     +'
    ++
    ++test_expect_success 'warn if using server-option with clone with legacy protocol' '
    ++	test_when_finished "rm -rf myclone" &&
    ++
    ++	GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
    ++		clone --server-option=hello --server-option=world \
    ++		"file://$(pwd)/file_parent" myclone 2>err &&
    ++
    ++	grep "Ignoring server options" err
    ++'
     +
      test_expect_success 'upload-pack respects config using protocol v2' '
      	git init server &&
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v2 1/2] transport: warn if server options are unsupported

[Jonathan Tan]

Server options were added in commit 5e3548ef16 ("fetch: send server
options when using protocol v2", 2018-04-24), supported only for
protocol version 2. Add a warning if server options are specified for
the user if a legacy protocol is used instead.

An effort is made to avoid printing the same warning more than once by
clearing transport->server_options after the warning, but this does not
fully avoid double-printing (for example, when backfulling tags using
another fetch with a non-reusable transport).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5702-protocol-v2.sh | 17 +++++++++++++++++
 transport.c            |  8 ++++++++
 2 files changed, 25 insertions(+)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index db4ae09f2f..f1a3ff1021 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -182,6 +182,12 @@ test_expect_success 'server-options are sent when using ls-remote' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
+	GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
+		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
+
+	grep "Ignoring server options" err
+'
 
 test_expect_success 'clone with file:// using protocol v2' '
 	test_when_finished "rm -f log" &&
@@ -251,6 +257,17 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'warn if using server-option with fetch with legacy protocol' '
+	test_when_finished "rm -rf temp_child" &&
+
+	git init temp_child &&
+
+	GIT_TEST_PROTOCOL_VERSION=0 git -C temp_child -c protocol.version=0 \
+		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
+
+	grep "Ignoring server options" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
diff --git a/transport.c b/transport.c
index e078812897..14f14ef93c 100644
--- a/transport.c
+++ b/transport.c
@@ -252,6 +252,12 @@ static int connect_setup(struct transport *transport, int for_push)
 	return 0;
 }
 
+static void warn_server_options_unsupported(struct transport *transport)
+{
+	warning(_("Ignoring server options because protocol version does not support it"));
+	transport->server_options = NULL;
+}
+
 /*
  * Obtains the protocol version from the transport and writes it to
  * transport->data->version, first connecting if not already connected.
@@ -286,6 +292,7 @@ static struct ref *handshake(struct transport *transport, int for_push,
 		break;
 	case protocol_v1:
 	case protocol_v0:
+		warn_server_options_unsupported(transport);
 		get_remote_heads(&reader, &refs,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
@@ -363,6 +370,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 		break;
 	case protocol_v1:
 	case protocol_v0:
+		warn_server_options_unsupported(transport);
 		refs = fetch_pack(&args, data->fd, data->conn,
 				  refs_tmp ? refs_tmp : transport->remote_refs,
 				  dest, to_fetch, nr_heads, &data->shallow,
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v2 2/2] clone: send server options when using protocol v2

[Jonathan Tan]

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".

Explain in the documentation, both for clone and for fetch, that server
handling of server options are server-specific.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/fetch-options.txt |  3 ++-
 Documentation/git-clone.txt     |  8 ++++++++
 builtin/clone.c                 |  6 ++++++
 t/t5702-protocol-v2.sh          | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fa0a3151b3..91c47752ec 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -216,7 +216,8 @@ endif::git-pull[]
 --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.
+	character.  The server's handling of server options, including
+	unknown ones, is server-specific.
 	When multiple `--server-option=<option>` are given, they are all
 	sent to the other side in the order listed on the command line.
 
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 2fd12524f9..a0f14b51f2 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -131,6 +131,14 @@ 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.  The server's handling of server options, including
+	unknown ones, is server-specific.
+	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..31a47d190a 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_NODUP;
 
 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 f1a3ff1021..7ecef5f768 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -268,6 +268,27 @@ test_expect_success 'warn if using server-option with fetch with legacy protocol
 	grep "Ignoring server options" err
 '
 
+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 'warn if using server-option with clone with legacy protocol' '
+	test_when_finished "rm -rf myclone" &&
+
+	GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
+		clone --server-option=hello --server-option=world \
+		"file://$(pwd)/file_parent" myclone 2>err &&
+
+	grep "Ignoring server options" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v2 1/2] transport: warn if server options are unsupported

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> Server options were added in commit 5e3548ef16 ("fetch: send server
> options when using protocol v2", 2018-04-24), supported only for
> protocol version 2. Add a warning if server options are specified for
> the user if a legacy protocol is used instead.
>
> An effort is made to avoid printing the same warning more than once by
> clearing transport->server_options after the warning, but this does not
> fully avoid double-printing (for example, when backfulling tags using
> another fetch with a non-reusable transport).
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  t/t5702-protocol-v2.sh | 17 +++++++++++++++++
>  transport.c            |  8 ++++++++
>  2 files changed, 25 insertions(+)

Thanks for writing this.  I'd be in favor of making this die().
Compare what we already do in push:

	if (args->push_options && !push_options_supported)
		die(_("the receiving end does not support push options"));

What happens in the case of push with protocol v0, where server options
are supported?

[...]
> --- a/transport.c
> +++ b/transport.c
> @@ -252,6 +252,12 @@ static int connect_setup(struct transport *transport, int for_push)
>  	return 0;
>  }
>  
> +static void warn_server_options_unsupported(struct transport *transport)
> +{
> +	warning(_("Ignoring server options because protocol version does not support it"));

nits:
- error and warning messages tend to use lowercase
- the user running into this may want to know how to switch to a better
  protocol version.  Is there e.g. a manpage we can point them to?

For example:

	fatal: server options require protocol version 2 or later
	hint: see protocol.version in "git help config" for more details

> +	transport->server_options = NULL;

Should this use a static to also warn only once in the tag-catchup case
you mentioned?

> +}
> +
>  /*
>   * Obtains the protocol version from the transport and writes it to
>   * transport->data->version, first connecting if not already connected.
> @@ -286,6 +292,7 @@ static struct ref *handshake(struct transport *transport, int for_push,
>  		break;
>  	case protocol_v1:
>  	case protocol_v0:
> +		warn_server_options_unsupported(transport);
>  		get_remote_heads(&reader, &refs,
>  				 for_push ? REF_NORMAL : 0,
>  				 &data->extra_have,
> @@ -363,6 +370,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>  		break;
>  	case protocol_v1:
>  	case protocol_v0:
> +		warn_server_options_unsupported(transport);
>  		refs = fetch_pack(&args, data->fd, data->conn,

Looks like this only affects fetch, so the question above about push
is only about the commit message.

With whatever subset of the suggested changes makes sense,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v2 2/2] clone: send server options when using protocol v2

[Jonathan Nieder]

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".
>
> Explain in the documentation, both for clone and for fetch, that server
> handling of server options are server-specific.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/fetch-options.txt |  3 ++-
>  Documentation/git-clone.txt     |  8 ++++++++
>  builtin/clone.c                 |  6 ++++++
>  t/t5702-protocol-v2.sh          | 21 +++++++++++++++++++++
>  4 files changed, 37 insertions(+), 1 deletion(-)

This is indeed
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v2 1/2] transport: warn if server options are unsupported

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thanks for writing this.  I'd be in favor of making this die().
> Compare what we already do in push:
>
> 	if (args->push_options && !push_options_supported)
> 		die(_("the receiving end does not support push options"));

That's a good point.

> What happens in the case of push with protocol v0, where server options
> are supported?

Are, or are not?

In any case, all other suggestions in your reviews are worth
following up, I would think.  Thanks for a quick review.

[PATCH v3 0/2] Server options when cloning

[Jonathan Tan]

Thanks, Jonathan Nieder, for your review.

> Thanks for writing this.  I'd be in favor of making this die().
> Compare what we already do in push:
> 
> 	if (args->push_options && !push_options_supported)
> 		die(_("the receiving end does not support push options"));

Thanks for pointing out what "push" does, and done.

> What happens in the case of push with protocol v0, where server options
> are supported?

They are just sent to the pre-receive and post-receive hooks, according
to the "git push" documentation. I added a mention of the push option
behavior in the 2nd commit's message.

> For example:
> 
> 	fatal: server options require protocol version 2 or later
> 	hint: see protocol.version in "git help config" for more details

Thanks - used your example (except I put the hint first, since the fatal
line is fatal).

> Should this use a static to also warn only once in the tag-catchup case
> you mentioned?

Since we're dying, this is no longer needed.

Jonathan Tan (2):
  transport: warn if server options are unsupported
  clone: send server options when using protocol v2

 Documentation/fetch-options.txt |  3 ++-
 Documentation/git-clone.txt     |  8 +++++++
 builtin/clone.c                 |  6 +++++
 t/t5702-protocol-v2.sh          | 41 +++++++++++++++++++++++++++++++++
 transport.c                     | 10 ++++++++
 5 files changed, 67 insertions(+), 1 deletion(-)

Range-diff against v2:
1:  af3cc05324 ! 1:  63049081c9 transport: warn if server options are unsupported
    @@ -4,13 +4,13 @@
     
         Server options were added in commit 5e3548ef16 ("fetch: send server
         options when using protocol v2", 2018-04-24), supported only for
    -    protocol version 2. Add a warning if server options are specified for
    -    the user if a legacy protocol is used instead.
    +    protocol version 2. But if the user specifies server options, and the
    +    protocol version being used doesn't support them, the server options are
    +    silently ignored.
     
    -    An effort is made to avoid printing the same warning more than once by
    -    clearing transport->server_options after the warning, but this does not
    -    fully avoid double-printing (for example, when backfulling tags using
    -    another fetch with a non-reusable transport).
    +    Teach any transport users to die instead in this situation, just like
    +    how "push" dies if push options are provided when the server doesn't
    +    support them.
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
    @@ -22,10 +22,11 @@
      '
      
     +test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
    -+	GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
    ++	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
     +		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
     +
    -+	grep "Ignoring server options" err
    ++	grep "see protocol.version in" err &&
    ++	grep "server options require protocol version 2 or later" err
     +'
      
      test_expect_success 'clone with file:// using protocol v2' '
    @@ -39,10 +40,11 @@
     +
     +	git init temp_child &&
     +
    -+	GIT_TEST_PROTOCOL_VERSION=0 git -C temp_child -c protocol.version=0 \
    ++	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -C temp_child -c protocol.version=0 \
     +		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
     +
    -+	grep "Ignoring server options" err
    ++	grep "see protocol.version in" err &&
    ++	grep "server options require protocol version 2 or later" err
     +'
     +
      test_expect_success 'upload-pack respects config using protocol v2' '
    @@ -56,10 +58,12 @@
      	return 0;
      }
      
    -+static void warn_server_options_unsupported(struct transport *transport)
    ++static void die_if_server_options(struct transport *transport)
     +{
    -+	warning(_("Ignoring server options because protocol version does not support it"));
    -+	transport->server_options = NULL;
    ++	if (!transport->server_options || !transport->server_options->nr)
    ++		return;
    ++	advise(_("see protocol.version in 'git help config' for more details"));
    ++	die(_("server options require protocol version 2 or later"));
     +}
     +
      /*
    @@ -69,7 +73,7 @@
      		break;
      	case protocol_v1:
      	case protocol_v0:
    -+		warn_server_options_unsupported(transport);
    ++		die_if_server_options(transport);
      		get_remote_heads(&reader, &refs,
      				 for_push ? REF_NORMAL : 0,
      				 &data->extra_have,
    @@ -77,7 +81,7 @@
      		break;
      	case protocol_v1:
      	case protocol_v0:
    -+		warn_server_options_unsupported(transport);
    ++		die_if_server_options(transport);
      		refs = fetch_pack(&args, data->fd, data->conn,
      				  refs_tmp ? refs_tmp : transport->remote_refs,
      				  dest, to_fetch, nr_heads, &data->shallow,
2:  142c25abd2 ! 2:  f59b8244eb clone: send server options when using protocol v2
    @@ -12,7 +12,9 @@
         "--server-option".
     
         Explain in the documentation, both for clone and for fetch, that server
    -    handling of server options are server-specific.
    +    handling of server options are server-specific. This is similar to
    +    receive-pack's handling of push options - currently, they are just sent
    +    to hooks to interpret as they see fit.
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
         Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
    @@ -86,7 +88,7 @@
      --- a/t/t5702-protocol-v2.sh
      +++ b/t/t5702-protocol-v2.sh
     @@
    - 	grep "Ignoring server options" err
    + 	grep "server options require protocol version 2 or later" err
      '
      
     +test_expect_success 'server-options are sent when cloning' '
    @@ -103,11 +105,12 @@
     +test_expect_success 'warn if using server-option with clone with legacy protocol' '
     +	test_when_finished "rm -rf myclone" &&
     +
    -+	GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
    ++	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
     +		clone --server-option=hello --server-option=world \
     +		"file://$(pwd)/file_parent" myclone 2>err &&
     +
    -+	grep "Ignoring server options" err
    ++	grep "see protocol.version in" err &&
    ++	grep "server options require protocol version 2 or later" err
     +'
     +
      test_expect_success 'upload-pack respects config using protocol v2' '
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v3 1/2] transport: warn if server options are unsupported

[Jonathan Tan]

Server options were added in commit 5e3548ef16 ("fetch: send server
options when using protocol v2", 2018-04-24), supported only for
protocol version 2. But if the user specifies server options, and the
protocol version being used doesn't support them, the server options are
silently ignored.

Teach any transport users to die instead in this situation, just like
how "push" dies if push options are provided when the server doesn't
support them.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5702-protocol-v2.sh | 19 +++++++++++++++++++
 transport.c            | 10 ++++++++++
 2 files changed, 29 insertions(+)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index db4ae09f2f..7ddcf26e76 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -182,6 +182,13 @@ test_expect_success 'server-options are sent when using ls-remote' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
+	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
+		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
+
+	grep "see protocol.version in" err &&
+	grep "server options require protocol version 2 or later" err
+'
 
 test_expect_success 'clone with file:// using protocol v2' '
 	test_when_finished "rm -f log" &&
@@ -251,6 +258,18 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'warn if using server-option with fetch with legacy protocol' '
+	test_when_finished "rm -rf temp_child" &&
+
+	git init temp_child &&
+
+	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -C temp_child -c protocol.version=0 \
+		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
+
+	grep "see protocol.version in" err &&
+	grep "server options require protocol version 2 or later" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
diff --git a/transport.c b/transport.c
index e078812897..77446119da 100644
--- a/transport.c
+++ b/transport.c
@@ -252,6 +252,14 @@ static int connect_setup(struct transport *transport, int for_push)
 	return 0;
 }
 
+static void die_if_server_options(struct transport *transport)
+{
+	if (!transport->server_options || !transport->server_options->nr)
+		return;
+	advise(_("see protocol.version in 'git help config' for more details"));
+	die(_("server options require protocol version 2 or later"));
+}
+
 /*
  * Obtains the protocol version from the transport and writes it to
  * transport->data->version, first connecting if not already connected.
@@ -286,6 +294,7 @@ static struct ref *handshake(struct transport *transport, int for_push,
 		break;
 	case protocol_v1:
 	case protocol_v0:
+		die_if_server_options(transport);
 		get_remote_heads(&reader, &refs,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
@@ -363,6 +372,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 		break;
 	case protocol_v1:
 	case protocol_v0:
+		die_if_server_options(transport);
 		refs = fetch_pack(&args, data->fd, data->conn,
 				  refs_tmp ? refs_tmp : transport->remote_refs,
 				  dest, to_fetch, nr_heads, &data->shallow,
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v3 2/2] clone: send server options when using protocol v2

[Jonathan Tan]

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".

Explain in the documentation, both for clone and for fetch, that server
handling of server options are server-specific. This is similar to
receive-pack's handling of push options - currently, they are just sent
to hooks to interpret as they see fit.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/fetch-options.txt |  3 ++-
 Documentation/git-clone.txt     |  8 ++++++++
 builtin/clone.c                 |  6 ++++++
 t/t5702-protocol-v2.sh          | 22 ++++++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fa0a3151b3..91c47752ec 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -216,7 +216,8 @@ endif::git-pull[]
 --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.
+	character.  The server's handling of server options, including
+	unknown ones, is server-specific.
 	When multiple `--server-option=<option>` are given, they are all
 	sent to the other side in the order listed on the command line.
 
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 2fd12524f9..a0f14b51f2 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -131,6 +131,14 @@ 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.  The server's handling of server options, including
+	unknown ones, is server-specific.
+	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..31a47d190a 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_NODUP;
 
 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 7ddcf26e76..b8d5264a45 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -270,6 +270,28 @@ test_expect_success 'warn if using server-option with fetch with legacy protocol
 	grep "server options require protocol version 2 or later" err
 '
 
+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 'warn if using server-option with clone with legacy protocol' '
+	test_when_finished "rm -rf myclone" &&
+
+	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
+		clone --server-option=hello --server-option=world \
+		"file://$(pwd)/file_parent" myclone 2>err &&
+
+	grep "see protocol.version in" err &&
+	grep "server options require protocol version 2 or later" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v3 1/2] transport: warn if server options are unsupported

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> +test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
> +	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
> +		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&

test_must_fail is not an executable but is a shell function.  Do not
expect VAR=VAL in front of it to work as expected.

"make test" triggered test-lint that caught this.

[PATCH v4 0/2] Server options when cloning

[Jonathan Tan]

Thanks, Junio, for spotting the test_must_fail interaction with
environment variables. I've updated that, and I also noticed that the
first patch's commit message title should say "die" instead of "warn",
so here is another version.

Jonathan Tan (2):
  transport: die if server options are unsupported
  clone: send server options when using protocol v2

 Documentation/fetch-options.txt |  3 ++-
 Documentation/git-clone.txt     |  8 +++++++
 builtin/clone.c                 |  6 +++++
 t/t5702-protocol-v2.sh          | 41 +++++++++++++++++++++++++++++++++
 transport.c                     | 10 ++++++++
 5 files changed, 67 insertions(+), 1 deletion(-)

Range-diff against v3:
1:  63049081c9 ! 1:  598b2de016 transport: warn if server options are unsupported
    @@ -1,6 +1,6 @@
     Author: Jonathan Tan <jonathantanmy@google.com>
     
    -    transport: warn if server options are unsupported
    +    transport: die if server options are unsupported
     
         Server options were added in commit 5e3548ef16 ("fetch: send server
         options when using protocol v2", 2018-04-24), supported only for
    @@ -22,7 +22,7 @@
      '
      
     +test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
    -+	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
    ++	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
     +		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
     +
     +	grep "see protocol.version in" err &&
    @@ -40,7 +40,7 @@
     +
     +	git init temp_child &&
     +
    -+	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -C temp_child -c protocol.version=0 \
    ++	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -C temp_child -c protocol.version=0 \
     +		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
     +
     +	grep "see protocol.version in" err &&
2:  f59b8244eb ! 2:  48bdd462c4 clone: send server options when using protocol v2
    @@ -105,7 +105,7 @@
     +test_expect_success 'warn if using server-option with clone with legacy protocol' '
     +	test_when_finished "rm -rf myclone" &&
     +
    -+	GIT_TEST_PROTOCOL_VERSION=0 test_must_fail git -c protocol.version=0 \
    ++	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
     +		clone --server-option=hello --server-option=world \
     +		"file://$(pwd)/file_parent" myclone 2>err &&
     +
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v4 1/2] transport: die if server options are unsupported

[Jonathan Tan]

Server options were added in commit 5e3548ef16 ("fetch: send server
options when using protocol v2", 2018-04-24), supported only for
protocol version 2. But if the user specifies server options, and the
protocol version being used doesn't support them, the server options are
silently ignored.

Teach any transport users to die instead in this situation, just like
how "push" dies if push options are provided when the server doesn't
support them.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5702-protocol-v2.sh | 19 +++++++++++++++++++
 transport.c            | 10 ++++++++++
 2 files changed, 29 insertions(+)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index db4ae09f2f..1e8357a4c7 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -182,6 +182,13 @@ test_expect_success 'server-options are sent when using ls-remote' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
+		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
+
+	grep "see protocol.version in" err &&
+	grep "server options require protocol version 2 or later" err
+'
 
 test_expect_success 'clone with file:// using protocol v2' '
 	test_when_finished "rm -f log" &&
@@ -251,6 +258,18 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'warn if using server-option with fetch with legacy protocol' '
+	test_when_finished "rm -rf temp_child" &&
+
+	git init temp_child &&
+
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -C temp_child -c protocol.version=0 \
+		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
+
+	grep "see protocol.version in" err &&
+	grep "server options require protocol version 2 or later" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
diff --git a/transport.c b/transport.c
index e078812897..77446119da 100644
--- a/transport.c
+++ b/transport.c
@@ -252,6 +252,14 @@ static int connect_setup(struct transport *transport, int for_push)
 	return 0;
 }
 
+static void die_if_server_options(struct transport *transport)
+{
+	if (!transport->server_options || !transport->server_options->nr)
+		return;
+	advise(_("see protocol.version in 'git help config' for more details"));
+	die(_("server options require protocol version 2 or later"));
+}
+
 /*
  * Obtains the protocol version from the transport and writes it to
  * transport->data->version, first connecting if not already connected.
@@ -286,6 +294,7 @@ static struct ref *handshake(struct transport *transport, int for_push,
 		break;
 	case protocol_v1:
 	case protocol_v0:
+		die_if_server_options(transport);
 		get_remote_heads(&reader, &refs,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
@@ -363,6 +372,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 		break;
 	case protocol_v1:
 	case protocol_v0:
+		die_if_server_options(transport);
 		refs = fetch_pack(&args, data->fd, data->conn,
 				  refs_tmp ? refs_tmp : transport->remote_refs,
 				  dest, to_fetch, nr_heads, &data->shallow,
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v4 2/2] clone: send server options when using protocol v2

[Jonathan Tan]

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".

Explain in the documentation, both for clone and for fetch, that server
handling of server options are server-specific. This is similar to
receive-pack's handling of push options - currently, they are just sent
to hooks to interpret as they see fit.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/fetch-options.txt |  3 ++-
 Documentation/git-clone.txt     |  8 ++++++++
 builtin/clone.c                 |  6 ++++++
 t/t5702-protocol-v2.sh          | 22 ++++++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fa0a3151b3..91c47752ec 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -216,7 +216,8 @@ endif::git-pull[]
 --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.
+	character.  The server's handling of server options, including
+	unknown ones, is server-specific.
 	When multiple `--server-option=<option>` are given, they are all
 	sent to the other side in the order listed on the command line.
 
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 2fd12524f9..a0f14b51f2 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -131,6 +131,14 @@ 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.  The server's handling of server options, including
+	unknown ones, is server-specific.
+	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..31a47d190a 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_NODUP;
 
 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 1e8357a4c7..2a3575d38c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -270,6 +270,28 @@ test_expect_success 'warn if using server-option with fetch with legacy protocol
 	grep "server options require protocol version 2 or later" err
 '
 
+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 'warn if using server-option with clone with legacy protocol' '
+	test_when_finished "rm -rf myclone" &&
+
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
+		clone --server-option=hello --server-option=world \
+		"file://$(pwd)/file_parent" myclone 2>err &&
+
+	grep "see protocol.version in" err &&
+	grep "server options require protocol version 2 or later" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v4 1/2] transport: die if server options are unsupported

[SZEDER Gábor]

On Fri, Apr 12, 2019 at 12:51:21PM -0700, Jonathan Tan wrote:

[Reordering the diffs...]

> diff --git a/transport.c b/transport.c
> index e078812897..77446119da 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -252,6 +252,14 @@ static int connect_setup(struct transport *transport, int for_push)
>  	return 0;
>  }
>  
> +static void die_if_server_options(struct transport *transport)
> +{
> +	if (!transport->server_options || !transport->server_options->nr)
> +		return;
> +	advise(_("see protocol.version in 'git help config' for more details"));
> +	die(_("server options require protocol version 2 or later"));

These two messages are translated ...


> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index db4ae09f2f..1e8357a4c7 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -182,6 +182,13 @@ test_expect_success 'server-options are sent when using ls-remote' '
>  	grep "server-option=world" log
>  '
>  
> +test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
> +	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
> +		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
> +
> +	grep "see protocol.version in" err &&
> +	grep "server options require protocol version 2 or later" err

... therefore these should be 'test_i18ngrep'.  This applies to the
other tests in this patch series as well.

> +'
>  
>  test_expect_success 'clone with file:// using protocol v2' '
>  	test_when_finished "rm -f log" &&
> @@ -251,6 +258,18 @@ test_expect_success 'server-options are sent when fetching' '
>  	grep "server-option=world" log
>  '
>  
> +test_expect_success 'warn if using server-option with fetch with legacy protocol' '
> +	test_when_finished "rm -rf temp_child" &&
> +
> +	git init temp_child &&
> +
> +	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -C temp_child -c protocol.version=0 \
> +		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
> +
> +	grep "see protocol.version in" err &&
> +	grep "server options require protocol version 2 or later" err
> +'
> +
>  test_expect_success 'upload-pack respects config using protocol v2' '
>  	git init server &&
>  	write_script server/.git/hook <<-\EOF &&

Re* [PATCH v4 1/2] transport: die if server options are unsupported

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> ... therefore these should be 'test_i18ngrep'.  This applies to the
> other tests in this patch series as well.
> ...

Hmph.  That would mean the following is needed, but I suspect that
6cdccfce ("i18n: make GETTEXT_POISON a runtime option", 2018-11-08)
is somewhat broken (see the bottom for a suggested fix).

 t/t5702-protocol-v2.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1e8357a4c7..f8912211e0 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -186,8 +186,8 @@ test_expect_success 'warn if using server-option with ls-remote with legacy prot
 	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
 		ls-remote -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
 
-	grep "see protocol.version in" err &&
-	grep "server options require protocol version 2 or later" err
+	test_i18ngrep "see protocol.version in" err &&
+	test_i18ngrep "server options require protocol version 2 or later" err
 '
 
 test_expect_success 'clone with file:// using protocol v2' '
@@ -266,8 +266,8 @@ test_expect_success 'warn if using server-option with fetch with legacy protocol
 	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -C temp_child -c protocol.version=0 \
 		fetch -o hello -o world "file://$(pwd)/file_parent" master 2>err &&
 
-	grep "see protocol.version in" err &&
-	grep "server options require protocol version 2 or later" err
+	test_i18ngrep "see protocol.version in" err &&
+	test_i18ngrep "server options require protocol version 2 or later" err
 '
 
 test_expect_success 'upload-pack respects config using protocol v2' '


-- >8 --
Subject: [PATCH] gettext tests: export the restored GIT_TEST_GETTEXT_POISON

6cdccfce ("i18n: make GETTEXT_POISON a runtime option", 2018-11-08)
made the gettext-poison test a runtime option (which was a good
move) and adjusted the test framework so that Git commands we run as
part of the framework, as opposed to the ones that are part of the
test proper, are not affected by the setting.  The original value
for the GIT_TEST_GETTEXT_POISON environment variable is saved away
in another variable and gets unset, and then later the saved value
is restored to the environment variable.

But the code forgot to export the variable again, which is necessary
to restore the "export" bit that was lost when the variable was unset.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8665b0a9b6..078efda9da 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1394,6 +1394,7 @@ test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 if test -n "$GIT_TEST_GETTEXT_POISON_ORIG"
 then
 	GIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG
+	export GIT_TEST_GETTEXT_POISON
 	unset GIT_TEST_GETTEXT_POISON_ORIG
 fi
 
-- 
2.21.0-313-ge35b8cb8e2

Re: Re* [PATCH v4 1/2] transport: die if server options are unsupported

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> ... therefore these should be 'test_i18ngrep'.  This applies to the
>> other tests in this patch series as well.
>> ...
>
> Hmph.  That would mean the following is needed,...

And a corresponding fix for step 2/2 would look like this.

 t/t5702-protocol-v2.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 324bdf5715..9b048f6c8b 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -288,8 +288,8 @@ test_expect_success 'warn if using server-option with clone with legacy protocol
 		clone --server-option=hello --server-option=world \
 		"file://$(pwd)/file_parent" myclone 2>err &&
 
-	grep "see protocol.version in" err &&
-	grep "server options require protocol version 2 or later" err
+	test_i18ngrep "see protocol.version in" err &&
+	test_i18ngrep "server options require protocol version 2 or later" err
 '
 
 test_expect_success 'upload-pack respects config using protocol v2' '

Re: Re* [PATCH v4 1/2] transport: die if server options are unsupported

[SZEDER Gábor]

On Mon, Apr 15, 2019 at 01:48:31PM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > ... therefore these should be 'test_i18ngrep'.  This applies to the
> > other tests in this patch series as well.
> > ...
> 
> Hmph.  That would mean the following is needed, but I suspect that
> 6cdccfce ("i18n: make GETTEXT_POISON a runtime option", 2018-11-08)
> is somewhat broken (see the bottom for a suggested fix).

Oh, wow, indeed.

I still notice these [1], because I prefer to run the GETTEXT_POISION
CI jobs in my half-baked "scrambled" mode [2], but haven't yet got
around to deal with the conflicts between those patches and
'ab/dynamic-gettext-poison', so my script prepping in-flight topics
for my CI builds still starts with reverting
'ab/dynamic-gettext-poison', restoring that missing 'export'.


[1] https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/519163854#L2724

[2] https://public-inbox.org/git/20181022202241.18629-1-szeder.dev@gmail.com/

> -- >8 --
> Subject: [PATCH] gettext tests: export the restored GIT_TEST_GETTEXT_POISON
> 
> 6cdccfce ("i18n: make GETTEXT_POISON a runtime option", 2018-11-08)
> made the gettext-poison test a runtime option (which was a good
> move) and adjusted the test framework so that Git commands we run as
> part of the framework, as opposed to the ones that are part of the
> test proper, are not affected by the setting.  The original value
> for the GIT_TEST_GETTEXT_POISON environment variable is saved away
> in another variable and gets unset, and then later the saved value
> is restored to the environment variable.
> 
> But the code forgot to export the variable again, which is necessary
> to restore the "export" bit that was lost when the variable was unset.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/test-lib.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 8665b0a9b6..078efda9da 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1394,6 +1394,7 @@ test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
>  if test -n "$GIT_TEST_GETTEXT_POISON_ORIG"
>  then
>  	GIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG
> +	export GIT_TEST_GETTEXT_POISON
>  	unset GIT_TEST_GETTEXT_POISON_ORIG
>  fi

Makes sense.