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