keyword question

[Christian Ebert]

* Benoit Boissinot on Thursday, February 18, 2010 at 01:20:40 +0100
> On Thu, Feb 18, 2010 at 12:49:18AM +0100, Christian Ebert wrote:
>> * Benoit Boissinot on Thursday, February 18, 2010 at 00:07:45 +0100
>>> I was just reading keyword.py and wondering why overwrite() needed to
>>> get the repo lock.
> 
> If you move wlock = repo.wlock() outside of the try/finally block (you
> should probably take it before calling status to avoid potential races),
> then you can use wlock.release()

Yes, much better.

> looking at overwrite() I know you don't need to take wlock, but taking
> wlock doesn't cost much anyway (it should already been taken).
> 
>> All tests pass, but I am very insecure whether this introduces
>> some race conditions or possibilites of concurrent writes ...
> 
> Can you save this patch for after the freeze?

Sure.

Thank you very much for your feedback.

I tried to incorporate your suggestions like so:


# HG changeset patch
# User Christian Ebert <blacktrash at gmx.net>
# Date 1266454376 -3600
# Branch stable
# Node ID e1321ba34588cb049b70abd4605742087f9a54a3
# Parent  2c2d2f1354b44427976c2279e18d7c51a47a921b
keyword: remove unneeded locks

- kwcommitctx is inside the wlock of repo.commit: no lock
- _kwfwrite only needs wlock
  wlock outside try block, so we don't need to import lock.release

diff --git a/hgext/keyword.py b/hgext/keyword.py
--- a/hgext/keyword.py
+++ b/hgext/keyword.py
@@ -79,7 +79,6 @@
 from mercurial import commands, cmdutil, dispatch, filelog, revlog, extensions
 from mercurial import patch, localrepo, templater, templatefilters, util, match
 from mercurial.hgweb import webcommands
-from mercurial.lock import release
 from mercurial.node import nullid
 from mercurial.i18n import _
 import re, shutil, tempfile
@@ -264,17 +263,15 @@
     if repo.dirstate.parents()[1] != nullid:
         raise util.Abort(_('outstanding uncommitted merge'))
     kwt = kwtools['templater']
-    status = _status(ui, repo, kwt, *pats, **opts)
-    modified, added, removed, deleted = status[:4]
-    if modified or added or removed or deleted:
-        raise util.Abort(_('outstanding uncommitted changes'))
-    wlock = lock = None
+    wlock = repo.wlock()
     try:
-        wlock = repo.wlock()
-        lock = repo.lock()
+        status = _status(ui, repo, kwt, *pats, **opts)
+        modified, added, removed, deleted = status[:4]
+        if modified or added or removed or deleted:
+            raise util.Abort(_('outstanding uncommitted changes'))
         kwt.overwrite(None, expand, status[6])
     finally:
-        release(lock, wlock)
+        wlock.release()
 
 def demo(ui, repo, *args, **opts):
     '''print [keywordmaps] configuration and an expansion example
@@ -485,15 +482,9 @@
                 del self.commitctx
 
         def kwcommitctx(self, ctx, error=False):
-            wlock = lock = None
-            try:
-                wlock = self.wlock()
-                lock = self.lock()
-                n = super(kwrepo, self).commitctx(ctx, error)
-                kwt.overwrite(n, True, None)
-                return n
-            finally:
-                release(lock, wlock)
+            n = super(kwrepo, self).commitctx(ctx, error)
+            kwt.overwrite(n, True, None)
+            return n
 
     # monkeypatches
     def kwpatchfile_init(orig, self, ui, fname, opener,

-- 
\black\trash movie               _S A M E_  _T I M E_  _S A M E_  _P L A C E_
                          --->> http://www.blacktrash.org/underdogma/stsp.php
\black\trash audio   _A N O T H E R_  _T I M E_  _A N O T H E R_  _P L A C E_
                         --->> http://www.blacktrash.org/underdogma/atap.html