Patchwork [3,of,3,V4] largefiles: setup "largefiles" feature in each repositories individually

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Sept. 21, 2013, 12:39 p.m.
Message ID <fd4caa9e11faa76ac5d9.1379767192@feefifofum>
Download mbox | patch
Permalink /patch/2591/
State Accepted
Commit fb6e87d939482fb8d7636fa1bc8861a8b34773b7
Headers show

Comments

Katsunori FUJIWARA - Sept. 21, 2013, 12:39 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1379766809 -32400
#      Sat Sep 21 21:33:29 2013 +0900
# Node ID fd4caa9e11faa76ac5d937edc63b7268968067d5
# Parent  1189975a37cecad041d60358dfc46ec4fe240ef4
largefiles: setup "largefiles" feature in each repositories individually

Before this patch, if largefiles extension is enabled once in any of
target repositories, commands handling multiple repositories at a time
like below misunderstand that "largefiles" feature is supported also
in all other local repositories:

  - clone/pull from or push to localhost
  - recursive execution in subrepo tree

This patch registers "featuresetup()" into "featuresetupfuncs" of
"localrepository" to support "largefiles" features only in
repositories enabling largefiles extension, instead of adding
"largefiles" feature to class variable "_basesupported" of
"localrepository".

This patch also adds checking below to the largefiles specific class
derived from "localrepository":

  - push to localhost: whether features supported in the local(= dst)
    repository satisfies ones required in the remote(= src)

This can prevent useless looking up in the remote repository, when
supported and required features are mismatched: "push()" of
"localrepository" also checks it, but it is executed after looking up
in the remote.
Matt Mackall - Sept. 23, 2013, 8:09 p.m.
On Sat, 2013-09-21 at 21:39 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1379766809 -32400
> #      Sat Sep 21 21:33:29 2013 +0900
> # Node ID fd4caa9e11faa76ac5d937edc63b7268968067d5
> # Parent  1189975a37cecad041d60358dfc46ec4fe240ef4
> largefiles: setup "largefiles" feature in each repositories individually

These are queued for default, thanks.
Siddharth Agarwal - Sept. 24, 2013, 9:05 p.m.
On 09/21/2013 05:39 AM, FUJIWARA Katsunori wrote:
> +def featuresetup(ui, supported):
> +    for name, module in extensions.extensions(ui):
> +        if __name__ == module.__name__:
> +            # don't die on seeing a repo with the largefiles requirement
> +            supported |= set(['largefiles'])
> +            return

Is there a more elegant way to do this? Seems like basically every 
extension that adds something to the supported list will need to do 
precisely this.
Siddharth Agarwal - Sept. 24, 2013, 10:33 p.m.
On 09/24/2013 02:05 PM, Siddharth Agarwal wrote:
> On 09/21/2013 05:39 AM, FUJIWARA Katsunori wrote:
>> +def featuresetup(ui, supported):
>> +    for name, module in extensions.extensions(ui):
>> +        if __name__ == module.__name__:
>> +            # don't die on seeing a repo with the largefiles 
>> requirement
>> +            supported |= set(['largefiles'])
>> +            return
>
> Is there a more elegant way to do this? Seems like basically every 
> extension that adds something to the supported list will need to do 
> precisely this.

Also, it seems like if the extension relies on the requires file to 
figure out when to disable itself, this shouldn't be necessary at all. 
For example, we have an out-of-tree extension called lz4revlog that 
switches to a different compression algorithm for the revlog. The 
extension looks at whether its corresponding entry is present in the 
requires file, and if it is, doesn't use the alternate compression 
algorithm to compress itself.

I guess there's an edge case here where you operate on two repositories:
repo 1 has lz4revlog enabled
repo 2 has lz4revlog disabled but has it listed in requires

then operating on repo 2 alone would fail, but if you operate on repo 1 
and 2 together within the same process, things would work.

... but then what happens if two repositories enable two different 
versions of lz4revlog? Does the first one that's initialized win? 
http://selenic.com/repo/hg/file/50d721553198/mercurial/extensions.py#l67 
seems to indicate that that's the case. Why is that different from the 
one enabled, one disabled case?
Siddharth Agarwal - Sept. 24, 2013, 10:40 p.m.
On 09/24/2013 03:33 PM, Siddharth Agarwal wrote:
> The extension looks at whether its corresponding entry is present in 
> the requires file, and if it is, doesn't use the alternate compression 
> algorithm to compress itself. 

Erm, poorly worded. A better attempt:

The extension looks at whether the line 'lz4revlog' is present in the 
requires file. If it is, it uses the alternate compression algorithm to 
compress revlogs. If it isn't, it doesn't.
Katsunori FUJIWARA - Sept. 25, 2013, 3:55 p.m.
At Tue, 24 Sep 2013 15:33:47 -0700,
Siddharth Agarwal wrote:
> 
> On 09/24/2013 02:05 PM, Siddharth Agarwal wrote:
> > On 09/21/2013 05:39 AM, FUJIWARA Katsunori wrote:
> >> +def featuresetup(ui, supported):
> >> +    for name, module in extensions.extensions(ui):
> >> +        if __name__ == module.__name__:
> >> +            # don't die on seeing a repo with the largefiles 
> >> requirement
> >> +            supported |= set(['largefiles'])
> >> +            return
> >
> > Is there a more elegant way to do this? Seems like basically every 
> > extension that adds something to the supported list will need to do 
> > precisely this.

