Patchwork [1,of,3,RFC] changelog: handle obsolete and secret changesets more gracefully

login
register
mail settings
Submitter Dan Villiom Podlaski Christiansen
Date Aug. 8, 2013, 7:48 a.m.
Message ID <02c24ad6f4d2d954da11.1375948124@dookie.local>
Download mbox | patch
Permalink /patch/2092/
State Changes Requested
Headers show

Comments

Dan Villiom Podlaski Christiansen - Aug. 8, 2013, 7:48 a.m.
# HG changeset patch
# User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
# Date 1375817670 -7200
#      Tue Aug 06 21:34:30 2013 +0200
# Node ID 02c24ad6f4d2d954da11d128280978e25aaa48fd
# Parent  df2155ebf502d14f7ef5276df806a35fc06dabd5
changelog: handle obsolete and secret changesets more gracefully

We now issue a 'changeset is hidden' message when the user attempts to
access a hidden (i.e. secret or obsolete) changeset.

Please notice that this introduces a slight information leak, as
knowledge of a 'secret' hash may now be used to test whether it's
present in a given repository.
Augie Fackler - Aug. 9, 2013, 3:10 p.m.
On Thu, Aug 08, 2013 at 09:48:44AM +0200, Dan Villiom Podlaski Christiansen wrote:
> # HG changeset patch
> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
> # Date 1375817670 -7200
> #      Tue Aug 06 21:34:30 2013 +0200
> # Node ID 02c24ad6f4d2d954da11d128280978e25aaa48fd
> # Parent  df2155ebf502d14f7ef5276df806a35fc06dabd5
> changelog: handle obsolete and secret changesets more gracefully
>
> We now issue a 'changeset is hidden' message when the user attempts to
> access a hidden (i.e. secret or obsolete) changeset.
>
> Please notice that this introduces a slight information leak, as
> knowledge of a 'secret' hash may now be used to test whether it's
> present in a given repository.

I mean, on the one hand yes, but on the other, the user has that
change somehow, so it's only leaking information to themselves? They
could always run log --hidden and see these changes, right?

>
> diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
> --- a/mercurial/branchmap.py
> +++ b/mercurial/branchmap.py
> @@ -7,7 +7,7 @@

[...]

>
>      def delayupdate(self):
> diff --git a/mercurial/error.py b/mercurial/error.py
> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -59,6 +59,14 @@ class RepoError(Exception):
>  class RepoLookupError(RepoError):
>      pass
>
> +class FilteredLookupError(RepoLookupError):
> +    """Exception raised on accessing filtered (i.e. hidden) changesets."""
> +
> +    def __init__(self, rev, message):
> +        self.rev = rev
> +
> +        RepoLookupError.__init__(self, message)

While it almost certainly doesn't matter in this case, please call the
superclass init before doing your own init work. Also, consider using
the super(type, self) construct here?

> +
>  class CapabilityError(RepoError):
>      pass
>
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -279,7 +279,7 @@ class revlog(object):
>          self.rev(self.node(0))
>          return self._nodecache
>
> -    def hasnode(self, node):
> +    def hasnode(self, node, hidden=False):
>          try:
>              self.rev(node)
>              return True
> @@ -752,7 +752,7 @@ class revlog(object):
>      def _partialmatch(self, id):
>          try:
>              n = self.index.partialmatch(id)
> -            if n and self.hasnode(n):
> +            if n and self.hasnode(n, hidden=True):
>                  return n
>              return None
>          except RevlogError:
> diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
> --- a/tests/test-hgweb-commands.t
> +++ b/tests/test-hgweb-commands.t
> @@ -1390,7 +1390,7 @@ proper status for filtered revision
>    Content-Type: text/plain; charset=ascii\r (esc)
>    \r (esc)
>
> -  error: unknown revision '5'
> +  error: revision 5 is hidden
>
>
>
> @@ -1404,7 +1404,7 @@ proper status for filtered revision
>    Content-Type: text/plain; charset=ascii\r (esc)
>    \r (esc)
>
> -  error: unknown revision '4'
> +  error: revision 4 is hidden
>
>  filtered '0' changeset
>
> diff --git a/tests/test-log.t b/tests/test-log.t
> --- a/tests/test-log.t
> +++ b/tests/test-log.t
> @@ -1226,6 +1226,15 @@ enable obsolete to test hidden feature
>    $ hg log -r a
>    abort: unknown revision 'a'!
>    [255]
> +  $ hg log -r a7
> +  abort: unknown revision 'a7'!
> +  [255]
> +  $ hg log -r a76
> +  abort: unknown revision 'a76'!
> +  [255]
> +  $ hg log -r a765
> +  abort: changeset a765632148dc is hidden!
> +  [255]
>
>  test that parent prevent a changeset to be hidden
>
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -211,7 +211,7 @@ check that various commands work well wi
>    abort: unknown revision '6'!
>    [255]
>    $ hg log -r 4
> -  abort: unknown revision '4'!
> +  abort: revision 4 is hidden!
>    [255]
>
>  Check that public changeset are not accounted as obsolete:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Aug. 15, 2013, 11:43 p.m.
On 8 août 2013, at 09:48, Dan Villiom Podlaski Christiansen wrote:

