[PATCH 2 of 8] discovery: move code for creating outgoing from heads and common (API)

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sun Aug 7 19:41:03 EDT 2016



On 08/06/2016 04:15 AM, Gregory Szorc wrote:
>
>
>> On Aug 5, 2016, at 17:37, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
>>
>>
>>
>>> On 08/05/2016 05:16 AM, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc at gmail.com>
>>> # Date 1470365488 25200
>>> #      Thu Aug 04 19:51:28 2016 -0700
>>> # Node ID 19d16c9bce2fdf86d8c84915a45815bb7d4fd932
>>> # Parent  8a046444390debab5372de0b4f38863c57058f69
>>> discovery: move code for creating outgoing from heads and common (API)
>>>
>>> We recently moved similar code from changegroup.py to
>>> discovery.outgoingbetween(). It makes sense for creation of outgoing
>>> instances to all live together in discovery.py. So move
>>> changegroup.computeoutgoing() to discovery.outgoingheadsandcommon() and
>>> update consumers to use the new function.
>>
>> I like the general idea of cleaning the changegroup API. but I think we can move toward something even saner:
>>
>> With your series we end up with 3 way to obtains an outgoing object
>>
>> - outgoingbetween(repo, roots, heads)
>>  # root can be empty//False
>>
>> - outgoingheadsandcommon(repo, heads, common)
>>  # common will be filtered from locally missing
>>  # heads can be empty (means all heads)
>>
>> - outgoing(revlog, commonheads, missingheads):
>>
>> That is probably 2 more function than we want.
>>
>> First, even if the object will only bear a changelog object, all creators seems to have access to a repository anyway. So we can probably make the class internal and have all caller go through a builder function.
>>
>> Second, we can probably have a single function handling all this with default value:
>>
>> - outgoing(repo, roots=None, commonheads=None, heads=None)
>>  # 'roots' and 'commonheads' cannot be both specified
>>  # if neither 'roots' or 'commonheads' is set, 'commonheads' is nullid
>>  # commonheads is filtered if specified
>>  # heads=None mean 'all heads in the repo
>>
>> In my opinion having a single high level API would make things much simpler.
>
> I agree there is room to improve these APIs. However, that's work for a follow-up series. It's not something that interests me greatly. So if you want to write the patches, go for it.

My main issue here is that it make some of the craziness of changegroup 
escape into other module. If we are to clean up this changegroup things, 
cleaning up the API in the process does not seems like a huges overhead. 
You are touching code that is messy and sensitive enough that some round 
trip of discussion is expected before we are happy about the API we are 
building.

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list