git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Josh Steadmon <steadmon@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, sbeller@google.com, jrnieder@gmail.com
Subject: Re: [PATCH v3 1/1] protocol: advertise multiple supported versions
Date: Tue, 13 Nov 2018 14:53:20 -0800	[thread overview]
Message-ID: <20181113225320.GG126896@google.com> (raw)
In-Reply-To: <xmqqr2fpwrqg.fsf@gitster-ct.c.googlers.com>

On 2018.11.13 13:01, Junio C Hamano wrote:
> steadmon@google.com writes:
> 
> > Currently the client advertises that it supports the wire protocol
> > version set in the protocol.version config. However, not all services
> > support the same set of protocol versions. When connecting to
> > git-receive-pack, the client automatically downgrades to v0 if
> > config.protocol is set to v2, but this check is not performed for other
> > services.
> 
> "downgrades to v0 even if ... is set to v2" you mean?  Otherwise it
> is unclear why asking for v2 leads to using v0.

The downgrade on push happens only when the the configured version is
v2. If v1 is requested, no downgrade is triggered. I'll clarify the
commit message in the next version.

> > This patch creates a protocol version registry. Individual operations
> > register all the protocol versions they support prior to communicating
> > with a server. Versions should be listed in preference order; the
> > version specified in protocol.version will automatically be moved to the
> > front of the registry.
> >
> > The protocol version registry is passed to remote helpers via the
> > GIT_PROTOCOL environment variable.
> >
> > Clients now advertise the full list of registered versions. Servers
> > select the first recognized version from this advertisement.
> 
> Makes sense.
> 
> > +void get_client_protocol_version_advertisement(struct strbuf *advert)
> > +{
> > +	int tmp_nr = nr_allowed_versions;
> > +	enum protocol_version *tmp_allowed_versions, config_version;
> > +	strbuf_reset(advert);
> > +
> > +	have_advertised_versions_already = 1;
> > +
> > +	config_version = get_protocol_version_config();
> > +	if (config_version == protocol_v0) {
> > +		strbuf_addstr(advert, "version=0");
> > +		return;
> > +	}
> > +
> > +	if (tmp_nr > 0) {
> > +		ALLOC_ARRAY(tmp_allowed_versions, tmp_nr);
> > +		copy_array(tmp_allowed_versions, allowed_versions, tmp_nr,
> > +			   sizeof(enum protocol_version));
> > +	} else {
> > +		ALLOC_ARRAY(tmp_allowed_versions, 1);
> > +		tmp_nr = 1;
> > +		tmp_allowed_versions[0] = config_version;
> > +	}
> > +
> > +	if (tmp_allowed_versions[0] != config_version)
> > +		for (int i = 1; i < nr_allowed_versions; i++)
> > +			if (tmp_allowed_versions[i] == config_version) {
> > +				enum protocol_version swap =
> > +					tmp_allowed_versions[0];
> > +				tmp_allowed_versions[0] =
> > +					tmp_allowed_versions[i];
> > +				tmp_allowed_versions[i] = swap;
> > +			}
> > +
> > +	strbuf_addf(advert, "version=%s",
> > +		    format_protocol_version(tmp_allowed_versions[0]));
> > +	for (int i = 1; i < tmp_nr; i++)
> > +		strbuf_addf(advert, ":version=%s",
> > +			    format_protocol_version(tmp_allowed_versions[i]));
> > +}
> > +
> 
> So the idea is that the protocols the other end can talk come in
> advert in their preferred order, and we take an intersection of them
> and our "allowed-versions", but the preference is further skewed
> with the "swap" thing if we have our own preference specified via
> config?

We currently don't intersect with our own allowed list, we just accept
the first version that we recognize. This introduces its own version
negotiation bug; if we add v2 push in the future, a new client
talking to an old server would try to use v2 even though the server may
not have the corresponding v2 push implementation. I'll fix this in the
next version.

In any case, the ordering of the server's allowed versions won't matter;
we'll pick the the first version in the client's list which is also
allowed on the server.