> # HG changeset patch
> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
> # Date 1375817670 -7200
> #      Tue Aug 06 21:34:30 2013 +0200
> # Node ID 02c24ad6f4d2d954da11d128280978e25aaa48fd
> # Parent  df2155ebf502d14f7ef5276df806a35fc06dabd5
> changelog: handle obsolete and secret changesets more gracefully

repoview: introduce specialized exception for filtered lookup

> 
> We now issue a 'changeset is hidden' message when the user attempts to
> access a hidden (i.e. secret or obsolete) changeset.

"hidden" sounds wrong we have multiple level of filtering. A more generic message stating that "changeset is filtered" may be a simpler first step.

You probably want to add a hint toward the "--hidden" flag at some point too (not in this changeset)

> Please notice that this introduces a slight information leak, as
> knowledge of a 'secret' hash may now be used to test whether it's
> present in a given repository.

I do not mind much information leak of the secret changeset, but I know some people do. Sensible code could double check the phases of the phases before printing information.

> diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
> --- a/mercurial/branchmap.py
> +++ b/mercurial/branchmap.py
> @@ -7,7 +7,7 @@
> 
> from node import bin, hex, nullid, nullrev
> import encoding
> -import util, repoview
> +import error, util, repoview
> 
> def _filename(repo):
>     """name of a branchcache file for a given repo or repoview"""
> @@ -121,7 +121,7 @@ class branchcache(dict):
>         try:
>             return ((self.tipnode == repo.changelog.node(self.tiprev))
>                     and (self.filteredhash == self._hashfiltered(repo)))
> -        except IndexError:
> +        except (IndexError, error.FilteredLookupError):

Your new exception -must- be a super class of the original exception. Then code that cares about the distinction may detect it and all other code can keep working unchanged.

>             return False
> 
>     def copy(self):
> diff --git a/mercurial/changelog.py b/mercurial/changelog.py
> --- a/mercurial/changelog.py
> +++ b/mercurial/changelog.py
> @@ -5,7 +5,7 @@
> # This software may be used and distributed according to the terms of the
> # GNU General Public License version 2 or any later version.
> 
> -from node import bin, hex, nullid
> +from node import bin, hex, nullid, short
> from i18n import _
> import util, error, revlog, encoding
> 
> @@ -159,12 +159,15 @@ class changelog(revlog.revlog):
>         self.rev(self.node(0))
>         return self._nodecache
> 
> -    def hasnode(self, node):
> +    def hasnode(self, node, hidden=False):
>         """filtered version of revlog.hasnode"""
> +        if hidden:
> +            return super(changelog, self).hasnode(node)
> +

Disabling filtering should be done using an unfiltered version instead. (1) we have more that the "visible" view. (2) we can't add such argument everywhere.

>         try:
>             i = self.rev(node)
>             return i not in self.filteredrevs
> -        except KeyError:
> +        except (KeyError, error.FilteredLookupError):

Same as above. raise a super class of KeyError

>             return False
> 
>     def headrevs(self):
> @@ -183,31 +186,36 @@ class changelog(revlog.revlog):
>         """filtered version of revlog.rev"""
>         r = super(changelog, self).rev(node)
>         if r in self.filteredrevs:
> -            raise error.LookupError(hex(node), self.indexfile, _('no node'))
> +            msg = _('changeset %s is hidden') % short(node)
> +            raise error.FilteredLookupError(r, msg)

(forging the message here is probably useless. useful information is probably <node> + <filtername>)

>         return r
> 
>     def node(self, rev):
>         """filtered version of revlog.node"""
>         if rev in self.filteredrevs:
> -            raise IndexError(rev)
> +            msg = _('revision %d is hidden') % rev
> +            raise error.FilteredLookupError(rev, msg)

This should be a different exception (and an IndexError subclass)

>         return super(changelog, self).node(rev)
> 
>     def linkrev(self, rev):
>         """filtered version of revlog.linkrev"""
>         if rev in self.filteredrevs:
> -            raise IndexError(rev)
> +            msg = _('revision %d is hidden') % rev
> +            raise error.FilteredLookupError(rev, msg)

