[PATCH 3 of 3] dispatch: only check compatibility against major and minor versions

[Ryan McElroy]

On 1/15/2015 8:36 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1421382963 28800
> #      Thu Jan 15 20:36:03 2015 -0800
> # Node ID 28e3fdafed22396c10964fa72c869c0e8c81ae05
> # Parent  d9c6f710438d71fb5b4114f0639d27f75d8cf1b9
> dispatch: only check compatibility against major and minor versions
>
> Extensions can declare compatibility with Mercurial versions. If an
> error occurs, Mercurial will attempt to pin blame on an extension that
> isn't marked as compatible.
>
> While all bets are off when it comes to the internal API, my experience
> has shown that a monthly/patch release of Mercurial has never broken any
> of the extensions I've written. I think that expecting extensions to
> declare compatibility with every patch release of Mercurial is asking a
> bit much and adds little to no value.
>
> This patch changes the blame logic from exact version matching to only
> match on the major and minor Mercurial versions. This means that
> extensions only need to mark themselves as compatible with the major,
> quarterly releases and not the monthly ones in order to stay current and
> avoid what is almost certainly unfair blame. This will mean less work
> for extension authors and almost certainly fewer false positives in the
> blame attribution.
>
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -282,14 +282,21 @@ def _runcatch(req):
>               if not testedwith.strip():
>                   # We found an untested extension. It's likely the culprit.
>                   worst = name, 'unknown', report
>                   break
> -            if compare not in testedwith.split() and testedwith != 'internal':
> -                tested = [tuplever(v) for v in testedwith.split()]
> -                lower = [t for t in tested if t < ct]
> -                nearest = max(lower or tested)
> -                if worst[0] is None or nearest < worst[1]:
> -                    worst = name, nearest, report
> +
> +            # Never blame on extensions bundled with Mercurial.
> +            if testedwith == 'internal':
> +                continue
> +
> +            tested = [tuplever(t) for t in testedwith.split()]
> +            if ct in tested:
> +                continue
> +
> +            lower = [t for t in tested if t < ct]
> +            nearest = max(lower or tested)
> +            if worst[0] is None or nearest < worst[1]:
> +                worst = name, nearest, report
>           if worst[0] is not None:
>               name, testedwith, report = worst
>               if not isinstance(testedwith, str):
>                   testedwith = '.'.join([str(c) for c in testedwith])
> @@ -314,9 +321,12 @@ def _runcatch(req):
>       return -1
>   
>   def tuplever(v):
>       try:
> -        return tuple([int(i) for i in v.split('.')])
> +        # Assertion: tuplever is only used for extension compatibility
> +        # checking. Otherwise, the discarding of extra version fields is
> +        # incorrect.
> +        return tuple([int(i) for i in v.split('.')[0:2]])
>       except ValueError:
>           return tuple()
>   
>   def aliasargs(fn, givenargs):
> diff --git a/tests/test-extension.t b/tests/test-extension.t
> --- a/tests/test-extension.t
> +++ b/tests/test-extension.t
> @@ -857,9 +857,9 @@ Broken disabled extension and command:
>     (try "hg help --keyword foo")
>     [255]
>   
>     $ cat > throw.py <<EOF
> -  > from mercurial import cmdutil, commands
> +  > from mercurial import cmdutil, commands, util
>     > cmdtable = {}
>     > command = cmdutil.command(cmdtable)
>     > class Bogon(Exception): pass
>     > @command('throw', [], 'hg throw', norepo=True)
> @@ -909,9 +909,9 @@ If the extensions declare outdated versi
>     $ rm -f throw.pyc throw.pyo
>     $ hg --config extensions.throw=throw.py --config extensions.older=older.py \
>     >   throw 2>&1 | egrep '^\*\*'
>     ** Unknown exception encountered with possibly-broken third-party extension older
> -  ** which supports versions 1.9.3 of Mercurial.
> +  ** which supports versions 1.9 of Mercurial.
>     ** Please disable older and try your action again.
>     ** If that fixes the bug please report it to the extension author.
>     ** Python * (glob)
>     ** Mercurial Distributed SCM (version 2.2)
> @@ -922,9 +922,9 @@ One extension only tested with older, on
>     $ rm -f older.pyc older.pyo
>     $ hg --config extensions.throw=throw.py --config extensions.older=older.py \
>     >   throw 2>&1 | egrep '^\*\*'
>     ** Unknown exception encountered with possibly-broken third-party extension older
> -  ** which supports versions 1.9.3 of Mercurial.
> +  ** which supports versions 1.9 of Mercurial.
>     ** Please disable older and try your action again.
>     ** If that fixes the bug please report it to the extension author.
>     ** Python * (glob)
>     ** Mercurial Distributed SCM (version 2.1)
> @@ -935,9 +935,9 @@ Older extension is tested with current v
>     $ rm -f older.pyc older.pyo
>     $ hg --config extensions.throw=throw.py --config extensions.older=older.py \
>     >   throw 2>&1 | egrep '^\*\*'
>     ** Unknown exception encountered with possibly-broken third-party extension throw
> -  ** which supports versions 2.1.1 of Mercurial.
> +  ** which supports versions 2.1 of Mercurial.
>     ** Please disable throw and try your action again.
>     ** If that fixes the bug please report it to https://urldefense.proofpoint.com/v1/url?u=http://example.com/bts&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=zxRJZ6melt%2FqLtQ%2Bw2Gaeg%3D%3D%0A&m=qoS%2F9eZdZYNoIzL0gpp6tWE%2BFpkmZjZRAV%2F%2BmKm2V5M%3D%0A&s=500584f65f60a10bafb67431f6b711c16907bd92e7a0bf50f1d9cf7f7172ca35
>     ** Python * (glob)
>     ** Mercurial Distributed SCM (version 1.9.3)
> @@ -953,8 +953,19 @@ Declare the version as supporting this h
>     ** Python * (glob)
>     ** Mercurial Distributed SCM (*) (glob)
>     ** Extensions loaded: throw
>   
> +Patch version is ignored during compatibility check
> +  $ echo "testedwith = '3.2'" >> throw.py
> +  $ echo "util.version = lambda:'3.2.2'" >> throw.py
> +  $ rm -f throw.pyc throw.pyo
> +  $ hg --config extensions.throw=throw.py throw 2>&1 | egrep '^\*\*'
> +  ** unknown exception encountered, please report by visiting
> +  ** http://mercurial.selenic.com/wiki/BugTracker
> +  ** Python * (glob)
> +  ** Mercurial Distributed SCM (*) (glob)
> +  ** Extensions loaded: throw
> +
>   Test version number support in 'hg version':
>     $ echo '__version__ = (1, 2, 3)' >> throw.py
>     $ rm -f throw.pyc throw.pyo
>     $ hg version -v
>

I'm happy enough with this series (I'm not for or against this third 
patch the the other two seem strictly positive). I might argue against 
this third patch on the basis that major revisions don't seem to have 
any semantic meaning in mercurial (as I understand it -- but I'm new 
here -- hg goes from 2.9 to 3.0 on a schedule, not on a set of 
features). However, I could argue for it in that it reduces the overhead 
of maintaining extensions to every ten freezes instead of every freeze, 
which is probably a net win for the world overall.

That being said, the whole blaming an extension based on declared 
compatibility is totally broken. I'd much rather see us try to guess 
where the break is based on any extension being in the stack trace, 
otherwise not assigning blame to any particular extension. In my 
experience at FB, this code *always* blames crecord, which is never 
actually at fault.

I'll probably take a crack at doing something like this eventually if 
nobody else ever does, but it's not very high on any priority list.

Another thing that I think would be good is to write the stack trace to 
a file (and mention this file in the crash report) but never expose the 
user to a stack trace. For most users, a stack trace is pretty scary 
looking and a nice "something went wrong" message with instructions on 
how to report it without the stack trace would be a lot more 
approachable. But now I'm just rambling :-)

~Ryan