Patchwork [5,of,5] computeoutgoing: move the function from 'changegroup' to 'exchange'

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 11, 2016, 8:06 p.m.
Message ID <c60098bda68be8901e56.1470946015@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16255/
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 1470755195 -7200
#      Tue Aug 09 17:06:35 2016 +0200
# Node ID c60098bda68be8901e564445fe157888f317bf5a
# Parent  0a35b949b92ac83e66ddaa706deb8c0f228a045b
# EXP-Topic outgoing
computeoutgoing: move the function from 'changegroup' to 'exchange'

Now that all users are in exchange, we can safely move the code in the
'exchange' module. This function is really about processing the argument of a
'getbundle' call, so it even makes senses to do so.
Gregory Szorc - Aug. 14, 2016, 3:57 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 1470755195 -7200
> #      Tue Aug 09 17:06:35 2016 +0200
> # Node ID c60098bda68be8901e564445fe157888f317bf5a
> # Parent  0a35b949b92ac83e66ddaa706deb8c0f228a045b
> # EXP-Topic outgoing
> computeoutgoing: move the function from 'changegroup' to 'exchange'
>
> Now that all users are in exchange, we can safely move the code in the
> 'exchange' module. This function is really about processing the argument
> of a
> 'getbundle' call, so it even makes senses to do so.
>
> diff -r 0a35b949b92a -r c60098bda68b mercurial/changegroup.py
> --- a/mercurial/changegroup.py  Tue Aug 09 17:00:38 2016 +0200
> +++ b/mercurial/changegroup.py  Tue Aug 09 17:06:35 2016 +0200
> @@ -15,7 +15,6 @@ import weakref
>  from .i18n import _
>  from .node import (
>      hex,
> -    nullid,
>      nullrev,
>      short,
>  )
> @@ -969,25 +968,6 @@ def getlocalchangegroup(repo, source, ou
>      bundler = getbundler(version, repo, bundlecaps)
>      return getsubset(repo, outgoing, bundler, source)
>
> -def computeoutgoing(repo, heads, common):
> -    """Computes which revs are outgoing given a set of common
> -    and a set of heads.
> -
> -    This is a separate function so extensions can have access to
> -    the logic.
> -
> -    Returns a discovery.outgoing object.
> -    """
> -    cl = repo.changelog
> -    if common:
> -        hasnode = cl.hasnode
> -        common = [n for n in common if hasnode(n)]
> -    else:
> -        common = [nullid]
> -    if not heads:
> -        heads = cl.heads()
> -    return discovery.outgoing(repo, common, heads)
> -
>  def getchangegroup(repo, source, outgoing, bundlecaps=None,
>                     version='01'):
>      """Like changegroupsubset, but returns the set difference between the
> diff -r 0a35b949b92a -r c60098bda68b mercurial/exchange.py
> --- a/mercurial/exchange.py     Tue Aug 09 17:00:38 2016 +0200
> +++ b/mercurial/exchange.py     Tue Aug 09 17:06:35 2016 +0200
> @@ -257,6 +257,25 @@ def buildobsmarkerspart(bundler, markers
>          return bundler.newpart('obsmarkers', data=stream)
>      return None
>
> +def _computeoutgoing(repo, heads, common):
> +    """Computes which revs are outgoing given a set of common
> +    and a set of heads.
> +
> +    This is a separate function so extensions can have access to
> +    the logic.
> +
> +    Returns a discovery.outgoing object.
> +    """
> +    cl = repo.changelog
> +    if common:
> +        hasnode = cl.hasnode
> +        common = [n for n in common if hasnode(n)]
> +    else:
> +        common = [nullid]
> +    if not heads:
> +        heads = cl.heads()
> +    return discovery.outgoing(repo, common, heads)
> +
>  def _forcebundle1(op):
>      """return true if a pull/push must use bundle1
>
> @@ -1535,7 +1554,7 @@ def getbundle(repo, source, heads=None,
>          if kwargs:
>              raise ValueError(_('unsupported getbundle arguments: %s')
>                               % ', '.join(sorted(kwargs.keys())))
> -        outgoing = changegroup.computeoutgoing(repo, heads, common)
> +        outgoing = _computeoutgoing(repo, heads, common)
>          return changegroup.getchangegroup(repo, source, outgoing,
>                                            bundlecaps=bundlecaps)
>
> @@ -1572,7 +1591,7 @@ def _getbundlechangegrouppart(bundler, r
>              if not cgversions:
>                  raise ValueError(_('no common changegroup version'))
>              version = max(cgversions)
> -        outgoing = changegroup.computeoutgoing(repo, heads, common)
> +        outgoing = _computeoutgoing(repo, heads, common)
>          cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
>                                                  bundlecaps=bundlecaps,
>                                                  version=version)
> @@ -1625,7 +1644,7 @@ def _getbundletagsfnodes(bundler, repo,
>      if not (kwargs.get('cg', True) and 'hgtagsfnodes' in b2caps):
>          return
>
> -    outgoing = changegroup.computeoutgoing(repo, heads, common)
> +    outgoing = _computeoutgoing(repo, heads, common)
>
>      if not outgoing.missingheads:
>          return
>

I have a slight preference for this code living in discovery.py since
discovery.py seems to be the place where we figure out the difference in
changesets between peers. But there is overlap between discovery.py and
exchange.py, so it isn't a strong preference.

I think the overall series is a good refactor. A lot of this code didn't
belong in changegroup.py and contributed to a more complicated than
necessary API.
Pierre-Yves David - Aug. 17, 2016, 12:35 a.m.
On 08/14/2016 05:57 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 1470755195 -7200
>     #      Tue Aug 09 17:06:35 2016 +0200
>     # Node ID c60098bda68be8901e564445fe157888f317bf5a
>     # Parent  0a35b949b92ac83e66ddaa706deb8c0f228a045b
>     # EXP-Topic outgoing
>     computeoutgoing: move the function from 'changegroup' to 'exchange'
>
>     Now that all users are in exchange, we can safely move the code in the
>     'exchange' module. This function is really about processing the
>     argument of a
>     'getbundle' call, so it even makes senses to do so.
>
>     diff -r 0a35b949b92a -r c60098bda68b mercurial/changegroup.py
>     --- a/mercurial/changegroup.py  Tue Aug 09 17:00:38 2016 +0200
>     +++ b/mercurial/changegroup.py  Tue Aug 09 17:06:35 2016 +0200
>     @@ -15,7 +15,6 @@ import weakref
>      from .i18n import _
>      from .node import (
>          hex,
>     -    nullid,
>          nullrev,
>          short,
>      )
>     @@ -969,25 +968,6 @@ def getlocalchangegroup(repo, source, ou
>          bundler = getbundler(version, repo, bundlecaps)
>          return getsubset(repo, outgoing, bundler, source)
>
>     -def computeoutgoing(repo, heads, common):
>     -    """Computes which revs are outgoing given a set of common
>     -    and a set of heads.
>     -
>     -    This is a separate function so extensions can have access to
>     -    the logic.
>     -
>     -    Returns a discovery.outgoing object.
>     -    """
>     -    cl = repo.changelog
>     -    if common:
>     -        hasnode = cl.hasnode
>     -        common = [n for n in common if hasnode(n)]
>     -    else:
>     -        common = [nullid]
>     -    if not heads:
>     -        heads = cl.heads()
>     -    return discovery.outgoing(repo, common, heads)
>     -
>      def getchangegroup(repo, source, outgoing, bundlecaps=None,
>                         version='01'):
>          """Like changegroupsubset, but returns the set difference
>     between the
>     diff -r 0a35b949b92a -r c60098bda68b mercurial/exchange.py
>     --- a/mercurial/exchange.py     Tue Aug 09 17:00:38 2016 +0200
>     +++ b/mercurial/exchange.py     Tue Aug 09 17:06:35 2016 +0200
>     @@ -257,6 +257,25 @@ def buildobsmarkerspart(bundler, markers
>              return bundler.newpart('obsmarkers', data=stream)
>          return None
>
>     +def _computeoutgoing(repo, heads, common):
>     +    """Computes which revs are outgoing given a set of common
>     +    and a set of heads.
>     +
>     +    This is a separate function so extensions can have access to
>     +    the logic.
>     +
>     +    Returns a discovery.outgoing object.
>     +    """
>     +    cl = repo.changelog
>     +    if common:
>     +        hasnode = cl.hasnode
>     +        common = [n for n in common if hasnode(n)]
>     +    else:
>     +        common = [nullid]
>     +    if not heads:
>     +        heads = cl.heads()
>     +    return discovery.outgoing(repo, common, heads)
>     +
>      def _forcebundle1(op):
>          """return true if a pull/push must use bundle1
>
>     @@ -1535,7 +1554,7 @@ def getbundle(repo, source, heads=None,
>              if kwargs:
>                  raise ValueError(_('unsupported getbundle arguments: %s')
>                                   % ', '.join(sorted(kwargs.keys())))
>     -        outgoing = changegroup.computeoutgoing(repo, heads, common)
>     +        outgoing = _computeoutgoing(repo, heads, common)
>              return changegroup.getchangegroup(repo, source, outgoing,
>                                                bundlecaps=bundlecaps)
>
>     @@ -1572,7 +1591,7 @@ def _getbundlechangegrouppart(bundler, r
>                  if not cgversions:
>                      raise ValueError(_('no common changegroup version'))
>                  version = max(cgversions)
>     -        outgoing = changegroup.computeoutgoing(repo, heads, common)
>     +        outgoing = _computeoutgoing(repo, heads, common)
>              cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
>                                                      bundlecaps=bundlecaps,
>                                                      version=version)
>     @@ -1625,7 +1644,7 @@ def _getbundletagsfnodes(bundler, repo,
>          if not (kwargs.get('cg', True) and 'hgtagsfnodes' in b2caps):
>              return
>
>     -    outgoing = changegroup.computeoutgoing(repo, heads, common)
>     +    outgoing = _computeoutgoing(repo, heads, common)
>
>          if not outgoing.missingheads:
>              return
>
>
> I have a slight preference for this code living in discovery.py since
> discovery.py seems to be the place where we figure out the difference in
> changesets between peers. But there is overlap between discovery.py and
> exchange.py, so it isn't a strong preference.

This live in exchange because what is actually does is processing 
argument from a getbundle call. This is highlighted by the fact the only 
users are in exchange.py. I think it belong exchange.py

Patch

diff -r 0a35b949b92a -r c60098bda68b mercurial/changegroup.py
--- a/mercurial/changegroup.py	Tue Aug 09 17:00:38 2016 +0200
+++ b/mercurial/changegroup.py	Tue Aug 09 17:06:35 2016 +0200
@@ -15,7 +15,6 @@  import weakref
 from .i18n import _
 from .node import (
     hex,
-    nullid,
     nullrev,
     short,
 )
@@ -969,25 +968,6 @@  def getlocalchangegroup(repo, source, ou
     bundler = getbundler(version, repo, bundlecaps)
     return getsubset(repo, outgoing, bundler, source)
 
-def computeoutgoing(repo, heads, common):
-    """Computes which revs are outgoing given a set of common
-    and a set of heads.
-
-    This is a separate function so extensions can have access to
-    the logic.
-
-    Returns a discovery.outgoing object.
-    """
-    cl = repo.changelog
-    if common:
-        hasnode = cl.hasnode
-        common = [n for n in common if hasnode(n)]
-    else:
-        common = [nullid]
-    if not heads:
-        heads = cl.heads()
-    return discovery.outgoing(repo, common, heads)
-
 def getchangegroup(repo, source, outgoing, bundlecaps=None,
                    version='01'):
     """Like changegroupsubset, but returns the set difference between the
diff -r 0a35b949b92a -r c60098bda68b mercurial/exchange.py
--- a/mercurial/exchange.py	Tue Aug 09 17:00:38 2016 +0200
+++ b/mercurial/exchange.py	Tue Aug 09 17:06:35 2016 +0200
@@ -257,6 +257,25 @@  def buildobsmarkerspart(bundler, markers
         return bundler.newpart('obsmarkers', data=stream)
     return None
 
+def _computeoutgoing(repo, heads, common):
+    """Computes which revs are outgoing given a set of common
+    and a set of heads.
+
+    This is a separate function so extensions can have access to
+    the logic.
+
+    Returns a discovery.outgoing object.
+    """
+    cl = repo.changelog
+    if common:
+        hasnode = cl.hasnode
+        common = [n for n in common if hasnode(n)]
+    else:
+        common = [nullid]
+    if not heads:
+        heads = cl.heads()
+    return discovery.outgoing(repo, common, heads)
+
 def _forcebundle1(op):
     """return true if a pull/push must use bundle1
 
@@ -1535,7 +1554,7 @@  def getbundle(repo, source, heads=None, 
         if kwargs:
             raise ValueError(_('unsupported getbundle arguments: %s')
                              % ', '.join(sorted(kwargs.keys())))
-        outgoing = changegroup.computeoutgoing(repo, heads, common)
+        outgoing = _computeoutgoing(repo, heads, common)
         return changegroup.getchangegroup(repo, source, outgoing,
                                           bundlecaps=bundlecaps)
 
@@ -1572,7 +1591,7 @@  def _getbundlechangegrouppart(bundler, r
             if not cgversions:
                 raise ValueError(_('no common changegroup version'))
             version = max(cgversions)
-        outgoing = changegroup.computeoutgoing(repo, heads, common)
+        outgoing = _computeoutgoing(repo, heads, common)
         cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
                                                 bundlecaps=bundlecaps,
                                                 version=version)
@@ -1625,7 +1644,7 @@  def _getbundletagsfnodes(bundler, repo, 
     if not (kwargs.get('cg', True) and 'hgtagsfnodes' in b2caps):
         return
 
-    outgoing = changegroup.computeoutgoing(repo, heads, common)
+    outgoing = _computeoutgoing(repo, heads, common)
 
     if not outgoing.missingheads:
         return