[PATCH STABLE] clone: enable only appropriate extensions in the target repository

Christian Ebert blacktrash at gmx.net
Sun Jun 24 05:42:08 CDT 2012


* Mads Kiilerich on Saturday, June 23, 2012 at 15:45:01 +0200
> FUJIWARA Katsunori wrote, On 06/22/2012 06:32 PM:
>> # HG changeset patch
>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>> # Date 1340382542 -32400
>> # Branch stable
>> # Node ID 916f86690b3e61903bdec44cfd20eef4c65fa986
>> # Parent  f8af57c00a29cb52c7561fad1f7ab2d9e68b96cf
>> clone: enable only appropriate extensions in the target repository
>> 
>> current implementation enables all extensions loaded before setup of
>> the repository in it, event if they should not be enabled in it.
>> 
>> for example, extensions enabled locally by '.hg/hgrc' of the cloning
>> source are also enabled in the cloning destination.
> 
> (I assume it is the previous, "before this patch" implementation you
> are describing as "current" here? Future readers of the commit
> description will see "current" as the revision that includes your
> patch. I prefer to use past tense for the description of what the
> problem was and perhaps explicitly say "before".)
> 
>> this patch checks whether extensions should be enabled or not by the
>> configuration for the target repository.
> 
> I think it would help to state explicitly that the problem is when
> extensions have been enabled or disabled in the .hg/hgrc of the source
> repo. The target repo should be reposetup'ed with exactly the
> extensions in the global configuration.
> 
> I guess pull had the same problem ... and subrepos even more?
> 
> It might also be worth mentioning that the extensions that are used
> still might have been uisetup'ed or extsetup'ed with the wrong ui, and
> extensions that only are used by the other repo(s) might have done
> monkey patching that will kick in anyway.
> 
> (Minor nit: http://mercurial.selenic.com/wiki/ContributingChanges#Patch_descriptions
> strongly hints that the body of the description should use normal
> capitalization.)
> 
>> diff -r f8af57c00a29 -r 916f86690b3e mercurial/hg.py
>> --- a/mercurial/hg.py	Mon Jun 18 11:16:24 2012 +0200
>> +++ b/mercurial/hg.py	Sat Jun 23 01:29:02 2012 +0900
>> @@ -93,6 +93,12 @@
>>     repo = _peerlookup(path).instance(ui, path, create)
>>     ui = getattr(repo, "ui", ui)
>>     for name, module in extensions.extensions():
>> +        for conf in [ui.config('extensions', name),
>> +                     ui.config('extensions', 'hgext.%s' % name)]:
>> +            if conf is not None and not conf.startswith('!'):
>> +                break
>> +        else:
>> +            continue
> 
> This looks like something that should live in the extensions module.
> It duplicates a part of the functionality in extensions.loadall(). It
> could perhaps be an optional ui parameter to extensions.extensions().
> 
> (And loadall uses   == '!'  .)
> 
>>         hook = getattr(module, 'reposetup', None)
>>         if hook:
>>             hook(ui, repo)
>> diff -r f8af57c00a29 -r 916f86690b3e tests/test-clone.t
>> --- a/tests/test-clone.t	Mon Jun 18 11:16:24 2012 +0200
>> +++ b/tests/test-clone.t	Sat Jun 23 01:29:02 2012 +0900
>> @@ -458,3 +458,72 @@
>>   updating to branch stable
>>   3 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>   $ rm -r ua
>> +
>> +Extensions enabled(disabled) only in the clone source should be
>> +disabled(enabled) in the destination while cloning.
> 
> It might not make sense from a white box testing perspective, but from
> a black box testing perspective it would be nice to also have a test
> case for correct handling of extensions enabled/disabled by
> configuration specified on the command line.

My first reaction to this patch was that the extension should
take care of this because the extensions themselves know best
what kind of havoc they might create or not. But my reaction is
certainly biased because that's what I'm trying to do in the
keyword extension. And I might completely misunderstand what the
patch is trying to achieve.

c
-- 
theatre - books - texts - movies
Black Trash Productions at home: http://www.blacktrash.org
Black Trash Productions on Facebook:
http://www.facebook.com/blacktrashproductions


More information about the Mercurial-devel mailing list