Patchwork [9,of,9,STABLE] transplant: widen wlock scope of transplant for consitency while processing

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Dec. 1, 2015, 6:18 p.m.
Message ID <136cb2a5b5f0c0c67483.1448993927@feefifofum>
Download mbox | patch
Permalink /patch/11723/
State Superseded
Commit ee33e677f0ac3b61acca7af7bcfbae8af7ed776e
Headers show

Comments

Katsunori FUJIWARA - Dec. 1, 2015, 6:18 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1448993528 -32400
#      Wed Dec 02 03:12:08 2015 +0900
# Branch stable
# Node ID 136cb2a5b5f0c0c67483c86d2fbe038147a2f288
# Parent  96c4b87476f3a511d06006bd70cfefe7fd672ebc
transplant: widen wlock scope of transplant for consitency while processing

Before this patch, "hg transplant" executes below before acquisition
of wlock.

  - cmdutil.checkunfinished()
  - repo.status() for dirty check
  - repo.dirstate.parents()

It may cause unintentional result, if another command runs parallelly
(see also issue4368).

This patch makes "hg transplant" acquire wlock before processing
instead of acquiring wlock in each of 'transplanter.apply()' and
'transplanter.recover()'.
Augie Fackler - Dec. 2, 2015, 3:24 p.m.
On Wed, Dec 02, 2015 at 03:18:47AM +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1448993528 -32400
> #      Wed Dec 02 03:12:08 2015 +0900
> # Branch stable
> # Node ID 136cb2a5b5f0c0c67483c86d2fbe038147a2f288
> # Parent  96c4b87476f3a511d06006bd70cfefe7fd672ebc
> transplant: widen wlock scope of transplant for consitency while processing

These patches look great to me, but I'm unclear why they're tagged for
stable? Are they fixing a regression, or should I drop them on default
instead?

Thanks!

>
> Before this patch, "hg transplant" executes below before acquisition
> of wlock.
>
>   - cmdutil.checkunfinished()
>   - repo.status() for dirty check
>   - repo.dirstate.parents()
>
> It may cause unintentional result, if another command runs parallelly
> (see also issue4368).
>
> This patch makes "hg transplant" acquire wlock before processing
> instead of acquiring wlock in each of 'transplanter.apply()' and
> 'transplanter.recover()'.
>
> diff --git a/hgext/transplant.py b/hgext/transplant.py
> --- a/hgext/transplant.py
> +++ b/hgext/transplant.py
> @@ -20,6 +20,7 @@ from mercurial.node import short
>  from mercurial import bundlerepo, hg, merge, match
>  from mercurial import patch, revlog, scmutil, util, error, cmdutil
>  from mercurial import revset, templatekw, exchange
> +from mercurial import lock as lockmod
>
>  class TransplantError(error.Abort):
>      pass
> @@ -127,9 +128,8 @@ class transplanter(object):
>          diffopts = patch.difffeatureopts(self.ui, opts)
>          diffopts.git = True
>
> -        lock = wlock = tr = None
> +        lock = tr = None
>          try:
> -            wlock = repo.wlock()
>              lock = repo.lock()
>              tr = repo.transaction('transplant')
>              for rev in revs:
> @@ -224,7 +224,6 @@ class transplanter(object):
>                  tr.release()
>              if lock:
>                  lock.release()
> -            wlock.release()
>
>      def filter(self, filter, node, changelog, patchfile):
>          '''arbitrarily rewrite changeset before applying it'''
> @@ -345,7 +344,6 @@ class transplanter(object):
>                  merge = True
>
>          extra = {'transplant_source': node}
> -        wlock = repo.wlock()
>          try:
>              p1, p2 = repo.dirstate.parents()
>              if p1 != parent:
> @@ -367,7 +365,9 @@ class transplanter(object):
>
>              return n, node
>          finally:
> -            wlock.release()
> +            # TODO: get rid of this meaningless try/finally enclosing.
> +            # this is kept only to reduce changes in a patch.
> +            pass
>
>      def readseries(self):
>          nodes = []
> @@ -572,6 +572,14 @@ def transplant(ui, repo, *revs, **opts):
>      and then resume where you left off by calling :hg:`transplant
>      --continue/-c`.
>      '''
> +    wlock = None
> +    try:
> +        wlock = repo.wlock()
> +        return _dotransplant(ui, repo, *revs, **opts)
> +    finally:
> +        lockmod.release(wlock)
> +
> +def _dotransplant(ui, repo, *revs, **opts):
>      def incwalk(repo, csets, match=util.always):
>          for node in csets:
>              if match(node):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Dec. 3, 2015, 8:57 a.m.
On 12/02/2015 07:24 AM, Augie Fackler wrote:
> On Wed, Dec 02, 2015 at 03:18:47AM +0900, FUJIWARA Katsunori wrote:
>> # HG changeset patch
>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>> # Date 1448993528 -32400
>> #      Wed Dec 02 03:12:08 2015 +0900
>> # Branch stable
>> # Node ID 136cb2a5b5f0c0c67483c86d2fbe038147a2f288
>> # Parent  96c4b87476f3a511d06006bd70cfefe7fd672ebc
>> transplant: widen wlock scope of transplant for consitency while processing
>
> These patches look great to me, but I'm unclear why they're tagged for
> stable? Are they fixing a regression, or should I drop them on default
> instead?

This seems quite too big to go on stable. We are fixing old bug, not 
regression I'll look at it for default.

Patch

diff --git a/hgext/transplant.py b/hgext/transplant.py
--- a/hgext/transplant.py
+++ b/hgext/transplant.py
@@ -20,6 +20,7 @@  from mercurial.node import short
 from mercurial import bundlerepo, hg, merge, match
 from mercurial import patch, revlog, scmutil, util, error, cmdutil
 from mercurial import revset, templatekw, exchange
+from mercurial import lock as lockmod
 
 class TransplantError(error.Abort):
     pass
@@ -127,9 +128,8 @@  class transplanter(object):
         diffopts = patch.difffeatureopts(self.ui, opts)
         diffopts.git = True
 
-        lock = wlock = tr = None
+        lock = tr = None
         try:
-            wlock = repo.wlock()
             lock = repo.lock()
             tr = repo.transaction('transplant')
             for rev in revs:
@@ -224,7 +224,6 @@  class transplanter(object):
                 tr.release()
             if lock:
                 lock.release()
-            wlock.release()
 
     def filter(self, filter, node, changelog, patchfile):
         '''arbitrarily rewrite changeset before applying it'''
@@ -345,7 +344,6 @@  class transplanter(object):
                 merge = True
 
         extra = {'transplant_source': node}
-        wlock = repo.wlock()
         try:
             p1, p2 = repo.dirstate.parents()
             if p1 != parent:
@@ -367,7 +365,9 @@  class transplanter(object):
 
             return n, node
         finally:
-            wlock.release()
+            # TODO: get rid of this meaningless try/finally enclosing.
+            # this is kept only to reduce changes in a patch.
+            pass
 
     def readseries(self):
         nodes = []
@@ -572,6 +572,14 @@  def transplant(ui, repo, *revs, **opts):
     and then resume where you left off by calling :hg:`transplant
     --continue/-c`.
     '''
+    wlock = None
+    try:
+        wlock = repo.wlock()
+        return _dotransplant(ui, repo, *revs, **opts)
+    finally:
+        lockmod.release(wlock)
+
+def _dotransplant(ui, repo, *revs, **opts):
     def incwalk(repo, csets, match=util.always):
         for node in csets:
             if match(node):