[PATCH 3 of 4 V2] ui: use the new configoverride context manager instead of saving/restoring

[Pierre-Yves David]

On 05/13/2015 08:36 PM, Laurent Charignon wrote:
>
>
> On 5/13/15, 6:29 PM, "Gregory Szorc" <gregory.szorc at gmail.com> wrote:
>
>>
>>
>>> On May 13, 2015, at 17:42, Pierre-Yves David
>>> <pierre-yves.david at ens-lyon.org> wrote:
>>>
>>>
>>>
>>>> On 05/13/2015 05:26 PM, Laurent Charignon wrote:
>>>> # HG changeset patch
>>>> # User Laurent Charignon <lcharignon at fb.com>
>>>> # Date 1431562081 25200
>>>> #      Wed May 13 17:08:01 2015 -0700
>>>> # Node ID cb2f0824776ea5d964813ab4bc5213e03b4e45fc
>>>> # Parent  480dbca00f19fb90b59f9c6fd511e4806fb642c4
>>>> ui: use the new configoverride context manager instead of
>>>> saving/restoring
>>>>
>>>> This patch changes all the point in the code (except two) where we
>>>> used the
>>>> saving restoring logic for config to use the new configoverride
>>>> contextmanager.
>>>> This makes the code more legible.
>>>>
>>>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>>>> --- a/hgext/histedit.py
>>>> +++ b/hgext/histedit.py
>>>> @@ -386,16 +386,12 @@
>>>>       """
>>>>       phasemin = src.phase()
>>>>       def commitfunc(**kwargs):
>>>> -        phasebackup = repo.ui.backupconfig('phases', 'new-commit')
>>>> -        try:
>>>> -            repo.ui.setconfig('phases', 'new-commit', phasemin,
>>>> -                              'histedit')
>>>> +        with repo.ui.configoverride('phases', 'new-commit', phasemin,
>>>> +                                    'histedit'):
>>>>               extra = kwargs.get('extra', {}).copy()
>>>>               extra['histedit_source'] = src.hex()
>>>>               kwargs['extra'] = extra
>>>>               return repo.commit(**kwargs)
>>>> -        finally:
>>>> -            repo.ui.restoreconfig(phasebackup)
>>>>       return commitfunc
>>>>
>>>>   def applychanges(ui, repo, ctx, opts):
>>>> @@ -574,14 +570,11 @@
>>>>           #       here. This is sufficient to solve issue3681 anyway.
>>>>           extra['histedit_source'] = '%s,%s' % (ctx.hex(), oldctx.hex())
>>>>           commitopts['extra'] = extra
>>>> -        phasebackup = repo.ui.backupconfig('phases', 'new-commit')
>>>> -        try:
>>>> -            phasemin = max(ctx.phase(), oldctx.phase())
>>>> -            repo.ui.setconfig('phases', 'new-commit', phasemin,
>>>> 'histedit')
>>>> +        phasemin = max(ctx.phase(), oldctx.phase())
>>>> +        with repo.ui.configoverride('phases', 'new-commit', phasemin,
>>>> +                                    'histedit'):
>>>>               n = collapse(repo, ctx, repo[newnode], commitopts,
>>>>                            skipprompt=self.skipprompt())
>>>> -        finally:
>>>> -            repo.ui.restoreconfig(phasebackup)
>>>>           if n is None:
>>>>               return ctx, []
>>>>           hg.update(repo, n)
>>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>>> --- a/hgext/rebase.py
>>>> +++ b/hgext/rebase.py
>>>> @@ -540,15 +540,12 @@
>>>>           if extrafn:
>>>>               extrafn(ctx, extra)
>>>>
>>>> -        backup = repo.ui.backupconfig('phases', 'new-commit')
>>>> -        try:
>>>> -            targetphase = max(ctx.phase(), phases.draft)
>>>> -            repo.ui.setconfig('phases', 'new-commit', targetphase,
>>>> 'rebase')
>>>> +        targetphase = max(ctx.phase(), phases.draft)
>>>> +        with repo.ui.configoverride('phases', 'new-commit',
>>>> targetphase,
>>>> +                                    'rebase'):
>>>>               # Commit might fail if unresolved files exist
>>>>               newnode = repo.commit(text=commitmsg, user=ctx.user(),
>>>>                                     date=ctx.date(), extra=extra,
>>>> editor=editor)
>>>> -        finally:
>>>> -            repo.ui.restoreconfig(backup)
>>>>
>>>>           repo.dirstate.setbranch(repo[newnode].branch())
>>>>           dsguard.close()
>>>> diff --git a/hgext/shelve.py b/hgext/shelve.py
>>>> --- a/hgext/shelve.py
>>>> +++ b/hgext/shelve.py
>>>> @@ -177,16 +177,15 @@
>>>>           hasmq = util.safehasattr(repo, 'mq')
>>>>           if hasmq:
>>>>               saved, repo.mq.checkapplied = repo.mq.checkapplied, False
>>>> -        backup = repo.ui.backupconfig('phases', 'new-commit')
>>>> -        try:
>>>> -            repo.ui. setconfig('phases', 'new-commit', phases.secret)
>>>> -            editor =
>>>> cmdutil.getcommiteditor(editform='shelve.shelve', **opts)
>>>> -            return repo.commit(message, user, opts.get('date'), match,
>>>> -                               editor=editor)
>>>> -        finally:
>>>> -            repo.ui.restoreconfig(backup)
>>>> -            if hasmq:
>>>> -                repo.mq.checkapplied = saved
>>>> +        with repo.ui.configoverride('phases', 'new-commit',
>>>> phases.secret):
>>>> +            try:
>>>> +                editor =
>>>> cmdutil.getcommiteditor(editform='shelve.shelve',
>>>> +                        **opts)
>>>> +                return repo.commit(message, user, opts.get('date'),
>>>> match,
>>>> +                                   editor=editor)
>>>> +            finally:
>>>> +                if hasmq:
>>>> +                    repo.mq.checkapplied = saved
>>>
>>> I'm not entirely sold in the benefit in these cases where using with
>>> introduce a new indentation level.
>>>
>>
>> Favor pattern consistency over minor style warts because it increases
>> readability and comprehension confidence because it eliminates questions
>> like, "why does all the code except here use a context manager for config
>> overrides?"
>>
>> I'm not a huge fan of introducing indentation either. But something tells
>> me that you wouldn't notice if it were new code. If you do take exception
>> to with..try, we should be talking about forbidding that pattern via the
>> style checker.
>
>  From the reception of these changes, I feel like we would be better off
> with continuing to do what we did before.
> I will send a V3 without the context manager.

This seems wise, we have access to some new syntax and utility with 2.6. 
However it will take some to time to figure out how the project want to 
use them. You probably do not want ot get delayed until then.

-- 
Pierre-Yves David