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

# 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.

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
>

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,