Patchwork D8414: nodemap: add a new mode option, with an optional "warn" value

login
register
mail settings
Submitter phabricator
Date April 14, 2020, 3:49 p.m.
Message ID <differential-rev-PHID-DREV-r76ns2pakcmqlu5jh4i5-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/46092/
State Superseded
Headers show

Comments

phabricator - April 14, 2020, 3:49 p.m.
marmoute created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When "warn" is set, user will get notified when the slow code, used for
  compatibility is used. This can help people to detect situation were using that
  feature will give them a slowdown instead of a speedup.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8414

AFFECTED FILES
  mercurial/configitems.py
  mercurial/localrepo.py
  mercurial/revlogutils/nodemap.py
  tests/test-persistent-nodemap.t

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-devel
phabricator - April 14, 2020, 6:45 p.m.
pulkit added inline comments.

INLINE COMMENTS

> localrepo.py:941
> +    epnm = ui.config(b'experimental', b'exp-persistent-nodemap.mode')
> +    options[b'exp-persistent-nodemap.mode'] = epnm
>      if ui.configbool(b'devel', b'persistent-nodemap'):

nit: we can directly assign the value instead of temp variable

> nodemap.py:148
> +    if not can_incremental:
> +        msg = _("persistent nodemap in strict mode without efficient method")
> +        if mode == b'warn':

needs a `b''` prefix

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8414/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8414

To: marmoute, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - April 14, 2020, 6:46 p.m.
pulkit added inline comments.

INLINE COMMENTS

> configitems.py:684
>  coreconfigitem(
> +    b'experimental', b'exp-persistent-nodemap.mode', default=b'compat',
> +)

another minor nit: since the config section is `experimental`, we can skip the `exp-` prefix.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8414/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8414

To: marmoute, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - April 15, 2020, 11:19 a.m.
marmoute added inline comments.

INLINE COMMENTS

> pulkit wrote in configitems.py:684
> another minor nit: since the config section is `experimental`, we can skip the `exp-` prefix.

This matcht he pattern used above. In addition the `exp-` prefix make sense here` the feature is (was) in a state were any actual usage is out of question. Using a specific prefix for this prevent people using older version to step into a non-working experimental feature when t hey try to use an experimental feature that works fine in a later version.

> pulkit wrote in localrepo.py:941
> nit: we can directly assign the value instead of temp variable

Then the line is too long an need wrapping. I would rather have the small tmp variable than complex line wrapping here.

> pulkit wrote in nodemap.py:148
> needs a `b''` prefix

good catch, thanks

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8414/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8414

To: marmoute, #hg-reviewers
Cc: pulkit, mercurial-devel

Patch

diff --git a/tests/test-persistent-nodemap.t b/tests/test-persistent-nodemap.t
--- a/tests/test-persistent-nodemap.t
+++ b/tests/test-persistent-nodemap.t
@@ -10,7 +10,9 @@ 
   > [devel]
   > persistent-nodemap=yes
   > EOF
-  $ hg debugbuilddag .+5000 --new-file
+  $ hg debugbuilddag .+5000 --new-file --config "experimental.exp-persistent-nodemap.mode=warn"
+  persistent nodemap in strict mode without efficient method (no-rust no-pure !)
+  persistent nodemap in strict mode without efficient method (no-rust no-pure !)
   $ hg debugnodemap --metadata
   uid: ???????????????? (glob)
   tip-rev: 5000
diff --git a/mercurial/revlogutils/nodemap.py b/mercurial/revlogutils/nodemap.py
--- a/mercurial/revlogutils/nodemap.py
+++ b/mercurial/revlogutils/nodemap.py
@@ -13,6 +13,8 @@ 
 import re
 import struct
 
+from ..i18n import _
+
 from .. import (
     error,
     node as nodemod,
@@ -105,6 +107,9 @@ 
     def addabort(self, *args, **kwargs):
         pass
 
+    def _report(self, *args):
+        pass
+
 
 def update_persistent_nodemap(revlog):
     """update the persistent nodemap right now
@@ -138,6 +143,11 @@ 
     ondisk_docket = revlog._nodemap_docket
     feed_data = util.safehasattr(revlog.index, "update_nodemap_data")
     use_mmap = revlog.opener.options.get(b"exp-persistent-nodemap.mmap")
+    mode = revlog.opener.options.get(b"exp-persistent-nodemap.mode")
+    if not can_incremental:
+        msg = _("persistent nodemap in strict mode without efficient method")
+        if mode == b'warn':
+            tr._report(b"%s\n" % msg)
 
     data = None
     # first attemp an incremental update of the data
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -937,6 +937,8 @@ 
         options[b'exp-persistent-nodemap'] = True
     if ui.configbool(b'experimental', b'exp-persistent-nodemap.mmap'):
         options[b'exp-persistent-nodemap.mmap'] = True
+    epnm = ui.config(b'experimental', b'exp-persistent-nodemap.mode')
+    options[b'exp-persistent-nodemap.mode'] = epnm
     if ui.configbool(b'devel', b'persistent-nodemap'):
         options[b'devel-force-nodemap'] = True
 
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -681,6 +681,9 @@ 
     b'experimental', b'exp-persistent-nodemap.mmap', default=True,
 )
 coreconfigitem(
+    b'experimental', b'exp-persistent-nodemap.mode', default=b'compat',
+)
+coreconfigitem(
     b'experimental', b'server.filesdata.recommended-batch-size', default=50000,
 )
 coreconfigitem(