Patchwork D9785: share: collapse 3 different bool configs into one enum config

login
register
mail settings
Submitter phabricator
Date Jan. 15, 2021, 6:48 a.m.
Message ID <differential-rev-PHID-DREV-wccinkdseq7l36dpt7ib-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/48096/
State Superseded
Headers show

Comments

phabricator - Jan. 15, 2021, 6:48 a.m.
pulkit created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  Recently I implemented various boolean configs which control how to behave when
  there is a share-safe mismatch between source and share repository. Mismatch
  means that source supports share-safe where as share does not or vice versa.
  
  However, while discussion and documentation we realized that it's too
  complicated and there are some combinations of values which makes no sense.
  
  We decided to introduce a single config option with 4 possible values which
  makes controlling and understanding things easier.
  
  The config option `share.source-safe-mismatch` can have following 4 values:
  
  - abort (default): error out if there is mismatch
  - allow: allow to work with respecting share source configuration
  - upgrade-abort: try to upgrade, if it fails, abort
  - upgrade-allow: try to upgrade, if it fails, continue in allow mode
  
  I am not sure if I can explain 3 config options which I deleted right now in
  just 5 lines which is a sign of how complex they became.
  
  No test changes demonstrate that functionality is same, only names have changed.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/configitems.py
  mercurial/localrepo.py
  mercurial/upgrade.py
  tests/test-share-safe.t

CHANGE DETAILS




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

Patch

diff --git a/tests/test-share-safe.t b/tests/test-share-safe.t
--- a/tests/test-share-safe.t
+++ b/tests/test-share-safe.t
@@ -484,12 +484,12 @@ 
 Testing automatic downgrade of shares when config is set
 
   $ touch ../ss-share/.hg/wlock
-  $ hg log -GT "{node}: {desc}\n" -R ../ss-share --config experimental.sharesafe-auto-downgrade-shares=true
+  $ hg log -GT "{node}: {desc}\n" -R ../ss-share --config share.source-safe-mismatch=upgrade-abort
   abort: failed to downgrade share, got error: Lock held
   [255]
   $ rm ../ss-share/.hg/wlock
 
-  $ hg log -GT "{node}: {desc}\n" -R ../ss-share --config experimental.sharesafe-auto-downgrade-shares=true
+  $ hg log -GT "{node}: {desc}\n" -R ../ss-share --config share.source-safe-mismatch=upgrade-abort
   repository downgraded to not use share-safe mode
   @  f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
   |
@@ -532,25 +532,25 @@ 
 
 Check that if lock is taken, upgrade fails but read operation are successful
   $ touch ../nss-share/.hg/wlock
-  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config experimental.sharesafe-auto-upgrade-shares=true
+  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config share.source-safe-mismatch=upgrade-allow
   failed to upgrade share, got error: Lock held
   @  f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
   |
   o  f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo
   
 
-  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config experimental.sharesafe-auto-upgrade-shares=true --config experimental.sharesafe-warn-outdated-shares=false
+  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config share.source-safe-mismatch=upgrade-allow --config experimental.sharesafe-warn-outdated-shares=false
   @  f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
   |
   o  f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo
   
 
-  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config experimental.sharesafe-auto-upgrade-shares=true --config experimental.sharesafe-auto-upgrade-fail-error=true
+  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config share.source-safe-mismatch=upgrade-abort
   abort: failed to upgrade share, got error: Lock held
   [255]
 
   $ rm ../nss-share/.hg/wlock
-  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config experimental.sharesafe-auto-upgrade-shares=true
+  $ hg log -GT "{node}: {desc}\n" -R ../nss-share --config share.source-safe-mismatch=upgrade-abort
   repository upgraded to use share-safe mode
   @  f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
   |
diff --git a/mercurial/upgrade.py b/mercurial/upgrade.py
--- a/mercurial/upgrade.py
+++ b/mercurial/upgrade.py
@@ -241,7 +241,9 @@ 
             upgrade_op.print_post_op_messages()
 
 
-def upgrade_share_to_safe(ui, hgvfs, storevfs, current_requirements):
+def upgrade_share_to_safe(
+    ui, hgvfs, storevfs, current_requirements, mismatch_config
+):
     """Upgrades a share to use share-safe mechanism"""
     wlock = None
     store_requirements = localrepo._readrequires(storevfs, False)
@@ -252,12 +254,16 @@ 
     # add share-safe requirement as it will mark the share as share-safe
     diffrequires.add(requirementsmod.SHARESAFE_REQUIREMENT)
     current_requirements.add(requirementsmod.SHARESAFE_REQUIREMENT)
+    # in `allow` case, we don't try to upgrade, we just respect the source
+    # state, update requirements and continue
+    if mismatch_config == b'allow':
+        return
     try:
         wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
         scmutil.writerequires(hgvfs, diffrequires)
         ui.warn(_(b'repository upgraded to use share-safe mode\n'))
     except error.LockError as e:
