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

Gregory Szorc gregory.szorc at gmail.com
Mon Aug 8 01:34:39 UTC 2016


On Sun, Aug 7, 2016 at 4:41 PM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> 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.


Changegroup craziness already escapes into other modules through a highly
complicated API with redundant functions that all do nearly the same thing,
but not obviously.


> 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.
>

One of the reasons I'm touching this code is *because* it is messy and
sensitive. I contend that nearly anything is better than what we have today.

I've submitted 2 series that clean up this code. One of them even got
queued - only to be unqueued later. This is quite frustrating, as it is
blocking other work on bundling.

I'm not sure what kind of refactor will pass your review. If you jot down
your ideas for an API, I'll implement them if I agree they are good.
Otherwise, I kindly ask you to consider that perfect is the enemy of done
and to let this refactoring proceed. Worst case, we refactor the API again
later. It's not like the internal API is stable or anything ;)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160807/dd3ab540/attachment-0002.html>


More information about the Mercurial-devel mailing list