What about below ?

  (1) register feature setup function to "featuresetupfuncs" of
      "localrepository" automatically, when extension module has
      "supportedfeatures" like attribute

        - are there already existing extensions with
          "supportedfeatures" like attribute for other purposes ?

and/or:

  (1) adding function like below to "extension.py", to use from
      "uisetup()" of each extensions adding features ?

      @extensions.py:
      ================================
      def featuresetupfunc(extmod, features):
          def setupfunc(ui, supported):
              for name, module in extensions(ui):
                  if extmod.__name__ == module.__name__:
                      supported |= set(features)
                      return
          return setupfunc
      ================================

      @ each extensions
      ================================
      def uisetup(ui):
          func = extensions.featuresetupfunc(uisetup.__module__)
          localrepo.localrepository.featuresetupfuncs.add(func)
      ================================


In addition to above, "localrepo.localrepository.featuresetupfuncs"
may have to be moved into "extensions" module to simplify import
dependency.


> Also, it seems like if the extension relies on the requires file to 
> figure out when to disable itself, this shouldn't be necessary at all. 
> For example, we have an out-of-tree extension called lz4revlog that 
> switches to a different compression algorithm for the revlog. The 
> extension looks at whether its corresponding entry is present in the 
> requires file, and if it is, doesn't use the alternate compression 
> algorithm to compress itself.
> 
> I guess there's an edge case here where you operate on two repositories:
> repo 1 has lz4revlog enabled
> repo 2 has lz4revlog disabled but has it listed in requires
> 
> then operating on repo 2 alone would fail, but if you operate on repo 1 
> and 2 together within the same process, things would work.

When I asked about the policy of extension enabling before posting the
first patches of this problem, I was suggested by Matt:

    We should probably strictly isolate secondary repos from the
    extensions/hooks of the primary repo

    http://selenic.com/pipermail/mercurial-devel/2012-May/040319.html

So, extensions should be disabled in repositories which disables them,
while we follow this policy.


> ... but then what happens if two repositories enable two different 
> versions of lz4revlog? Does the first one that's initialized win? 
> http://selenic.com/repo/hg/file/50d721553198/mercurial/extensions.py#l67 
> seems to indicate that that's the case. Why is that different from the 
> one enabled, one disabled case?

I have thought that this unexpected sharing may cause unexpected
result, too.

At least, Mercurial should warning message when there is precedence
extension loaded from another path.

Enabling multiple extensions in same name at a time in each
repositories itself seems to be possible. I'll try to achieve it.

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Siddharth Agarwal - Sept. 26, 2013, 4:46 p.m.
On 09/25/2013 08:55 AM, FUJIWARA Katsunori wrote:
> What about below ?
>
>    (1) register feature setup function to "featuresetupfuncs" of
>        "localrepository" automatically, when extension module has
>        "supportedfeatures" like attribute
>
>          - are there already existing extensions with
>            "supportedfeatures" like attribute for other purposes ?

As I mentioned, our lz4revlog extension is one. You can find the source 
code at https://bitbucket.org/facebook/lz4revlog. For now, pending the 
results of this discussion, I've added lz4revlog to _basesupported.

Seems like what we really need is some sort of parallel to 
uisetup/extsetup. This would ideally be part of reposetup, but as you 
mentioned that happens too late. How about adding another hook before 
the repo is fully initialized, called only on enabled extensions? I 
haven't thought about the exact mechanics of that, though.

def featuresetup(ui, repo):
     repo.supported.add('lz4revlog')

and that's all my extension would ideally need to do.
Katsunori FUJIWARA - Oct. 17, 2013, 12:55 p.m.
At Thu, 26 Sep 2013 09:46:22 -0700,
Siddharth Agarwal wrote:
> 
> On 09/25/2013 08:55 AM, FUJIWARA Katsunori wrote:
> > What about below ?
> >
> >    (1) register feature setup function to "featuresetupfuncs" of
> >        "localrepository" automatically, when extension module has
> >        "supportedfeatures" like attribute
> >
> >          - are there already existing extensions with
> >            "supportedfeatures" like attribute for other purposes ?
> 
> As I mentioned, our lz4revlog extension is one. You can find the source 
> code at https://bitbucket.org/facebook/lz4revlog. For now, pending the 
> results of this discussion, I've added lz4revlog to _basesupported.
> 
> Seems like what we really need is some sort of parallel to 
> uisetup/extsetup. This would ideally be part of reposetup, but as you 
> mentioned that happens too late. How about adding another hook before 
> the repo is fully initialized, called only on enabled extensions? I 
> haven't thought about the exact mechanics of that, though.
> 
> def featuresetup(ui, repo):
>      repo.supported.add('lz4revlog')
> 
> and that's all my extension would ideally need to do.

