Patchwork [03,of,11] obsolete: raise richer exception on unknown version

login
register
mail settings
Submitter Pierre-Yves David
Date May 28, 2017, 4:32 p.m.
Message ID <b97c492388f6b14c3466.1495989124@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20988/
State Accepted
Headers show

Comments

Pierre-Yves David - May 28, 2017, 4:32 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1495723823 -7200
#      Thu May 25 16:50:23 2017 +0200
# Node ID b97c492388f6b14c346600e1c1ed4e400f6b47ca
# Parent  dbce9b63b68d45706d9c269543070e0eee88b376
# EXP-Topic obsstrip
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r b97c492388f6
obsolete: raise richer exception on unknown version

We raise a more precise subclass of Abort with details about the faulty
version.  This will be used to detect this case  and display some information
in debugbundle.
Gregory Szorc - May 28, 2017, 6:56 p.m.
On Sun, May 28, 2017 at 9:32 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1495723823 -7200
> #      Thu May 25 16:50:23 2017 +0200
> # Node ID b97c492388f6b14c346600e1c1ed4e400f6b47ca
> # Parent  dbce9b63b68d45706d9c269543070e0eee88b376
> # EXP-Topic obsstrip
> # Available At https://www.mercurial-scm.org/
> repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/
> repo/users/marmoute/mercurial/ -r b97c492388f6
> obsolete: raise richer exception on unknown version
>
> We raise a more precise subclass of Abort with details about the faulty
> version.  This will be used to detect this case  and display some
> information
> in debugbundle.
>
> diff --git a/mercurial/error.py b/mercurial/error.py
> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -138,6 +138,14 @@ class UnsupportedMergeRecords(Abort):
>              hint=_('see https://mercurial-scm.org/wiki/MergeStateRecords
> for '
>                     'more information'))
>
> +class UnknownVersion(Abort):
> +    """generic exception for aborting from an encounter with an unknown
> version
> +    """
> +
> +    def __init__(self, *args, **kwargs):
> +        self.version = kwargs.pop('version')
>

Since the arguments are always (msg, version), please define them as such
and don't abuse **kwargs. Also consider having the constructor format an
error message from arguments and pass that result to Abort.__init__.


> +        super(UnknownVersion, self).__init__(*args, **kwargs)
> +
>  class LockError(IOError):
>      def __init__(self, errno, strerror, filename, desc):
>          IOError.__init__(self, errno, strerror, filename)
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -446,8 +446,8 @@ def _readmarkers(data):
>      diskversion = _unpack('>B', data[off:off + 1])[0]
>      off += 1
>      if diskversion not in formats:
> -        raise error.Abort(_('parsing obsolete marker: unknown version %r')
> -                         % diskversion)
> +        msg = _('parsing obsolete marker: unknown version %r') %
> diskversion
> +        raise error.UnknownVersion(msg, version=diskversion)
>      return diskversion, formats[diskversion][0](data, off)
>
>  def encodemarkers(markers, addheader=False, version=_fm0version):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - May 28, 2017, 10:31 p.m.
On 05/28/2017 08:56 PM, Gregory Szorc wrote:
> On Sun, May 28, 2017 at 9:32 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>
>     # Date 1495723823 -7200
>     #      Thu May 25 16:50:23 2017 +0200
>     # Node ID b97c492388f6b14c346600e1c1ed4e400f6b47ca
>     # Parent  dbce9b63b68d45706d9c269543070e0eee88b376
>     # EXP-Topic obsstrip
>     # Available At
>     https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>     <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>     #              hg pull
>     https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>     <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/> -r
>     b97c492388f6
>     obsolete: raise richer exception on unknown version
>
>     We raise a more precise subclass of Abort with details about the faulty
>     version.  This will be used to detect this case  and display some
>     information
>     in debugbundle.
>
>     diff --git a/mercurial/error.py b/mercurial/error.py
>     --- a/mercurial/error.py
>     +++ b/mercurial/error.py
>     @@ -138,6 +138,14 @@ class UnsupportedMergeRecords(Abort):
>                  hint=_('see
>     https://mercurial-scm.org/wiki/MergeStateRecords
>     <https://mercurial-scm.org/wiki/MergeStateRecords> for '
>                         'more information'))
>
>     +class UnknownVersion(Abort):
>     +    """generic exception for aborting from an encounter with an
>     unknown version
>     +    """
>     +
>     +    def __init__(self, *args, **kwargs):
>     +        self.version = kwargs.pop('version')
>
>
> Since the arguments are always (msg, version), please define them as
> such and don't abuse **kwargs.

I wanted to preserve the 'Abort' feel of the exception with an option 
"version" argument, but I see you point.

I'll shortly send a V2 with

   def __init__(self, msg, hint=None, version=None):

(note: 'hint' use the same construct[1], but is probably more justified 
in this case)

[1] 
https://www.mercurial-scm.org/repo/hg/file/14e8fef9158d/mercurial/error.py#l24


> Also consider having the constructor
> format an error message from arguments and pass that result to
> Abort.__init__.

I made the exception a bit wider than the current usecase 
(UnknownVersion, not UnknownObsMarkersVersion) as I assume we'll be able 
to reuse it for other usecase. Building the error message in the 
exception itself and keeping it reusable is a bit challenging. I'm 
skipping this in my V2.

Cheers,

Patch

diff --git a/mercurial/error.py b/mercurial/error.py
--- a/mercurial/error.py
+++ b/mercurial/error.py
@@ -138,6 +138,14 @@  class UnsupportedMergeRecords(Abort):
             hint=_('see https://mercurial-scm.org/wiki/MergeStateRecords for '
                    'more information'))
 
+class UnknownVersion(Abort):
+    """generic exception for aborting from an encounter with an unknown version
+    """
+
+    def __init__(self, *args, **kwargs):
+        self.version = kwargs.pop('version')
+        super(UnknownVersion, self).__init__(*args, **kwargs)
+
 class LockError(IOError):
     def __init__(self, errno, strerror, filename, desc):
         IOError.__init__(self, errno, strerror, filename)
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -446,8 +446,8 @@  def _readmarkers(data):
     diskversion = _unpack('>B', data[off:off + 1])[0]
     off += 1
     if diskversion not in formats:
-        raise error.Abort(_('parsing obsolete marker: unknown version %r')
-                         % diskversion)
+        msg = _('parsing obsolete marker: unknown version %r') % diskversion
+        raise error.UnknownVersion(msg, version=diskversion)
     return diskversion, formats[diskversion][0](data, off)
 
 def encodemarkers(markers, addheader=False, version=_fm0version):