> I am wondering if the code added by this patch outside this
> function, with if (strcmp(client_ad.buf, "version=0") sprinkled all
> over the place, works sensibly when the other side says "I prefer
> version=0 but I do not mind talking version=1".

Depends on what you mean by "sensibly" :). In the current case, if the
client prefers v0, it will always end up using v0. After the fixes
described above, it will always use v0 unless the server no longer
supports v0. Is that what you would expect?

> Isn't tmp_allowed_versions[] leaking when we return from this
> function?

Yes, sorry about that. Will fix.

  reply	other threads:[~2018-11-13 22:53 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-02 21:59 [PATCH 0/1] Limit client version advertisements Josh Steadmon
2018-10-02 21:59 ` [PATCH 1/1] protocol: limit max protocol version per service Josh Steadmon
2018-10-02 22:28   ` Stefan Beller
2018-10-03 21:33     ` Josh Steadmon
2018-10-03 22:47       ` Stefan Beller
2018-10-05  0:18         ` Josh Steadmon
2018-10-05 19:25           ` Stefan Beller
2018-10-10 23:53             ` Josh Steadmon
2018-10-12 23:30               ` Jonathan Nieder
2018-10-12 23:32               ` Jonathan Nieder
2018-10-12 23:45                 ` Josh Steadmon
2018-10-12 23:53                   ` Jonathan Nieder
2018-10-13  7:58                     ` Junio C Hamano
2018-10-12  1:02 ` [PATCH v2 0/1] Advertise multiple supported proto versions steadmon
2018-10-12  1:02   ` [PATCH v2 1/1] protocol: advertise multiple supported versions steadmon
2018-10-12 22:30     ` Stefan Beller
2018-10-22 22:55       ` Josh Steadmon
2018-10-23  0:37         ` Stefan Beller
2018-11-12 21:49   ` [PATCH v3 0/1] Advertise multiple supported proto versions steadmon
2018-11-12 21:49     ` [PATCH v3 1/1] protocol: advertise multiple supported versions steadmon
2018-11-12 22:33       ` Stefan Beller
2018-11-13  3:24         ` Re* " Junio C Hamano
2018-11-13 19:21           ` Stefan Beller
2018-11-14  2:31             ` Junio C Hamano
2018-11-13  4:01       ` Junio C Hamano
2018-11-13 22:53         ` Josh Steadmon [this message]
2018-11-14  2:38           ` Junio C Hamano
2018-11-14 19:57             ` Josh Steadmon
2018-11-16  2:45               ` Junio C Hamano
2018-11-16 19:59                 ` Josh Steadmon
2018-11-13 13:35       ` Junio C Hamano
2018-11-13 18:28       ` SZEDER Gábor
2018-11-13 23:03         ` Josh Steadmon
2018-11-14  0:47           ` SZEDER Gábor
2018-11-14  2:44         ` Junio C Hamano
2018-11-14 11:01           ` SZEDER Gábor
2018-11-14  2:30     ` [PATCH v4 0/1] Advertise multiple supported proto versions Josh Steadmon
2018-11-14  2:30       ` [PATCH v4 1/1] protocol: advertise multiple supported versions Josh Steadmon
2018-11-14 10:22       ` [PATCH v4 0/1] Advertise multiple supported proto versions Junio C Hamano
2018-11-14 19:51         ` Josh Steadmon
2018-11-16 10:46           ` Junio C Hamano
2018-11-16 22:34       ` [PATCH v5 " Josh Steadmon
2018-11-16 22:34         ` [PATCH v5 1/1] protocol: advertise multiple supported versions Josh Steadmon
2018-12-14 20:20           ` Ævar Arnfjörð Bjarmason
2018-12-14 21:58             ` Josh Steadmon
2018-12-14 22:39               ` Ævar Arnfjörð Bjarmason
2018-12-18 23:05                 ` Josh Steadmon
2018-12-20 21:58 ` [PATCH v6 0/1] Advertise multiple supported proto versions Josh Steadmon
2018-12-20 21:58   ` [PATCH v6 1/1] protocol: advertise multiple supported versions Josh Steadmon
2019-02-05 19:42     ` Jonathan Tan
2019-02-07 23:58       ` Josh Steadmon
2019-02-11 18:53         ` Jonathan Tan

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181113225320.GG126896@google.com \
    --to=steadmon@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).