Patchwork D1927: bundle2: move version of stream clone into part name

login
register
mail settings
Submitter phabricator
Date Jan. 21, 2018, 12:45 a.m.
Message ID <differential-rev-PHID-DREV-c7tka5uvwqbg5tbq3cln-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27015/
State Superseded
Headers show

Comments

phabricator - Jan. 21, 2018, 12:45 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I don't like having version numbers as part parameters. It means
  that parts can theoretically vary wildly in their generation and
  processing semantics. I think that a named part should have consistent
  behavior over time. In other words, if you need to introduce new
  functionality or behavior, that should be expressed by inventing
  a new bundle2 part, not adding functionality to an existing part.
  
  This commit applies this advice to the just-introduced stream clone
  via bundle2 feature.
  
  The "version" part parameter is removed. The name of the bundle2 part
  is now "stream1" instead of "stream" with "version=v2".
  
  Note that the version changed from 2 to 1. This is because in the
  context of bundle2, this is the first support for stream clones,
  not the second. It is a bit weird that internally the stream clone
  data format is defined by both version 1 and 2. However, the
  stream clone format was always somewhat hacky and under-defined.
  Now that we have a proper container for it in bundle2, we can define
  the format in terms of bundle2 and not have to worry about
  supporting a standalone stream clone format. We should eventually
  deprecate the old format in code and documentation by referring
  to it as "legacy" or some such.
  
  Despite my reasoning in the previous paragraph, if someone wanted
  to name the part "stream2" and we would skip "stream1", I would not
  be opposed. It is wonky for people implementing the protocol to
  not have a 1st version. But it would likely increase code
  comprehension - at least until we refactor the old code to and refer
  to it as "legacy."

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundle2.py
  mercurial/exchange.py
  mercurial/streamclone.py
  tests/test-clone-uncompressed.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 22, 2018, 4:20 p.m.
lothiraldan added a comment.


  Having a part named `stream1` that used a V2 version of the streaming protocol and transporting a V2 bundle format seems confusing, I'm voting for a `stream2` name.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: lothiraldan, mercurial-devel

Patch

diff --git a/tests/test-clone-uncompressed.t b/tests/test-clone-uncompressed.t
--- a/tests/test-clone-uncompressed.t
+++ b/tests/test-clone-uncompressed.t
@@ -55,23 +55,23 @@ 
   
 
   $ f --size --hex --bytes 256 body
-  body: size=112328
+  body: size=112318
   0000: 04 6e 6f 6e 65 48 47 32 30 00 00 00 00 00 00 00 |.noneHG20.......|
-  0010: 72 06 53 54 52 45 41 4d 00 00 00 00 04 00 09 05 |r.STREAM........|
-  0020: 09 04 0c 2d 07 02 62 79 74 65 63 6f 75 6e 74 39 |...-..bytecount9|
-  0030: 38 37 35 38 66 69 6c 65 63 6f 75 6e 74 31 30 33 |8758filecount103|
-  0040: 30 72 65 71 75 69 72 65 6d 65 6e 74 73 64 6f 74 |0requirementsdot|
-  0050: 65 6e 63 6f 64 65 20 66 6e 63 61 63 68 65 20 67 |encode fncache g|
-  0060: 65 6e 65 72 61 6c 64 65 6c 74 61 20 72 65 76 6c |eneraldelta revl|
-  0070: 6f 67 76 31 20 73 74 6f 72 65 76 65 72 73 69 6f |ogv1 storeversio|
-  0080: 6e 76 32 00 00 10 00 73 08 42 64 61 74 61 2f 30 |nv2....s.Bdata/0|
-  0090: 2e 69 00 03 00 01 00 00 00 00 00 00 00 02 00 00 |.i..............|
-  00a0: 00 01 00 00 00 00 00 00 00 01 ff ff ff ff ff ff |................|
-  00b0: ff ff 80 29 63 a0 49 d3 23 87 bf ce fe 56 67 92 |...)c.I.#....Vg.|
-  00c0: 67 2c 69 d1 ec 39 00 00 00 00 00 00 00 00 00 00 |g,i..9..........|
-  00d0: 00 00 75 30 73 08 42 64 61 74 61 2f 31 2e 69 00 |..u0s.Bdata/1.i.|
-  00e0: 03 00 01 00 00 00 00 00 00 00 02 00 00 00 01 00 |................|
-  00f0: 00 00 00 00 00 00 01 ff ff ff ff ff ff ff ff f9 |................|
+  0010: 68 07 53 54 52 45 41 4d 31 00 00 00 00 03 00 09 |h.STREAM1.......|
+  0020: 05 09 04 0c 2d 62 79 74 65 63 6f 75 6e 74 39 38 |....-bytecount98|
+  0030: 37 35 38 66 69 6c 65 63 6f 75 6e 74 31 30 33 30 |758filecount1030|
+  0040: 72 65 71 75 69 72 65 6d 65 6e 74 73 64 6f 74 65 |requirementsdote|
+  0050: 6e 63 6f 64 65 20 66 6e 63 61 63 68 65 20 67 65 |ncode fncache ge|
+  0060: 6e 65 72 61 6c 64 65 6c 74 61 20 72 65 76 6c 6f |neraldelta revlo|
+  0070: 67 76 31 20 73 74 6f 72 65 00 00 10 00 73 08 42 |gv1 store....s.B|
+  0080: 64 61 74 61 2f 30 2e 69 00 03 00 01 00 00 00 00 |data/0.i........|
+  0090: 00 00 00 02 00 00 00 01 00 00 00 00 00 00 00 01 |................|
+  00a0: ff ff ff ff ff ff ff ff 80 29 63 a0 49 d3 23 87 |.........)c.I.#.|
+  00b0: bf ce fe 56 67 92 67 2c 69 d1 ec 39 00 00 00 00 |...Vg.g,i..9....|
+  00c0: 00 00 00 00 00 00 00 00 75 30 73 08 42 64 61 74 |........u0s.Bdat|
+  00d0: 61 2f 31 2e 69 00 03 00 01 00 00 00 00 00 00 00 |a/1.i...........|
+  00e0: 02 00 00 00 01 00 00 00 00 00 00 00 01 ff ff ff |................|
+  00f0: ff ff ff ff ff f9 76 da 1d 0d f2 25 6c de 08 db |......v....%l...|
 
 --uncompressed is an alias to --stream
 
