pull: fix inconsistent view of bookmarks during pull (issue4700)

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

--- 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