This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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.

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.

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?

I will have a look.