Patchwork [1,of,5] repair: use ProgrammingError

login
register
mail settings
Submitter Jun Wu
Date March 27, 2017, 12:14 a.m.
Message ID <92d7eff3f4c7e44e8aab.1490573690@localhost.localdomain>
Download mbox | patch
Permalink /patch/19714/
State Accepted
Headers show

Comments

Jun Wu - March 27, 2017, 12:14 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490572408 25200
#      Sun Mar 26 16:53:28 2017 -0700
# Node ID 92d7eff3f4c7e44e8aab62b486bda78a37b73b83
# Parent  e86eb75e74ce1b0803c26d86a229b9b711f6d76a
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 92d7eff3f4c7
repair: use ProgrammingError
Pierre-Yves David - March 27, 2017, 6:27 a.m.
On 03/27/2017 02:14 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490572408 25200
> #      Sun Mar 26 16:53:28 2017 -0700
> # Node ID 92d7eff3f4c7e44e8aab62b486bda78a37b73b83
> # Parent  e86eb75e74ce1b0803c26d86a229b9b711f6d76a
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 92d7eff3f4c7
> repair: use ProgrammingError
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -164,6 +164,5 @@ def strip(ui, repo, nodelist, backup=Tru
>      if curtr is not None:
>          del curtr  # avoid carrying reference to transaction for nothing
> -        msg = _('programming error: cannot strip from inside a transaction')
> -        raise error.Abort(msg, hint=_('contact your extension maintainer'))
> +        raise error.ProgrammingError('cannot strip from inside a transaction')

AS much as I like the idea of moving to Programming error, this patches 
series drops an important hint. Can we have a version that preserve the 
hint ?
Jun Wu - March 27, 2017, 6:57 a.m.
I think ProgrammingError should all have that hint. So this is not repair.py
specific.

Excerpts from Pierre-Yves David's message of 2017-03-27 08:27:51 +0200:
> 
> On 03/27/2017 02:14 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490572408 25200
> > #      Sun Mar 26 16:53:28 2017 -0700
> > # Node ID 92d7eff3f4c7e44e8aab62b486bda78a37b73b83
> > # Parent  e86eb75e74ce1b0803c26d86a229b9b711f6d76a
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 92d7eff3f4c7
> > repair: use ProgrammingError
> >
> > diff --git a/mercurial/repair.py b/mercurial/repair.py
> > --- a/mercurial/repair.py
> > +++ b/mercurial/repair.py
> > @@ -164,6 +164,5 @@ def strip(ui, repo, nodelist, backup=Tru
> >      if curtr is not None:
> >          del curtr  # avoid carrying reference to transaction for nothing
> > -        msg = _('programming error: cannot strip from inside a transaction')
> > -        raise error.Abort(msg, hint=_('contact your extension maintainer'))
> > +        raise error.ProgrammingError('cannot strip from inside a transaction')
> 
> AS much as I like the idea of moving to Programming error, this patches 
> series drops an important hint. Can we have a version that preserve the 
> hint ?
>
Pierre-Yves David - March 27, 2017, 7:18 a.m.
Sure, that seems like a good idea. Can you see to it ?
(I just replied on repair because this is the first changeset in the series)

On 03/27/2017 08:57 AM, Jun Wu wrote:
> I think ProgrammingError should all have that hint. So this is not repair.py
> specific.
>
> Excerpts from Pierre-Yves David's message of 2017-03-27 08:27:51 +0200:
>>
>> On 03/27/2017 02:14 AM, Jun Wu wrote:
>>> # HG changeset patch
>>> # User Jun Wu <quark@fb.com>
>>> # Date 1490572408 25200
>>> #      Sun Mar 26 16:53:28 2017 -0700
>>> # Node ID 92d7eff3f4c7e44e8aab62b486bda78a37b73b83
>>> # Parent  e86eb75e74ce1b0803c26d86a229b9b711f6d76a
>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 92d7eff3f4c7
>>> repair: use ProgrammingError
>>>
>>> diff --git a/mercurial/repair.py b/mercurial/repair.py
>>> --- a/mercurial/repair.py
>>> +++ b/mercurial/repair.py
>>> @@ -164,6 +164,5 @@ def strip(ui, repo, nodelist, backup=Tru
>>>      if curtr is not None:
>>>          del curtr  # avoid carrying reference to transaction for nothing
>>> -        msg = _('programming error: cannot strip from inside a transaction')
>>> -        raise error.Abort(msg, hint=_('contact your extension maintainer'))
>>> +        raise error.ProgrammingError('cannot strip from inside a transaction')
>>
>> AS much as I like the idea of moving to Programming error, this patches
>> series drops an important hint. Can we have a version that preserve the
>> hint ?
>>
Augie Fackler - March 27, 2017, 1:11 p.m.
On Mon, Mar 27, 2017 at 09:18:55AM +0200, Pierre-Yves David wrote:
> Sure, that seems like a good idea. Can you see to it ?

It's a good idea, but I feel like the name "ProgrammingError" alone
provides some clue as to what's going on, and the migration is good.

Queued, I wouldn't mind a followup to make the hint generic to all
ProgrammingError instances, but I'm not willing to block on it either.

> (I just replied on repair because this is the first changeset in the series)
>
> On 03/27/2017 08:57 AM, Jun Wu wrote:
> > I think ProgrammingError should all have that hint. So this is not repair.py
> > specific.
> >
> > Excerpts from Pierre-Yves David's message of 2017-03-27 08:27:51 +0200:
> > >
> > > On 03/27/2017 02:14 AM, Jun Wu wrote:
> > > > # HG changeset patch
> > > > # User Jun Wu <quark@fb.com>
> > > > # Date 1490572408 25200
> > > > #      Sun Mar 26 16:53:28 2017 -0700
> > > > # Node ID 92d7eff3f4c7e44e8aab62b486bda78a37b73b83
> > > > # Parent  e86eb75e74ce1b0803c26d86a229b9b711f6d76a
> > > > # Available At https://bitbucket.org/quark-zju/hg-draft
> > > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 92d7eff3f4c7
> > > > repair: use ProgrammingError
> > > >
> > > > diff --git a/mercurial/repair.py b/mercurial/repair.py
> > > > --- a/mercurial/repair.py
> > > > +++ b/mercurial/repair.py
> > > > @@ -164,6 +164,5 @@ def strip(ui, repo, nodelist, backup=Tru
> > > >      if curtr is not None:
> > > >          del curtr  # avoid carrying reference to transaction for nothing
> > > > -        msg = _('programming error: cannot strip from inside a transaction')
> > > > -        raise error.Abort(msg, hint=_('contact your extension maintainer'))
> > > > +        raise error.ProgrammingError('cannot strip from inside a transaction')
> > >
> > > AS much as I like the idea of moving to Programming error, this patches
> > > series drops an important hint. Can we have a version that preserve the
> > > hint ?
> > >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Ryan McElroy - March 28, 2017, 6:59 p.m.
On 3/27/17 2:11 PM, Augie Fackler wrote:
> On Mon, Mar 27, 2017 at 09:18:55AM +0200, Pierre-Yves David wrote:
>> Sure, that seems like a good idea. Can you see to it ?
> It's a good idea, but I feel like the name "ProgrammingError" alone
> provides some clue as to what's going on, and the migration is good.
>
> Queued, I wouldn't mind a followup to make the hint generic to all
> ProgrammingError instances, but I'm not willing to block on it either.

FWIW, I agree that losing the hint is sad. How confident are we that 
core won't have these errors? Can we blame all ProgrammingError 
exceptions on extensions?
Pierre-Yves David - March 29, 2017, 11:24 a.m.
On 03/28/2017 08:59 PM, Ryan McElroy wrote:
> On 3/27/17 2:11 PM, Augie Fackler wrote:
>> On Mon, Mar 27, 2017 at 09:18:55AM +0200, Pierre-Yves David wrote:
>>> Sure, that seems like a good idea. Can you see to it ?
>> It's a good idea, but I feel like the name "ProgrammingError" alone
>> provides some clue as to what's going on, and the migration is good.
>>
>> Queued, I wouldn't mind a followup to make the hint generic to all
>> ProgrammingError instances, but I'm not willing to block on it either.
>
> FWIW, I agree that losing the hint is sad. How confident are we that
> core won't have these errors? Can we blame all ProgrammingError
> exceptions on extensions?

The general idea about pointing at extensions here is that such bug in 
Mercurial should be caught by the Mercurial test suite (or local 
development process) so user should not be exposed to it (in theory). On 
the other hand, existing extensions code might use API wrong in a way 
that did not raised the error in previous version. So if the user get 
exposed to that error, it it probably because of an extension.

Cheers,
Jun Wu - March 29, 2017, 3:56 p.m.
Excerpts from Pierre-Yves David's message of 2017-03-29 13:24:39 +0200:
> 
> On 03/28/2017 08:59 PM, Ryan McElroy wrote:
> > On 3/27/17 2:11 PM, Augie Fackler wrote:
> >> On Mon, Mar 27, 2017 at 09:18:55AM +0200, Pierre-Yves David wrote:
> >>> Sure, that seems like a good idea. Can you see to it ?
> >> It's a good idea, but I feel like the name "ProgrammingError" alone
> >> provides some clue as to what's going on, and the migration is good.
> >>
> >> Queued, I wouldn't mind a followup to make the hint generic to all
> >> ProgrammingError instances, but I'm not willing to block on it either.
> >
> > FWIW, I agree that losing the hint is sad. How confident are we that
> > core won't have these errors? Can we blame all ProgrammingError
> > exceptions on extensions?
> 
> The general idea about pointing at extensions here is that such bug in 
> Mercurial should be caught by the Mercurial test suite (or local 
> development process) so user should not be exposed to it (in theory). On 
> the other hand, existing extensions code might use API wrong in a way 
> that did not raised the error in previous version. So if the user get 
> exposed to that error, it it probably because of an extension.

I think the question was about *all* ProgrammingErrors, not about a
particular one. I'm not sure if you have answered that. But maybe my English
reading is just bad.

FWIW, I think you can answer the question by either:

  Yes, all ProgrammingErrors are extensions' fault.

or,

  No, some ProgrammingErrors are unrelated to extensions.

which is just 60 bytes, more cleaner, 5x shorter, and save time for us all.
Ryan McElroy - March 30, 2017, 8:10 a.m.
On 3/29/17 4:56 PM, Jun Wu wrote:
> Excerpts from Pierre-Yves David's message of 2017-03-29 13:24:39 +0200:
>> On 03/28/2017 08:59 PM, Ryan McElroy wrote:
>>> On 3/27/17 2:11 PM, Augie Fackler wrote:
>>>> On Mon, Mar 27, 2017 at 09:18:55AM +0200, Pierre-Yves David wrote:
>>>>> Sure, that seems like a good idea. Can you see to it ?
>>>> It's a good idea, but I feel like the name "ProgrammingError" alone
>>>> provides some clue as to what's going on, and the migration is good.
>>>>
>>>> Queued, I wouldn't mind a followup to make the hint generic to all
>>>> ProgrammingError instances, but I'm not willing to block on it either.
>>> FWIW, I agree that losing the hint is sad. How confident are we that
>>> core won't have these errors? Can we blame all ProgrammingError
>>> exceptions on extensions?
>> The general idea about pointing at extensions here is that such bug in
>> Mercurial should be caught by the Mercurial test suite (or local
>> development process) so user should not be exposed to it (in theory). On
>> the other hand, existing extensions code might use API wrong in a way
>> that did not raised the error in previous version. So if the user get
>> exposed to that error, it it probably because of an extension.
> I think the question was about *all* ProgrammingErrors, not about a
> particular one. I'm not sure if you have answered that. But maybe my English
> reading is just bad.
>
> FWIW, I think you can answer the question by either:
>
>    Yes, all ProgrammingErrors are extensions' fault.
>
> or,
>
>    No, some ProgrammingErrors are unrelated to extensions.
>
> which is just 60 bytes, more cleaner, 5x shorter, and save time for us all.

Not sure how this is intended, Jun, but this statement come across text 
pretty harshly. Just be careful with your wording and your statements 
please.

I think Pierre-Yves' statement adds important subtlety, which is that we 
expect all ProgrammingErrors in the wild to be caused by extensions. If 
a core developer sees one, we quickly fix it and add a test. I think we 
can use that subtlety in a hint for any programming error. I'll think 
about some possible hints to add here.

Remember we're all on the same team here.

Cheers,

~Ryan

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -164,6 +164,5 @@  def strip(ui, repo, nodelist, backup=Tru
     if curtr is not None:
         del curtr  # avoid carrying reference to transaction for nothing
-        msg = _('programming error: cannot strip from inside a transaction')
-        raise error.Abort(msg, hint=_('contact your extension maintainer'))
+        raise error.ProgrammingError('cannot strip from inside a transaction')
 
     try:
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -107,9 +107,8 @@ 
   $ hg add a
   $ hg commit -m a
-  $ hg stripintr
+  $ hg stripintr 2>&1 | egrep -v '^(\*\*|  )'
   saved backup bundle to $TESTTMP/lock-checker/.hg/strip-backup/*-backup.hg (glob)
-  abort: programming error: cannot strip from inside a transaction
-  (contact your extension maintainer)
-  [255]
+  Traceback (most recent call last):
+  mercurial.error.ProgrammingError: cannot strip from inside a transaction
 
   $ hg log -r "oldstyle()" -T '{rev}\n'