Patchwork [4,of,4,V3] dirstate: add exception when calling setparent without begin/end

login
register
mail settings
Submitter Durham Goode
Date Sept. 9, 2014, 8:21 p.m.
Message ID <2328d9edda009ab9ae9d.1410294090@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/5758/
State Accepted
Headers show

Comments

Durham Goode - Sept. 9, 2014, 8:21 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1409942264 25200
#      Fri Sep 05 11:37:44 2014 -0700
# Node ID 2328d9edda009ab9ae9da8156d96763ff1ef76fc
# Parent  af3352972f8fddfb0c5b755e6ceb2470a31e6f7c
dirstate: add exception when calling setparent without begin/end

Adds an exception when calling dirstate.setparent without having first called
dirstate.beginparentchange. This will prevent people from writing code that
modifies the dirstate parent without considering the transactionality of their
change.

This will break third party extensions that call setparents.
Augie Fackler - Sept. 10, 2014, 7:15 p.m.
On Tue, Sep 09, 2014 at 01:21:30PM -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1409942264 25200
> #      Fri Sep 05 11:37:44 2014 -0700
> # Node ID 2328d9edda009ab9ae9da8156d96763ff1ef76fc
> # Parent  af3352972f8fddfb0c5b755e6ceb2470a31e6f7c
> dirstate: add exception when calling setparent without begin/end
>
> Adds an exception when calling dirstate.setparent without having first called
> dirstate.beginparentchange. This will prevent people from writing code that
> modifies the dirstate parent without considering the transactionality of their
> change.
>
> This will break third party extensions that call setparents.
>
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -256,6 +256,10 @@
>
>          See localrepo.setparents()
>          """
> +        if self._parentwriters == 0:
> +            raise util.Abort(_("cannot set dirstate parent without " +
> +                " calling dirstate.begingparentchange"))

Hm. Can we make this some non-Abort exception? This is really
something that should never happen, and a stacktrace seems warranted
(the message here strikes me as something that'd only confuse users
anyway). Maybe ValueError for now? Do you mind if I make that change?

> +
>          self._dirty = self._dirtypl = True
>          oldp2 = self._pl[1]
>          self._pl = p1, p2
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Sept. 10, 2014, 7:23 p.m.
On 9/10/14, 7:15 PM, "Augie Fackler" <raf@durin42.com> wrote:

>On Tue, Sep 09, 2014 at 01:21:30PM -0700, Durham Goode wrote:

>> # HG changeset patch

>> # User Durham Goode <durham@fb.com>

>> # Date 1409942264 25200

>> #      Fri Sep 05 11:37:44 2014 -0700

>> # Node ID 2328d9edda009ab9ae9da8156d96763ff1ef76fc

>> # Parent  af3352972f8fddfb0c5b755e6ceb2470a31e6f7c

>> dirstate: add exception when calling setparent without begin/end

>>

>> Adds an exception when calling dirstate.setparent without having first 

>>called

>> dirstate.beginparentchange. This will prevent people from writing code 

>>that

>> modifies the dirstate parent without considering the transactionality 

>>of their

>> change.

>>

>> This will break third party extensions that call setparents.

>>

>> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py

>> --- a/mercurial/dirstate.py

>> +++ b/mercurial/dirstate.py

>> @@ -256,6 +256,10 @@

>>

>>          See localrepo.setparents()

>>          """

>> +        if self._parentwriters == 0:

>> +            raise util.Abort(_("cannot set dirstate parent without " +

>> +                " calling dirstate.begingparentchange"))

>

>Hm. Can we make this some non-Abort exception? This is really

>something that should never happen, and a stacktrace seems warranted

>(the message here strikes me as something that'd only confuse users

>anyway). Maybe ValueError for now? Do you mind if I make that change?


Yea, it’s definitely only there for developers.  You can change it.

>

>> +

>>          self._dirty = self._dirtypl = True

>>          oldp2 = self._pl[1]

>>          self._pl = p1, p2

>> _______________________________________________

>> Mercurial-devel mailing list

>> Mercurial-devel@selenic.com

>> http://selenic.com/mailman/listinfo/mercurial-devel

>
Augie Fackler - Sept. 10, 2014, 7:25 p.m.
On Wed, Sep 10, 2014 at 07:23:18PM +0000, Durham Goode wrote:
> On 9/10/14, 7:15 PM, "Augie Fackler" <raf@durin42.com> wrote:
>
> >On Tue, Sep 09, 2014 at 01:21:30PM -0700, Durham Goode wrote:
> >> # HG changeset patch
> >> # User Durham Goode <durham@fb.com>
> >> # Date 1409942264 25200
> >> #      Fri Sep 05 11:37:44 2014 -0700
> >> # Node ID 2328d9edda009ab9ae9da8156d96763ff1ef76fc
> >> # Parent  af3352972f8fddfb0c5b755e6ceb2470a31e6f7c
> >> dirstate: add exception when calling setparent without begin/end
> >>
> >> Adds an exception when calling dirstate.setparent without having first
> >>called
> >> dirstate.beginparentchange. This will prevent people from writing code
> >>that
> >> modifies the dirstate parent without considering the transactionality
> >>of their
> >> change.
> >>
> >> This will break third party extensions that call setparents.
> >>
> >> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> >> --- a/mercurial/dirstate.py
> >> +++ b/mercurial/dirstate.py
> >> @@ -256,6 +256,10 @@
> >>
> >>          See localrepo.setparents()
> >>          """
> >> +        if self._parentwriters == 0:
> >> +            raise util.Abort(_("cannot set dirstate parent without " +
> >> +                " calling dirstate.begingparentchange"))
> >
> >Hm. Can we make this some non-Abort exception? This is really
> >something that should never happen, and a stacktrace seems warranted
> >(the message here strikes me as something that'd only confuse users
> >anyway). Maybe ValueError for now? Do you mind if I make that change?
>
> Yea, it’s definitely only there for developers.  You can change it.

Excellent. Queued with ValueError instead of Abort. Thanks!

>
> >
> >> +
> >>          self._dirty = self._dirtypl = True
> >>          oldp2 = self._pl[1]
> >>          self._pl = p1, p2
> >> _______________________________________________
> >> Mercurial-devel mailing list
> >> Mercurial-devel@selenic.com
> >> http://selenic.com/mailman/listinfo/mercurial-devel
> >
>
>
>

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -256,6 +256,10 @@ 
 
         See localrepo.setparents()
         """
+        if self._parentwriters == 0:
+            raise util.Abort(_("cannot set dirstate parent without " +
+                " calling dirstate.begingparentchange"))
+
         self._dirty = self._dirtypl = True
         oldp2 = self._pl[1]
         self._pl = p1, p2