pull: fix inconsistent view of bookmarks during pull (issue4700)
authorValentin Gatien-Baron <valentin.gatienbaron@gmail.com>
Thu, 20 Dec 2018 22:28:39 -0500
changeset 41054 bad05a6afdc8
parent 41053 6faaf3a1c6ec
child 41055 4c25038c112c
pull: fix inconsistent view of bookmarks during pull (issue4700) I had a share where a pull apparently pulled a bookmark but not the revision pointed to by the bookmark, which I suspect is due to this (and if not, we might as well remove known issues in this area). I do this by combining doing all the queries that could read the bookmarks in one round trip. I had to change the handling of the case where the server doesn't support the lookup query, because if it fails, it would otherwise make fremotebookmark.result() block forever. This is due to wireprotov1peer.peerexecutor.sendcommands's behavior (it fills a single future if any query fails synchronously and leaves all other futures unchanged), but I don't know if the fix is to cancel all other futures, or to keep going with the other queries. Differential Revision: https://phab.mercurial-scm.org/D5449
mercurial/commands.py
tests/test-bookmarks-pushpull.t
--- a/mercurial/commands.py	Sun Dec 23 13:16:25 2018 +0530
+++ b/mercurial/commands.py	Thu Dec 20 22:28:39 2018 -0500
@@ -4414,49 +4414,47 @@
         revs, checkout = hg.addbranchrevs(repo, other, branches,
                                           opts.get('rev'))
 
-
         pullopargs = {}
-        if opts.get('bookmark'):
-            if not revs:
-                revs = []
-            # The list of bookmark used here is not the one used to actually
-            # update the bookmark name. This can result in the revision pulled
-            # not ending up with the name of the bookmark because of a race
-            # condition on the server. (See issue 4689 for details)
-            remotebookmarks = other.listkeys('bookmarks')
+
+        nodes = None
+        if opts['bookmark'] or revs:
+            # The list of bookmark used here is the same used to actually update
+            # the bookmark names, to avoid the race from issue 4689 and we do
+            # all lookup and bookmark queries in one go so they see the same
+            # version of the server state (issue 4700).
+            nodes = []
+            fnodes = []
+            revs = revs or []
+            if revs and not other.capable('lookup'):
+                err = _("other repository doesn't support revision lookup, "
+                        "so a rev cannot be specified.")
+                raise error.Abort(err)
+            with other.commandexecutor() as e:
+                fremotebookmarks = e.callcommand('listkeys', {
+                    'namespace': 'bookmarks'
+                })
+                for r in revs:
+                    fnodes.append(e.callcommand('lookup', {'key': r}))
+            remotebookmarks = fremotebookmarks.result()
             remotebookmarks = bookmarks.unhexlifybookmarks(remotebookmarks)
             pullopargs['remotebookmarks'] = remotebookmarks
             for b in opts['bookmark']:
                 b = repo._bookmarks.expandname(b)
                 if b not in remotebookmarks:
                     raise error.Abort(_('remote bookmark %s not found!') % b)
-                revs.append(hex(remotebookmarks[b]))
-
-        if revs:
-            try:
-                # When 'rev' is a bookmark name, we cannot guarantee that it
-                # will be updated with that name because of a race condition
-                # server side. (See issue 4689 for details)
-                oldrevs = revs
-                revs = [] # actually, nodes
-                for r in oldrevs:
-                    with other.commandexecutor() as e:
-                        node = e.callcommand('lookup', {'key': r}).result()
-
-                    revs.append(node)
-                    if r == checkout:
-                        checkout = node
-            except error.CapabilityError:
-                err = _("other repository doesn't support revision lookup, "
-                        "so a rev cannot be specified.")
-                raise error.Abort(err)
+                nodes.append(remotebookmarks[b])
+            for i, rev in enumerate(revs):
+                node = fnodes[i].result()
+                nodes.append(node)
+                if rev == checkout:
+                    checkout = node
 
         wlock = util.nullcontextmanager()
         if opts.get('update'):
             wlock = repo.wlock()
         with wlock:
             pullopargs.update(opts.get('opargs', {}))
-            modheads = exchange.pull(repo, other, heads=revs,
+            modheads = exchange.pull(repo, other, heads=nodes,
                                      force=opts.get('force'),
                                      bookmarks=opts.get('bookmark', ()),
                                      opargs=pullopargs).cgresult
--- a/tests/test-bookmarks-pushpull.t	Sun Dec 23 13:16:25 2018 +0530
+++ b/tests/test-bookmarks-pushpull.t	Thu Dec 20 22:28:39 2018 -0500
@@ -673,12 +673,13 @@
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
+  updating bookmark Y
   new changesets 0d60821d2197 (1 drafts)
   (run 'hg update' to get a working copy)
   $ hg book
      @                         1:0d2164f0ce0d
      X                         1:0d2164f0ce0d
-   * Y                         5:35d1ef0a8d1b
+   * Y                         6:0d60821d2197
      Z                         1:0d2164f0ce0d
   $ hg -R $TESTTMP/pull-race book
      @                         1:0d2164f0ce0d