[PATCH 3 of 5] outgoing: add a 'missingroots' argument

[Gregory Szorc]

On Tue, Aug 16, 2016 at 5:37 PM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> On 08/14/2016 06:07 PM, Gregory Szorc wrote:
>
>> On Thu, Aug 11, 2016 at 1:06 PM, Pierre-Yves David
>> <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
>> wrote:
>>
>>     # HG changeset patch
>>     # User Pierre-Yves David <pierre-yves.david at ens-lyon.org
>>     <mailto:pierre-yves.david at ens-lyon.org>>
>>
>>     # Date 1470774698 -7200
>>     #      Tue Aug 09 22:31:38 2016 +0200
>>     # Node ID 8c4fcb1244bdf79caadadc73fc1e489a160a07ec
>>     # Parent  9ff7059253fd00094799f592462590cd837fb457
>>     # EXP-Topic outgoing
>>     outgoing: add a 'missingroots' argument
>>
>>     This argument can be used instead of 'commonheads' to determine the
>>     'outgoing'
>>     set. We remove the outgoingbetween function as its role can now be
>>     handled by
>>     'outgoing' itself.
>>
>>     I've thought of using an external function instead of making the
>>     constructor
>>     more complicated. However, there is low hanging fruit to improve the
>>     current
>>     code flow by storing some side products of the processing of
>>     'missingroots'. So
>>     in my opinion it make senses to add all this to the class.
>>
>>     diff -r 9ff7059253fd -r 8c4fcb1244bd mercurial/changegroup.py
>>     --- a/mercurial/changegroup.py  Tue Aug 09 15:55:44 2016 +0200
>>     +++ b/mercurial/changegroup.py  Tue Aug 09 22:31:38 2016 +0200
>>     @@ -943,7 +943,7 @@ def changegroupsubset(repo, roots, heads
>>          Another wrinkle is doing the reverse, figuring out which
>>     changeset in
>>          the changegroup a particular filenode or manifestnode belongs to.
>>          """
>>     -    outgoing = discovery.outgoingbetween(repo, roots, heads)
>>     +    outgoing = discovery.outgoing(repo, missingroots=roots,
>>     missingheads=heads)
>>          bundler = getbundler(version, repo)
>>          return getsubset(repo, outgoing, bundler, source)
>>
>>     diff -r 9ff7059253fd -r 8c4fcb1244bd mercurial/discovery.py
>>     --- a/mercurial/discovery.py    Tue Aug 09 15:55:44 2016 +0200
>>     +++ b/mercurial/discovery.py    Tue Aug 09 22:31:38 2016 +0200
>>     @@ -76,11 +76,25 @@ class outgoing(object):
>>          The sets are computed on demand from the heads, unless provided
>>     upfront
>>          by discovery.'''
>>
>>     -    def __init__(self, repo, commonheads=None, missingheads=None):
>>     +    def __init__(self, repo, commonheads=None, missingheads=None,
>>     +                 missingroots=None):
>>     +        # at least one of them must not be set
>>     +        assert None in (commonheads, missingroots)
>>              cl = repo.changelog
>>              if not missingheads:
>>                  missingheads = cl.heads()
>>     -        if not commonheads:
>>     +        if missingroots:
>>     +            discbases = []
>>     +            for n in missingroots:
>>     +                discbases.extend([p for p in cl.parents(n) if p !=
>>     nullid])
>>     +            # TODO remove call to nodesbetween.
>>     +            # TODO populate attributes on outgoing instance instead
>>     of setting
>>     +            # discbases.
>>     +            csets, roots, heads = cl.nodesbetween(missingroots,
>>     missingheads)
>>     +            included = set(csets)
>>     +            missingheads = heads
>>     +            commonheads = [n for n in discbases if n not in included]
>>     +        elif not commonheads:
>>                  commonheads = [nullid]
>>              self.commonheads = commonheads
>>              self.missingheads = missingheads
>>     @@ -106,27 +120,6 @@ class outgoing(object):
>>                  self._computecommonmissing()
>>              return self._missing
>>
>>     -def outgoingbetween(repo, roots, heads):
>>     -    """create an ``outgoing`` consisting of nodes between roots and
>>     heads
>>     -
>>     -    The ``missing`` nodes will be descendants of any of the
>>     ``roots`` and
>>     -    ancestors of any of the ``heads``, both are which are defined
>>     as a list
>>     -    of binary nodes.
>>     -    """
>>     -    cl = repo.changelog
>>     -    if not roots:
>>     -        roots = [nullid]
>>     -    discbases = []
>>     -    for n in roots:
>>     -        discbases.extend([p for p in cl.parents(n) if p != nullid])
>>     -    # TODO remove call to nodesbetween.
>>     -    # TODO populate attributes on outgoing instance instead of
>> setting
>>     -    # discbases.
>>     -    csets, roots, heads = cl.nodesbetween(roots, heads)
>>     -    included = set(csets)
>>     -    discbases = [n for n in discbases if n not in included]
>>     -    return outgoing(repo, discbases, heads)
>>     -
>>      def findcommonoutgoing(repo, other, onlyheads=None, force=False,
>>                             commoninc=None, portable=False):
>>          '''Return an outgoing instance to identify the nodes present in
>>     repo but
>>
>>
>> I'm not thrilled about the complexity of outgoing.__init__. Personally,
>> I'd rather have a "dumb" container object that is populated by N
>> specialized functions (like "outgoingbetween"). But that's just my style.
>>
>
> Next step after this is to move the complexe computation from __init__ to
> dedicated method and have the object computed other attributes on the fly
> when requested (as we do for other ones). This should trim the complexity
> of __init__ back to picking default value when needed. I think that would
> be fine. What do you think?
>

Yeah, that sounds fine.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160817/2e23e3c9/attachment-0002.html>