Patchwork D3748: import: use context manager for lock, dirstateguard, transaction

login
register
mail settings
Submitter phabricator
Date June 14, 2018, 11:34 p.m.
Message ID <differential-rev-PHID-DREV-gcfsk3spkh7zkyrcnpue-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32158/
State Superseded
Headers show

Comments

phabricator - June 14, 2018, 11:34 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  A tiny side-effect is that the transaction is now closed after saving
  the commit message.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3748

AFFECTED FILES
  mercurial/commands.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - June 15, 2018, 12:37 p.m.
> +        if not opts.get('no_commit'):
> +            lock = repo.lock()
> +            tr = repo.transaction('import')
> +            dsguard = util.nullcontextmanager()
> +        else:
> +            lock = util.nullcontextmanager()
> +            tr = util.nullcontextmanager()
> +            dsguard = dirstateguard.dirstateguard(repo, 'import')
> +        with lock, tr, dsguard:

Doesn't it leave a stale `lock` if `repo.transaction()` raises exception?
phabricator - June 15, 2018, 12:39 p.m.
yuja added a comment.


  > +        if not opts.get('no_commit'):
  >  +            lock = repo.lock()
  >  +            tr = repo.transaction('import')
  >  +            dsguard = util.nullcontextmanager()
  >  +        else:
  >  +            lock = util.nullcontextmanager()
  >  +            tr = util.nullcontextmanager()
  >  +            dsguard = dirstateguard.dirstateguard(repo, 'import')
  >  +        with lock, tr, dsguard:
  
  Doesn't it leave a stale `lock` if `repo.transaction()` raises exception?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3748

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 15, 2018, 5:04 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3748#58738, @yuja wrote:
  
  > > +        if not opts.get('no_commit'):
  > >  +            lock = repo.lock()
  > >  +            tr = repo.transaction('import')
  > >  +            dsguard = util.nullcontextmanager()
  > >  +        else:
  > >  +            lock = util.nullcontextmanager()
  > >  +            tr = util.nullcontextmanager()
  > >  +            dsguard = dirstateguard.dirstateguard(repo, 'import')
  > >  +        with lock, tr, dsguard:
  >
  > Doesn't it leave a stale `lock` if `repo.transaction()` raises exception?
  
  
  Good catch. I have often wished that our transactions and locks respected the context manager protocol and did the locking in __enter__. How do you feel about this workaround (see updated code)?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3748

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 17, 2018, 1:33 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3748#58833, @yuja wrote:
  
  > > +        if not opts.get('no_commit'):
  > >  +            lock = repo.lock()
  > >  +            try:
  > > 
  > >   tr = repo.transaction('import')
  > > 
  > > - else:
  > > - dsguard = dirstateguard.dirstateguard(repo, 'import') +            except: +                lock.release()
  >
  > Needs to re-raise.
  >
  > > +            dsguard = util.nullcontextmanager()
  > >  +        else:
  > >  +            lock = util.nullcontextmanager()
  > >  +            tr = util.nullcontextmanager()
  > >  +            dsguard = dirstateguard.dirstateguard(repo, 'import')
  > >  +        with lock, tr, dsguard:
  >
  > Another option is to wrap transaction and dirstateguard by lambda or partial,
  >  and call them with "with".
  >
  >   with lock(), tr(), dsguard():
  >
  
  
  Good idea. Done.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3748

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -40,7 +40,6 @@ 
     hbisect,
     help,
     hg,
-    lock as lockmod,
     logcmdutil,
     merge as mergemod,
     obsolete,
@@ -67,8 +66,6 @@ 
     stringutil,
 )
 
-release = lockmod.release
-
 table = {}
 table.update(debugcommandsmod.command._table)
 
@@ -3108,22 +3105,24 @@ 
             raise error.Abort(_('cannot use --exact with --prefix'))
 
     base = opts["base"]
-    dsguard = lock = tr = None
     msgs = []
     ret = 0
 
     with repo.wlock():
-        try:
-            if update:
-                cmdutil.checkunfinished(repo)
-                if (exact or not opts.get('force')):
-                    cmdutil.bailifchanged(repo)
-
-            if not opts.get('no_commit'):
-                lock = repo.lock()
-                tr = repo.transaction('import')
-            else:
-                dsguard = dirstateguard.dirstateguard(repo, 'import')
+        if update:
+            cmdutil.checkunfinished(repo)
+            if (exact or not opts.get('force')):
+                cmdutil.bailifchanged(repo)
+
+        if not opts.get('no_commit'):
+            lock = repo.lock()
+            tr = repo.transaction('import')
+            dsguard = util.nullcontextmanager()
+        else:
+            lock = util.nullcontextmanager()
+            tr = util.nullcontextmanager()
+            dsguard = dirstateguard.dirstateguard(repo, 'import')
+        with lock, tr, dsguard:
             parents = repo[None].parents()
             for patchurl in patches:
                 if patchurl == '-':
@@ -3159,17 +3158,9 @@ 
                 if not haspatch:
                     raise error.Abort(_('%s: no diffs found') % patchurl)
 
-            if tr:
-                tr.close()
             if msgs:
                 repo.savecommitmessage('\n* * *\n'.join(msgs))
-            if dsguard:
-                dsguard.close()
-            return ret
-        finally:
-            if tr:
-                tr.release()
-            release(lock, dsguard)
+        return ret
 
 @command('incoming|in',
     [('f', 'force', None,