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

[FUJIWARA Katsunori]

At Sat, 23 Jun 2012 15:45:01 +0200,
Mads Kiilerich wrote:
> 
> 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.)

Thank you for your comments on my description.

How about this ?

# this description assumes that fixing is done in
# "extensions.extensions()", as you suggested.

    extensions: select extensions only enabled by specified ui configuration

    Before this patch, operations covering repositories enable
    extensions unexpectedly. For example:

      - one enabled locally in the source of local push/clone is also
        enabled in the destination (one enabled in the destination of
        pull is enabled in the source)

      - one enabled locally in the parent or elder siblings of subrepo
        tree is also enabled in the other

    This patch checks whether each extensions should be enabled or not
    for specified ui configuration to list extensions up.


> > 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   == '!'  .)

I think that suggested fixing is better than mine, too.

I'll re-write patch so.

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

I'll add such tests, too.

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