Patchwork [1,of,2] bookmarks: don't use bookmarks.listbookmarks in local computations

login
register
mail settings
Submitter Kevin Bullock
Date Jan. 27, 2013, 9:15 p.m.
Message ID <14fe65a26547fe63963f.1359321358@vpn0-813.vpn.umn.edu>
Download mbox | patch
Permalink /patch/747/
State Accepted
Commit 8260fa9f30b952048ed8961bbb76a62204334b98
Headers show

Comments

Kevin Bullock - Jan. 27, 2013, 9:15 p.m.
# HG changeset patch
# User Kevin Bullock <kbullock@ringworld.org>
# Date 1359318277 21600
# Branch stable
# Node ID 14fe65a26547fe63963f4e6062b26f07a9443860
# Parent  b404fa103253960824500066dffa2b7d6cff33ee
bookmarks: don't use bookmarks.listbookmarks in local computations

bookmarks.listbookmarks is for wire-protocol use. The normal way to get
all the bookmarks on a local repository is repo._bookmarks.
Pierre-Yves David - Jan. 27, 2013, 11:59 p.m.
On 27 janv. 2013, at 22:15, Kevin Bullock wrote:

> # HG changeset patch
> # User Kevin Bullock <kbullock@ringworld.org>
> # Date 1359318277 21600
> # Branch stable
> # Node ID 14fe65a26547fe63963f4e6062b26f07a9443860
> # Parent  b404fa103253960824500066dffa2b7d6cff33ee
> bookmarks: don't use bookmarks.listbookmarks in local computations
> 
> bookmarks.listbookmarks is for wire-protocol use. The normal way to get
> all the bookmarks on a local repository is repo._bookmarks.

repo._bookmarks.values() and bookmarks.listbookmarks(repo) have different returns.
(divergent bookmark are excluded from listbookmarks)

I would make sense to test that neither filtering nor revset ignore them.

Patches (1 and 2) looks good to me otherwise.

> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -9,7 +9,7 @@
> import copy
> import phases
> import util
> -import obsolete, bookmarks, revset
> +import obsolete, revset
> 
> 
> def hideablerevs(repo):
> @@ -32,7 +32,7 @@ def computehidden(repo):
>                       if r not in hideable]
>         for par in repo[None].parents():
>             blockers.append(par.rev())
> -        for bm in bookmarks.listbookmarks(repo).values():
> +        for bm in repo._bookmarks.values():
>             blockers.append(repo[bm].rev())
>         blocked = cl.ancestors(blockers, inclusive=True)
>         return frozenset(r for r in hideable if r not in blocked)
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -8,7 +8,6 @@
> import re
> import parser, util, error, discovery, hbisect, phases
> import node
> -import bookmarks as bookmarksmod
> import match as matchmod
> from i18n import _
> import encoding
> @@ -375,14 +374,14 @@ def bookmark(repo, subset, x):
>                        _('the argument to bookmark must be a string'))
>         kind, pattern, matcher = _stringmatcher(bm)
>         if kind == 'literal':
> -            bmrev = bookmarksmod.listbookmarks(repo).get(bm, None)
> +            bmrev = repo._bookmarks.get(bm, None)
>             if not bmrev:
>                 raise util.Abort(_("bookmark '%s' does not exist") % bm)
>             bmrev = repo[bmrev].rev()
>             return [r for r in subset if r == bmrev]
>         else:
>             matchrevs = set()
> -            for name, bmrev in bookmarksmod.listbookmarks(repo).iteritems():
> +            for name, bmrev in repo._bookmarks.iteritems():
>                 if matcher(name):
>                     matchrevs.add(bmrev)
>             if not matchrevs:
> @@ -394,7 +393,7 @@ def bookmark(repo, subset, x):
>             return [r for r in subset if r in bmrevs]
> 
>     bms = set([repo[r].rev()
> -               for r in bookmarksmod.listbookmarks(repo).values()])
> +               for r in repo._bookmarks.values()])
>     return [r for r in subset if r in bms]
> 
> def branch(repo, subset, x):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Jan. 29, 2013, 7:55 a.m.
On Mon, 2013-01-28 at 21:24 -0600, Kevin Bullock wrote:
> On 27 Jan 2013, at 5:59 PM, Pierre-Yves David wrote:
> 
> > On 27 janv. 2013, at 22:15, Kevin Bullock wrote:
> > 
> >> # HG changeset patch
> >> # User Kevin Bullock <kbullock@ringworld.org>
> >> # Date 1359318277 21600
> >> # Branch stable
> >> # Node ID 14fe65a26547fe63963f4e6062b26f07a9443860
> >> # Parent  b404fa103253960824500066dffa2b7d6cff33ee
> >> bookmarks: don't use bookmarks.listbookmarks in local computations
> >> 
> >> bookmarks.listbookmarks is for wire-protocol use. The normal way to get
> >> all the bookmarks on a local repository is repo._bookmarks.
> > 
> > repo._bookmarks.values() and bookmarks.listbookmarks(repo) have different returns.
> > (divergent bookmark are excluded from listbookmarks)
> > 
> > I would make sense to test that neither filtering nor revset ignore them.
> > 
> > Patches (1 and 2) looks good to me otherwise.
> 
> Crewed the series, along with a test that bookmarks prevent changesets
> from being hidden (didn't appear to already be one). Thanks for the
> review.

Is there a bug that needs to be marked testing for this?
Matt Mackall - Jan. 29, 2013, 8:49 p.m.
On Tue, 2013-01-29 at 09:18 -0600, Kevin Bullock wrote:
> On 29 Jan 2013, at 1:55 AM, Matt Mackall wrote:
> 
> > On Mon, 2013-01-28 at 21:24 -0600, Kevin Bullock wrote:
> >> On 27 Jan 2013, at 5:59 PM, Pierre-Yves David wrote:
> >> 
> >>> On 27 janv. 2013, at 22:15, Kevin Bullock wrote:
> >>> 
> >>>> # HG changeset patch
> >>>> # User Kevin Bullock <kbullock@ringworld.org>
> >>>> # Date 1359318277 21600
> >>>> # Branch stable
> >>>> # Node ID 14fe65a26547fe63963f4e6062b26f07a9443860
> >>>> # Parent  b404fa103253960824500066dffa2b7d6cff33ee
> >>>> bookmarks: don't use bookmarks.listbookmarks in local computations
> >>>> 
> >>>> bookmarks.listbookmarks is for wire-protocol use. The normal way to get
> >>>> all the bookmarks on a local repository is repo._bookmarks.
> >>> 
> >>> repo._bookmarks.values() and bookmarks.listbookmarks(repo) have different returns.
> >>> (divergent bookmark are excluded from listbookmarks)
> >>> 
> >>> I would make sense to test that neither filtering nor revset ignore them.
> >>> 
> >>> Patches (1 and 2) looks good to me otherwise.
> >> 
> >> Crewed the series, along with a test that bookmarks prevent changesets
> >> from being hidden (didn't appear to already be one). Thanks for the
> >> review.
> > 
> > Is there a bug that needs to be marked testing for this?
> 
> Pretty sure there's not. I just noticed it while tracking down the
> issue fixed in the following patch. How likely is it that someone
> would have a divergent bookmark on a secret changeset?

Dunno, just making sure we didn't fail to connect some dots.

Patch

diff --git a/mercurial/repoview.py b/mercurial/repoview.py
--- a/mercurial/repoview.py
+++ b/mercurial/repoview.py
@@ -9,7 +9,7 @@ 
 import copy
 import phases
 import util
-import obsolete, bookmarks, revset
+import obsolete, revset
 
 
 def hideablerevs(repo):
@@ -32,7 +32,7 @@  def computehidden(repo):
                       if r not in hideable]
         for par in repo[None].parents():
             blockers.append(par.rev())
-        for bm in bookmarks.listbookmarks(repo).values():
+        for bm in repo._bookmarks.values():
             blockers.append(repo[bm].rev())
         blocked = cl.ancestors(blockers, inclusive=True)
         return frozenset(r for r in hideable if r not in blocked)
diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -8,7 +8,6 @@ 
 import re
 import parser, util, error, discovery, hbisect, phases
 import node
-import bookmarks as bookmarksmod
 import match as matchmod
 from i18n import _
 import encoding
@@ -375,14 +374,14 @@  def bookmark(repo, subset, x):
                        _('the argument to bookmark must be a string'))
         kind, pattern, matcher = _stringmatcher(bm)
         if kind == 'literal':
-            bmrev = bookmarksmod.listbookmarks(repo).get(bm, None)
+            bmrev = repo._bookmarks.get(bm, None)
             if not bmrev:
                 raise util.Abort(_("bookmark '%s' does not exist") % bm)
             bmrev = repo[bmrev].rev()
             return [r for r in subset if r == bmrev]
         else:
             matchrevs = set()
-            for name, bmrev in bookmarksmod.listbookmarks(repo).iteritems():
+            for name, bmrev in repo._bookmarks.iteritems():
                 if matcher(name):
                     matchrevs.add(bmrev)
             if not matchrevs:
@@ -394,7 +393,7 @@  def bookmark(repo, subset, x):
             return [r for r in subset if r in bmrevs]
 
     bms = set([repo[r].rev()
-               for r in bookmarksmod.listbookmarks(repo).values()])
+               for r in repo._bookmarks.values()])
     return [r for r in subset if r in bms]
 
 def branch(repo, subset, x):