git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFCv2 12/16] transport: get_refs_via_connect exchanges capabilities before refs.

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 2, 2015, 21:55 UTC
Message-ID
<xmqqr3ptzsfj.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1433203338-27493-13-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 20 quoted lines
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     A minor issue I am unsure about here is the
>     line
>     	&& transport->smart_options->transport_version)
>     which could be prevented if we always set the transport_version
>     in make_remote to be the default remote version.
>     
>     The advantage of having it always set in make_remote would
>     be a cleaner mind model (the version set is always accurate)
>     as opposed to now (version may be 0, then the default
>     applies as we don't care enough to set a version)
>     
>     However I think the code may be more ugly if we were to
>     always set the version in make_remote as then we would need
>     to move the DEFAULT_TRANSPORT_VERSION define into remote.h
>     or somewhere else (transport.h is not included in remote.c,
>     I guess that's on purpose?)

Interesting. After reading 9/16, I was somehow expecting to see that a new method get_capability() would be added to the transport layer, and a function get_capability_via_connect() that calls get_remote_capabilities() would be its implementation for the "connect" transport.

The capability thing however is limited to the Git-aware aka smart transports and it is unlikely that other transports would benefit from such an organization, so I think the way this step integrates the new get_remote_capabilities() to the system would be not just sufficient but is more appropriate.

If you do not like the switching based on version in this function, however, it is probably an option to add the new method and define connect_v1 and connect_v2 as two separate transports. The latter would have get_capability() method, while the former (and all the other transports) does not define it. And the overall transport layer would call get_capability() method when defined, and then get_refs() method next, or something like that.

Show 71 quoted lines
>  transport.c | 28 ++++++++++++++++++++++++----
>  transport.h |  6 ++++++
>  2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index b49fc60..ba40677 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -523,14 +523,33 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
>  
>  static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
>  {
> +	struct transport_options options;
>  	struct git_transport_data *data = transport->data;
>  	struct ref *refs;
> +	int version = DEFAULT_TRANSPORT_VERSION;
>  
> +	if (transport->smart_options
> +	    && transport->smart_options->transport_version)
> +		version = transport->smart_options->transport_version;
>  	connect_setup(transport, for_push, 0);
> -	get_remote_heads(data->fd[0], NULL, 0, &refs,
> -			 for_push ? REF_NORMAL : 0,
> -			 &data->extra_have,
> -			 &data->shallow);
> +	switch (version) {
> +	case 2: /* first talk about capabilities, then get the heads */
> +		get_remote_capabilities(data->fd[0], NULL, 0);
> +		preselect_capabilities(&options);
> +		if (transport->select_capabilities)
> +			transport->select_capabilities(&options);
> +		request_capabilities(data->fd[1], &options);
> +		/* fall through */
> +	case 1:
> +		get_remote_heads(data->fd[0], NULL, 0, &refs,
> +				 for_push ? REF_NORMAL : 0,
> +				 &data->extra_have,
> +				 &data->shallow);
> +		break;
> +	default:
> +		die("BUG: Transport version %d not supported", version);
> +		break;
> +	}
>  	data->got_remote_heads = 1;
>  
>  	return refs;
> @@ -987,6 +1006,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  		struct git_transport_data *data = xcalloc(1, sizeof(*data));
>  		ret->data = data;
>  		ret->set_option = NULL;
> +		ret->select_capabilities = NULL;
>  		ret->get_refs_list = get_refs_via_connect;
>  		ret->fetch = fetch_refs_via_pack;
>  		ret->push_refs = git_transport_push;
> diff --git a/transport.h b/transport.h
> index 6095d7a..3e63efc 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -74,6 +74,12 @@ struct transport {
>  	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
>  
>  	/**
> +	 * A callback to select protocol options. Must be set if
> +	 * the caller wants to change transport options.
> +	 */
> +	void (*select_capabilities)(struct transport_options *);
> +
> +	/**
>  	 * Push the objects and refs. Send the necessary objects, and
>  	 * then, for any refs where peer_ref is set and
>  	 * peer_ref->new_sha1 is different from old_sha1, tell the
Previous: Stefan BellerNext: Stefan Beller
Message 33 of 44 in “[RFCv2 00/16] Protocol version 2”
  1. Stefan BellerJun 2, 2015
  2. 01/16 stringlist: add from_space_separated_stringStefan Beller, Jun 2, 2015
  3. Duy NguyenJun 2, 2015
  4. Eric SunshineJun 2, 2015
  5. Stefan BellerJun 2, 2015
  6. 02/16 upload-pack: make client capability parsing code a separate functionStefan Beller, Jun 2, 2015
  7. 03/16 connect: rewrite feature parsing to work on string_listStefan Beller, Jun 2, 2015
  8. Junio C HamanoJun 2, 2015
  9. 04/16 upload-pack-2: Implement the version 2 of upload-packStefan Beller, Jun 2, 2015
  10. Junio C HamanoJun 2, 2015
  11. Stefan BellerJun 2, 2015
  12. 05/16 remote.h: Change get_remote_heads return to voidStefan Beller, Jun 2, 2015
  13. Junio C HamanoJun 2, 2015
  14. Stefan BellerJun 2, 2015
  15. Junio C HamanoJun 2, 2015
  16. 06/16 remote.h: add new struct for optionsStefan Beller, Jun 2, 2015
  17. Junio C HamanoJun 2, 2015
  18. Stefan BellerJun 2, 2015
  19. Junio C HamanoJun 2, 2015
  20. 07/16 transport: add infrastructure to support a protocol version numberStefan Beller, Jun 2, 2015
  21. 08/16 transport: select transport version via command line or configStefan Beller, Jun 2, 2015
  22. 09/16 remote.h: add get_remote_capabilities, request_capabilitiesStefan Beller, Jun 2, 2015
  23. Junio C HamanoJun 2, 2015
  24. 10/16 transport: connect_setup appends protocol version numberStefan Beller, Jun 2, 2015
  25. Duy NguyenJun 2, 2015
  26. Stefan BellerJun 2, 2015
  27. Junio C HamanoJun 2, 2015
  28. Stefan BellerJun 2, 2015
  29. Junio C HamanoJun 2, 2015
  30. 11/16 remote: have preselect_capabilitiesStefan Beller, Jun 2, 2015
  31. Junio C HamanoJun 2, 2015
  32. 12/16 transport: get_refs_via_connect exchanges capabilities before refs.Stefan Beller, Jun 2, 2015
  33. Junio C HamanoJun 2, 2015
  34. Stefan BellerJun 2, 2015
  35. 13/16 fetch-pack: use the configured transport protocolStefan Beller, Jun 2, 2015
  36. Duy NguyenJun 2, 2015
  37. Duy NguyenJun 2, 2015
  38. Ilari LiusvaaraJun 2, 2015
  39. 14/16 t5544: add a test case for the new protocolStefan Beller, Jun 2, 2015
  40. Eric SunshineJun 3, 2015
  41. 15/16 Documentation/technical/pack-protocol: Mention http as possible protocolStefan Beller, Jun 2, 2015
  42. Junio C HamanoJun 2, 2015
  43. Junio C HamanoJun 2, 2015
  44. 16/16 Document protocol version 2Stefan Beller, Jun 2, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.