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

Stanislau Hlebik stash at fb.com
Tue Dec 20 08:37:28 EST 2016


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 at 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,
> 


More information about the Mercurial-devel mailing list