idem

>         return super(changelog, self).linkrev(rev)
> 
>     def parentrevs(self, rev):
>         """filtered version of revlog.parentrevs"""
>         if rev in self.filteredrevs:
> -            raise IndexError(rev)
> +            msg = _('revision %d is hidden') % rev
> +            raise error.FilteredLookupError(rev, msg)

idem ;-)

>         return super(changelog, self).parentrevs(rev)
> 
>     def flags(self, rev):
>         """filtered version of revlog.flags"""
>         if rev in self.filteredrevs:
> -            raise IndexError(rev)
> +            msg = _('revision %d is hidden') % rev
> +            raise error.FilteredLookupError(rev, msg)

idem ;-p

>         return super(changelog, self).flags(rev)
> 
>     def delayupdate(self):
> diff --git a/mercurial/error.py b/mercurial/error.py
> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -59,6 +59,14 @@ class RepoError(Exception):
> class RepoLookupError(RepoError):
>     pass
> 
> +class FilteredLookupError(RepoLookupError):
> +    """Exception raised on accessing filtered (i.e. hidden) changesets."""

not necessarily "hidden"

> +    def __init__(self, rev, message):
> +        self.rev = rev
> +        RepoLookupError.__init__(self, message)

See above comment about message and argument. You can probably forge a default message to be passed to RepoLookupError.

> 
> class CapabilityError(RepoError):
>     pass
> 
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -279,7 +279,7 @@ class revlog(object):
>         self.rev(self.node(0))
>         return self._nodecache
> 
> -    def hasnode(self, node):
> +    def hasnode(self, node, hidden=False):
>         try:
>             self.rev(node)
>             return True
> @@ -752,7 +752,7 @@ class revlog(object):
>     def _partialmatch(self, id):
>         try:
>             n = self.index.partialmatch(id)
> -            if n and self.hasnode(n):
> +            if n and self.hasnode(n, hidden=True):
>                 return n
>             return None
>         except RevlogError:
> diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
> --- a/tests/test-hgweb-commands.t
> +++ b/tests/test-hgweb-commands.t
> @@ -1390,7 +1390,7 @@ proper status for filtered revision
>   Content-Type: text/plain; charset=ascii\r (esc)
>   \r (esc)
> 
> -  error: unknown revision '5'
> +  error: revision 5 is hidden
> 
> 
> 
> @@ -1404,7 +1404,7 @@ proper status for filtered revision
>   Content-Type: text/plain; charset=ascii\r (esc)
>   \r (esc)
> 
> -  error: unknown revision '4'
> +  error: revision 4 is hidden
> 
> filtered '0' changeset
> 
> diff --git a/tests/test-log.t b/tests/test-log.t
> --- a/tests/test-log.t
> +++ b/tests/test-log.t
> @@ -1226,6 +1226,15 @@ enable obsolete to test hidden feature
>   $ hg log -r a
>   abort: unknown revision 'a'!
>   [255]
> +  $ hg log -r a7
> +  abort: unknown revision 'a7'!
> +  [255]
> +  $ hg log -r a76
> +  abort: unknown revision 'a76'!
> +  [255]
> +  $ hg log -r a765
> +  abort: changeset a765632148dc is hidden!
> +  [255]
> 
> test that parent prevent a changeset to be hidden
> 
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -211,7 +211,7 @@ check that various commands work well wi
>   abort: unknown revision '6'!
>   [255]
>   $ hg log -r 4
> -  abort: unknown revision '4'!
> +  abort: revision 4 is hidden!
>   [255]
> 
> Check that public changeset are not accounted as obsolete:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
--- a/mercurial/branchmap.py
+++ b/mercurial/branchmap.py
@@ -7,7 +7,7 @@ 
 
 from node import bin, hex, nullid, nullrev
 import encoding
-import util, repoview
+import error, util, repoview
 
 def _filename(repo):
     """name of a branchcache file for a given repo or repoview"""
@@ -121,7 +121,7 @@  class branchcache(dict):
         try:
             return ((self.tipnode == repo.changelog.node(self.tiprev))
                     and (self.filteredhash == self._hashfiltered(repo)))
-        except IndexError:
+        except (IndexError, error.FilteredLookupError):
             return False
 
     def copy(self):
diff --git a/mercurial/changelog.py b/mercurial/changelog.py
--- a/mercurial/changelog.py
+++ b/mercurial/changelog.py
@@ -5,7 +5,7 @@ 
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-from node import bin, hex, nullid
+from node import bin, hex, nullid, short
 from i18n import _
 import util, error, revlog, encoding
 
@@ -159,12 +159,15 @@  class changelog(revlog.revlog):
         self.rev(self.node(0))
         return self._nodecache
 
