[PATCH 3 of 3] dispatch: only check compatibility against major and minor versions
Gregory Szorc
gregory.szorc at gmail.com
Fri Jan 16 01:38:28 CST 2015
On Thu, Jan 15, 2015 at 10:52 PM, Ryan McElroy <rm at fb.com> wrote:
> 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=500584f65f60a10bafb67431f6b711
>> c16907bd92e7a0bf50f1d9cf7f7172ca35
>> ** 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.
>
Unless I did something wrong, I reduced the warn threshold to every 3rd
freeze, not ten. We should be looking at X.Y versions.
> 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 :-)
>
I completely agree about blaming being almost completely busted. I, too,
wish it were based on stacks. But even that isn't perfect. For example, you
can get a TypeError at a call site when passing an unknown argument into a
function in an extension that doesn't accept that argument. Of course, you
can also get a TypeError inside a function. How do you attribute which file
is to blame? I argue that in the current world of relying on
monkeypatching, you can't. One of the benefits of a more well-defined
extensibility API (such as the events proposal I sent a few months back) is
that call sites into extensions are more clearly defined. So, if an error
occurs, we can look at the module/file being called into an attribute it to
an extension. That's better but still not perfect. You still have a general
class of errors around "extension manipulated something wrong and we didn't
find out about it until later." Oy.
I like your idea of writing stacks to files! I'd love to see that in 3.4,
even if it's behind a feature flag.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150115/31d17fc6/attachment.html>
More information about the Mercurial-devel
mailing list