This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7512

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

Authored by mharbison72 on Nov 23 2019, 12:15 AM.

Details

Reviewers
dlax
indygreg
Group Reviewers
hg-reviewers
Commits
rHG44b605638918: exchange: guard against method invocation on `b2caps=None` args
rHG0dbea180525c: exchange: guard against method invocation on `b2caps=None` args
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mharbison72 created this revision.Nov 23 2019, 12:15 AM
dlax accepted this revision.Nov 23 2019, 3:43 AM
indygreg accepted this revision.Nov 23 2019, 8:00 PM
This revision is now accepted and ready to land.Nov 23 2019, 8:00 PM
yuja added a subscriber: yuja.Nov 24 2019, 12:16 AM
"""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.

mharbison72 added a comment.Nov 25 2019, 12:54 PM
In 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

yuja added a comment.Nov 26 2019, 8:09 AM
>>   """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.

mharbison72 added a subscriber: marmoute.Nov 26 2019, 8:21 AM
In 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?

marmoute added a comment.Nov 26 2019, 10:06 AM
In D7512#110590, @mharbison72 wrote:

@marmoute?

I will have a look.

Revision Contents
Changeset List

PathPackages
Mmercurial/exchange.py (9 lines)

Diff 18379

mercurial/exchange.py

source, source,
bundlecaps=None, bundlecaps=None,
b2caps=None, b2caps=None,
heads=None, heads=None,
common=None, common=None,
**kwargs **kwargs
): ):
"""add a changegroup part to the requested bundle""" """add a changegroup part to the requested bundle"""
if not kwargs.get('cg', True): if not kwargs.get('cg', True) or not b2caps:
return return
version = b'01' version = b'01'
cgversions = b2caps.get(b'changegroup') cgversions = b2caps.get(b'changegroup')
if cgversions: # 3.1 and 3.2 ship with an empty value if cgversions: # 3.1 and 3.2 ship with an empty value
cgversions = [ cgversions = [
v v
for v in cgversions for v in cgversions
@getbundle2partsgenerator(b'bookmarks') @getbundle2partsgenerator(b'bookmarks')
def _getbundlebookmarkpart( def _getbundlebookmarkpart(
bundler, repo, source, bundlecaps=None, b2caps=None, **kwargs bundler, repo, source, bundlecaps=None, b2caps=None, **kwargs
): ):
"""add a bookmark part to the requested bundle""" """add a bookmark part to the requested bundle"""
if not kwargs.get('bookmarks', False): if not kwargs.get('bookmarks', False):
return 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')) raise error.Abort(_(b'no common bookmarks exchange method'))
books = bookmod.listbinbookmarks(repo) books = bookmod.listbinbookmarks(repo)
data = bookmod.binaryencode(books) data = bookmod.binaryencode(books)
if data: if data:
bundler.newpart(b'bookmarks', data=data) bundler.newpart(b'bookmarks', data=data)
@getbundle2partsgenerator(b'listkeys') @getbundle2partsgenerator(b'listkeys')
@getbundle2partsgenerator(b'phases') @getbundle2partsgenerator(b'phases')
def _getbundlephasespart( def _getbundlephasespart(
bundler, repo, source, bundlecaps=None, b2caps=None, heads=None, **kwargs bundler, repo, source, bundlecaps=None, b2caps=None, heads=None, **kwargs
): ):
"""add phase heads part to the requested bundle""" """add phase heads part to the requested bundle"""
if kwargs.get('phases', False): 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')) raise error.Abort(_(b'no common phases exchange method'))
if heads is None: if heads is None:
heads = repo.heads() heads = repo.heads()
headsbyphase = collections.defaultdict(set) headsbyphase = collections.defaultdict(set)
if repo.publishing(): if repo.publishing():
headsbyphase[phases.public] = heads headsbyphase[phases.public] = heads
else: else:
Only values for heads in this bundle will be transferred. Only values for heads in this bundle will be transferred.
The part data consists of pairs of 20 byte changeset node and .hgtags The part data consists of pairs of 20 byte changeset node and .hgtags
filenodes raw values. filenodes raw values.
""" """
# Don't send unless: # Don't send unless:
# - changeset are being exchanged, # - changeset are being exchanged,
# - the client supports it. # - 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 return
outgoing = _computeoutgoing(repo, heads, common) outgoing = _computeoutgoing(repo, heads, common)
bundle2.addparttagsfnodescache(repo, bundler, outgoing) bundle2.addparttagsfnodescache(repo, bundler, outgoing)
@getbundle2partsgenerator(b'cache:rev-branch-cache') @getbundle2partsgenerator(b'cache:rev-branch-cache')
def _getbundlerevbranchcache( def _getbundlerevbranchcache(
5) closed heads nodes 5) closed heads nodes
""" """
# Don't send unless: # Don't send unless:
# - changeset are being exchanged, # - changeset are being exchanged,
# - the client supports it. # - the client supports it.
# - narrow bundle isn't in play (not currently compatible). # - narrow bundle isn't in play (not currently compatible).
if ( if (
not kwargs.get('cg', True) not kwargs.get('cg', True)
or not b2caps
or b'rev-branch-cache' not in b2caps or b'rev-branch-cache' not in b2caps
or kwargs.get('narrow', False) or kwargs.get('narrow', False)
or repo.ui.has_section(_NARROWACL_SECTION) or repo.ui.has_section(_NARROWACL_SECTION)
): ):
return return
outgoing = _computeoutgoing(repo, heads, common) outgoing = _computeoutgoing(repo, heads, common)
bundle2.addpartrevbranchcache(repo, bundler, outgoing) bundle2.addpartrevbranchcache(repo, bundler, outgoing)