Page MenuHomePhabricator

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

Authored by mharbison72 on Sat, Nov 23, 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.Sat, Nov 23, 12:15 AM
dlax accepted this revision.Sat, Nov 23, 3:43 AM
indygreg accepted this revision.Sat, Nov 23, 8:00 PM
This revision is now accepted and ready to land.Sat, Nov 23, 8:00 PM
yuja added a subscriber: yuja.Sun, Nov 24, 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.Tue, Nov 26, 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.