Patchwork [3,of,3] localrepo: move new repo requirements into standalone function (API)

login
register
mail settings
Submitter Gregory Szorc
Date Feb. 15, 2016, 9:32 p.m.
Message ID <3d3dd48003bf005a1e8e.1455571953@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/13216/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Gregory Szorc - Feb. 15, 2016, 9:32 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1455571220 28800
#      Mon Feb 15 13:20:20 2016 -0800
# Node ID 3d3dd48003bf005a1e8ebe3b60cd926505a3da9e
# Parent  87d2d6947b2f47d6812151ce667918804876d077
localrepo: move new repo requirements into standalone function (API)

This patch extracts the code for determining requirements for a new
repo into a standalone function. By doing so, future code that will
perform an in-place repository upgrade (e.g. to generaldelta) can
examine the set of proposed new requirements and possibly take
additional actions (such as adding dotencode or fncache) when
performing the upgrade.

This patch is marked as API because _baserequirements (which was added
in b090601a80d1 so extensions could override it) has been removed and
will presumably impact whatever extension it was added for. Consumers
should be able to monkeypatch the new function to achieve the same
functionality.

The "create" argument has been dropped because the function is only
called in one location and "create" is always true in that case.

While it makes logical sense for this code to be a method so extensions
can implement a custom repo class / method to override it, this won't
actually work. This is because requirements determination occurs during
localrepository.__init__ and this is before the "reposetup"
"callback" is fired. So, the only way for extensions to customize
requirements would be to overwrite localrepo.localrepository or to
monkeypatch a function on a module during extsetup(). Since we try to
keep localrepository small, we use a standalone function. There is
probably room to offer extensions a "hook" point to alter repository
creation. But that is scope bloat.
Yuya Nishihara - Feb. 19, 2016, 12:49 p.m.
On Mon, 15 Feb 2016 13:32:33 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1455571220 28800
> #      Mon Feb 15 13:20:20 2016 -0800
> # Node ID 3d3dd48003bf005a1e8ebe3b60cd926505a3da9e
> # Parent  87d2d6947b2f47d6812151ce667918804876d077
> localrepo: move new repo requirements into standalone function (API)

The code looks cleaner. Queued for the clowncopter, thanks.

> +                self.requirements = newreporequirements(self)

Can we avoid passing a repo object that isn't fully initialized? Or is that
necessary for future extension?
Gregory Szorc - Feb. 20, 2016, 5 p.m.
> On Feb 19, 2016, at 04:49, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Mon, 15 Feb 2016 13:32:33 -0800, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1455571220 28800
>> #      Mon Feb 15 13:20:20 2016 -0800
>> # Node ID 3d3dd48003bf005a1e8ebe3b60cd926505a3da9e
>> # Parent  87d2d6947b2f47d6812151ce667918804876d077
>> localrepo: move new repo requirements into standalone function (API)
> 
> The code looks cleaner. Queued for the clowncopter, thanks.
> 
>> +                self.requirements = newreporequirements(self)
> 
> Can we avoid passing a repo object that isn't fully initialized? Or is that
> necessary for future extension?

I use it in the RFC series for in-place-upgrade. I too wish we had a better hook point for repo creation :/

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -237,19 +237,16 @@  class localrepository(object):
                                              'dotencode'))
     openerreqs = set(('revlogv1', 'generaldelta', 'treemanifest', 'manifestv2'))
     filtername = None
 
     # a list of (ui, featureset) functions.
     # only functions defined in module of enabled extensions are invoked
     featuresetupfuncs = set()
 
-    def _baserequirements(self, create):
-        return ['revlogv1']
-
     def __init__(self, baseui, path=None, create=False):
         self.requirements = set()
         self.wvfs = scmutil.vfs(path, expandpath=True, realpath=True)
         self.wopener = self.wvfs
         self.root = self.wvfs.base
         self.path = self.wvfs.join(".hg")
         self.origroot = path
         self.auditor = pathutil.pathauditor(self.root, self._checknested)
@@ -277,38 +274,23 @@  class localrepository(object):
             for setupfunc in self.featuresetupfuncs:
                 if setupfunc.__module__ in extmods:
                     setupfunc(self.ui, self.supported)
         else:
             self.supported = self._basesupported
 
         if not self.vfs.isdir():
             if create:
-                requirements = set(self._baserequirements(create))
-                if self.ui.configbool('format', 'usestore', True):
-                    requirements.add("store")
-                    if self.ui.configbool('format', 'usefncache', True):
-                        requirements.add("fncache")
-                        if self.ui.configbool('format', 'dotencode', True):
-                            requirements.add('dotencode')
-
-                if scmutil.gdinitconfig(self.ui):
-                    requirements.add("generaldelta")
-                if self.ui.configbool('experimental', 'treemanifest', False):
-                    requirements.add("treemanifest")
-                if self.ui.configbool('experimental', 'manifestv2', False):
-                    requirements.add("manifestv2")
-
-                self.requirements = requirements
+                self.requirements = newreporequirements(self)
 
                 if not self.wvfs.exists():
                     self.wvfs.makedirs()
                 self.vfs.makedir(notindexed=True)
 
-                if 'store' in requirements:
+                if 'store' in self.requirements:
                     self.vfs.mkdir("store")
 
                     # create an invalid changelog
                     self.vfs.append(
                         "00changelog.i",
                         '\0\0\0\2' # represents revlogv2
                         ' dummy changelog to prevent using the old repo layout'
                     )
@@ -1964,8 +1946,32 @@  def undoname(fn):
     assert name.startswith('journal')
     return os.path.join(base, name.replace('journal', 'undo', 1))
 
 def instance(ui, path, create):
     return localrepository(ui, util.urllocalpath(path), create)
 
 def islocal(path):
     return True
+
+def newreporequirements(repo):
+    """Determine the set of requirements for a new local repository.
+
+    Extensions can wrap this function to specify custom requirements for
+    new repositories.
+    """
+    ui = repo.ui
+    requirements = set(['revlogv1'])
+    if ui.configbool('format', 'usestore', True):
+        requirements.add('store')
+        if ui.configbool('format', 'usefncache', True):
+            requirements.add('fncache')
+            if ui.configbool('format', 'dotencode', True):
+                requirements.add('dotencode')
+
+    if scmutil.gdinitconfig(ui):
+        requirements.add('generaldelta')
+    if ui.configbool('experimental', 'treemanifest', False):
+        requirements.add('treemanifest')
+    if ui.configbool('experimental', 'manifestv2', False):
+        requirements.add('manifestv2')
+
+    return requirements