I just posted the path to make feature setup easier.

    http://www.selenic.com/pipermail/mercurial-devel/2013-October/054323.html

This doesn't:

    - register feature setup function of each extensions
      automatically:

      it may cause unexpected invocation of "featuresetup()" not for
      this purpose

    - pass "repo" object to setup function:

      at feature setup step, "repo" is not fully initialized

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/hgext/largefiles/__init__.py b/hgext/largefiles/__init__.py
--- a/hgext/largefiles/__init__.py
+++ b/hgext/largefiles/__init__.py
@@ -105,16 +105,26 @@ 
 command.
 '''
 
-from mercurial import commands
+from mercurial import commands, localrepo, extensions
 
 import lfcommands
 import reposetup
-import uisetup
+import uisetup as uisetupmod
 
 testedwith = 'internal'
 
 reposetup = reposetup.reposetup
-uisetup = uisetup.uisetup
+
+def featuresetup(ui, supported):
+    for name, module in extensions.extensions(ui):
+        if __name__ == module.__name__:
+            # don't die on seeing a repo with the largefiles requirement
+            supported |= set(['largefiles'])
+            return
+
+def uisetup(ui):
+    localrepo.localrepository.featuresetupfuncs.add(featuresetup)
+    uisetupmod.uisetup(ui)
 
 commands.norepo += " lfconvert"
 
diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -407,6 +407,14 @@ 
                 wlock.release()
 
         def push(self, remote, force=False, revs=None, newbranch=False):
+            if remote.local():
+                missing = set(self.requirements) - remote.local().supported
+                if missing:
+                    msg = _("required features are not"
+                            " supported in the destination:"
+                            " %s") % (', '.join(sorted(missing)))
+                    raise util.Abort(msg)
+
             outgoing = discovery.findcommonoutgoing(repo, remote.peer(),
                                                     force=force)
             if outgoing.missing:
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -9,7 +9,7 @@ 
 '''setup for largefiles extension: uisetup'''
 
 from mercurial import archival, cmdutil, commands, extensions, filemerge, hg, \
-    httppeer, localrepo, merge, scmutil, sshpeer, wireproto, revset
+    httppeer, merge, scmutil, sshpeer, wireproto, revset
 from mercurial.i18n import _
 from mercurial.hgweb import hgweb_mod, webcommands
 from mercurial.subrepo import hgsubrepo
@@ -152,9 +152,6 @@ 
     sshpeer.sshpeer._callstream = proto.sshrepocallstream
     httppeer.httppeer._callstream = proto.httprepocallstream
 
-    # don't die on seeing a repo with the largefiles requirement
-    localrepo.localrepository._basesupported |= set(['largefiles'])
-
     # override some extensions' stuff as well
     for name, module in extensions.extensions():
         if name == 'fetch':
diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
--- a/tests/test-largefiles.t
+++ b/tests/test-largefiles.t
@@ -2191,3 +2191,64 @@ 
   
 
   $ cd ..
+
+Check whether "largefiles" feature is supported only in repositories
+enabling largefiles extension.
+
+  $ mkdir individualenabling
+  $ cd individualenabling
+
+  $ hg init enabledlocally
+  $ echo large > enabledlocally/large
+  $ hg -R enabledlocally add --large enabledlocally/large
+  $ hg -R enabledlocally commit -m '#0'
+  Invoking status precommit hook
+  A large
+
+  $ hg init notenabledlocally
+  $ echo large > notenabledlocally/large
+  $ hg -R notenabledlocally add --large notenabledlocally/large
+  $ hg -R notenabledlocally commit -m '#0'
+  Invoking status precommit hook
+  A large
+
+  $ cat >> $HGRCPATH <<EOF
+  > [extensions]
+  > # disable globally
+  > largefiles=!
+  > EOF
+  $ cat >> enabledlocally/.hg/hgrc <<EOF
+  > [extensions]
+  > # enable locally
+  > largefiles=
+  > EOF
+  $ hg -R enabledlocally root
+  $TESTTMP/individualenabling/enabledlocally
+  $ hg -R notenabledlocally root
+  abort: unknown repository format: requires features 'largefiles' (upgrade Mercurial)!
+  [255]
+
+  $ hg init push-dst
+  $ hg -R enabledlocally push push-dst
+  pushing to push-dst
+  abort: required features are not supported in the destination: largefiles
+  [255]
+
+  $ hg init pull-src
+  $ hg -R pull-src pull enabledlocally
+  pulling from enabledlocally
+  abort: required features are not supported in the destination: largefiles
+  [255]
+
+  $ hg clone enabledlocally clone-dst
+  abort: unknown repository format: requires features 'largefiles' (upgrade Mercurial)!
+  [255]
+  $ test -d clone-dst
+  [1]
+  $ hg clone --pull enabledlocally clone-pull-dst
+  abort: required features are not supported in the destination: largefiles
+  [255]
+  $ test -d clone-pull-dst
+  [1]
+
+  $ cd ..