Patchwork pull: hold wlock for the full operation when --update is used

login
register
mail settings
Submitter Boris Feld
Date Jan. 11, 2018, 12:11 p.m.
Message ID <ba6760b14c221377e11f.1515672696@FB>
Download mbox | patch
Permalink /patch/26682/
State Accepted
Headers show

Comments

Boris Feld - Jan. 11, 2018, 12:11 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1515671879 0
#      Thu Jan 11 11:57:59 2018 +0000
# Node ID ba6760b14c221377e11f5a841b6794e7b1f858ad
# Parent  58fda95a0202fc6327d1f5d9df26f7ff16538d57
# EXP-Topic update-wlock
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r ba6760b14c22
pull: hold wlock for the full operation when --update is used

With now, the wlock is not held between the pull and the update. This can lead
to race condition and make logic checking to post pull results more complicated
(eg: with _afterlock).
Yuya Nishihara - Jan. 11, 2018, 1:19 p.m.
On Thu, 11 Jan 2018 12:11:36 +0000, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1515671879 0
> #      Thu Jan 11 11:57:59 2018 +0000
> # Node ID ba6760b14c221377e11f5a841b6794e7b1f858ad
> # Parent  58fda95a0202fc6327d1f5d9df26f7ff16538d57
> # EXP-Topic update-wlock
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r ba6760b14c22
> pull: hold wlock for the full operation when --update is used
> 
> With now, the wlock is not held between the pull and the update. This can lead
> to race condition and make logic checking to post pull results more complicated
> (eg: with _afterlock).

Queued, thanks.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4022,36 +4022,40 @@  def pull(ui, repo, source="default", **o
                         "so a rev cannot be specified.")
                 raise error.Abort(err)
 
-        pullopargs.update(opts.get('opargs', {}))
-        modheads = exchange.pull(repo, other, heads=revs,
-                                 force=opts.get('force'),
-                                 bookmarks=opts.get('bookmark', ()),
-                                 opargs=pullopargs).cgresult
-
-        # brev is a name, which might be a bookmark to be activated at
-        # the end of the update. In other words, it is an explicit
-        # destination of the update
-        brev = None
-
-        if checkout:
-            checkout = str(repo.changelog.rev(checkout))
-
-            # order below depends on implementation of
-            # hg.addbranchrevs(). opts['bookmark'] is ignored,
-            # because 'checkout' is determined without it.
-            if opts.get('rev'):
-                brev = opts['rev'][0]
-            elif opts.get('branch'):
-                brev = opts['branch'][0]
-            else:
-                brev = branches[0]
-        repo._subtoppath = source
-        try:
-            ret = postincoming(ui, repo, modheads, opts.get('update'),
-                               checkout, brev)
-
-        finally:
-            del repo._subtoppath
+        wlock = util.nullcontextmanager()
+        if opts.get('update'):
+            wlock = repo.wlock()
+        with wlock:
+            pullopargs.update(opts.get('opargs', {}))
+            modheads = exchange.pull(repo, other, heads=revs,
+                                     force=opts.get('force'),
+                                     bookmarks=opts.get('bookmark', ()),
+                                     opargs=pullopargs).cgresult
+
+            # brev is a name, which might be a bookmark to be activated at
+            # the end of the update. In other words, it is an explicit
+            # destination of the update
+            brev = None
+
+            if checkout:
+                checkout = str(repo.changelog.rev(checkout))
+
+                # order below depends on implementation of
+                # hg.addbranchrevs(). opts['bookmark'] is ignored,
+                # because 'checkout' is determined without it.
+                if opts.get('rev'):
+                    brev = opts['rev'][0]
+                elif opts.get('branch'):
+                    brev = opts['branch'][0]
+                else:
+                    brev = branches[0]
+            repo._subtoppath = source
+            try:
+                ret = postincoming(ui, repo, modheads, opts.get('update'),
+                                   checkout, brev)
+
+            finally:
+                del repo._subtoppath
 
     finally:
         other.close()
diff --git a/tests/test-keyword.t b/tests/test-keyword.t
--- a/tests/test-keyword.t
+++ b/tests/test-keyword.t
@@ -262,6 +262,7 @@  Pull from bundle and trigger notify
   adding file changes
   added 2 changesets with 3 changes to 3 files
   new changesets a2392c293916:ef63ca68695b
+  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -314,7 +315,6 @@  Pull from bundle and trigger notify
   +++ b/b	Thu Jan 01 00:00:00 1970 +0000
   @@ -0,0 +1,1 @@
   +ignore $Id$
-  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
   $ cp $HGRCPATH.nohooks $HGRCPATH