mbox series

[v3,0/2] Server options when cloning

Message ID cover.1555014408.git.jonathantanmy@google.com (mailing list archive)
Headers show
Series Server options when cloning | expand

Message

Jonathan Tan April 11, 2019, 8:30 p.m. UTC 
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' '