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

[timeless]

Ok, what you have isn't the right fix.

If a Japanese server talks to a Russian client, and the server wants
to use clonebundles,
what does the Russian client see? :)
... the answer appears to be Japanese.

We should *not* be sending localized messages over the wire. *Ever*.

In fact, if we had this rule initially, you wouldn't have been able to
make this mistake.

-- I'm sorry I didn't review your original work.

On Thu, Jan 7, 2016 at 11:38 PM, Gregory Szorc <gregory.szorc at gmail.com> wrote:
> 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
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>