Patchwork D7512: exchange: guard against method invocation on `b2caps=None` args

login
register
mail settings
Submitter phabricator
Date Nov. 23, 2019, 5:15 a.m.
Message ID <differential-rev-PHID-DREV-ggynhzjcpyfaggsml5w5-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43465/
State Superseded
Headers show

Comments

phabricator - Nov. 23, 2019, 5:15 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I couldn't figure out how these are called, but the value is pretty obviously
  set at least for the cases that have tests.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/exchange.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - Nov. 24, 2019, 5:14 a.m.
>      """add a changegroup part to the requested bundle"""
> -    if not kwargs.get('cg', True):
> +    if not kwargs.get('cg', True) or not b2caps:
>          return

Is it valid to call these functions with `b2caps=None`? I suspect it would
be a bug or a data corruption.
phabricator - Nov. 24, 2019, 5:16 a.m.
yuja added a comment.


  >   """add a changegroup part to the requested bundle"""
  >
  > - if not kwargs.get('cg', True):
  >
  > +    if not kwargs.get('cg', True) or not b2caps:
  >
  >   return
  
  Is it valid to call these functions with `b2caps=None`? I suspect it would
  be a bug or a data corruption.

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, dlax, indygreg
Cc: yuja, mercurial-devel
phabricator - Nov. 25, 2019, 5:54 p.m.
mharbison72 added a comment.


  In D7512#110528 <https://phab.mercurial-scm.org/D7512#110528>, @yuja wrote:
  
  >>   """add a changegroup part to the requested bundle"""
  >>
  >> - if not kwargs.get('cg', True):
  >>
  >> +    if not kwargs.get('cg', True) or not b2caps:
  >>
  >>   return
  >
  > Is it valid to call these functions with `b2caps=None`? I suspect it would
  > be a bug or a data corruption.
  
  The only caller I can find[1] will indeed pass something, even if it is `{}`.  I can change these to asserts if you want.
  
  [1] https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/exchange.py#l2448

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, dlax, indygreg
Cc: yuja, mercurial-devel
Yuya Nishihara - Nov. 26, 2019, 1:04 p.m.
>   >>   """add a changegroup part to the requested bundle"""
>   >>
>   >> - if not kwargs.get('cg', True):
>   >>
>   >> +    if not kwargs.get('cg', True) or not b2caps:
>   >>
>   >>   return
>   >
>   > Is it valid to call these functions with `b2caps=None`? I suspect it would
>   > be a bug or a data corruption.
>   
>   The only caller I can find[1] will indeed pass something, even if it is `{}`.  I can change these to asserts if you want.
>   
>   [1] https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/exchange.py#l2448

Well, I have no expertise around this module, so I have no idea which is
better. I just don't know if it's valid to return successfully if b2caps
is None.
phabricator - Nov. 26, 2019, 1:09 p.m.
yuja added a comment.


  >   >>   """add a changegroup part to the requested bundle"""
  >   >>
  >   >> - if not kwargs.get('cg', True):
  >   >>
  >   >> +    if not kwargs.get('cg', True) or not b2caps:
  >   >>
  >   >>   return
  >   >
  >   > Is it valid to call these functions with `b2caps=None`? I suspect it would
  >   > be a bug or a data corruption.
  >   The only caller I can find[1] will indeed pass something, even if it is `{}`.  I can change these to asserts if you want.
  >   [1] https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/exchange.py#l2448
  
  Well, I have no expertise around this module, so I have no idea which is
  better. I just don't know if it's valid to return successfully if b2caps
  is None.

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, dlax, indygreg
Cc: yuja, mercurial-devel
phabricator - Nov. 26, 2019, 1:21 p.m.
mharbison72 added a comment.
mharbison72 added a subscriber: marmoute.


  In D7512#110589 <https://phab.mercurial-scm.org/D7512#110589>, @yuja wrote:
  
  >>   >>   """add a changegroup part to the requested bundle"""
  >>   >>
  >>   >> - if not kwargs.get('cg', True):
  >>   >>
  >>   >> +    if not kwargs.get('cg', True) or not b2caps:
  >>   >>
  >>   >>   return
  >>   >
  >>   > Is it valid to call these functions with `b2caps=None`? I suspect it would
  >>   > be a bug or a data corruption.
  >>   The only caller I can find[1] will indeed pass something, even if it is `{}`.  I can change these to asserts if you want.
  >>   [1] https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/exchange.py#l2448
  >
  > Well, I have no expertise around this module, so I have no idea which is
  > better. I just don't know if it's valid to return successfully if b2caps
  > is None.
  
  @marmoute?

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, dlax, indygreg
Cc: marmoute, yuja, mercurial-devel
phabricator - Nov. 26, 2019, 3:06 p.m.
marmoute added a comment.


  In D7512#110590 <https://phab.mercurial-scm.org/D7512#110590>, @mharbison72 wrote:
  
  > @marmoute?
  
  I will have a look.

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, dlax, indygreg
Cc: marmoute, yuja, mercurial-devel

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -2474,7 +2474,7 @@ 
     **kwargs
 ):
     """add a changegroup part to the requested bundle"""
-    if not kwargs.get('cg', True):
+    if not kwargs.get('cg', True) or not b2caps:
         return
 
     version = b'01'
@@ -2536,7 +2536,7 @@ 
     """add a bookmark part to the requested bundle"""
     if not kwargs.get('bookmarks', False):
         return
-    if b'bookmarks' not in b2caps:
+    if not b2caps or b'bookmarks' not in b2caps:
         raise error.Abort(_(b'no common bookmarks exchange method'))
     books = bookmod.listbinbookmarks(repo)
     data = bookmod.binaryencode(books)
@@ -2577,7 +2577,7 @@ 
 ):
     """add phase heads part to the requested bundle"""
     if kwargs.get('phases', False):
-        if not b'heads' in b2caps.get(b'phases'):
+        if not b2caps or not b'heads' in b2caps.get(b'phases'):
             raise error.Abort(_(b'no common phases exchange method'))
         if heads is None:
             heads = repo.heads()
@@ -2641,7 +2641,7 @@ 
     # Don't send unless:
     # - changeset are being exchanged,
     # - the client supports it.
-    if not (kwargs.get('cg', True) and b'hgtagsfnodes' in b2caps):
+    if not b2caps or not (kwargs.get('cg', True) and b'hgtagsfnodes' in b2caps):
         return
 
     outgoing = _computeoutgoing(repo, heads, common)
@@ -2675,6 +2675,7 @@ 
     # - narrow bundle isn't in play (not currently compatible).
     if (
         not kwargs.get('cg', True)
+        or not b2caps
         or b'rev-branch-cache' not in b2caps
         or kwargs.get('narrow', False)
         or repo.ui.has_section(_NARROWACL_SECTION)