Re: [PATCH 2 of 2] bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)
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
> > # 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, d
Re: [PATCH 2 of 2] bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)
On 12/09/2016 12:31 PM, Stanislau Hlebik wrote:
# HG changeset patch
# User Stanislau Hlebik
# 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, s
Re: [PATCH 2 of 2] bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)
On Fri, Dec 09, 2016 at 03:31:45AM -0800, Stanislau Hlebik wrote: > # HG changeset patch > # User Stanislau Hlebik > # 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH 2 of 2] bookmarks: make bookmarks.comparebookmarks accept binary nodes (API)
# HG changeset patch
# User Stanislau Hlebik
# 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.
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")
-
