[PATCH 07 of 10 V9] exchange: getbundle `bookmarks` part generator

[Stanislau Hlebik]

On Sun, Nov 13, 2016 at 2:30 AM, Stanislau Hlebik <stash at fb.com<mailto:stash at fb.com>> wrote:
# HG changeset patch
# User Stanislau Hlebik <stash at fb.com<mailto:stash at fb.com>>
# Date 1479032456 28800
#      Sun Nov 13 02:20:56 2016 -0800
# Branch stable
# Node ID 606bb4a7fb818f24d52e764828ba0d0a7921f999
# Parent  bf21586f26e5a41f7d8bf342d4b4c16d71dbc6d2
exchange: getbundle `bookmarks` part generator

This generator will be used during pull operation.

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1680,6 +1680,21 @@
     if chunks:
         bundler.newpart('hgtagsfnodes', data=''.join(chunks))

+ at getbundle2partsgenerator('bookmarks')
+def _getbundlebookmarkspart(bundler, repo, source, bundlecaps=None,
+                            b2caps=None, heads=None, common=None,
+                            **kwargs):
+    if not kwargs.get('bookmarks'):
+        return
+    if 'bookmarks' not in b2caps:
+        raise ValueError(
+            _('bookmarks are requested but client is not capable '
+              'of receiving it'))
+
+    bookmarks = _getbookmarks(repo, **kwargs)
+    encodedbookmarks = bookmod.encodebookmarks(bookmarks)
+    bundler.newpart('bookmarks', data=encodedbookmarks)
+
 def _getbookmarks(repo, **kwargs):
     """Returns bookmark to node mapping.

diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -228,7 +228,9 @@
              'bundlecaps': 'scsv',
              'listkeys': 'csv',
              'cg': 'boolean',
-             'cbattempted': 'boolean'}
+             'cbattempted': 'boolean',
+             'bookmarks': 'boolean',
+            }

IIRC there were patches submitted last cycle around bookmark filtering - clients saying which bookmarks they were interested in. This implies there is advanced functionality around bookmarks exchange around the corner. So I have reservations about a "bookmarks" argument that is simply a boolean and doesn't allow more advanced uses later.
Anyway, does this argument to the "getbundle" wire protocol command need to exist at all? Today, clients can ask for bookmarks data by adding a "bookmarks" value to the "listkeys" getbundle wire protocol argument. If the client sent a bundle2 capability that indicated it can receive the "bookmarks" dedicated bundle2 part, then we wouldn't need to introduce a new argument to "getbundle."
That being said, I can go both ways on this. Sometimes having an explicit argument for "send me X" is nice to have for clarity. My final opinion likely hinges on what other bookmarks exchange features are planned. If "send a subset of bookmarks" or a similar feature is in the works, I'd prefer to shoehorn that into the same getbundle argument so there is only 1 variable controlling how bookmarks are sent.


Are you talking about patches like this one - https://patchwork.mercurial-scm.org/patch/16392/ ?
If yes then these patches are abandoned in favor of this series with separate `bookmarks` bundle2 part.
So in this case having a separate parameter is probably fine.



 # client side

_______________________________________________
Mercurial-devel mailing list
Mercurial-devel at mercurial-scm.org<mailto:Mercurial-devel at mercurial-scm.org>
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel<https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DgMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=1EQ58Dmb5uX1qHujcsT1Mg&m=8qvKc-Wfp63n5Hm-gElNd1-jLtfrPpY3gcb2FGjc21I&s=S64g5yxUkkES-cW4wiutUC_jJD2xwXVdiTfnhd8CzF4&e=>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161116/ab838c8e/attachment-0001.html>