Patchwork [2,of,2,v2] evolve: lock the working copy early in next and prev (issue5244)

login
register
mail settings
Submitter Simon Farnsworth
Date Oct. 16, 2016, 2:25 p.m.
Message ID <99c560f7d07d3d08b83f.1476627921@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/17095/
State Superseded
Headers show

Comments

Simon Farnsworth - Oct. 16, 2016, 2:25 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1476627823 25200
#      Sun Oct 16 07:23:43 2016 -0700
# Node ID 99c560f7d07d3d08b83f5f1802fc329d67e5de2e
# Parent  ee284d7c5faa5d9f17853f51c17883844b985c58
evolve: lock the working copy early in next and prev (issue5244)

Both next and prev depend on a consistent working copy, but were waiting to
take the lock until they were ready to alter the working copy.

Take the lock before reading the working copy state, and do not release it
until we're definitely not going to change the working copy.
Pierre-Yves David - Oct. 16, 2016, 2:50 p.m.
On 10/16/2016 04:25 PM, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1476627823 25200
> #      Sun Oct 16 07:23:43 2016 -0700
> # Node ID 99c560f7d07d3d08b83f5f1802fc329d67e5de2e
> # Parent  ee284d7c5faa5d9f17853f51c17883844b985c58
> evolve: lock the working copy early in next and prev (issue5244)
>
> Both next and prev depend on a consistent working copy, but were waiting to
> take the lock until they were ready to alter the working copy.
>
> Take the lock before reading the working copy state, and do not release it
> until we're definitely not going to change the working copy.
>
> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -2213,10 +2213,13 @@
>      """update to parent revision
>
>      Displays the summary line of the destination for clarity."""
> -    if True:
> +    wlock = None
> +    dryrunopt = opts['dry_run']
> +    if not dryrunopt:
> +        wlock = repo.wlock()
> +    try:
>          wkctx = repo[None]
>          wparents = wkctx.parents()
> -        dryrunopt = opts['dry_run']
>          if len(wparents) != 1:
>              raise error.Abort('merge in progress')
>          if not opts['merge']:
> @@ -2246,7 +2249,6 @@
>                  ret = hg.update(repo, p.rev())
>                  if not ret:
>                      tr = lock = None
> -                    wlock = repo.wlock()
>                      try:
>                          lock = repo.lock()
>                          tr = repo.transaction('previous')
> @@ -2258,6 +2260,8 @@
>                          tr.close()
>                      finally:
>                          lockmod.release(tr, lock, wlock)
> +                        wlock = None
> +

Given that we dropped  the lock grabbing associated with this 
try:/except: why don't we simply from that release too ? (and the 
associated try:/except

(same apply for the other command)

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -2213,10 +2213,13 @@ 
     """update to parent revision
 
     Displays the summary line of the destination for clarity."""
-    if True:
+    wlock = None
+    dryrunopt = opts['dry_run']
+    if not dryrunopt:
+        wlock = repo.wlock()
+    try:
         wkctx = repo[None]
         wparents = wkctx.parents()
-        dryrunopt = opts['dry_run']
         if len(wparents) != 1:
             raise error.Abort('merge in progress')
         if not opts['merge']:
@@ -2246,7 +2249,6 @@ 
                 ret = hg.update(repo, p.rev())
                 if not ret:
                     tr = lock = None
-                    wlock = repo.wlock()
                     try:
                         lock = repo.lock()
                         tr = repo.transaction('previous')
@@ -2258,6 +2260,8 @@ 
                         tr.close()
                     finally:
                         lockmod.release(tr, lock, wlock)
+                        wlock = None
+
             displayer.show(p)
             return 0
         else:
@@ -2265,6 +2269,8 @@ 
                 displayer.show(p)
             ui.warn(_('multiple parents, explicitly update to one\n'))
             return 1
+    finally:
+        lockmod.release(wlock)
 
 @command('^next',
          [('B', 'move-bookmark', False,
@@ -2282,10 +2288,13 @@ 
 
     Displays the summary line of the destination for clarity.
     """
-    if True:
+    wlock = None
+    dryrunopt = opts['dry_run']
+    if not dryrunopt:
+        wlock = repo.wlock()
+    try:
         wkctx = repo[None]
         wparents = wkctx.parents()
-        dryrunopt = opts['dry_run']
         if len(wparents) != 1:
             raise error.Abort('merge in progress')
         if not opts['merge']:
@@ -2315,7 +2324,6 @@ 
                 ret = hg.update(repo, c.rev())
                 if not ret:
                     lock = tr = None
-                    wlock = repo.wlock()
                     try:
                         lock = repo.lock()
                         tr = repo.transaction('next')
@@ -2327,6 +2335,7 @@ 
                         tr.close()
                     finally:
                         lockmod.release(tr, lock, wlock)
+                        wlock = None
             displayer.show(c)
             result = 0
         elif children:
@@ -2368,6 +2377,8 @@ 
                 return result
             return 1
         return result
+    finally:
+        lockmod.release(wlock)
 
 def _reachablefrombookmark(repo, revs, bookmarks):
     """filter revisions and bookmarks reachable from the given bookmark
diff --git a/tests/fake-editor.sh b/tests/fake-editor.sh
new file mode 100755
--- /dev/null
+++ b/tests/fake-editor.sh
@@ -0,0 +1,3 @@ 
+#!/bin/sh
+sleep 5
+echo "new desc" >> $1
diff --git a/tests/test-prev-next.t b/tests/test-prev-next.t
--- a/tests/test-prev-next.t
+++ b/tests/test-prev-next.t
@@ -206,3 +206,34 @@ 
   move:[5] added d
   atop:[6] added b (3)
   working directory is now at 47ea25be8aea
+
+prev and next should lock properly against other commands
+
+  $ hg init repo
+  $ cd repo
+  $ HGEDITOR=${TESTDIR}/fake-editor.sh
+  $ echo hi > foo
+  $ hg ci -Am 'one'
+  adding foo
+  $ echo bye > foo
+  $ hg ci -Am 'two'
+
+  $ hg amend --edit &
+  $ sleep 1
+  $ hg prev
+  waiting for lock on working directory of $TESTTMP/repo held by process '*' on host '*' (glob)
+  got lock after [4-6] seconds (re)
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  [0] one
+  $ wait
+
+  $ hg amend --edit &
+  $ sleep 1
+  $ hg next --evolve
+  waiting for lock on working directory of $TESTTMP/repo held by process '*' on host '*' (glob)
+  1 new unstable changesets
+  got lock after [4-6] seconds (re)
+  move:[2] two
+  atop:[3] one
+  working directory now at a7d885c75614
+  $ wait