[PATCH] repair: fix transaction with no lock case in strip

[Laurent Charignon]

Alternatively we could add a comment to ask extensions to have a lock when calling strip (this is what we do with obsolete.createmarkers).
Let me know what you think.

Thanks,

Laurent

> On Dec 28, 2015, at 2:20 PM, Laurent Charignon <lcharignon at fb.com> wrote:
> 
> # HG changeset patch
> # User Laurent Charignon <lcharignon at fb.com>
> # Date 1451341019 28800
> #      Mon Dec 28 14:16:59 2015 -0800
> # Node ID bef9c27c77012ad82dc954f97205c6042793d558
> # Parent  23541bdd1610c08af247f9c8719045cf247ce541
> repair: fix transaction with no lock case in strip
> 
> Before this patch, if repair.strip was called without holding a lock,
> the strip transaction would be made without lock. There was no occurrences of
> the problem in core (because we are always calling repair.strip while holding
> a lock), so I added a test with a test extension that shows the issue.
> 
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -158,11 +158,12 @@ def strip(ui, repo, nodelist, backup=Tru
>         del curtr  # avoid carrying reference to transaction for nothing
>         msg = _('programming error: cannot strip from inside a transaction')
>         raise error.Abort(msg, hint=_('contact your extension maintainer'))
> -
> -    tr = repo.transaction("strip")
> -    offset = len(tr.entries)
> -
> +    tr = lock = wlock = None
>     try:
> +        wlock = repo.wlock()
> +        lock = repo.lock()
> +        tr = repo.transaction("strip")
> +        offset = len(tr.entries)
>         tr.startgroup()
>         cl.strip(striprev, tr)
>         mfst.strip(striprev, tr)
> @@ -205,15 +206,13 @@ def strip(ui, repo, nodelist, backup=Tru
> 
>         for m in updatebm:
>             bm[m] = repo[newbmtarget].node()
> -        lock = tr = None
> +        tr = None
>         try:
> -            lock = repo.lock()
>             tr = repo.transaction('repair')
>             bm.recordchange(tr)
>             tr.close()
>         finally:
>             tr.release()
> -            lock.release()
> 
>         # remove undo files
>         for undovfs, undofile in repo.undofiles():
> @@ -236,6 +235,11 @@ def strip(ui, repo, nodelist, backup=Tru
>         if saveheads or savebases:
>             # Remove partial backup only if there were no exceptions
>             vfs.unlink(chgrpfile)
> +    finally:
> +        if lock is not None:
> +            lock.release()
> +        if wlock is not None:
> +            wlock.release()
> 
>     repo.destroyed()
> 
> diff --git a/tests/test-strip.t b/tests/test-strip.t
> --- a/tests/test-strip.t
> +++ b/tests/test-strip.t
> @@ -858,4 +858,18 @@ Error during post-close callback of the 
>   abort: boom
>   [255]
> 
> +Wrapping strip does not require to hold a lock
> 
> +  $ cat > ../lockstrip.py << EOF
> +  > from mercurial import cmdutil, repair
> +  > 
> +  > cmdtable = {}
> +  > command = cmdutil.command(cmdtable)
> +  > 
> +  > @command('testlockstrip')
> +  > def testlock(ui, repo):
> +  >     repair.strip(repo.ui, repo, [repo['.'].node()])
> +  > EOF
> +  $ hg testlockstrip --config extensions.lockstrip=$TESTTMP/lockstrip.py
> +  saved backup bundle to * (glob)
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel