| 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
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.
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.
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?
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.
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
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.
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 ..
# 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.