[PATCH 3 of 3] clonebundles: change advertisement to reflect feature being enabled

[Gregory Szorc]

On Thu, Jan 7, 2016 at 8:35 PM, Gregory Szorc <gregory.szorc at gmail.com>
wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1452227679 28800
> #      Thu Jan 07 20:34:39 2016 -0800
> # Node ID 251241c354e2edbaf31eed28dc2028c3c318ca5b
> # Parent  03780b773ac3206e23ded090986e26a9eeb52f73
> clonebundles: change advertisement to reflect feature being enabled
>

This one is more RFC. And I'm really uncertain the best path forward here.
I'm reluctant to hack up the wire protocol for a mistake in an experimental
feature. But I don't want to lie to 3.6 clients for eternity either. I'm
leaning towards dropping the advertisement, as its existence was mostly
about encouraging testers of the feature while it was still experimental.


>
> I screwed up.
>
> When clone bundles is enabled on the server and a compatible client
> without the feature enabled clones, the server sends down an
> advertisement saying to enable the feature. The server creates the
> message which is printed verbatim on the client as an "output" part.
> And here's the problem: the message contains a reference to the
> "experimental.clonebundles" option.
>
> This is a problem because a 3.6 server will tell a 3.7 client with
> clone bundles disabled to use "experimental.clonebundles," which is
> not supported. And, changing the server message to "ui.clonebundles"
> would result in a 3.7+ server telling 3.6 clients to use an option
> that isn't supported.
>
> We should have abstracted the option name somehow, possibly by having
> the server send down a special message type rather than the generic
> "output" part. Instead, we're left with a little bit of a mess.
>
> This patch takes the easier path forward: referencing both options
> so the client can make an informed decision.
>
> A slightly more complicated solution would be to introduce a new
> getbundle wire protocol command option that differentiates
> non-experimental clone bundles support from experimental. e.g.
> "cbattempted2." Servers running 3.6 would see a warning due to the
> client passing an unrecognized option. But other than that it should
> be harmless. (To make the warning go away would require a new
> server capability for the client to key off of.)
>
> Alternatively, we could remove the advertisement. It arguably isn't
> needed any more since the feature is enabled by default. If it is
> enabled, clients probably have a reason why. We wouldn't reach 3.6
> clients any more, but that shouldn't be a major concern since the 3.6
> population will only dwindle over time.
>
> diff --git a/hgext/clonebundles.py b/hgext/clonebundles.py
> --- a/hgext/clonebundles.py
> +++ b/hgext/clonebundles.py
> @@ -238,17 +238,17 @@ def advertiseclonebundlespart(bundler, r
>          return
>
>      # And when a full clone is requested.
>      # Note: client should not send "cbattempted" for regular pulls. This
> check
>      # is defense in depth.
>      if common and common != [nullid]:
>          return
>
> -    msg = _('this server supports the experimental "clone bundles"
> feature '
> -            'that should enable faster and more reliable cloning\n'
> -            'help test it by setting the "experimental.clonebundles"
> config '
> -            'flag to "true"')
> +    msg = _('this server supports the "clone bundles" feature that should
> '
> +            'enable faster and more reliable cloning\n'
> +            'use it by setting the "ui.clonebundles" config flag '
> +            '("experimental.clonebundles" on version 3.6) to "true"')
>
>      bundler.newpart('output', data=msg)
>
>  def extsetup(ui):
>      extensions.wrapfunction(wireproto, '_capabilities', capabilities)
> diff --git a/tests/test-clonebundles.t b/tests/test-clonebundles.t
> --- a/tests/test-clonebundles.t
> +++ b/tests/test-clonebundles.t
> @@ -46,18 +46,18 @@ Empty manifest file results in retrieval
>    adding manifests
>    adding file changes
>    added 2 changesets with 2 changes to 2 files
>
>  Server advertises presence of feature to client requesting full clone
>
>    $ hg --config ui.clonebundles=false clone -U http://localhost:$HGPORT
> advertise-on-clone
>    requesting all changes
> -  remote: this server supports the experimental "clone bundles" feature
> that should enable faster and more reliable cloning
> -  remote: help test it by setting the "experimental.clonebundles" config
> flag to "true"
> +  remote: this server supports the "clone bundles" feature that should
> enable faster and more reliable cloning
> +  remote: use it by setting the "ui.clonebundles" config flag
> ("experimental.clonebundles" on version 3.6) to "true"
>    adding changesets
>    adding manifests
>    adding file changes
>    added 2 changesets with 2 changes to 2 files
>
>  Manifest file with invalid URL aborts
>
>    $ echo 'http://does.not.exist/bundle.hg' >
> server/.hg/clonebundles.manifest
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160107/c525d5d1/attachment-0002.html>