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

[Yuya Nishihara]

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.da...@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
> >> >
> >> wrote:
> >>
> >> # HG changeset patch
> >> # User Pierre-Yves David  >> >
> >>
> >> # 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Pierre-Yves David]

On 08/14/2016 06:07 PM, Gregory Szorc wrote:

On Thu, Aug 11, 2016 at 1:06 PM, Pierre-Yves David
>
wrote:

# HG changeset patch
# User Pierre-Yves David >
# 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.pyTue Aug 09 15:55:44 2016 +0200
+++ b/mercurial/discovery.pyTue 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 

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

[Gregory Szorc]

On Thu, Aug 11, 2016 at 1:06 PM, Pierre-Yves David <
pierre-yves.da...@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David 
> # 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.pyTue Aug 09 15:55:44 2016 +0200
> +++ b/mercurial/discovery.pyTue 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel