[PATCH 2 of 2] evolve: lock the working copy early in next and prev (issue5244)
Simon Farnsworth
simonfar at fb.com
Sun Oct 16 10:03:06 EDT 2016
On 16/10/2016 12:43, Pierre-Yves David wrote:
>
>
> On 10/08/2016 05:18 PM, Simon Farnsworth wrote:
>> # HG changeset patch
>> # User Simon Farnsworth <simonfar at fb.com>
>> # Date 1475939634 25200
>> # Sat Oct 08 08:13:54 2016 -0700
>> # Node ID 8a0c9c0158b3e9574a4571af3dce9978844b825d
>> # 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.
>
> The fix approach seems good, but the implementation is a bit strangeto
> me. See inline.
>
I can remove the early unlocks, but the goal is to unlock early when we
no longer need the locks.
>>
>> 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,13 +2260,22 @@
>> tr.close()
>> finally:
>> lockmod.release(tr, lock, wlock)
>> + wlock = None
>> + else:
>> + lockmod.release(wlock)
>> + wlock = None
>> +
>> displayer.show(p)
>> return 0
>> else:
>> + lockmod.release(wlock)
>> + wlock = None
>
>
> Why are we issuing multiple lockmode.release call instead of just using
> the final finally clause ?
>
Each lockmod.release() happens at a point where we no longer need the
lock for correctness (and in the old code, didn't have the lock at all).
The aim is to minimise the time we're holding the lock for. I can easily
redo this with wlock held for the entire duration of the command if
you'd prefer that version?
>> for p in parents:
>> 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 +2293,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 +2329,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,15 +2340,23 @@
>> tr.close()
>> finally:
>> lockmod.release(tr, lock, wlock)
>> + wlock = None
>> + else:
>> + lockmod.release(wlock)
>> + wlock = None
>> displayer.show(c)
>> result = 0
>> elif children:
>> + lockmod.release(wlock)
>> + wlock = None
>> ui.warn(_("ambigious next changeset:\n"))
>> for c in children:
>> displayer.show(c)
>> ui.warn(_('explicitly update to one of them\n'))
>> result = 1
>> else:
>> + lockmod.release(wlock)
>> + wlock = None
>> aspchildren = _aspiringchildren(repo, [repo['.'].rev()])
>> if topic:
>> filtered.extend(repo[c] for c in children
>> @@ -2368,6 +2389,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://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQICaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=0ZuyKYuyHM9hbUFUidarDO5LsEegsCBoK_VAKP3aRHQ&s=8758F-YEyhfMSvO5kgMSC6hUxWqBxsGpOIyAeeaePoU&e=
>>
>
--
Simon Farnsworth
More information about the Mercurial-devel
mailing list