Stefan Beller <sbeller@xxxxxxxxxx> writes:
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html