Patchwork repair: forbid strip from inside a transaction

login
register
mail settings
Submitter Pierre-Yves David
Date May 27, 2015, 1:40 a.m.
Message ID <0c71ff45e361550948fc.1432690841@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/9293/
State Accepted
Commit 678d0bfdd31a1f74fddc399c3ee07013efa9a652
Headers show

Comments

Pierre-Yves David - May 27, 2015, 1:40 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1432441127 25200
#      Sat May 23 21:18:47 2015 -0700
# Node ID 0c71ff45e361550948fcc3cbfa03898c8ba9fd96
# Parent  61b3529e23778f542044d24bb1ccd5688516a7f0
repair: forbid strip from inside a transaction

Striping inside a transaction will, at best, crash or , at worst, result in very
unexpected result. We explicitly forbid it early.
Augie Fackler - May 27, 2015, 3:15 p.m.
On Tue, May 26, 2015 at 06:40:41PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1432441127 25200
> #      Sat May 23 21:18:47 2015 -0700
> # Node ID 0c71ff45e361550948fcc3cbfa03898c8ba9fd96
> # Parent  61b3529e23778f542044d24bb1ccd5688516a7f0
> repair: forbid strip from inside a transaction
>
> Striping inside a transaction will, at best, crash or , at worst, result in very

Spellcheck betrayed you: stripping (as opposed to striping).

(copyediting here because there's one sentence in a comment I can't parse.)

