[PATCH 1 of 6 evolve-ext-V2] evolve: mechanism to load some commands selectively

[Laurent Charignon]

> On Jun 4, 2015, at 10:32 PM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
> 
> 
> 
> On 06/04/2015 04:09 PM, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon at fb.com>
>> # Date 1432164945 25200
>> #      Wed May 20 16:35:45 2015 -0700
>> # Node ID a72a896bfc902806d3847915f29c2ffe1db8c37b
>> # Parent  82dd98428b8d85ea0c9acefebcff40ea59902c66
>> evolve: mechanism to load some commands selectively
>> 
>> This patch introduces a new config option: experimental.evolutioncommands to
>> load evolve's command selectively.
>> It is part of a sequence of patches to make evolve's command respect the value
>> of experimental.evolution. Once these commands are ready and actually respect
>> the flag, they are safe to use and can be enabled with the mechanism developed
>> in this patch.
>> 
>> diff --git a/hgext/evolve.py b/hgext/evolve.py
>> --- a/hgext/evolve.py
>> +++ b/hgext/evolve.py
>> @@ -46,9 +46,6 @@
>>  except (ImportError, AttributeError):
>>      gboptslist = gboptsmap = None
>> 
>> -# Flags for enabling optional parts of evolve
>> -commandopt = 'allnewcommands'
>> -
>>  from mercurial import bookmarks
>>  from mercurial import cmdutil
>>  from mercurial import commands
>> @@ -366,10 +363,23 @@
>>      # This must be in the same function as the option configuration above to
>>      # guarantee it happens after the above configuration, but before the
>>      # extsetup functions.
>> +    evolvecommands = ui.configlist('experimental', 'evolutioncommands')
> 
> I would probably set a default value of 'all' here (yes this is a change compared to the 'commands' entry in evolution.

- I don't understand what you mean with the sentence in the parentheses.
- At this point (1/6), all has no meaning. Just to make sure, you suggest to make 'all' the default value in the patch that introduces 'all' as a value?

> 
>>      evolveopts = ui.configlist('experimental', 'evolution')
>> -    if evolveopts and (commandopt not in evolveopts and
>> -                       'all' not in evolveopts):
>> -        cmdtable.clear()
>> +    if not evolveopts or 'all' in evolveopts:
>> +        # Evolve is fully loaded, don't disable commands
>> +        return
>> +    else:
>> +        whitelist = set()
>> +        for cmd in evolvecommands:
>> +            matchingevolvecommands = [e for e in cmdtable.keys() if cmd in e]
>> +            if not matchingevolvecommands:
>> +                raise error.Abort(_('Unknown command'))
> 
> - do not capitalize error message

Ok

> - If we do not specify the command who is unkown, the user will flip his laptop.

Good suggestion


> 
>> +            elif len(matchingevolvecommands) > 1:
>> +                raise error.Abort(_('Ambiguous command specification'))
> 
> ditto

Ok

> 
>> +            else:
>> +                whitelist.add(matchingevolvecommands[0])
>> +        for disabledcmd in set(cmdtable) - whitelist:
> 
> You could also start with disabledcmd and remove element of the set as they match.

It should come down to the same thing, as you suggest my suggestion is a little less safe (blacklisting vs whitelisting), I might revisit it in V3.

Laurent

> 
> -- 
> Pierre-Yves David