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

Laurent Charignon lcharignon at fb.com
Wed May 13 22:36:21 CDT 2015



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.



More information about the Mercurial-devel mailing list