-        if ui.configbool(b'experimental', b'sharesafe-auto-upgrade-fail-error'):
+        if mismatch_config == b'upgrade-abort':
             raise error.Abort(
                 _(b'failed to upgrade share, got error: %s')
                 % stringutil.forcebytestr(e.strerror)
@@ -277,6 +283,7 @@ 
     hgvfs,
     sharedvfs,
     current_requirements,
+    mismatch_config,
 ):
     """Downgrades a share which use share-safe to not use it"""
     wlock = None
@@ -287,18 +294,23 @@ 
     source_requirements -= requirementsmod.WORKING_DIR_REQUIREMENTS
     current_requirements |= source_requirements
     current_requirements.remove(requirementsmod.SHARESAFE_REQUIREMENT)
+    # in `allow` mode, we just update the config for this run and don't try
+    # write them
+    if mismatch_config == b'allow':
+        return
 
     try:
         wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
         scmutil.writerequires(hgvfs, current_requirements)
         ui.warn(_(b'repository downgraded to not use share-safe mode\n'))
     except error.LockError as e:
-        # raise error right away because if downgrade failed, we cannot load
-        # the repository because it does not have complete set of requirements
-        raise error.Abort(
-            _(b'failed to downgrade share, got error: %s')
-            % stringutil.forcebytestr(e.strerror)
-        )
+        # If upgrade-abort is set, abort when upgrade fails, else let the
+        # process continue as `upgrade-allow` is set
+        if mismatch_config == b'upgrade-abort':
+            raise error.Abort(
+                _(b'failed to downgrade share, got error: %s')
+                % stringutil.forcebytestr(e.strerror)
+            )
     finally:
         if wlock:
             wlock.release()
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -567,6 +567,7 @@ 
     # repository was shared the old way. We check the share source .hg/requires
     # for SHARESAFE_REQUIREMENT to detect whether the current repository needs
     # to be reshared
+    mismatch_config = ui.config(b'share', b'source-safe-mismatch')
     if requirementsmod.SHARESAFE_REQUIREMENT in requirements:
 
         if (
@@ -574,8 +575,10 @@ 
             and requirementsmod.SHARESAFE_REQUIREMENT
             not in _readrequires(sharedvfs, True)
         ):
-            if ui.configbool(
-                b'experimental', b'sharesafe-auto-downgrade-shares'
+            if mismatch_config in (
+                b'upgrade-allow',
+                b'allow',
+                b'upgrade-abort',
             ):
                 # prevent cyclic import localrepo -> upgrade -> localrepo
                 from . import upgrade
@@ -585,11 +588,19 @@ 
                     hgvfs,
                     sharedvfs,
                     requirements,
+                    mismatch_config,
+                )
+            elif mismatch_config == b'abort':
+                raise error.Abort(
+                    _(
+                        b"share source does not support exp-sharesafe requirement"
+                    )
                 )
             else:
                 raise error.Abort(
                     _(
-                        b"share source does not support exp-sharesafe requirement"
+                        b"share-safe mismatch with source.\nUnrecognized"
+                        b" value of `share.source-safe-mismatch` set."
                     )
                 )
         else:
@@ -597,7 +608,11 @@ 
     elif shared:
         sourcerequires = _readrequires(sharedvfs, False)
         if requirementsmod.SHARESAFE_REQUIREMENT in sourcerequires:
-            if ui.configbool(b'experimental', b'sharesafe-auto-upgrade-shares'):
+            if mismatch_config in (
+                b'upgrade-allow',
+                b'allow',
+                b'upgrade-abort',
+            ):
                 # prevent cyclic import localrepo -> upgrade -> localrepo
                 from . import upgrade
 
@@ -606,12 +621,20 @@ 
                     hgvfs,
                     storevfs,
                     requirements,
+                    mismatch_config,
+                )
+            elif mismatch_config == b'abort':
+                raise error.Abort(
+                    _(
+                        b'version mismatch: source use share-safe'
+                        b' functionality while current share does not'
+                    )
                 )
             else:
                 raise error.Abort(
                     _(
-                        b'version mismatch: source use share-safe'
-                        b' functionality while current share does not'
+                        b"share-safe mismatch with source.\nUnrecognized"
+                        b" value of `share.source-safe-mismatch` set."
                     )
                 )
 
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -1074,21 +1074,6 @@ 
 )
 coreconfigitem(
     b'experimental',
-    b'sharesafe-auto-downgrade-shares',
-    default=False,
-)
-coreconfigitem(
-    b'experimental',
-    b'sharesafe-auto-upgrade-shares',
-    default=False,
-)
-coreconfigitem(
-    b'experimental',
-    b'sharesafe-auto-upgrade-fail-error',
-    default=False,
-)
-coreconfigitem(
-    b'experimental',
     b'sharesafe-warn-outdated-shares',
     default=True,
 )
@@ -1898,6 +1883,11 @@ 
     default=b'identity',
 )
 coreconfigitem(
+    b'share',
+    b'source-safe-mismatch',
+    default=b'abort',
+)
+coreconfigitem(
     b'shelve',
     b'maxbackups',
     default=10,