Patchwork [02,of,10,V8] bookmarks: introduce listbookmarks()

login
register
mail settings
Submitter Stanislau Hlebik
Date Nov. 12, 2016, 8:19 p.m.
Message ID <13b3e16c68d303a52355.1478981992@dev1918.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17517/
State Superseded
Headers show

Comments

Stanislau Hlebik - Nov. 12, 2016, 8:19 p.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1478980589 28800
#      Sat Nov 12 11:56:29 2016 -0800
# Branch stable
# Node ID 13b3e16c68d303a523550980d7739bb676420851
# Parent  fe9e78883230a875ae7acf6c7a5b324c1d9016f5
bookmarks: introduce listbookmarks()

`bookmarks` bundle2 part will work with binary nodes.
To avoid unnecessary conversions between binary and hex nodes
let's add `listbookmarks()` that returns binary nodes.
Gregory Szorc - Nov. 12, 2016, 11:41 p.m.
On Sat, Nov 12, 2016 at 12:19 PM, Stanislau Hlebik <stash@fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1478980589 28800
> #      Sat Nov 12 11:56:29 2016 -0800
> # Branch stable
> # Node ID 13b3e16c68d303a523550980d7739bb676420851
> # Parent  fe9e78883230a875ae7acf6c7a5b324c1d9016f5
> bookmarks: introduce listbookmarks()
>
> `bookmarks` bundle2 part will work with binary nodes.
> To avoid unnecessary conversions between binary and hex nodes
> let's add `listbookmarks()` that returns binary nodes.
>

Between this and part 1, you've introduced API compatibility breakage
without an "(API)" annotation in the commit message.

I'm not sure why you've done it this way. Introducing `listbookmarksbin()`
gets you to a near identical end state without the API breakage.

I'm -0 on the current approach.


>
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -284,17 +284,21 @@
>              lockmod.release(tr, lock)
>      return update
>
> -def listhexbookmarks(repo):
> +def listbookmarks(repo):
>      # We may try to list bookmarks on a repo type that does not
>      # support it (e.g., statichttprepository).
>      marks = getattr(repo, '_bookmarks', {})
>
> -    d = {}
>      hasnode = repo.changelog.hasnode
>      for k, v in marks.iteritems():
>          # don't expose local divergent bookmarks
>          if hasnode(v) and ('@' not in k or k.endswith('@')):
> -            d[k] = hex(v)
> +            yield k, v
> +
> +def listhexbookmarks(repo):
> +    d = {}
> +    for book, node in listbookmarks(repo):
> +        d[book] = hex(node)
>      return d
>
>  def pushbookmark(repo, key, old, new):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Nov. 15, 2016, 2:22 p.m.
On 11/12/2016 11:41 PM, Gregory Szorc wrote:
> On Sat, Nov 12, 2016 at 12:19 PM, Stanislau Hlebik <stash@fb.com
> <mailto:stash@fb.com>> wrote:
>
>     # HG changeset patch
>     # User Stanislau Hlebik <stash@fb.com <mailto:stash@fb.com>>
>     # Date 1478980589 28800
>     #      Sat Nov 12 11:56:29 2016 -0800
>     # Branch stable
>     # Node ID 13b3e16c68d303a523550980d7739bb676420851
>     # Parent  fe9e78883230a875ae7acf6c7a5b324c1d9016f5
>     bookmarks: introduce listbookmarks()
>
>     `bookmarks` bundle2 part will work with binary nodes.
>     To avoid unnecessary conversions between binary and hex nodes
>     let's add `listbookmarks()` that returns binary nodes.
>
>
> Between this and part 1, you've introduced API compatibility breakage
> without an "(API)" annotation in the commit message.
>
> I'm not sure why you've done it this way. Introducing
> `listbookmarksbin()` gets you to a near identical end state without the
> API breakage.

Having a different function name seems like a good idea. We could add a 
deprecation warning on the old one. (and drop it when due)

Cheers,

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -284,17 +284,21 @@ 
             lockmod.release(tr, lock)
     return update
 
-def listhexbookmarks(repo):
+def listbookmarks(repo):
     # We may try to list bookmarks on a repo type that does not
     # support it (e.g., statichttprepository).
     marks = getattr(repo, '_bookmarks', {})
 
-    d = {}
     hasnode = repo.changelog.hasnode
     for k, v in marks.iteritems():
         # don't expose local divergent bookmarks
         if hasnode(v) and ('@' not in k or k.endswith('@')):
-            d[k] = hex(v)
+            yield k, v
+
+def listhexbookmarks(repo):
+    d = {}
+    for book, node in listbookmarks(repo):
+        d[book] = hex(node)
     return d
 
 def pushbookmark(repo, key, old, new):