Patchwork [2,of,4] changegroup: refactor emitrevision to use a `deltamode` argument

login
register
mail settings
Submitter Boris Feld
Date Oct. 16, 2018, 8:33 a.m.
Message ID <a075ac3487d6d266ec5f.1539678811@localhost.localdomain>
Download mbox | patch
Permalink /patch/36032/
State New
Headers show

Comments

Boris Feld - Oct. 16, 2018, 8:33 a.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1539115321 -7200
#      Tue Oct 09 22:02:01 2018 +0200
# Node ID a075ac3487d6d266ec5f2dbd6901da21c38d4ed9
# Parent  a69790aaac1946e80e35b28c8f41b14a7f6c0f64
# EXP-Topic slim-bundle
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r a075ac3487d6
changegroup: refactor emitrevision to use a `deltamode` argument

This new argument gathers the semantic of `sendfulltext` and `deltaprevious`
in a single value. We are about to introduce a new type of constraints.
Avoiding yet another argument sounds like a plus.
Gregory Szorc - Oct. 16, 2018, 4:36 p.m.
On Tue, Oct 16, 2018 at 10:36 AM Boris Feld <boris.feld@octobus.net> wrote:

> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1539115321 -7200
> #      Tue Oct 09 22:02:01 2018 +0200
> # Node ID a075ac3487d6d266ec5f2dbd6901da21c38d4ed9
> # Parent  a69790aaac1946e80e35b28c8f41b14a7f6c0f64
> # EXP-Topic slim-bundle
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> a075ac3487d6
> changegroup: refactor emitrevision to use a `deltamode` argument
>
> This new argument gathers the semantic of `sendfulltext` and
> `deltaprevious`
> in a single value. We are about to introduce a new type of constraints.
> Avoiding yet another argument sounds like a plus.
>

I have mixed feelings about this patch. I agree the API is cleaner. It's
just that I feel with shallow clone, we'll need more flexibility in terms
of delta generation - too much flexibility to shoehorn into a single
"deltamode" argument. I think we'll need multiple arguments to influence
delta generation behavior.

But that is the future and this patch is now and is in support of a feature
to facilitate performance testing. We shouldn't let the future dictate
today. So I think I'll take this once the rest of the patches in this
series clear.


