Patchwork [2,of,2] bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)

login
register
mail settings
Submitter Stanislau Hlebik
Date Dec. 9, 2016, 11:31 a.m.
Message ID <df861963a18c00d72362.1481283105@devvm957.lla2.facebook.com>
Download mbox | patch
Permalink /patch/17883/
State Accepted
Headers show

Comments

Stanislau Hlebik - Dec. 9, 2016, 11:31 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1481282546 28800
#      Fri Dec 09 03:22:26 2016 -0800
# Node ID df861963a18c00d72362b415a77a62d2c18660bf
# Parent  08ab8f9d0abcbd1b2405ecb0a8670d212866af1f
bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)

Binary bookmark format should be used internally. It doesn't make sense to have
optional parameters `srchex` and `dsthex`. This patch removes them. It will
also be useful for `bookmarks` bundle2 part because unnecessary conversions
between hex and bin nodes will be avoided.
Augie Fackler - Dec. 13, 2016, 10:59 p.m.
On Fri, Dec 09, 2016 at 03:31:45AM -0800, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1481282546 28800
> #      Fri Dec 09 03:22:26 2016 -0800
> # Node ID df861963a18c00d72362b415a77a62d2c18660bf
> # Parent  08ab8f9d0abcbd1b2405ecb0a8670d212866af1f
> bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)

I've queued these two, thanks.
Pierre-Yves David - Dec. 17, 2016, 7:33 a.m.
On 12/09/2016 12:31 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1481282546 28800
> #      Fri Dec 09 03:22:26 2016 -0800
> # Node ID df861963a18c00d72362b415a77a62d2c18660bf
> # Parent  08ab8f9d0abcbd1b2405ecb0a8670d212866af1f
> bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)
>
> Binary bookmark format should be used internally. It doesn't make sense to have
> optional parameters `srchex` and `dsthex`. This patch removes them. It will
> also be useful for `bookmarks` bundle2 part because unnecessary conversions
> between hex and bin nodes will be avoided.

Great, I've put a second acceptance stamp on this changeset and it 
should show as public shortly.

There is a couple thing in this patch that highlight the need for extra 
work on that topic to remove all list key call from the client/server 
exchange. I'm not sure if we should have something special to track 
these, but see my comment below.

> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -391,8 +391,7 @@
>      finally:
>          lockmod.release(tr, l, w)
>
> -def comparebookmarks(repo, srcmarks, dstmarks,
> -                     srchex=None, dsthex=None, targets=None):
> +def comparebookmarks(repo, srcmarks, dstmarks, targets=None):
>      '''Compare bookmarks between srcmarks and dstmarks
>
>      This returns tuple "(addsrc, adddst, advsrc, advdst, diverge,
> @@ -415,19 +414,9 @@
>      Changeset IDs of tuples in "addsrc", "adddst", "differ" or
>       "invalid" list may be unknown for repo.
>
> -    This function expects that "srcmarks" and "dstmarks" return
> -    changeset ID in 40 hexadecimal digit string for specified
> -    bookmark. If not so (e.g. bmstore "repo._bookmarks" returning
> -    binary value), "srchex" or "dsthex" should be specified to convert
> -    into such form.
> -
>      If "targets" is specified, only bookmarks listed in it are
>      examined.
>      '''
> -    if not srchex:
> -        srchex = lambda x: x
> -    if not dsthex:
> -        dsthex = lambda x: x
>
>      if targets:
>          bset = set(targets)
> @@ -449,14 +438,14 @@
>      for b in sorted(bset):
>          if b not in srcmarks:
>              if b in dstmarks:
> -                adddst((b, None, dsthex(dstmarks[b])))
> +                adddst((b, None, dstmarks[b]))
>              else:
>                  invalid((b, None, None))
>          elif b not in dstmarks:
> -            addsrc((b, srchex(srcmarks[b]), None))
> +            addsrc((b, srcmarks[b], None))
>          else:
> -            scid = srchex(srcmarks[b])
> -            dcid = dsthex(dstmarks[b])
> +            scid = srcmarks[b]
> +            dcid = dstmarks[b]
>              if scid == dcid:
>                  same((b, scid, dcid))
>              elif scid in repo and dcid in repo:
> @@ -507,11 +496,17 @@
>
>      return None
>
> +def unhexlifybookmarks(marks):
> +    binremotemarks = {}
> +    for name, node in marks.items():
> +        binremotemarks[name] = bin(node)
> +    return binremotemarks
> +

This function looked suspicious and was the start of my quest to know 
more. My thinking seeing this was, "wait", if we moved internal to 
binary why do we still needs it?

Keep scrolling for the result of that investigation.

>  def updatefromremote(ui, repo, remotemarks, path, trfunc, explicit=()):
>      ui.debug("checking for updated bookmarks\n")
>      localmarks = repo._bookmarks
>      (addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same
> -     ) = comparebookmarks(repo, remotemarks, localmarks, dsthex=hex)
> +    ) = comparebookmarks(repo, remotemarks, localmarks)
>
>      status = ui.status
>      warn = ui.warn
> @@ -522,15 +517,15 @@
>      changed = []
>      for b, scid, dcid in addsrc:
>          if scid in repo: # add remote bookmarks for changes we already have
> -            changed.append((b, bin(scid), status,
> +            changed.append((b, scid, status,
>                              _("adding remote bookmark %s\n") % (b)))
>          elif b in explicit:
>              explicit.remove(b)
>              ui.warn(_("remote bookmark %s points to locally missing %s\n")
> -                    % (b, scid[:12]))
> +                    % (b, hex(scid)[:12]))
>
>      for b, scid, dcid in advsrc:
> -        changed.append((b, bin(scid), status,
> +        changed.append((b, scid, status,
>                          _("updating bookmark %s\n") % (b)))
>      # remove normal movement from explicit set
>      explicit.difference_update(d[0] for d in changed)
> @@ -538,13 +533,12 @@
>      for b, scid, dcid in diverge:
>          if b in explicit:
>              explicit.discard(b)
> -            changed.append((b, bin(scid), status,
> +            changed.append((b, scid, status,
>                              _("importing bookmark %s\n") % (b)))
>          else:
> -            snode = bin(scid)
> -            db = _diverge(ui, b, path, localmarks, snode)
> +            db = _diverge(ui, b, path, localmarks, scid)
>              if db:
> -                changed.append((db, snode, warn,
> +                changed.append((db, scid, warn,
>                                  _("divergent bookmark %s stored as %s\n") %
>                                  (b, db)))
>              else:
> @@ -553,13 +547,13 @@
>      for b, scid, dcid in adddst + advdst:
>          if b in explicit:
>              explicit.discard(b)
> -            changed.append((b, bin(scid), status,
> +            changed.append((b, scid, status,
>                              _("importing bookmark %s\n") % (b)))
>      for b, scid, dcid in differ:
>          if b in explicit:
>              explicit.remove(b)
>              ui.warn(_("remote bookmark %s points to locally missing %s\n")
> -                    % (b, scid[:12]))
> +                    % (b, hex(scid)[:12]))
>
>      if changed:
>          tr = trfunc()
> @@ -573,8 +567,8 @@
>      '''
>      ui.status(_("searching for changed bookmarks\n"))
>
> -    r = comparebookmarks(repo, other.listkeys('bookmarks'), repo._bookmarks,
> -                         dsthex=hex)
> +    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))

This code is about 'hg incoming'
(you should set diff.showfunc=1 in your config.)

What happened here is that incoming is still using listkeys for 
bookmarks in all cases. This is not very surprising since incoming have 
actually never been migrated to bundle2.

This means incoming will be unable to benefit from your change 
(selective bookmark pull) but your probably do not use incoming and 
therefor don't care.

Given that lack of migration, we could just move on and think about 
migrating this at when doing that migration. However there is a similar 
usecase later that would need more attention. In the interest of keeping 
the suspense high, I'll keep the rest for that user.

> +    r = comparebookmarks(repo, remotemarks, repo._bookmarks)
>      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
>
>      incomings = []
> @@ -590,16 +584,16 @@
>              incomings.append("   %-25s %s\n" % (b, getid(id)))
>      for b, scid, dcid in addsrc:
>          # i18n: "added" refers to a bookmark
> -        add(b, scid, _('added'))
> +        add(b, hex(scid), _('added'))
>      for b, scid, dcid in advsrc:
>          # i18n: "advanced" refers to a bookmark
> -        add(b, scid, _('advanced'))
> +        add(b, hex(scid), _('advanced'))
>      for b, scid, dcid in diverge:
>          # i18n: "diverged" refers to a bookmark
> -        add(b, scid, _('diverged'))
> +        add(b, hex(scid), _('diverged'))
>      for b, scid, dcid in differ:
>          # i18n: "changed" refers to a bookmark
> -        add(b, scid, _('changed'))
> +        add(b, hex(scid), _('changed'))
>
>      if not incomings:
>          ui.status(_("no changed bookmarks found\n"))
> @@ -615,8 +609,8 @@
>      '''
>      ui.status(_("searching for changed bookmarks\n"))
>
> -    r = comparebookmarks(repo, repo._bookmarks, other.listkeys('bookmarks'),
> -                         srchex=hex)
> +    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
> +    r = comparebookmarks(repo, repo._bookmarks, remotemarks)

This code is about 'hg outgoing', there is no bundling involved it is 
probably more important to you to make it use the new method.

>      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
>
>      outgoings = []
> @@ -632,19 +626,19 @@
>              outgoings.append("   %-25s %s\n" % (b, getid(id)))
>      for b, scid, dcid in addsrc:
>          # i18n: "added refers to a bookmark
> -        add(b, scid, _('added'))
> +        add(b, hex(scid), _('added'))
>      for b, scid, dcid in adddst:
>          # i18n: "deleted" refers to a bookmark
>          add(b, ' ' * 40, _('deleted'))
>      for b, scid, dcid in advsrc:
>          # i18n: "advanced" refers to a bookmark
> -        add(b, scid, _('advanced'))
> +        add(b, hex(scid), _('advanced'))
>      for b, scid, dcid in diverge:
>          # i18n: "diverged" refers to a bookmark
> -        add(b, scid, _('diverged'))
> +        add(b, hex(scid), _('diverged'))
>      for b, scid, dcid in differ:
>          # i18n: "changed" refers to a bookmark
> -        add(b, scid, _('changed'))
> +        add(b, hex(scid), _('changed'))
>
>      if not outgoings:
>          ui.status(_("no changed bookmarks found\n"))
> @@ -660,8 +654,8 @@
>
>      This returns "(# of incoming, # of outgoing)" tuple.
>      '''
> -    r = comparebookmarks(repo, other.listkeys('bookmarks'), repo._bookmarks,
> -                         dsthex=hex)
> +    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))

That code is about 'hg summary', nothing too important here (except that 
if we get with a better solution we should use it and get ride of the 
old one)

> +    r = comparebookmarks(repo, remotemarks, repo._bookmarks)
>      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
>      return (len(addsrc), len(adddst))
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -603,9 +603,21 @@
>      explicit = set([repo._bookmarks.expandname(bookmark)
>                      for bookmark in pushop.bookmarks])
>
> -    comp = bookmod.comparebookmarks(repo, repo._bookmarks,
> -                                    remotebookmark, srchex=hex)
> +    remotebookmark = bookmod.unhexlifybookmarks(remotebookmark)

This is part of _pushdiscoverybookmarks (so the function that find what 
bookmark operation needs to be done on push). This remote bookmark comes 
from a listkeys call so it won't benefit of improvements you made with 
your bookmark work. The way forward here seems to  be as follow:

1) introduce a function responsible to get remote bookmark from SOMETHING.
- That something is probably a peer as a start.
- That function probably lives in bookmarks.py or exchanges.py
2) That function will check the peer capabilities and use the most 
relevant option (nothing, listkeys or bundle2 part),
3) when using listkey the translation from hex to bin will be made 
within the function,
4) All current users of listkeys for bookmarks are switched to this 
function.

What do you think?

(Having that function will help your extensions (pulling patterns of 
bookmarks) work)

> +    comp = bookmod.comparebookmarks(repo, repo._bookmarks, remotebookmark)
> +
> +    def safehex(x):
> +        if x is None:
> +            return x
> +        return hex(x)
> +
> +    def hexifycompbookmarks(bookmarks):
> +        for b, scid, dcid in bookmarks:
> +            yield b, safehex(scid), safehex(dcid)

(another fishy function, more later)

Translating from a list to a generator is bit dangerous, because only 
one iteration can be made on that generator. It does not seems to 
introduce bug as we use them only one. But I wanted to point it out as a 
coding style.

> +
> +    comp = [hexifycompbookmarks(marks) for marks in comp]
>      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = comp
> +

It appears we are converting back to hex to store data into the push 
operation. We should migrate that part to binary too.

>      for b, scid, dcid in advsrc:
>          if b in explicit:
>              explicit.remove(b)
> @@ -617,7 +629,7 @@
>              explicit.remove(b)
>              pushop.outbookmarks.append((b, '', scid))
>      # search for overwritten bookmark
> -    for b, scid, dcid in advdst + diverge + differ:
> +    for b, scid, dcid in list(advdst) + list(diverge) + list(differ):

(building lists directly would avoid some dancing here).

>          if b in explicit:
>              explicit.remove(b)
>              pushop.outbookmarks.append((b, dcid, scid))
> @@ -1456,6 +1468,7 @@
>      pullop.stepsdone.add('bookmarks')
>      repo = pullop.repo
>      remotebookmarks = pullop.remotebookmarks
> +    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
>      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
>                               pullop.remote.url(),
>                               pullop.gettransaction,

It looks like 'updatefromremote' is using hex internally too. We should 
update it to use binary..

Cheers,
Stanislau Hlebik - Dec. 20, 2016, 1:37 p.m.
Excerpts from Pierre-Yves David's message of 2016-12-17 08:33:20 +0100:
> 
> On 12/09/2016 12:31 PM, Stanislau Hlebik wrote:
> > # HG changeset patch
> > # User Stanislau Hlebik <stash@fb.com>
> > # Date 1481282546 28800
> > #      Fri Dec 09 03:22:26 2016 -0800
> > # Node ID df861963a18c00d72362b415a77a62d2c18660bf
> > # Parent  08ab8f9d0abcbd1b2405ecb0a8670d212866af1f
> > bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)
> >
> > Binary bookmark format should be used internally. It doesn't make sense to have
> > optional parameters `srchex` and `dsthex`. This patch removes them. It will
> > also be useful for `bookmarks` bundle2 part because unnecessary conversions
> > between hex and bin nodes will be avoided.
> 
> Great, I've put a second acceptance stamp on this changeset and it 
> should show as public shortly.
> 
> There is a couple thing in this patch that highlight the need for extra 
> work on that topic to remove all list key call from the client/server 
> exchange. I'm not sure if we should have something special to track 
> these, but see my comment below.
> 
> > diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> > --- a/mercurial/bookmarks.py
> > +++ b/mercurial/bookmarks.py
> > @@ -391,8 +391,7 @@
> >      finally:
> >          lockmod.release(tr, l, w)
> >
> > -def comparebookmarks(repo, srcmarks, dstmarks,
> > -                     srchex=None, dsthex=None, targets=None):
> > +def comparebookmarks(repo, srcmarks, dstmarks, targets=None):
> >      '''Compare bookmarks between srcmarks and dstmarks
> >
> >      This returns tuple "(addsrc, adddst, advsrc, advdst, diverge,
> > @@ -415,19 +414,9 @@
> >      Changeset IDs of tuples in "addsrc", "adddst", "differ" or
> >       "invalid" list may be unknown for repo.
> >
> > -    This function expects that "srcmarks" and "dstmarks" return
> > -    changeset ID in 40 hexadecimal digit string for specified
> > -    bookmark. If not so (e.g. bmstore "repo._bookmarks" returning
> > -    binary value), "srchex" or "dsthex" should be specified to convert
> > -    into such form.
> > -
> >      If "targets" is specified, only bookmarks listed in it are
> >      examined.
> >      '''
> > -    if not srchex:
> > -        srchex = lambda x: x
> > -    if not dsthex:
> > -        dsthex = lambda x: x
> >
> >      if targets:
> >          bset = set(targets)
> > @@ -449,14 +438,14 @@
> >      for b in sorted(bset):
> >          if b not in srcmarks:
> >              if b in dstmarks:
> > -                adddst((b, None, dsthex(dstmarks[b])))
> > +                adddst((b, None, dstmarks[b]))
> >              else:
> >                  invalid((b, None, None))
> >          elif b not in dstmarks:
> > -            addsrc((b, srchex(srcmarks[b]), None))
> > +            addsrc((b, srcmarks[b], None))
> >          else:
> > -            scid = srchex(srcmarks[b])
> > -            dcid = dsthex(dstmarks[b])
> > +            scid = srcmarks[b]
> > +            dcid = dstmarks[b]
> >              if scid == dcid:
> >                  same((b, scid, dcid))
> >              elif scid in repo and dcid in repo:
> > @@ -507,11 +496,17 @@
> >
> >      return None
> >
> > +def unhexlifybookmarks(marks):
> > +    binremotemarks = {}
> > +    for name, node in marks.items():
> > +        binremotemarks[name] = bin(node)
> > +    return binremotemarks
> > +
> 
> This function looked suspicious and was the start of my quest to know 
> more. My thinking seeing this was, "wait", if we moved internal to 
> binary why do we still needs it?
> 
> Keep scrolling for the result of that investigation.
> 
> >  def updatefromremote(ui, repo, remotemarks, path, trfunc, explicit=()):
> >      ui.debug("checking for updated bookmarks\n")
> >      localmarks = repo._bookmarks
> >      (addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same
> > -     ) = comparebookmarks(repo, remotemarks, localmarks, dsthex=hex)
> > +    ) = comparebookmarks(repo, remotemarks, localmarks)
> >
> >      status = ui.status
> >      warn = ui.warn
> > @@ -522,15 +517,15 @@
> >      changed = []
> >      for b, scid, dcid in addsrc:
> >          if scid in repo: # add remote bookmarks for changes we already have
> > -            changed.append((b, bin(scid), status,
> > +            changed.append((b, scid, status,
> >                              _("adding remote bookmark %s\n") % (b)))
> >          elif b in explicit:
> >              explicit.remove(b)
> >              ui.warn(_("remote bookmark %s points to locally missing %s\n")
> > -                    % (b, scid[:12]))
> > +                    % (b, hex(scid)[:12]))
> >
> >      for b, scid, dcid in advsrc:
> > -        changed.append((b, bin(scid), status,
> > +        changed.append((b, scid, status,
> >                          _("updating bookmark %s\n") % (b)))
> >      # remove normal movement from explicit set
> >      explicit.difference_update(d[0] for d in changed)
> > @@ -538,13 +533,12 @@
> >      for b, scid, dcid in diverge:
> >          if b in explicit:
> >              explicit.discard(b)
> > -            changed.append((b, bin(scid), status,
> > +            changed.append((b, scid, status,
> >                              _("importing bookmark %s\n") % (b)))
> >          else:
> > -            snode = bin(scid)
> > -            db = _diverge(ui, b, path, localmarks, snode)
> > +            db = _diverge(ui, b, path, localmarks, scid)
> >              if db:
> > -                changed.append((db, snode, warn,
> > +                changed.append((db, scid, warn,
> >                                  _("divergent bookmark %s stored as %s\n") %
> >                                  (b, db)))
> >              else:
> > @@ -553,13 +547,13 @@
> >      for b, scid, dcid in adddst + advdst:
> >          if b in explicit:
> >              explicit.discard(b)
> > -            changed.append((b, bin(scid), status,
> > +            changed.append((b, scid, status,
> >                              _("importing bookmark %s\n") % (b)))
> >      for b, scid, dcid in differ:
> >          if b in explicit:
> >              explicit.remove(b)
> >              ui.warn(_("remote bookmark %s points to locally missing %s\n")
> > -                    % (b, scid[:12]))
> > +                    % (b, hex(scid)[:12]))
> >
> >      if changed:
> >          tr = trfunc()
> > @@ -573,8 +567,8 @@
> >      '''
> >      ui.status(_("searching for changed bookmarks\n"))
> >
> > -    r = comparebookmarks(repo, other.listkeys('bookmarks'), repo._bookmarks,
> > -                         dsthex=hex)
> > +    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
> 
> This code is about 'hg incoming'
> (you should set diff.showfunc=1 in your config.)
> 
> What happened here is that incoming is still using listkeys for 
> bookmarks in all cases. This is not very surprising since incoming have 
> actually never been migrated to bundle2.
> 
> This means incoming will be unable to benefit from your change 
> (selective bookmark pull) but your probably do not use incoming and 
> therefor don't care.
> 
> Given that lack of migration, we could just move on and think about 
> migrating this at when doing that migration. However there is a similar 
> usecase later that would need more attention. In the interest of keeping 
> the suspense high, I'll keep the rest for that user.
> 
> > +    r = comparebookmarks(repo, remotemarks, repo._bookmarks)
> >      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
> >
> >      incomings = []
> > @@ -590,16 +584,16 @@
> >              incomings.append("   %-25s %s\n" % (b, getid(id)))
> >      for b, scid, dcid in addsrc:
> >          # i18n: "added" refers to a bookmark
> > -        add(b, scid, _('added'))
> > +        add(b, hex(scid), _('added'))
> >      for b, scid, dcid in advsrc:
> >          # i18n: "advanced" refers to a bookmark
> > -        add(b, scid, _('advanced'))
> > +        add(b, hex(scid), _('advanced'))
> >      for b, scid, dcid in diverge:
> >          # i18n: "diverged" refers to a bookmark
> > -        add(b, scid, _('diverged'))
> > +        add(b, hex(scid), _('diverged'))
> >      for b, scid, dcid in differ:
> >          # i18n: "changed" refers to a bookmark
> > -        add(b, scid, _('changed'))
> > +        add(b, hex(scid), _('changed'))
> >
> >      if not incomings:
> >          ui.status(_("no changed bookmarks found\n"))
> > @@ -615,8 +609,8 @@
> >      '''
> >      ui.status(_("searching for changed bookmarks\n"))
> >
> > -    r = comparebookmarks(repo, repo._bookmarks, other.listkeys('bookmarks'),
> > -                         srchex=hex)
> > +    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
> > +    r = comparebookmarks(repo, repo._bookmarks, remotemarks)
> 
> This code is about 'hg outgoing', there is no bundling involved it is 
> probably more important to you to make it use the new method.
> 
> >      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
> >
> >      outgoings = []
> > @@ -632,19 +626,19 @@
> >              outgoings.append("   %-25s %s\n" % (b, getid(id)))
> >      for b, scid, dcid in addsrc:
> >          # i18n: "added refers to a bookmark
> > -        add(b, scid, _('added'))
> > +        add(b, hex(scid), _('added'))
> >      for b, scid, dcid in adddst:
> >          # i18n: "deleted" refers to a bookmark
> >          add(b, ' ' * 40, _('deleted'))
> >      for b, scid, dcid in advsrc:
> >          # i18n: "advanced" refers to a bookmark
> > -        add(b, scid, _('advanced'))
> > +        add(b, hex(scid), _('advanced'))
> >      for b, scid, dcid in diverge:
> >          # i18n: "diverged" refers to a bookmark
> > -        add(b, scid, _('diverged'))
> > +        add(b, hex(scid), _('diverged'))
> >      for b, scid, dcid in differ:
> >          # i18n: "changed" refers to a bookmark
> > -        add(b, scid, _('changed'))
> > +        add(b, hex(scid), _('changed'))
> >
> >      if not outgoings:
> >          ui.status(_("no changed bookmarks found\n"))
> > @@ -660,8 +654,8 @@
> >
> >      This returns "(# of incoming, # of outgoing)" tuple.
> >      '''
> > -    r = comparebookmarks(repo, other.listkeys('bookmarks'), repo._bookmarks,
> > -                         dsthex=hex)
> > +    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
> 
> That code is about 'hg summary', nothing too important here (except that 
> if we get with a better solution we should use it and get ride of the 
> old one)
> 
> > +    r = comparebookmarks(repo, remotemarks, repo._bookmarks)
> >      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
> >      return (len(addsrc), len(adddst))
> >
> > diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> > --- a/mercurial/exchange.py
> > +++ b/mercurial/exchange.py
> > @@ -603,9 +603,21 @@
> >      explicit = set([repo._bookmarks.expandname(bookmark)
> >                      for bookmark in pushop.bookmarks])
> >
> > -    comp = bookmod.comparebookmarks(repo, repo._bookmarks,
> > -                                    remotebookmark, srchex=hex)
> > +    remotebookmark = bookmod.unhexlifybookmarks(remotebookmark)
> 
> This is part of _pushdiscoverybookmarks (so the function that find what 
> bookmark operation needs to be done on push). This remote bookmark comes 
> from a listkeys call so it won't benefit of improvements you made with 
> your bookmark work. The way forward here seems to  be as follow:
> 
> 1) introduce a function responsible to get remote bookmark from SOMETHING.
> - That something is probably a peer as a start.
> - That function probably lives in bookmarks.py or exchanges.py
> 2) That function will check the peer capabilities and use the most 
> relevant option (nothing, listkeys or bundle2 part),
> 3) when using listkey the translation from hex to bin will be made 
> within the function,
> 4) All current users of listkeys for bookmarks are switched to this 
> function.
> 
> What do you think?
> 
> (Having that function will help your extensions (pulling patterns of 
> bookmarks) work)
>

Hmm, I don't really understand. Do you mean this function should accept
bookmarks to list? Smth like 'def getremotebooks(peer,
remotebookmarks)'?

> > +    comp = bookmod.comparebookmarks(repo, repo._bookmarks, remotebookmark)
> > +
> > +    def safehex(x):
> > +        if x is None:
> > +            return x
> > +        return hex(x)
> > +
> > +    def hexifycompbookmarks(bookmarks):
> > +        for b, scid, dcid in bookmarks:
> > +            yield b, safehex(scid), safehex(dcid)
> 
> (another fishy function, more later)
> 
> Translating from a list to a generator is bit dangerous, because only 
> one iteration can be made on that generator. It does not seems to 
> introduce bug as we use them only one. But I wanted to point it out as a 
> coding style.
>

Thanks, that makes sense

> > +
> > +    comp = [hexifycompbookmarks(marks) for marks in comp]
> >      addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = comp
> > +
> 
> It appears we are converting back to hex to store data into the push 
> operation. We should migrate that part to binary too.
> 
> >      for b, scid, dcid in advsrc:
> >          if b in explicit:
> >              explicit.remove(b)
> > @@ -617,7 +629,7 @@
> >              explicit.remove(b)
> >              pushop.outbookmarks.append((b, '', scid))
> >      # search for overwritten bookmark
> > -    for b, scid, dcid in advdst + diverge + differ:
> > +    for b, scid, dcid in list(advdst) + list(diverge) + list(differ):
> 
> (building lists directly would avoid some dancing here).
> 
> >          if b in explicit:
> >              explicit.remove(b)
> >              pushop.outbookmarks.append((b, dcid, scid))
> > @@ -1456,6 +1468,7 @@
> >      pullop.stepsdone.add('bookmarks')
> >      repo = pullop.repo
> >      remotebookmarks = pullop.remotebookmarks
> > +    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
> >      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
> >                               pullop.remote.url(),
> >                               pullop.gettransaction,
> 
> It looks like 'updatefromremote' is using hex internally too. We should 
> update it to use binary..

Will do

> 
> Cheers,
>

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -391,8 +391,7 @@ 
     finally:
         lockmod.release(tr, l, w)
 
-def comparebookmarks(repo, srcmarks, dstmarks,
-                     srchex=None, dsthex=None, targets=None):
+def comparebookmarks(repo, srcmarks, dstmarks, targets=None):
     '''Compare bookmarks between srcmarks and dstmarks
 
     This returns tuple "(addsrc, adddst, advsrc, advdst, diverge,
@@ -415,19 +414,9 @@ 
     Changeset IDs of tuples in "addsrc", "adddst", "differ" or
      "invalid" list may be unknown for repo.
 
-    This function expects that "srcmarks" and "dstmarks" return
-    changeset ID in 40 hexadecimal digit string for specified
-    bookmark. If not so (e.g. bmstore "repo._bookmarks" returning
-    binary value), "srchex" or "dsthex" should be specified to convert
-    into such form.
-
     If "targets" is specified, only bookmarks listed in it are
     examined.
     '''
-    if not srchex:
-        srchex = lambda x: x
-    if not dsthex:
-        dsthex = lambda x: x
 
     if targets:
         bset = set(targets)
@@ -449,14 +438,14 @@ 
     for b in sorted(bset):
         if b not in srcmarks:
             if b in dstmarks:
-                adddst((b, None, dsthex(dstmarks[b])))
+                adddst((b, None, dstmarks[b]))
             else:
                 invalid((b, None, None))
         elif b not in dstmarks:
-            addsrc((b, srchex(srcmarks[b]), None))
+            addsrc((b, srcmarks[b], None))
         else:
-            scid = srchex(srcmarks[b])
-            dcid = dsthex(dstmarks[b])
+            scid = srcmarks[b]
+            dcid = dstmarks[b]
             if scid == dcid:
                 same((b, scid, dcid))
             elif scid in repo and dcid in repo:
@@ -507,11 +496,17 @@ 
 
     return None
 
+def unhexlifybookmarks(marks):
+    binremotemarks = {}
+    for name, node in marks.items():
+        binremotemarks[name] = bin(node)
+    return binremotemarks
+
 def updatefromremote(ui, repo, remotemarks, path, trfunc, explicit=()):
     ui.debug("checking for updated bookmarks\n")
     localmarks = repo._bookmarks
     (addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same
-     ) = comparebookmarks(repo, remotemarks, localmarks, dsthex=hex)
+    ) = comparebookmarks(repo, remotemarks, localmarks)
 
     status = ui.status
     warn = ui.warn
@@ -522,15 +517,15 @@ 
     changed = []
     for b, scid, dcid in addsrc:
         if scid in repo: # add remote bookmarks for changes we already have
-            changed.append((b, bin(scid), status,
+            changed.append((b, scid, status,
                             _("adding remote bookmark %s\n") % (b)))
         elif b in explicit:
             explicit.remove(b)
             ui.warn(_("remote bookmark %s points to locally missing %s\n")
-                    % (b, scid[:12]))
+                    % (b, hex(scid)[:12]))
 
     for b, scid, dcid in advsrc:
-        changed.append((b, bin(scid), status,
+        changed.append((b, scid, status,
                         _("updating bookmark %s\n") % (b)))
     # remove normal movement from explicit set
     explicit.difference_update(d[0] for d in changed)
@@ -538,13 +533,12 @@ 
     for b, scid, dcid in diverge:
         if b in explicit:
             explicit.discard(b)
-            changed.append((b, bin(scid), status,
+            changed.append((b, scid, status,
                             _("importing bookmark %s\n") % (b)))
         else:
-            snode = bin(scid)
-            db = _diverge(ui, b, path, localmarks, snode)
+            db = _diverge(ui, b, path, localmarks, scid)
             if db:
-                changed.append((db, snode, warn,
+                changed.append((db, scid, warn,
                                 _("divergent bookmark %s stored as %s\n") %
                                 (b, db)))
             else:
@@ -553,13 +547,13 @@ 
     for b, scid, dcid in adddst + advdst:
         if b in explicit:
             explicit.discard(b)
-            changed.append((b, bin(scid), status,
+            changed.append((b, scid, status,
                             _("importing bookmark %s\n") % (b)))
     for b, scid, dcid in differ:
         if b in explicit:
             explicit.remove(b)
             ui.warn(_("remote bookmark %s points to locally missing %s\n")
-                    % (b, scid[:12]))
+                    % (b, hex(scid)[:12]))
 
     if changed:
         tr = trfunc()
@@ -573,8 +567,8 @@ 
     '''
     ui.status(_("searching for changed bookmarks\n"))
 
-    r = comparebookmarks(repo, other.listkeys('bookmarks'), repo._bookmarks,
-                         dsthex=hex)
+    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
+    r = comparebookmarks(repo, remotemarks, repo._bookmarks)
     addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
 
     incomings = []
@@ -590,16 +584,16 @@ 
             incomings.append("   %-25s %s\n" % (b, getid(id)))
     for b, scid, dcid in addsrc:
         # i18n: "added" refers to a bookmark
-        add(b, scid, _('added'))
+        add(b, hex(scid), _('added'))
     for b, scid, dcid in advsrc:
         # i18n: "advanced" refers to a bookmark
-        add(b, scid, _('advanced'))
+        add(b, hex(scid), _('advanced'))
     for b, scid, dcid in diverge:
         # i18n: "diverged" refers to a bookmark
-        add(b, scid, _('diverged'))
+        add(b, hex(scid), _('diverged'))
     for b, scid, dcid in differ:
         # i18n: "changed" refers to a bookmark
-        add(b, scid, _('changed'))
+        add(b, hex(scid), _('changed'))
 
     if not incomings:
         ui.status(_("no changed bookmarks found\n"))
@@ -615,8 +609,8 @@ 
     '''
     ui.status(_("searching for changed bookmarks\n"))
 
-    r = comparebookmarks(repo, repo._bookmarks, other.listkeys('bookmarks'),
-                         srchex=hex)
+    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
+    r = comparebookmarks(repo, repo._bookmarks, remotemarks)
     addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
 
     outgoings = []
@@ -632,19 +626,19 @@ 
             outgoings.append("   %-25s %s\n" % (b, getid(id)))
     for b, scid, dcid in addsrc:
         # i18n: "added refers to a bookmark
-        add(b, scid, _('added'))
+        add(b, hex(scid), _('added'))
     for b, scid, dcid in adddst:
         # i18n: "deleted" refers to a bookmark
         add(b, ' ' * 40, _('deleted'))
     for b, scid, dcid in advsrc:
         # i18n: "advanced" refers to a bookmark
-        add(b, scid, _('advanced'))
+        add(b, hex(scid), _('advanced'))
     for b, scid, dcid in diverge:
         # i18n: "diverged" refers to a bookmark
-        add(b, scid, _('diverged'))
+        add(b, hex(scid), _('diverged'))
     for b, scid, dcid in differ:
         # i18n: "changed" refers to a bookmark
-        add(b, scid, _('changed'))
+        add(b, hex(scid), _('changed'))
 
     if not outgoings:
         ui.status(_("no changed bookmarks found\n"))
@@ -660,8 +654,8 @@ 
 
     This returns "(# of incoming, # of outgoing)" tuple.
     '''
-    r = comparebookmarks(repo, other.listkeys('bookmarks'), repo._bookmarks,
-                         dsthex=hex)
+    remotemarks = unhexlifybookmarks(other.listkeys('bookmarks'))
+    r = comparebookmarks(repo, remotemarks, repo._bookmarks)
     addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
     return (len(addsrc), len(adddst))
 
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -603,9 +603,21 @@ 
     explicit = set([repo._bookmarks.expandname(bookmark)
                     for bookmark in pushop.bookmarks])
 
-    comp = bookmod.comparebookmarks(repo, repo._bookmarks,
-                                    remotebookmark, srchex=hex)
+    remotebookmark = bookmod.unhexlifybookmarks(remotebookmark)
+    comp = bookmod.comparebookmarks(repo, repo._bookmarks, remotebookmark)
+
+    def safehex(x):
+        if x is None:
+            return x
+        return hex(x)
+
+    def hexifycompbookmarks(bookmarks):
+        for b, scid, dcid in bookmarks:
+            yield b, safehex(scid), safehex(dcid)
+
+    comp = [hexifycompbookmarks(marks) for marks in comp]
     addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = comp
+
     for b, scid, dcid in advsrc:
         if b in explicit:
             explicit.remove(b)
@@ -617,7 +629,7 @@ 
             explicit.remove(b)
             pushop.outbookmarks.append((b, '', scid))
     # search for overwritten bookmark
-    for b, scid, dcid in advdst + diverge + differ:
+    for b, scid, dcid in list(advdst) + list(diverge) + list(differ):
         if b in explicit:
             explicit.remove(b)
             pushop.outbookmarks.append((b, dcid, scid))
@@ -1456,6 +1468,7 @@ 
     pullop.stepsdone.add('bookmarks')
     repo = pullop.repo
     remotebookmarks = pullop.remotebookmarks
+    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
     bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
                              pullop.remote.url(),
                              pullop.gettransaction,