| 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
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 >
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):