Patchwork [3,of,8,V11] bookmarks: make bookmarks.compare accept binary nodes (API)

login
register
mail settings
Submitter Stanislau Hlebik
Date Nov. 22, 2016, 10:17 a.m.
Message ID <36d83269edc51168908c.1479809865@devvm957.lla2.facebook.com>
Download mbox | patch
Permalink /patch/17693/
State Superseded
Headers show

Comments

Stanislau Hlebik - Nov. 22, 2016, 10:17 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1479807361 28800
#      Tue Nov 22 01:36:01 2016 -0800
# Node ID 36d83269edc51168908c9b1160e22282ab6c5c04
# Parent  20e2f13b7a1083f8d44a7a9e554eb3d5735b80f2
bookmarks: make bookmarks.compare accept binary nodes (API)

New `bookmarks` bundle2 part will contain byte nodes, and since bookmarks are
stored in binary format, it doesn't make sense to convert them from binary to
hex and then back to binary again
Gregory Szorc - Nov. 30, 2016, 4:40 a.m.
On Tue, Nov 22, 2016 at 2:17 AM, Stanislau Hlebik <stash@fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1479807361 28800
> #      Tue Nov 22 01:36:01 2016 -0800
> # Node ID 36d83269edc51168908c9b1160e22282ab6c5c04
> # Parent  20e2f13b7a1083f8d44a7a9e554eb3d5735b80f2
> bookmarks: make bookmarks.compare accept binary nodes (API)
>

Nit: commit message needs updating to reflect renamed function.


>
> New `bookmarks` bundle2 part will contain byte nodes, and since bookmarks
> are
> stored in binary format, it doesn't make sense to convert them from binary
> to
> hex and then back to binary again
>
> 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 hexifybookmarks(marks):
> +    binremotemarks = {}
> +    for name, node in marks.items():
> +        binremotemarks[name] = bin(node)
> +    return binremotemarks
> +
>

"hexify" in a name that actually "unhexifies" is a bit confusing. "unhex"
is not a word. But we use "unhexlify" already in the code base. So this
should be "unhexlifybookmarks." I'm sorry to make you go through yet
another iteration. This series is getting close though...


>  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 = hexifybookmarks(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 = hexifybookmarks(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 = hexifybookmarks(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.hexifybookmarks(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.hexifybookmarks(remotebookmarks)
>      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
>                               pullop.remote.url(),
>                               pullop.gettransaction,
>
Pierre-Yves David - Dec. 3, 2016, 3:40 a.m.
On 11/30/2016 05:40 AM, Gregory Szorc wrote:
> On Tue, Nov 22, 2016 at 2:17 AM, Stanislau Hlebik <stash@fb.com> wrote:
>
>> # HG changeset patch
>> # User Stanislau Hlebik <stash@fb.com>
>> # Date 1479807361 28800
>> #      Tue Nov 22 01:36:01 2016 -0800
>> # Node ID 36d83269edc51168908c9b1160e22282ab6c5c04
>> # Parent  20e2f13b7a1083f8d44a7a9e554eb3d5735b80f2
>> bookmarks: make bookmarks.compare accept binary nodes (API)
>>
>
> Nit: commit message needs updating to reflect renamed function.
>
>
>>
>> New `bookmarks` bundle2 part will contain byte nodes, and since bookmarks
>> are
>> stored in binary format, it doesn't make sense to convert them from binary
>> to
>> hex and then back to binary again
>>
>> 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 hexifybookmarks(marks):
>> +    binremotemarks = {}
>> +    for name, node in marks.items():
>> +        binremotemarks[name] = bin(node)
>> +    return binremotemarks
>> +
>>
>
> "hexify" in a name that actually "unhexifies" is a bit confusing. "unhex"
> is not a word. But we use "unhexlify" already in the code base. So this
> should be "unhexlifybookmarks." I'm sorry to make you go through yet
> another iteration. This series is getting close though...

In the rest of mercurial, we have functions that 'hex' 'bin', we can 
probably reuse this wording here.

(I've not looked at anything else this series yet)


>>  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 = hexifybookmarks(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 = hexifybookmarks(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 = hexifybookmarks(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.hexifybookmarks(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.hexifybookmarks(remotebookmarks)
>>      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
>>                               pullop.remote.url(),
>>                               pullop.gettransaction,
>>
>

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 hexifybookmarks(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 = hexifybookmarks(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 = hexifybookmarks(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 = hexifybookmarks(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.hexifybookmarks(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.hexifybookmarks(remotebookmarks)
     bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
                              pullop.remote.url(),
                              pullop.gettransaction,