Patchwork D6939: sidedata: apply basic but tight security around exchange

login
register
mail settings
Submitter phabricator
Date Oct. 1, 2019, 4:08 p.m.
Message ID <differential-rev-PHID-DREV-gm5zyezvqllhk6b5zbgq-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41897/
State Superseded
Headers show

Comments

phabricator - Oct. 1, 2019, 4:08 p.m.
marmoute created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We don't currently have code to deal with exchange between repository using
  sidedata and repository not using sidedata. Until we implement such code (eg:
  dropping side data when pushing to a non-sidedata repo) we prevent the two kind
  of repo to speak to each other. This is somewhere similar to what 'treemanifest'
  does.
  
  Note that sidedata exchange is broken unless one use changegroup v3 anyway. See
  next changeset for details.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundle2.py
  mercurial/exchange.py

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 5, 2019, 3:55 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> bundle2.py:1837-1838
> +
> +    bundlesidedata = bool('exp-sidedata' in inpart.params)
> +    reposidedata = bool('exp-sidedata-flag' in op.repo.requirements)
> +    if reposidedata and not bundlesidedata:

Isn't `bool()` redundant here?

> bundle2.py:1840-1841
> +    if reposidedata and not bundlesidedata:
> +        msg = "repository is using sidedata but the bundle source do not"
> +        hint = 'this is currently unsupported'
> +        raise error.Abort(msg, hint=hint)

Need `b''` prefix, I think (also pick one type of quotes maybe? but maybe Black will soon change this anyway...)

REPOSITORY
  rHG Mercurial

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

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

To: marmoute, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Oct. 5, 2019, 3:59 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> martinvonz wrote in bundle2.py:1840-1841
> Need `b''` prefix, I think (also pick one type of quotes maybe? but maybe Black will soon change this anyway...)

Never mind... I've just gotten used to looking at extension code where the source transformer is not run. It is run on this code, of course, so this is correct as is.

REPOSITORY
  rHG Mercurial

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

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

To: marmoute, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Oct. 6, 2019, 1:49 p.m.
indygreg added inline comments.

INLINE COMMENTS

> bundle2.py:2343
> +        if 'exp-sidedata-flag' in repo.requirements:
> +            part.addparam('sidedata', '1')
>  

Shouldn't this by `exp-sidedata`?

REPOSITORY
  rHG Mercurial

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

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

To: marmoute, #hg-reviewers
Cc: indygreg, martinvonz, mercurial-devel
phabricator - Oct. 8, 2019, 12:51 a.m.
marmoute added inline comments.
marmoute marked an inline comment as done.

INLINE COMMENTS

> martinvonz wrote in bundle2.py:1837-1838
> Isn't `bool()` redundant here?

It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up (either resend or followup)

> indygreg wrote in bundle2.py:2343
> Shouldn't this by `exp-sidedata`?

Hum, yeah. That would be safer. Updated code coming

REPOSITORY
  rHG Mercurial

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

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

To: marmoute, #hg-reviewers
Cc: indygreg, martinvonz, mercurial-devel

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -919,6 +919,8 @@ 
         cgpart.addparam('version', version)
     if 'treemanifest' in pushop.repo.requirements:
         cgpart.addparam('treemanifest', '1')
+    if 'exp-sidedata-flag' in pushop.repo.requirements:
+        cgpart.addparam('exp-sidedata', '1')
     def handlereply(op):
         """extract addchangegroup returns from server reply"""
         cgreplies = op.records.getreplies(cgpart.id)
@@ -2211,6 +2213,9 @@ 
     if 'treemanifest' in repo.requirements:
         part.addparam('treemanifest', '1')
 
+    if 'exp-sidedata-flag' in repo.requirements:
+        part.addparam('exp-sidedata', '1')
+
     if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
         and (include or exclude)):
         # this is mandatory because otherwise ACL clients won't work
diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1802,8 +1802,8 @@ 
         result = -1 + changedheads
     return result
 
-@parthandler('changegroup', ('version', 'nbchanges', 'treemanifest',
-                             'targetphase'))
+@parthandler('changegroup', ('version', 'nbchanges', 'exp-sidedata',
+                             'treemanifest', 'targetphase'))
 def handlechangegroup(op, inpart):
     """apply a changegroup part on the repo
 
@@ -1831,6 +1831,14 @@ 
         op.repo.svfs.options = localrepo.resolvestorevfsoptions(
             op.repo.ui, op.repo.requirements, op.repo.features)
         op.repo._writerequirements()
+
+    bundlesidedata = bool('exp-sidedata' in inpart.params)
+    reposidedata = bool('exp-sidedata-flag' in op.repo.requirements)
+    if reposidedata and not bundlesidedata:
+        msg = "repository is using sidedata but the bundle source do not"
+        hint = 'this is currently unsupported'
+        raise error.Abort(msg, hint=hint)
+
     extrakwargs = {}
     targetphase = inpart.params.get('targetphase')
     if targetphase is not None:
@@ -2329,5 +2337,7 @@ 
         part.addparam('version', cgversion)
         if 'treemanifest' in repo.requirements:
             part.addparam('treemanifest', '1')
+        if 'exp-sidedata-flag' in repo.requirements:
+            part.addparam('sidedata', '1')
 
     return bundler