Patchwork D7967: exchange: recognize changegroup3 bundles in `getbundlespec()`

login
register
mail settings
Submitter phabricator
Date Jan. 21, 2020, 11:32 p.m.
Message ID <differential-rev-PHID-DREV-6mccun6o2onegah3o6gi-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44565/
State New
Headers show

Comments

phabricator - Jan. 21, 2020, 11:32 p.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, `hg bundle --spec $bundle` complained that changegroup3 didn't have
  a known bundlespec and suggested upgrading the client, even if the same binary
  generated the bundle.

REPOSITORY
  rHG Mercurial

BRANCH
  stable

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

AFFECTED FILES
  mercurial/exchange.py
  tests/test-lfs-bundle.t

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 22, 2020, 12:47 p.m.
pulkit added inline comments.
pulkit added subscribers: joerg.sonnenberger, pulkit.

INLINE COMMENTS

> exchange.py:321
> +                ):
> +                    version = b'v2'
>                  else:

Should not this be `v3`? I believe this is what @joerg.sonnenberger is also trying to point out on IRC.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Jan. 22, 2020, 1:04 p.m.
mharbison72 added inline comments.
mharbison72 added a subscriber: marmoute.

INLINE COMMENTS

> pulkit wrote in exchange.py:321
> Should not this be `v3`? I believe this is what @joerg.sonnenberger is also trying to point out on IRC.

I’m not sure now. I think I went this way because I’m pretty sure it complained when I tried to generate a bzip2-v3 bundle.  And since 01 is mapped to v2 and the error message is talking about finding a bundlespec, it seemed like v2 was the only option.  But maybe something is missing elsewhere too.  @marmoute?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Jan. 27, 2020, 9:58 a.m.
marmoute added a comment.


  I can confirm the spec version number are different to the changegroup version number. For the rest. I'll try to have a look soon.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Feb. 26, 2020, 7:53 p.m.
mharbison72 added a comment.


  In D7967#118158 <https://phab.mercurial-scm.org/D7967#118158>, @marmoute wrote:
  
  > I can confirm the spec version number are different to the changegroup version number. For the rest. I'll try to have a look soon.
  
  Gentle ping on this

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Feb. 26, 2020, 7:54 p.m.
marmoute added a comment.


  It seems like I faild to have a look soon™

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Feb. 26, 2020, 11:30 p.m.
indygreg added a comment.


  To clarify, bundle specifications are user-facing whereas changegroup versions are an internal implementation detail. Their version numbers are thus on different timelines and aren't strictly related.
  
  A bundle specification version is essentially a collection of related bundle features at a given point in time. One of those bundle features would be the changegroup version. The idea is that every time we add a significant feature to bundles and want to expose that feature to users, we would bake a new bundle specification version that encapsulates that change. Over time, I would expect the total number of bundle specification versions to outnumber the changegroup versions, as any BC change to a bundle would incur a new bundle specification and there are more changes to bundles than just the changegroup format.
  
  While I'm here, in the context of bundle2, a monolithic changegroup bundle part doesn't make as much sense any more. We should arguably do away with the monolithic changegroup bundle2 part and instead send a series of deltas to named paths. But that's way beyond the scope of this review :)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: indygreg, marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Feb. 26, 2020, 11:37 p.m.
indygreg added inline comments.

INLINE COMMENTS

> mharbison72 wrote in exchange.py:321
> I’m not sure now. I think I went this way because I’m pretty sure it complained when I tried to generate a bzip2-v3 bundle.  And since 01 is mapped to v2 and the error message is talking about finding a bundlespec, it seemed like v2 was the only option.  But maybe something is missing elsewhere too.  @marmoute?

Since changegroup `v3` is not advertised in an existing bundle spec definition at the top of this file, the proper thing to do here is create a new `v3` bundle specification that uses changegroup v3 by default. Then all bundle spec `v3` will be compatible with changegroup `v3`.

But before we create a new bundle spec, all features within it need to be non-experimental. The whole point of bundle specs is that if 2 clients of different versions claim they support a bundle spec, they actually do. If the content of a bundle change within a bundle spec, that's a BC.

If changegroup `v3` isn't stable yet, we should not be associating it with a bundle spec because a claimed `v2` bundle spec may not be readable by an old version or a client without the experimental feature enabled. That breaks the contract of bundle specifications.

That being said, for purposes of parsing a bundle, it _might_ be acceptable to allow experimental features through. We absolutely cannot do that on the write side, however, as it completely breaks the bundle spec contract that content is well-defined.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: indygreg, marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - Feb. 27, 2020, 4:29 a.m.
mharbison72 added inline comments.

INLINE COMMENTS

> indygreg wrote in exchange.py:321
> Since changegroup `v3` is not advertised in an existing bundle spec definition at the top of this file, the proper thing to do here is create a new `v3` bundle specification that uses changegroup v3 by default. Then all bundle spec `v3` will be compatible with changegroup `v3`.
> 
> But before we create a new bundle spec, all features within it need to be non-experimental. The whole point of bundle specs is that if 2 clients of different versions claim they support a bundle spec, they actually do. If the content of a bundle change within a bundle spec, that's a BC.
> 
> If changegroup `v3` isn't stable yet, we should not be associating it with a bundle spec because a claimed `v2` bundle spec may not be readable by an old version or a client without the experimental feature enabled. That breaks the contract of bundle specifications.
> 
> That being said, for purposes of parsing a bundle, it _might_ be acceptable to allow experimental features through. We absolutely cannot do that on the write side, however, as it completely breaks the bundle spec contract that content is well-defined.

> That being said, for purposes of parsing a bundle, it _might_ be acceptable to allow experimental
> features through. We absolutely cannot do that on the write side, however, as it completely breaks
> the bundle spec contract that content is well-defined.

That's concerning (I think).

I got onto this path trying to generate clonebundles for a repo that uses LFS.  LFS forces the experimental changegroup3 (and removes the others as options).  I was able to generate the bundle, and was using this to try to figure out what the magic string was for filling out the manifest.  So there are bundles of a not-reported spec in the wild (I assume this is what you meant by the write side).  And if we were to BC changegroup3 for example, IDK how we tell that these older bundles are *not* whatever the new bundlespec becomes, since it seems that the only additional needed logic is to look for changegroup3.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers
Cc: indygreg, marmoute, pulkit, joerg.sonnenberger, mercurial-devel
phabricator - March 20, 2020, 1:33 a.m.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


  It looks like futher change will be necessary on this patch, moving it out of yadaa

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7967/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7967

To: mharbison72, #hg-reviewers, marmoute
Cc: indygreg, marmoute, pulkit, joerg.sonnenberger, mercurial-devel

Patch

diff --git a/tests/test-lfs-bundle.t b/tests/test-lfs-bundle.t
--- a/tests/test-lfs-bundle.t
+++ b/tests/test-lfs-bundle.t
@@ -95,3 +95,27 @@ 
   OK
   ---- Applying src-lfs.bundle to dst-lfs ----
   OK
+
+  $ for i in $SRCNAMES; do
+  >   echo % test bundle type $i
+  >   hg debugbundle $TESTTMP/$i.bundle
+  >   hg debugbundle --config experimental.changegroup3=True \
+  >                  --spec $TESTTMP/$i.bundle
+  >   echo
+  > done
+  % test bundle type src-normal
+  Stream params: {Compression: BZ}
+  changegroup -- {nbchanges: 2, version: 03} (mandatory: True)
+      92a23f1a59ca009ca9631a529d05e020c3873842
+      7ee5ca02c65436e529977365458ef3062cf9f746
+  cache:rev-branch-cache -- {} (mandatory: False)
+  bzip2-v2
+  
+  % test bundle type src-lfs
+  Stream params: {Compression: BZ}
+  changegroup -- {nbchanges: 2, version: 03} (mandatory: True)
+      92a23f1a59ca009ca9631a529d05e020c3873842
+      7ee5ca02c65436e529977365458ef3062cf9f746
+  cache:rev-branch-cache -- {} (mandatory: False)
+  bzip2-v2
+  
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -315,6 +315,10 @@ 
                 version = part.params[b'version']
                 if version in (b'01', b'02'):
                     version = b'v2'
+                elif version == b'03' and ui.configbool(
+                    b'experimental', b'changegroup3'
+                ):
+                    version = b'v2'
                 else:
                     raise error.Abort(
                         _(