@@ -124,7 +124,7 @@ 
   streaming all changes
   sending getbundle command
   bundle2-input-bundle: with-transaction
-  bundle2-input-part: "stream" (params: 4 mandatory) supported
+  bundle2-input-part: "stream1" (params: 3 mandatory) supported
   applying stream bundle
   1030 files to transfer, 96.4 KB of data
   starting 4 threads for background file closing
diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py
--- a/mercurial/streamclone.py
+++ b/mercurial/streamclone.py
@@ -39,7 +39,7 @@ 
 
     bundle2supported = False
     if pullop.canusebundle2:
-        if 'v2' in pullop.remotebundle2caps.get('stream', []):
+        if 'v1' in pullop.remotebundle2caps.get('stream', []):
             bundle2supported = True
         # else
             # Server doesn't support bundle2 stream clone or doesn't support
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1767,9 +1767,9 @@ 
 
     return info, bundler.getchunks()
 
-@getbundle2partsgenerator('stream')
-def _getbundlestream(bundler, repo, source, bundlecaps=None,
-                     b2caps=None, heads=None, common=None, **kwargs):
+@getbundle2partsgenerator('stream1')
+def _getbundlestream1(bundler, repo, source, bundlecaps=None,
+                      b2caps=None, heads=None, common=None, **kwargs):
     if not kwargs.get('stream', False):
         return
 
@@ -1780,11 +1780,10 @@ 
 
     filecount, bytecount, it = streamclone.generatev2(repo)
     requirements = ' '.join(sorted(repo.requirements))
-    part = bundler.newpart('stream', data=it)
+    part = bundler.newpart('stream1', data=it)
     part.addparam('bytecount', '%d' % bytecount, mandatory=True)
     part.addparam('filecount', '%d' % filecount, mandatory=True)
     part.addparam('requirements', requirements, mandatory=True)
-    part.addparam('version', 'v2', mandatory=True)
 
 @getbundle2partsgenerator('changegroup')
 def _getbundlechangegrouppart(bundler, repo, source, bundlecaps=None,
diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1491,7 +1491,7 @@ 
                 'remote-changegroup': ('http', 'https'),
                 'hgtagsfnodes': (),
                 'phases': ('heads',),
-                'stream': ('v2',),
+                'stream': ('v1',),
                }
 
 def getrepocaps(repo, allowpushback=False, role=None):
@@ -2130,12 +2130,9 @@ 
             hookargs[key] = value
         op.addhookargs(hookargs)
 
-@parthandler('stream', ('requirements', 'filecount', 'bytecount', 'version'))
-def handlestreambundle(op, part):
+@parthandler('stream1', ('requirements', 'filecount', 'bytecount'))
+def handlestreamv2bundle(op, part):
 
-    version = part.params['version']
-    if version != 'v2':
-        raise error.Abort(_('unknown stream bundle version %s') % version)
     requirements = part.params['requirements'].split()
     filecount = int(part.params['filecount'])
     bytecount = int(part.params['bytecount'])