Patchwork [1,of,8] bundle: allow bundle command to use changegroup3 in tests

login
register
mail settings
Submitter Jun Wu
Date April 7, 2017, 2:08 a.m.
Message ID <3d62d68ed4245359b5ae.1491530889@x1c>
Download mbox | patch
Permalink /patch/19982/
State Accepted
Headers show

Comments

Jun Wu - April 7, 2017, 2:08 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1491523318 25200
#      Thu Apr 06 17:01:58 2017 -0700
# Node ID 3d62d68ed4245359b5ae5b6b6c1959a15ffa84e9
# Parent  45761ef1bc935b1fab74adccf2541ef854b1c2eb
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 3d62d68ed424
bundle: allow bundle command to use changegroup3 in tests

Since bundle2 writes changegroup version, we can just reuse the bundle2
format for changegroup3.

This won't enable the bundle command to write changegroup3 in the wild,
since exchange.parsebundlespec only returns changegroup2. It unlocks tests
to override exchange.parsebundlespec and get "hg bundle" write changegroup3.
Ryan McElroy - April 7, 2017, 1:41 p.m.
On 4/7/17 3:08 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1491523318 25200
> #      Thu Apr 06 17:01:58 2017 -0700
> # Node ID 3d62d68ed4245359b5ae5b6b6c1959a15ffa84e9
> # Parent  45761ef1bc935b1fab74adccf2541ef854b1c2eb
> bundle: allow bundle command to use changegroup3 in tests
>
> Since bundle2 writes changegroup version, we can just reuse the bundle2
> format for changegroup3.
>
> This won't enable the bundle command to write changegroup3 in the wild,
> since exchange.parsebundlespec only returns changegroup2. It unlocks tests
> to override exchange.parsebundlespec and get "hg bundle" write changegroup3.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1377,7 +1377,9 @@ def bundle(ui, repo, fname, dest=None, *
>           bversion = 'HG10' + bcompression
>           bcompression = None
> +    elif cgversion in ('02', '03'):
> +        bversion = 'HG20'
>       else:
> -        assert cgversion == '02'
> -        bversion = 'HG20'
> +        raise error.ProgrammingError(
> +            'bundle: unexpected changegroup version %s' % cgversion)
>   
>       # TODO compression options should be derived from bundlespec parsing.
>

A test of this abort would be a nice-to-have add-on.
Yuya Nishihara - April 7, 2017, 1:50 p.m.
On Thu, 6 Apr 2017 19:08:09 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1491523318 25200
> #      Thu Apr 06 17:01:58 2017 -0700
> # Node ID 3d62d68ed4245359b5ae5b6b6c1959a15ffa84e9
> # Parent  45761ef1bc935b1fab74adccf2541ef854b1c2eb
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 3d62d68ed424
> bundle: allow bundle command to use changegroup3 in tests

The overall change looks good to me. I have a few questions in line.
Pierre-Yves David - April 7, 2017, 2:07 p.m.
On 04/07/2017 04:08 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1491523318 25200
> #      Thu Apr 06 17:01:58 2017 -0700
> # Node ID 3d62d68ed4245359b5ae5b6b6c1959a15ffa84e9
> # Parent  45761ef1bc935b1fab74adccf2541ef854b1c2eb
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 3d62d68ed424
> bundle: allow bundle command to use changegroup3 in tests
>
> Since bundle2 writes changegroup version, we can just reuse the bundle2
> format for changegroup3.
>
> This won't enable the bundle command to write changegroup3 in the wild,
> since exchange.parsebundlespec only returns changegroup2.

Is there any reasons why we can't just have bundle spec to support cg3?

The usual way to go here is:
1) add a way to specify the bundle content you want as a bundlespec
2) automatically upgrade to the minimal subset we need to not loose 
information when special feature is used. (in your case cg3)
Jun Wu - April 7, 2017, 2:14 p.m.
Excerpts from Pierre-Yves David's message of 2017-04-07 16:07:08 +0200:
> 
> On 04/07/2017 04:08 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1491523318 25200
> > #      Thu Apr 06 17:01:58 2017 -0700
> > # Node ID 3d62d68ed4245359b5ae5b6b6c1959a15ffa84e9
> > # Parent  45761ef1bc935b1fab74adccf2541ef854b1c2eb
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 3d62d68ed424
> > bundle: allow bundle command to use changegroup3 in tests
> >
> > Since bundle2 writes changegroup version, we can just reuse the bundle2
> > format for changegroup3.
> >
> > This won't enable the bundle command to write changegroup3 in the wild,
> > since exchange.parsebundlespec only returns changegroup2.
> 
> Is there any reasons why we can't just have bundle spec to support cg3?

