keyword question

[Benoit Boissinot]

On Thu, Feb 18, 2010 at 12:49:18AM +0100, Christian Ebert wrote:
> Salut Benoît,
> 
> * 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.
> 
> I've been wondering too since I wrapped repo.commitctx for
> overwriting during commit.
> 
> > Could you explain?
> 
> The way I understand it the following should be safe:
> 
> # HG changeset patch
> # User Christian Ebert <blacktrash at gmx.net>
> # Date 1266449886 -3600
> # Branch stable
> # Node ID 3484ede77de1ecb4d5f50213b0337dbd3598bd55
> # Parent  2c2d2f1354b44427976c2279e18d7c51a47a921b
> keyword: remove unneeded locks
> 
> - kwcommitctx is inside the wlock of repo.commit: no lock
> - _kwfwrite only need wlock
> 
> diff --git a/hgext/keyword.py b/hgext/keyword.py
> --- a/hgext/keyword.py
> +++ b/hgext/keyword.py
> @@ -271,10 +271,9 @@
>      wlock = lock = None
>      try:
>          wlock = repo.wlock()
> -        lock = repo.lock()
>          kwt.overwrite(None, expand, status[6])
>      finally:
> -        release(lock, wlock)
> +        release(wlock)

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()
>  
>  def demo(ui, repo, *args, **opts):
>      '''print [keywordmaps] configuration and an expansion example
> @@ -485,15 +484,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

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).
>  
>      # monkeypatches
>      def kwpatchfile_init(orig, self, ui, fname, opener,
> 
>  
> All tests pass, but I am very insecure whether this introduces
> some race conditions or possibilites of concurrent writes ...
> 
> Of course I would prefer it that way. Perhaps you could double
> check/critized the reasoning I've outlined above.
> 
> One danger I can think of is that overwrite() forces a clean
> dirstate when called for _kwfwrite (kwexpand, kwshrink commands),
> it doesn't do it any more during commit since the commitctx
> wrapping.

Can you save this patch for after the freeze?

thanks,

Benoit

-- 
:wq