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

login
register
mail settings
Submitter phabricator
Date Dec. 21, 2018, 3:50 a.m.
Message ID <0d8335cc30b999d033a1a73db415f38f@localhost.localdomain>
Download mbox | patch
Permalink /patch/37288/
State Not Applicable
Headers show

Comments

phabricator - Dec. 21, 2018, 3:50 a.m.
valentin.gatienbaron updated this revision to Diff 12938.
valentin.gatienbaron edited the summary of this revision.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5449?vs=12895&id=12938

REVISION DETAIL
  https://phab.mercurial-scm.org/D5449

AFFECTED FILES
  mercurial/commands.py
  tests/test-bookmarks-pushpull.t

CHANGE DETAILS




To: valentin.gatienbaron, #hg-reviewers
Cc: yuja, indygreg, mercurial-devel
Yuya Nishihara - Dec. 24, 2018, 3:07 a.m.
Queued, thanks.

I have a concern about compatibility with ancient Mercurial. Can you check it
and send a follow-up as needed?

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

@indygreg

> +        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}))

IIRC, `listkeys` is a newer command than `lookup`. If the peer doesn't support
`listkeys`, I suspect this batch query would fail. In that case, maybe `listkeys`
has to be skipped if the peer doesn't support it and if --bookmark is not
specified.
phabricator - Dec. 24, 2018, 3:08 a.m.
yuja added a comment.


  Queued, thanks.
  
  I have a concern about compatibility with ancient Mercurial. Can you check it
  and send a follow-up as needed?
  
  >   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.
  
  @indygreg
  
  > +        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}))
  
  IIRC, `listkeys` is a newer command than `lookup`. If the peer doesn't support
  `listkeys`, I suspect this batch query would fail. In that case, maybe `listkeys`
  has to be skipped if the peer doesn't support it and if --bookmark is not
  specified.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5449

To: valentin.gatienbaron, #hg-reviewers
Cc: yuja, indygreg, mercurial-devel

Patch

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 @@ 
   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
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4414,49 +4414,49 @@ 
         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