-    def hasnode(self, node):
+    def hasnode(self, node, hidden=False):
         """filtered version of revlog.hasnode"""
+        if hidden:
+            return super(changelog, self).hasnode(node)
+
         try:
             i = self.rev(node)
             return i not in self.filteredrevs
-        except KeyError:
+        except (KeyError, error.FilteredLookupError):
             return False
 
     def headrevs(self):
@@ -183,31 +186,36 @@  class changelog(revlog.revlog):
         """filtered version of revlog.rev"""
         r = super(changelog, self).rev(node)
         if r in self.filteredrevs:
-            raise error.LookupError(hex(node), self.indexfile, _('no node'))
+            msg = _('changeset %s is hidden') % short(node)
+            raise error.FilteredLookupError(r, msg)
         return r
 
     def node(self, rev):
         """filtered version of revlog.node"""
         if rev in self.filteredrevs:
-            raise IndexError(rev)
+            msg = _('revision %d is hidden') % rev
+            raise error.FilteredLookupError(rev, msg)
         return super(changelog, self).node(rev)
 
     def linkrev(self, rev):
         """filtered version of revlog.linkrev"""
         if rev in self.filteredrevs:
-            raise IndexError(rev)
+            msg = _('revision %d is hidden') % rev
+            raise error.FilteredLookupError(rev, msg)
         return super(changelog, self).linkrev(rev)
 
     def parentrevs(self, rev):
         """filtered version of revlog.parentrevs"""
         if rev in self.filteredrevs:
-            raise IndexError(rev)
+            msg = _('revision %d is hidden') % rev
+            raise error.FilteredLookupError(rev, msg)
         return super(changelog, self).parentrevs(rev)
 
     def flags(self, rev):
         """filtered version of revlog.flags"""
         if rev in self.filteredrevs:
-            raise IndexError(rev)
+            msg = _('revision %d is hidden') % rev
+            raise error.FilteredLookupError(rev, msg)
         return super(changelog, self).flags(rev)
 
     def delayupdate(self):
diff --git a/mercurial/error.py b/mercurial/error.py
--- a/mercurial/error.py
+++ b/mercurial/error.py
@@ -59,6 +59,14 @@  class RepoError(Exception):
 class RepoLookupError(RepoError):
     pass
 
+class FilteredLookupError(RepoLookupError):
+    """Exception raised on accessing filtered (i.e. hidden) changesets."""
+
+    def __init__(self, rev, message):
+        self.rev = rev
+
+        RepoLookupError.__init__(self, message)
+
 class CapabilityError(RepoError):
     pass
 
diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -279,7 +279,7 @@  class revlog(object):
         self.rev(self.node(0))
         return self._nodecache
 
-    def hasnode(self, node):
+    def hasnode(self, node, hidden=False):
         try:
             self.rev(node)
             return True
@@ -752,7 +752,7 @@  class revlog(object):
     def _partialmatch(self, id):
         try:
             n = self.index.partialmatch(id)
-            if n and self.hasnode(n):
+            if n and self.hasnode(n, hidden=True):
                 return n
             return None
         except RevlogError:
diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
--- a/tests/test-hgweb-commands.t
+++ b/tests/test-hgweb-commands.t
@@ -1390,7 +1390,7 @@  proper status for filtered revision
   Content-Type: text/plain; charset=ascii\r (esc)
   \r (esc)
   
-  error: unknown revision '5'
+  error: revision 5 is hidden
 
 
 
@@ -1404,7 +1404,7 @@  proper status for filtered revision
   Content-Type: text/plain; charset=ascii\r (esc)
   \r (esc)
   
-  error: unknown revision '4'
+  error: revision 4 is hidden
 
 filtered '0' changeset
 
diff --git a/tests/test-log.t b/tests/test-log.t
--- a/tests/test-log.t
+++ b/tests/test-log.t
@@ -1226,6 +1226,15 @@  enable obsolete to test hidden feature
   $ hg log -r a
   abort: unknown revision 'a'!
   [255]
+  $ hg log -r a7
+  abort: unknown revision 'a7'!
+  [255]
+  $ hg log -r a76
+  abort: unknown revision 'a76'!
+  [255]
+  $ hg log -r a765
+  abort: changeset a765632148dc is hidden!
+  [255]
 
 test that parent prevent a changeset to be hidden
 
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -211,7 +211,7 @@  check that various commands work well wi
   abort: unknown revision '6'!
   [255]
   $ hg log -r 4
-  abort: unknown revision '4'!
+  abort: revision 4 is hidden!
   [255]
 
 Check that public changeset are not accounted as obsolete: