Patchwork [1,of,9,V6] exchange: add `_getbookmarks()` function

login
register
mail settings
Submitter Stanislau Hlebik
Date Oct. 11, 2016, 4:25 p.m.
Message ID <55e997127023d7208488.1476203143@dev1918.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17033/
State Changes Requested
Headers show

Comments

Stanislau Hlebik - Oct. 11, 2016, 4:25 p.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1476195835 25200
#      Tue Oct 11 07:23:55 2016 -0700
# Node ID 55e997127023d7208488c593adb933a1bfb23312
# Parent  b85fa6bf298be07804a74d8fdec0d19fdbc6d740
exchange: add `_getbookmarks()` function

This function will be used to generate bookmarks bundle2 part.
It is a separate function in order to make it easy to overwrite it
in extensions. Passing `kwargs` to the function makes it easy to
add new parameters in extensions.
Pierre-Yves David - Oct. 13, 2016, 4 p.m.
On 10/11/2016 06:25 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1476195835 25200
> #      Tue Oct 11 07:23:55 2016 -0700
> # Node ID 55e997127023d7208488c593adb933a1bfb23312
> # Parent  b85fa6bf298be07804a74d8fdec0d19fdbc6d740
> exchange: add `_getbookmarks()` function
>
> This function will be used to generate bookmarks bundle2 part.
> It is a separate function in order to make it easy to overwrite it
> in extensions. Passing `kwargs` to the function makes it easy to
> add new parameters in extensions.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1672,6 +1672,17 @@
>      if chunks:
>          bundler.newpart('hgtagsfnodes', data=''.join(chunks))
>
> +def _getbookmarks(repo, **kwargs):
> +    """Returns list of bookmarks.
> +
> +    This function is primarily used to generate `bookmarks` bundle2 part.
> +    It is a separate function in order to make it easy to wrap it
> +    in extensions. Passing `kwargs` to the function makes it easy to
> +    add new parameters in extensions.
> +    """
> +
> +    return repo.listkeys(namespace='bookmarks')

I find it quite suspicious that we need to get through listkeys to get 
the list of bookmarks. Instead I would expect us to have a function 
ready to use in the bookmark module itself. Can you look into this ?

Cheers,
Pierre-Yves David - Oct. 16, 2016, 9:54 a.m.
On 10/11/2016 06:25 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1476195835 25200
> #      Tue Oct 11 07:23:55 2016 -0700
> # Node ID 55e997127023d7208488c593adb933a1bfb23312
> # Parent  b85fa6bf298be07804a74d8fdec0d19fdbc6d740
> exchange: add `_getbookmarks()` function

I was thinking about the various in-flight series and it made me 
realized that the various important point about this bookmarks series 
are spread in various emails and might be a bit hard to follow.

Since I still had them in my head I've made a summary of the important 
point that needs to be addressed or discussed. Since the 4.0 feature 
freeze is Tuesday, this will probably not reach conclusion this cycle.

- Do not go through pushkey to list the bookmarks
   (using a function from mercurial.bookmarks seems the way to go),

- either drop the dedicated  exception classes (BookmarksEncodeError and 
BookmarksDecodeError) or make it clear why we need them,

- write internal documentations about the new parts and the process 
around them in mercurial/helps/internals/,

- double check the binary encoding with Greg. I personnaly think we 
should drop the optional 'nodeid' and include one in all case,

- introduce a new attributes on the unbundle2 to pass "inputs" around 
(instead of our previous approach of using dedicated attributes for each)

(there is also a small number of various questions and comments inline 
but they seems less important)

Cheers,

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1672,6 +1672,17 @@ 
     if chunks:
         bundler.newpart('hgtagsfnodes', data=''.join(chunks))
 
+def _getbookmarks(repo, **kwargs):
+    """Returns list of bookmarks.
+
+    This function is primarily used to generate `bookmarks` bundle2 part.
+    It is a separate function in order to make it easy to wrap it
+    in extensions. Passing `kwargs` to the function makes it easy to
+    add new parameters in extensions.
+    """
+
+    return repo.listkeys(namespace='bookmarks')
+
 def check_heads(repo, their_heads, context):
     """check if the heads of a repo have been modified