martinvonz reminds me that the treemanifest part is tricky.

> 
> The usual way to go here is:
> 1) add a way to specify the bundle content you want as a bundlespec
> 2) automatically upgrade to the minimal subset we need to not loose 
> information when special feature is used. (in your case cg3)
>
Pierre-Yves David - April 7, 2017, 2:39 p.m.
On 04/07/2017 04:14 PM, Jun Wu wrote:
> Excerpts from Pierre-Yves David's message of 2017-04-07 16:07:08 +0200:
>>
>> On 04/07/2017 04:08 AM, Jun Wu wrote:
>>> # HG changeset patch
>>> # User Jun Wu <quark@fb.com>
>>> # Date 1491523318 25200
>>> #      Thu Apr 06 17:01:58 2017 -0700
>>> # Node ID 3d62d68ed4245359b5ae5b6b6c1959a15ffa84e9
>>> # Parent  45761ef1bc935b1fab74adccf2541ef854b1c2eb
>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 3d62d68ed424
>>> bundle: allow bundle command to use changegroup3 in tests
>>>
>>> Since bundle2 writes changegroup version, we can just reuse the bundle2
>>> format for changegroup3.
>>>
>>> This won't enable the bundle command to write changegroup3 in the wild,
>>> since exchange.parsebundlespec only returns changegroup2.
>>
>> Is there any reasons why we can't just have bundle spec to support cg3?
>
> martinvonz reminds me that the treemanifest part is tricky.

You mean that currently, your cg3 bundle does not support treemanifest?

If so, I'm not sure this is a blocker since treemanifest are still 
experimental and already unsupported by 'hg bundle'.

Cheers,
Jun Wu - April 7, 2017, 2:44 p.m.
Excerpts from Pierre-Yves David's message of 2017-04-07 16:39:03 +0200:
> >> Is there any reasons why we can't just have bundle spec to support cg3?
> >
> > martinvonz reminds me that the treemanifest part is tricky.
> 
> You mean that currently, your cg3 bundle does not support treemanifest?
> 
> If so, I'm not sure this is a blocker since treemanifest are still 
> experimental and already unsupported by 'hg bundle'.

afaik, cg3 is also experimental.

> 
> Cheers,
>
Pierre-Yves David - April 7, 2017, 3:19 p.m.
On 04/07/2017 04:44 PM, Jun Wu wrote:
> Excerpts from Pierre-Yves David's message of 2017-04-07 16:39:03 +0200:
>>>> Is there any reasons why we can't just have bundle spec to support cg3?
>>>
>>> martinvonz reminds me that the treemanifest part is tricky.
>>
>> You mean that currently, your cg3 bundle does not support treemanifest?
>>
>> If so, I'm not sure this is a blocker since treemanifest are still
>> experimental and already unsupported by 'hg bundle'.
>
> afaik, cg3 is also experimental.

Okay, so it seems fine to not expose it in bundlespec

Out of curiosity, any idea of why it is still experimental?

Cheers,
Augie Fackler - April 8, 2017, 11:42 p.m.
On Fri, Apr 07, 2017 at 05:19:20PM +0200, Pierre-Yves David wrote:
>
>
> On 04/07/2017 04:44 PM, Jun Wu wrote:
> > Excerpts from Pierre-Yves David's message of 2017-04-07 16:39:03 +0200:
> > > > > Is there any reasons why we can't just have bundle spec to support cg3?
> > > >
> > > > martinvonz reminds me that the treemanifest part is tricky.
> > >
> > > You mean that currently, your cg3 bundle does not support treemanifest?
> > >
> > > If so, I'm not sure this is a blocker since treemanifest are still
> > > experimental and already unsupported by 'hg bundle'.
> >
> > afaik, cg3 is also experimental.
>
> Okay, so it seems fine to not expose it in bundlespec
>
> Out of curiosity, any idea of why it is still experimental?

I don't think so. There are still some performance nasties that I need
to chase down in narrowhg that *might* turn out to be a treemanifest
problem, but I think it's far more likely to be a "matchers are slow"
problem.

It's fine with me if we want to remove the experimental label - I
think greg was interested in this for Mozilla reasons anyway...

>
> Cheers,
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - April 9, 2017, 4:22 p.m.
On 04/09/2017 01:42 AM, Augie Fackler wrote:
> On Fri, Apr 07, 2017 at 05:19:20PM +0200, Pierre-Yves David wrote:
>>
>>
>> On 04/07/2017 04:44 PM, Jun Wu wrote:
>>> Excerpts from Pierre-Yves David's message of 2017-04-07 16:39:03 +0200:
>>>>>> Is there any reasons why we can't just have bundle spec to support cg3?
>>>>>
>>>>> martinvonz reminds me that the treemanifest part is tricky.
>>>>
>>>> You mean that currently, your cg3 bundle does not support treemanifest?
>>>>
>>>> If so, I'm not sure this is a blocker since treemanifest are still
>>>> experimental and already unsupported by 'hg bundle'.
>>>
>>> afaik, cg3 is also experimental.
>>
>> Okay, so it seems fine to not expose it in bundlespec
>>
>> Out of curiosity, any idea of why it is still experimental?
>
> I don't think so.

I'm a bit confused from a language perspective.
Are you saying they are no reason for it to stay experimental?

> There are still some performance nasties that I need
> to chase down in narrowhg that *might* turn out to be a treemanifest
> problem, but I think it's far more likely to be a "matchers are slow"
> problem.
>
> It's fine with me if we want to remove the experimental label - I
> think greg was interested in this for Mozilla reasons anyway...


Cheers,
Augie Fackler - April 9, 2017, 5:08 p.m.
On Apr 9, 2017 12:22 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:



On 04/09/2017 01:42 AM, Augie Fackler wrote:

> On Fri, Apr 07, 2017 at 05:19:20PM +0200, Pierre-Yves David wrote:
>
>>
>>
>> On 04/07/2017 04:44 PM, Jun Wu wrote:
>>
>>> Excerpts from Pierre-Yves David's message of 2017-04-07 16:39:03 +0200:
>>>
>>>> Is there any reasons why we can't just have bundle spec to support cg3?
>>>>>>
>>>>>
>>>>> martinvonz reminds me that the treemanifest part is tricky.
>>>>>
>>>>
>>>> You mean that currently, your cg3 bundle does not support treemanifest?
>>>>
>>>> If so, I'm not sure this is a blocker since treemanifest are still
>>>> experimental and already unsupported by 'hg bundle'.
>>>>
>>>
>>> afaik, cg3 is also experimental.
>>>
>>
>> Okay, so it seems fine to not expose it in bundlespec
>>
>> Out of curiosity, any idea of why it is still experimental?
>>
>
> I don't think so.
>

I'm a bit confused from a language perspective.
Are you saying they are no reason for it to stay experimental?


No reasons that I know of to keep the format experimental.



There are still some performance nasties that I need
> to chase down in narrowhg that *might* turn out to be a treemanifest
> problem, but I think it's far more likely to be a "matchers are slow"
> problem.
>
> It's fine with me if we want to remove the experimental label - I
> think greg was interested in this for Mozilla reasons anyway...
>


Cheers,

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1377,7 +1377,9 @@  def bundle(ui, repo, fname, dest=None, *
         bversion = 'HG10' + bcompression
         bcompression = None
+    elif cgversion in ('02', '03'):
+        bversion = 'HG20'
     else:
-        assert cgversion == '02'
-        bversion = 'HG20'
+        raise error.ProgrammingError(
+            'bundle: unexpected changegroup version %s' % cgversion)
 
     # TODO compression options should be derived from bundlespec parsing.