[PATCH 2 of 2 v3] evolve: lock the working copy early in next and prev (issue5244)

[Pierre-Yves David]

On 10/16/2016 06:53 PM, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar at fb.com>
> # Date 1476636773 25200
> #      Sun Oct 16 09:52:53 2016 -0700
> # Node ID 778468237ecf195c4eb1d87bc4f7b5d30e538c63
> # 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.

These are pushed on stable, thanks (the fake-editor hack is a bit ugly, 
but lets move forward).

> 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')
> @@ -2257,7 +2259,8 @@
>                              bmdeactivate(repo)
>                          tr.close()
>                      finally:
> -                        lockmod.release(tr, lock, wlock)
> +                        lockmod.release(tr, lock)
> +
>              displayer.show(p)
>              return 0
>          else:
> @@ -2265,6 +2268,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 +2287,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 +2323,6 @@
>                  ret = hg.update(repo, c.rev())
>                  if not ret:
>                      lock = tr = None
> -                    wlock = repo.wlock()
>                      try:
>                          lock = repo.lock()
>                          tr = repo.transaction('next')
> @@ -2326,7 +2333,7 @@
>                              bmdeactivate(repo)
>                          tr.close()
>                      finally:
> -                        lockmod.release(tr, lock, wlock)
> +                        lockmod.release(tr, lock)
>              displayer.show(c)
>              result = 0
>          elif children:
> @@ -2368,6 +2375,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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

-- 
Pierre-Yves David