From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: git@vger•kernel.org, pclouds@gmail•com, peff@peff•net
Subject: Re: [RFCv2 12/16] transport: get_refs_via_connect exchanges capabilities before refs.
Date: Tue, 02 Jun 2015 14:55:28 -0700 [thread overview]
Message-ID: <xmqqr3ptzsfj.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1433203338-27493-13-git-send-email-sbeller@google.com> (Stefan Beller's message of "Mon, 1 Jun 2015 17:02:14 -0700")
Stefan Beller <sbeller@google•com> writes:
> 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.
> 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
next prev parent reply other threads:[~2015-06-02 21:55 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-02 0:02 [RFCv2 00/16] Protocol version 2 Stefan Beller
2015-06-02 0:02 ` [RFCv2 01/16] stringlist: add from_space_separated_string Stefan Beller
2015-06-02 9:42 ` Duy Nguyen
2015-06-02 15:10 ` Eric Sunshine
2015-06-02 17:54 ` Stefan Beller
2015-06-02 0:02 ` [RFCv2 02/16] upload-pack: make client capability parsing code a separate function Stefan Beller
2015-06-02 0:02 ` [RFCv2 03/16] connect: rewrite feature parsing to work on string_list Stefan Beller
2015-06-02 18:48 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack Stefan Beller
2015-06-02 18:59 ` Junio C Hamano
2015-06-02 23:08 ` Stefan Beller
2015-06-02 0:02 ` [RFCv2 05/16] remote.h: Change get_remote_heads return to void Stefan Beller
2015-06-02 21:17 ` Junio C Hamano
2015-06-02 21:25 ` Stefan Beller
2015-06-02 21:41 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 06/16] remote.h: add new struct for options Stefan Beller
2015-06-02 21:18 ` Junio C Hamano
2015-06-02 21:40 ` Stefan Beller
2015-06-02 21:43 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 07/16] transport: add infrastructure to support a protocol version number Stefan Beller
2015-06-02 0:02 ` [RFCv2 08/16] transport: select transport version via command line or config Stefan Beller
2015-06-02 0:02 ` [RFCv2 09/16] remote.h: add get_remote_capabilities, request_capabilities Stefan Beller
2015-06-02 21:24 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 10/16] transport: connect_setup appends protocol version number Stefan Beller
2015-06-02 9:58 ` Duy Nguyen
2015-06-02 18:04 ` Stefan Beller
2015-06-02 21:37 ` Junio C Hamano
2015-06-02 22:09 ` Stefan Beller
2015-06-02 22:27 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 11/16] remote: have preselect_capabilities Stefan Beller
2015-06-02 21:45 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 12/16] transport: get_refs_via_connect exchanges capabilities before refs Stefan Beller
2015-06-02 21:55 ` Junio C Hamano [this message]
2015-06-02 22:19 ` Stefan Beller
2015-06-02 0:02 ` [RFCv2 13/16] fetch-pack: use the configured transport protocol Stefan Beller
2015-06-02 9:55 ` Duy Nguyen
2015-06-02 10:02 ` Duy Nguyen
2015-06-02 11:32 ` Ilari Liusvaara
2015-06-02 0:02 ` [RFCv2 14/16] t5544: add a test case for the new protocol Stefan Beller
2015-06-03 0:16 ` Eric Sunshine
2015-06-02 0:02 ` [RFCv2 15/16] Documentation/technical/pack-protocol: Mention http as possible protocol Stefan Beller
2015-06-02 21:57 ` Junio C Hamano
2015-06-02 22:00 ` Junio C Hamano
2015-06-02 0:02 ` [RFCv2 16/16] Document protocol version 2 Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr3ptzsfj.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox