[PATCH 02 of 15 V2] bookmark: add methods to binary encode and decode bookmark values

[Boris Feld]

On Fri, 2017-11-10 at 17:35 -0500, Augie Fackler wrote:
> (+indygreg, who also is a formats enthusiast)
> 
> On Thu, Nov 02, 2017 at 02:17:59PM +0100, Boris Feld wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld at octobus.net>
> > # Date 1508072395 -7200
> > #      Sun Oct 15 14:59:55 2017 +0200
> > # Node ID 4d0c6772a81aa1e2b25f32f944563db1f33fd327
> > # Parent  8c9a9eecdcd61401a1604a08a5272f7dabd4b912
> > # EXP-Topic b2.bookmarks
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-deve
> > l/ -r 4d0c6772a81a
> > bookmark: add methods to binary encode and decode bookmark values
> > 
> > Coming new bundle2 parts related to bookmark will use a binary
> > encoding. It
> > encodes a series of '(bookmark, node)' pairs. Bookmark name has a
> > high enough
> > size limit to not be affected by issue5165. (64K length, we are
> > well covered)
> 
> I'm not thrilled here. Could we do some sort of varint encoding,
> which
> would be generally useful going forward for a variety of things,
> rather than just shrugging and setting a limit which we (today) deem
> absurdly large?

We are not sure what the value of that would be. Bundle2 makes it very
easy to move to a newer encoding version if needed. We are already
doing so for changegroup and obsmarkers.

The biggest bookmark we have seen was about 800 characters, that should
leave us some room for the future without blocking us.

> 
> I agree that practically speaking, nobody should have a 64k bookmark,
> but we can do better. We also know that \0 shouldn't appear in a
> bookmark name, so we could just make the format be
> null-terminated. I'd much rather we get in the practice of
> deliberately crafting formats that are maximally flexible and
> reusable.

That would be significantly harder to parse since we would need to
search for the '\0' in the stream. We do not think it brings much value
either since any new data we sneak into the last field would have to be
understood by the receiver. If we need to negotiate for such
capabilities, this is equivalent to rolling out a new encoding.

> 
> IOW, encode something like this:
> 
> struct {
>   node [20]byte
>   name [] byte // continues until you see the first NUL
> }
> 
> Greg, am I talking crazy?
> 
> 
> > 
> > diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> > --- a/mercurial/bookmarks.py
> > +++ b/mercurial/bookmarks.py
> > @@ -8,12 +8,14 @@
> >  from __future__ import absolute_import
> > 
> >  import errno
> > +import struct
> > 
> >  from .i18n import _
> >  from .node import (
> >      bin,
> >      hex,
> >      short,
> > +    wdirid,
> >  )
> >  from . import (
> >      encoding,
> > @@ -550,6 +552,60 @@ def unhexlifybookmarks(marks):
> >          binremotemarks[name] = bin(node)
> >      return binremotemarks
> > 
> > +_binaryentry = struct.Struct('>20sH')
> > +
> > +def binaryencode(bookmarks):
> > +    """encode a '(bookmark, node)' iterable into a binary stream
> > +
> > +    the binary format is:
> > +
> > +        <node><bookmark-length><bookmark-name>
> > +
> > +    :node: is a 20 bytes binary node,
> > +    :bookmark-length: an unsigned short,
> > +    :bookmark-name: the name of the bookmark (of length <bookmark-
> > length>)
> > +
> > +    wdirid (all bits set) will be used as a special value for
> > "missing"
> > +    """
> > +    binarydata = []
> > +    for book, node in bookmarks:
> > +        if not node: # None or ''
> > +            node = wdirid
> > +        binarydata.append(_binaryentry.pack(node, len(book)))
> > +        binarydata.append(book)
> > +    return ''.join(binarydata)
> > +
> > +def binarydecode(stream):
> > +    """decode a binary stream into an '(bookmark, node)' iterable
> > +
> > +    the binary format is:
> > +
> > +        <node><bookmark-length><bookmark-name>
> > +
> > +    :node: is a 20 bytes binary node,
> > +    :bookmark-length: an unsigned short,
> > +    :bookmark-name: the name of the bookmark (of length <bookmark-
> > length>))
> > +
> > +    wdirid (all bits set) will be used as a special value for
> > "missing"
> > +    """
> > +    entrysize = _binaryentry.size
> > +    books = []
> > +    while True:
> > +        entry = stream.read(entrysize)
> > +        if len(entry) < entrysize:
> > +            if entry:
> > +                raise error.Abort(_('bad bookmark stream'))
> > +            break
> > +        node, length = _binaryentry.unpack(entry)
> > +        bookmark = stream.read(length)
> > +        if len(bookmark) < length:
> > +            if entry:
> > +                raise error.Abort(_('bad bookmark stream'))
> > +        if node == wdirid:
> > +            node = None
> > +        books.append((bookmark, node))
> > +    return books
> > +
> >  def updatefromremote(ui, repo, remotemarks, path, trfunc,
> > explicit=()):
> >      ui.debug("checking for updated bookmarks\n")
> >      localmarks = repo._bookmarks
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel