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

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 11, 2016, 8:06 p.m.
Message ID <8c4fcb1244bdf79caada.1470946013@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16253/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 11, 2016, 8:06 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@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.
Yuya Nishihara - Aug. 14, 2016, 9:18 a.m.
On Thu, 11 Aug 2016 22:06:53 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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.

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

I can't tell if this is an improvement or not, but the overall changes look
good to me. I'll let Greg or Augie take 2nd pass.
Gregory Szorc - Aug. 14, 2016, 4:07 p.m.
On Thu, Aug 11, 2016 at 1:06 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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.
Pierre-Yves David - Aug. 17, 2016, 12:37 a.m.
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@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@ens-lyon.org
>     <mailto:pierre-yves.david@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?

Cheers.
Gregory Szorc - Aug. 18, 2016, 3:16 a.m.
On Tue, Aug 16, 2016 at 5:37 PM, Pierre-Yves David <
pierre-yves.david@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@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>     # HG changeset patch
>>     # User Pierre-Yves David <pierre-yves.david@ens-lyon.org
>>     <mailto:pierre-yves.david@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.
Yuya Nishihara - Aug. 18, 2016, 9:19 a.m.
On Wed, 17 Aug 2016 20:16:18 -0700, Gregory Szorc wrote:
> On Tue, Aug 16, 2016 at 5:37 PM, Pierre-Yves David <
> pierre-yves.david@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@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> >> wrote:
> >>
> >>     # HG changeset patch
> >>     # User Pierre-Yves David <pierre-yves.david@ens-lyon.org
> >>     <mailto:pierre-yves.david@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

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

Queued the series, thanks.

Patch

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