revs = [] # actually, nodes

+ if other.capable('lookupns'):
+ def lookupns(e, r):
+ return e.callcommand('lookupns', {'key': r}).result()
+ else:
+ def lookupns(e, r):
+ return e.callcommand('lookup', {'key': r}).result(), ''

for r in oldrevs:
    with other.commandexecutor() as e:

• node = e.callcommand('lookup', {'key': r}).result() -

+ node, ns = lookupns(e, r)
+ if ns == 'bookmarks':
+ if r in remotebookmarks():
+ node = remotebookmarks()[r]

I'm not an expert, but I feel it's wrong to rely on client for data
consistency. Can't we somehow make the peer serve a "snapshot" of the
repository for the entire session? @indygreg Any updates in the v2 protocol
regarding this?

I went this way in part because it's fairly simple change, and might be useful in other circumstances (to interpret pulling a tag as actually pulling the tag itself perhaps, though I don't know concretely how that would work).

AFAIK, the protocol is never stateful (not sure how that'd work with http), so you can't rely on a notion of session-level state on the server to guarantee consistency.
I can see two plausible alternatives:

1. doing all the lookup queries and the bookmarks query in a single roundtrip. Is it guaranteed that a group of batchable queries will be sent in one go and won't be split up (if there are many of them for instance)?
2. tweak the new lookup query to be instead lookup(name, bookmark=node), which would mean: lookup(name) as if there exist a bookmark called name pointing to node. So kind of the snapshot idea, where you give the server the relevant part of the snapshot.

AFAIK, the protocol is never stateful (not sure how that'd work with http), so you can't rely on a notion of session-level state on the server to guarantee consistency.

True. I think the new v2 protocol will eventually get around the issue
since it will be more properly batched. @indygreg ?

I can see two plausible alternatives:
1. doing all the lookup queries and the bookmarks query in a single roundtrip. Is it guaranteed that a group of batchable queries will be sent in one go and won't be split up (if there are many of them for instance)?
2. tweak the new lookup query to be instead `lookup(name, bookmark=node)`, which would mean: `lookup(name)` as if there exist a bookmark called `name` pointing to `node`. So kind of the snapshot idea, where you give the server the relevant part of the snapshot.

3. do lookup() again at the end of the transaction to detect race, and abort

If the race doesn't occur frequently, I think it's okay to discard the pulled
data.

I'd prefer not to have unpredictable aborts where, when they happen, the solution is "try again". A year or two ago, the "remote heads changed during push" errors were like that, and they were annoying.

I tried the batching approach, and it seems to work fine, and doesn't require protocol changes. Based on reading the code, batch of rpcs never get split up. Perhaps one concern is whether this limits the amount of -r arguments that can be passed when using http.

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.

Thanks!

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.

listkeys shouldn't need compatibility check in the caller, because it's defined like this in wireprotov1peer.py (https://www.mercurial-scm.org/repo/hg-committed/file/tip/mercurial/wireprotov1peer.py#l381):

 @batchable
 def listkeys(self, namespace):
     if not self.capable('pushkey'):
         yield {}, None
...

> 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.
listkeys shouldn't need compatibility check in the caller, because it's defined like this in wireprotov1peer.py (https://www.mercurial-scm.org/repo/hg-committed/file/tip/mercurial/wireprotov1peer.py#l381):
   @batchable
   def listkeys(self, namespace):
       if not self.capable('pushkey'):
           yield {}, None

Yeah, but the batch executor appears not handle such cases.

https://www.mercurial-scm.org/repo/hg-committed/file/tip/mercurial/wireprotov1peer.py#l241