# HG changeset patch
# User Valentin Gatien-Baron <valentin.gatienbaron@gmail.com>
# Date 2018-12-21 03:28:39
# Node ID bad05a6afdc89cc58a2af320698ab29bd8de62d4
# Parent  6faaf3a1c6ec9342e45f6ebb506f823f8e578daf

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

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4414,49 +4414,47 @@ def pull(ui, repo, source="default", **o
         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
diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -673,12 +673,13 @@ Update a bookmark right after the initia
   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