> unexpected result. We explicitly forbid it early.
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -149,10 +149,16 @@ def strip(ui, repo, nodelist, backup=Tru
>          chgrpfile = _bundle(repo, savebases, saveheads, node, 'temp',
>                              compress=False)
>
>      mfst = repo.manifest
>
> +    curtr = repo.currenttransaction()
> +    if curtr is not None:
> +        del curtr  # avoid carrying reference to transaction for nothing
> +        msg = _('programming error: cannot strip from inside a transaction')
> +        raise util.Abort(msg, hint=_('contact your extensions maintainer'))

"contact your extension maintainer"

> +
>      tr = repo.transaction("strip")
>      offset = len(tr.entries)
>
>      try:
>          tr.startgroup()
> 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
> @@ -1,11 +1,11 @@
>
>    $ cat << EOF > buggylocking.py
>    > """A small extension that acquire locks in the wrong order
>    > """
>    >
> -  > from mercurial import cmdutil
> +  > from mercurial import cmdutil, repair
>    >
>    > cmdtable = {}
>    > command = cmdutil.command(cmdtable)
>    >
>    > @command('buggylocking', [], '')
> @@ -36,10 +36,17 @@
>    > def nowaitlocking(ui, repo):
>    >     lo = repo.lock()
>    >     wl = repo.wlock(wait=False)
>    >     wl.release()
>    >     lo.release()
> +  >
> +  > @command('stripintr', [], '')
> +  > def stripintr(ui, repo):
> +  >     lo = repo.lock()
> +  >     # not a warning no worth testing.

Not sure what this sentence should be (the "no" doesn't make sense)


> +  >     tr = repo.transaction('foobar')
> +  >     repair.strip(repo.ui, repo, [repo['.'].node()])
>    > EOF
>
>    $ cat << EOF >> $HGRCPATH
>    > [extensions]
>    > buggylocking=$TESTTMP/buggylocking.py
> @@ -85,6 +92,16 @@
>     */mercurial/dispatch.py:* in <lambda> (glob)
>     */mercurial/util.py:* in check (glob)
>     $TESTTMP/buggylocking.py:* in buggylocking (glob)
>    $ hg properlocking
>    $ hg nowaitlocking
> +
> +  $ echo a > a
> +  $ hg add a
> +  $ hg commit -m a
> +  $ hg stripintr
> +  saved backup bundle to $TESTTMP/lock-checker/.hg/strip-backup/cb9a9f314b8b-cc5ccb0b-backup.hg (glob)
> +  abort: programming error: cannot strip from inside a transaction
> +  (contact your extensions maintainer)
> +  [255]
> +
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 27, 2015, 6:13 p.m.
On 05/27/2015 08:15 AM, Augie Fackler wrote:
> On Tue, May 26, 2015 at 06:40:41PM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1432441127 25200
>> #      Sat May 23 21:18:47 2015 -0700
>> # Node ID 0c71ff45e361550948fcc3cbfa03898c8ba9fd96
>> # Parent  61b3529e23778f542044d24bb1ccd5688516a7f0
>> repair: forbid strip from inside a transaction
>>
>> Striping inside a transaction will, at best, crash or , at worst, result in very
>
> Spellcheck betrayed you: stripping (as opposed to striping).
>
> (copyediting here because there's one sentence in a comment I can't parse.)
>
>> unexpected result. We explicitly forbid it early.
>>
>> diff --git a/mercurial/repair.py b/mercurial/repair.py
>> --- a/mercurial/repair.py
>> +++ b/mercurial/repair.py
>> @@ -149,10 +149,16 @@ def strip(ui, repo, nodelist, backup=Tru
>>           chgrpfile = _bundle(repo, savebases, saveheads, node, 'temp',
>>                               compress=False)
>>
>>       mfst = repo.manifest
>>
>> +    curtr = repo.currenttransaction()
>> +    if curtr is not None:
>> +        del curtr  # avoid carrying reference to transaction for nothing
>> +        msg = _('programming error: cannot strip from inside a transaction')
>> +        raise util.Abort(msg, hint=_('contact your extensions maintainer'))
>
> "contact your extension maintainer"
>
>> +
>>       tr = repo.transaction("strip")
>>       offset = len(tr.entries)
>>
>>       try:
>>           tr.startgroup()
>> 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
>> @@ -1,11 +1,11 @@
>>
>>     $ cat << EOF > buggylocking.py
>>     > """A small extension that acquire locks in the wrong order
>>     > """
>>     >
>> -  > from mercurial import cmdutil
>> +  > from mercurial import cmdutil, repair
>>     >
>>     > cmdtable = {}
>>     > command = cmdutil.command(cmdtable)
>>     >
>>     > @command('buggylocking', [], '')
>> @@ -36,10 +36,17 @@
>>     > def nowaitlocking(ui, repo):
>>     >     lo = repo.lock()
>>     >     wl = repo.wlock(wait=False)
>>     >     wl.release()
>>     >     lo.release()
>> +  >
>> +  > @command('stripintr', [], '')
>> +  > def stripintr(ui, repo):
>> +  >     lo = repo.lock()
>> +  >     # not a warning no worth testing.
>
> Not sure what this sentence should be (the "no" doesn't make sense)

me neither, lets drop the line.
Augie Fackler - May 27, 2015, 6:28 p.m.
On Wed, May 27, 2015 at 11:13:39AM -0700, Pierre-Yves David wrote:
>
>
> On 05/27/2015 08:15 AM, Augie Fackler wrote:
> >On Tue, May 26, 2015 at 06:40:41PM -0700, Pierre-Yves David wrote:
> >># HG changeset patch
> >># User Pierre-Yves David <pierre-yves.david@fb.com>
> >># Date 1432441127 25200
> >>#      Sat May 23 21:18:47 2015 -0700
> >># Node ID 0c71ff45e361550948fcc3cbfa03898c8ba9fd96
> >># Parent  61b3529e23778f542044d24bb1ccd5688516a7f0
> >>repair: forbid strip from inside a transaction

This is now queued with some fixup in flight.

> >>
> >>Striping inside a transaction will, at best, crash or , at worst, result in very
> >
> >Spellcheck betrayed you: stripping (as opposed to striping).
> >
> >(copyediting here because there's one sentence in a comment I can't parse.)
> >
> >>unexpected result. We explicitly forbid it early.
> >>
> >>diff --git a/mercurial/repair.py b/mercurial/repair.py
> >>--- a/mercurial/repair.py
> >>+++ b/mercurial/repair.py
> >>@@ -149,10 +149,16 @@ def strip(ui, repo, nodelist, backup=Tru
> >>          chgrpfile = _bundle(repo, savebases, saveheads, node, 'temp',
> >>                              compress=False)
> >>
> >>      mfst = repo.manifest
> >>
> >>+    curtr = repo.currenttransaction()
> >>+    if curtr is not None:
> >>+        del curtr  # avoid carrying reference to transaction for nothing
> >>+        msg = _('programming error: cannot strip from inside a transaction')
> >>+        raise util.Abort(msg, hint=_('contact your extensions maintainer'))
> >
> >"contact your extension maintainer"
> >
> >>+
> >>      tr = repo.transaction("strip")
> >>      offset = len(tr.entries)
> >>
> >>      try:
> >>          tr.startgroup()
> >>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
> >>@@ -1,11 +1,11 @@
> >>
> >>    $ cat << EOF > buggylocking.py
> >>    > """A small extension that acquire locks in the wrong order
> >>    > """
> >>    >
> >>-  > from mercurial import cmdutil
> >>+  > from mercurial import cmdutil, repair
> >>    >
> >>    > cmdtable = {}
> >>    > command = cmdutil.command(cmdtable)
> >>    >
> >>    > @command('buggylocking', [], '')
> >>@@ -36,10 +36,17 @@
> >>    > def nowaitlocking(ui, repo):
> >>    >     lo = repo.lock()
> >>    >     wl = repo.wlock(wait=False)
> >>    >     wl.release()
> >>    >     lo.release()
> >>+  >
> >>+  > @command('stripintr', [], '')
> >>+  > def stripintr(ui, repo):
> >>+  >     lo = repo.lock()
> >>+  >     # not a warning no worth testing.
> >
> >Not sure what this sentence should be (the "no" doesn't make sense)
>
> me neither, lets drop the line.
>
> --
> Pierre-Yves David

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -149,10 +149,16 @@  def strip(ui, repo, nodelist, backup=Tru
         chgrpfile = _bundle(repo, savebases, saveheads, node, 'temp',
                             compress=False)
 
     mfst = repo.manifest
 
+    curtr = repo.currenttransaction()
+    if curtr is not None:
+        del curtr  # avoid carrying reference to transaction for nothing
+        msg = _('programming error: cannot strip from inside a transaction')
+        raise util.Abort(msg, hint=_('contact your extensions maintainer'))
+
     tr = repo.transaction("strip")
     offset = len(tr.entries)
 
     try:
         tr.startgroup()
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
@@ -1,11 +1,11 @@ 
 
   $ cat << EOF > buggylocking.py
   > """A small extension that acquire locks in the wrong order
   > """
   > 
-  > from mercurial import cmdutil
+  > from mercurial import cmdutil, repair
   > 
   > cmdtable = {}
   > command = cmdutil.command(cmdtable)
   > 
   > @command('buggylocking', [], '')
@@ -36,10 +36,17 @@ 
   > def nowaitlocking(ui, repo):
   >     lo = repo.lock()
   >     wl = repo.wlock(wait=False)
   >     wl.release()
   >     lo.release()
+  > 
+  > @command('stripintr', [], '')
+  > def stripintr(ui, repo):
+  >     lo = repo.lock()
+  >     # not a warning no worth testing.
+  >     tr = repo.transaction('foobar')
+  >     repair.strip(repo.ui, repo, [repo['.'].node()])
   > EOF
 
   $ cat << EOF >> $HGRCPATH
   > [extensions]
   > buggylocking=$TESTTMP/buggylocking.py
@@ -85,6 +92,16 @@ 
    */mercurial/dispatch.py:* in <lambda> (glob)
    */mercurial/util.py:* in check (glob)
    $TESTTMP/buggylocking.py:* in buggylocking (glob)
   $ hg properlocking
   $ hg nowaitlocking
+
+  $ echo a > a
+  $ hg add a
+  $ hg commit -m a
+  $ hg stripintr
+  saved backup bundle to $TESTTMP/lock-checker/.hg/strip-backup/cb9a9f314b8b-cc5ccb0b-backup.hg (glob)
+  abort: programming error: cannot strip from inside a transaction
+  (contact your extensions maintainer)
+  [255]
+
   $ cd ..