>
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -2229,6 +2229,12 @@ class revlog(object):
>          if nodesorder is None and not self._generaldelta:
>              nodesorder = 'storage'
>
> +        deltamode = storageutil.DELTAMODE_STD
> +        if deltaprevious:
> +            deltamode = storageutil.DELTAMODE_PREV
> +        elif not self._storedeltachains:
> +            deltamode = storageutil.DELTAMODE_FULL
> +
>          return storageutil.emitrevisions(
>              self, nodes, nodesorder, revlogrevisiondelta,
>              deltaparentfn=self.deltaparent,
> @@ -2236,10 +2242,9 @@ class revlog(object):
>              rawsizefn=self.rawsize,
>              revdifffn=self.revdiff,
>              flagsfn=self.flags,
> -            sendfulltext=not self._storedeltachains,
> +            deltamode=deltamode,
>              revisiondata=revisiondata,
> -            assumehaveparentrevisions=assumehaveparentrevisions,
> -            deltaprevious=deltaprevious)
> +            assumehaveparentrevisions=assumehaveparentrevisions)
>
>      DELTAREUSEALWAYS = 'always'
>      DELTAREUSESAMEREVS = 'samerevs'
> diff --git a/mercurial/utils/storageutil.py
> b/mercurial/utils/storageutil.py
> --- a/mercurial/utils/storageutil.py
> +++ b/mercurial/utils/storageutil.py
> @@ -265,11 +265,14 @@ def resolvestripinfo(minlinkrev, tiprev,
>
>      return strippoint, brokenrevs
>
> +DELTAMODE_STD = 'default'
> +DELTAMODE_PREV = 'previous'
> +DELTAMODE_FULL = 'fulltext'
> +
>  def emitrevisions(store, nodes, nodesorder, resultcls, deltaparentfn=None,
>                    candeltafn=None, rawsizefn=None, revdifffn=None,
> flagsfn=None,
> -                  sendfulltext=False,
> -                  revisiondata=False, assumehaveparentrevisions=False,
> -                  deltaprevious=False):
> +                  deltamode=DELTAMODE_STD,
> +                  revisiondata=False, assumehaveparentrevisions=False):
>      """Generic implementation of ifiledata.emitrevisions().
>
>      Emitting revision data is subtly complex. This function attempts to
> @@ -320,14 +323,17 @@ def emitrevisions(store, nodes, nodesord
>         Callable receiving a revision number and returns the integer flags
>         value for it. If not defined, flags value will be 0.
>
> -    ``sendfulltext``
> +    ``deltamode``
> +       constaint on delta to be sent:
> +       * DELTAMODE_STD  - normal mode, try to reuse storage deltas,
> +       * DELTAMODE_PREV - only delta against "prev",
> +       * DELTAMODE_FULL - only issue full snapshot.
> +
>         Whether to send fulltext revisions instead of deltas, if allowed.
>
>      ``nodesorder``
>      ``revisiondata``
>      ``assumehaveparentrevisions``
> -    ``deltaprevious``
> -       See ``ifiledata.emitrevisions()`` interface documentation.
>      """
>
>      fnode = store.node
> @@ -343,7 +349,7 @@ def emitrevisions(store, nodes, nodesord
>
>      prevrev = None
>
> -    if deltaprevious or assumehaveparentrevisions:
> +    if deltamode == DELTAMODE_PREV or assumehaveparentrevisions:
>          prevrev = store.parentrevs(revs[0])[0]
>
>      # Set of revs available to delta against.
> @@ -362,11 +368,11 @@ def emitrevisions(store, nodes, nodesord
>              deltaparentrev = nullrev
>
>          # Forced delta against previous mode.
> -        if deltaprevious:
> +        if deltamode == DELTAMODE_PREV:
>              baserev = prevrev
>
>          # We're instructed to send fulltext. Honor that.
> -        elif sendfulltext:
> +        elif deltamode == DELTAMODE_FULL:
>              baserev = nullrev
>
>          # There is a delta in storage. We try to use that because it
> @@ -427,7 +433,7 @@ def emitrevisions(store, nodes, nodesord
>                          baserevisionsize = len(store.revision(baserev,
>                                                                raw=True))
>
> -            elif baserev == nullrev and not deltaprevious:
> +            elif baserev == nullrev and not deltamode == DELTAMODE_PREV:
>                  revision = store.revision(node, raw=True)
>                  available.add(rev)
>              else:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -2229,6 +2229,12 @@  class revlog(object):
         if nodesorder is None and not self._generaldelta:
             nodesorder = 'storage'
 
+        deltamode = storageutil.DELTAMODE_STD
+        if deltaprevious:
+            deltamode = storageutil.DELTAMODE_PREV
+        elif not self._storedeltachains:
+            deltamode = storageutil.DELTAMODE_FULL
+
         return storageutil.emitrevisions(
             self, nodes, nodesorder, revlogrevisiondelta,
             deltaparentfn=self.deltaparent,
@@ -2236,10 +2242,9 @@  class revlog(object):
             rawsizefn=self.rawsize,
             revdifffn=self.revdiff,
             flagsfn=self.flags,
-            sendfulltext=not self._storedeltachains,
+            deltamode=deltamode,
             revisiondata=revisiondata,
-            assumehaveparentrevisions=assumehaveparentrevisions,
-            deltaprevious=deltaprevious)
+            assumehaveparentrevisions=assumehaveparentrevisions)
 
     DELTAREUSEALWAYS = 'always'
     DELTAREUSESAMEREVS = 'samerevs'
diff --git a/mercurial/utils/storageutil.py b/mercurial/utils/storageutil.py
--- a/mercurial/utils/storageutil.py
+++ b/mercurial/utils/storageutil.py
@@ -265,11 +265,14 @@  def resolvestripinfo(minlinkrev, tiprev,
 
     return strippoint, brokenrevs
 
+DELTAMODE_STD = 'default'
+DELTAMODE_PREV = 'previous'
+DELTAMODE_FULL = 'fulltext'
+
 def emitrevisions(store, nodes, nodesorder, resultcls, deltaparentfn=None,
                   candeltafn=None, rawsizefn=None, revdifffn=None, flagsfn=None,
-                  sendfulltext=False,
-                  revisiondata=False, assumehaveparentrevisions=False,
-                  deltaprevious=False):
+                  deltamode=DELTAMODE_STD,
+                  revisiondata=False, assumehaveparentrevisions=False):
     """Generic implementation of ifiledata.emitrevisions().
 
     Emitting revision data is subtly complex. This function attempts to
@@ -320,14 +323,17 @@  def emitrevisions(store, nodes, nodesord
        Callable receiving a revision number and returns the integer flags
        value for it. If not defined, flags value will be 0.
 
-    ``sendfulltext``
+    ``deltamode``
+       constaint on delta to be sent:
+       * DELTAMODE_STD  - normal mode, try to reuse storage deltas,
+       * DELTAMODE_PREV - only delta against "prev",
+       * DELTAMODE_FULL - only issue full snapshot.
+
        Whether to send fulltext revisions instead of deltas, if allowed.
 
     ``nodesorder``
     ``revisiondata``
     ``assumehaveparentrevisions``
-    ``deltaprevious``
-       See ``ifiledata.emitrevisions()`` interface documentation.
     """
 
     fnode = store.node
@@ -343,7 +349,7 @@  def emitrevisions(store, nodes, nodesord
 
     prevrev = None
 
-    if deltaprevious or assumehaveparentrevisions:
+    if deltamode == DELTAMODE_PREV or assumehaveparentrevisions:
         prevrev = store.parentrevs(revs[0])[0]
 
     # Set of revs available to delta against.
@@ -362,11 +368,11 @@  def emitrevisions(store, nodes, nodesord
             deltaparentrev = nullrev
 
         # Forced delta against previous mode.
-        if deltaprevious:
+        if deltamode == DELTAMODE_PREV:
             baserev = prevrev
 
         # We're instructed to send fulltext. Honor that.
-        elif sendfulltext:
+        elif deltamode == DELTAMODE_FULL:
             baserev = nullrev
 
         # There is a delta in storage. We try to use that because it
@@ -427,7 +433,7 @@  def emitrevisions(store, nodes, nodesord
                         baserevisionsize = len(store.revision(baserev,
                                                               raw=True))
 
-            elif baserev == nullrev and not deltaprevious:
+            elif baserev == nullrev and not deltamode == DELTAMODE_PREV:
                 revision = store.revision(node, raw=True)
                 available.add(rev)
             else: