Patchwork [STABLE] strip: use the 'finally: tr.release' pattern during stripping

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 10, 2015, 8:25 a.m.
Message ID <adc4ca995f037b5f2fd8.1439195128@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/10185/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 10, 2015, 8:25 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1439070603 25200
#      Sat Aug 08 14:50:03 2015 -0700
# Branch stable
# Node ID adc4ca995f037b5f2fd8f13f6ee23dd8e495037a
# Parent  d4e1e947444b81a9e0a7d5dc7bfa282b38d00b94
strip: use the 'finally: tr.release' pattern during stripping

The previous code, was calling 'abort' in all exception cases. This was wrong
when an exception was raised by post-close callback on the transaction. Calling
'abort' on an already closed transaction resulted in a error, shadowing the
original error.

We now use the same pattern as everywhere else. 'tr.release()' will abort the
transaction if we escape the scope without closing it. We add a test to make
sure we do not regress.
Sean Farley - Aug. 10, 2015, 5:55 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1439070603 25200
> #      Sat Aug 08 14:50:03 2015 -0700
> # Branch stable
> # Node ID adc4ca995f037b5f2fd8f13f6ee23dd8e495037a
> # Parent  d4e1e947444b81a9e0a7d5dc7bfa282b38d00b94
> strip: use the 'finally: tr.release' pattern during stripping
>
> The previous code, was calling 'abort' in all exception cases. This was wrong
> when an exception was raised by post-close callback on the transaction. Calling
> 'abort' on an already closed transaction resulted in a error, shadowing the
> original error.
>
> We now use the same pattern as everywhere else. 'tr.release()' will abort the
> transaction if we escape the scope without closing it. We add a test to make
> sure we do not regress.

This looks like it might fix some errors I've seen on some server logs
about stripping. Thanks for this!
Augie Fackler - Aug. 11, 2015, 7:47 p.m.
On Mon, Aug 10, 2015 at 01:25:28AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1439070603 25200
> #      Sat Aug 08 14:50:03 2015 -0700
> # Branch stable
> # Node ID adc4ca995f037b5f2fd8f13f6ee23dd8e495037a
> # Parent  d4e1e947444b81a9e0a7d5dc7bfa282b38d00b94
> strip: use the 'finally: tr.release' pattern during stripping

queued for stable, nice catch

>
> The previous code, was calling 'abort' in all exception cases. This was wrong
> when an exception was raised by post-close callback on the transaction. Calling
> 'abort' on an already closed transaction resulted in a error, shadowing the
> original error.
>
> We now use the same pattern as everywhere else. 'tr.release()' will abort the
> transaction if we escape the scope without closing it. We add a test to make
> sure we do not regress.
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -171,13 +171,12 @@ def strip(ui, repo, nodelist, backup=Tru
>                  file, troffset, ignore = tr.entries[i]
>                  repo.svfs(file, 'a').truncate(troffset)
>                  if troffset == 0:
>                      repo.store.markremoved(file)
>              tr.close()
> -        except: # re-raises
> -            tr.abort()
> -            raise
> +        finally:
> +            tr.release()
>
>          if saveheads or savebases:
>              ui.note(_("adding branch\n"))
>              f = vfs.open(chgrpfile, "rb")
>              gen = exchange.readbundle(ui, f, chgrpfile, vfs)
> diff --git a/tests/test-strip.t b/tests/test-strip.t
> --- a/tests/test-strip.t
> +++ b/tests/test-strip.t
> @@ -824,5 +824,28 @@ strip backup content
>    parent:      3:6625a5168474
>    user:        test
>    date:        Thu Jan 01 00:00:00 1970 +0000
>    summary:     mergeCD
>
> +
> +Error during post-close callback of the strip transaction
> +(They should be gracefully handled and reported)
> +
> +  $ cat > ../crashstrip.py << EOF
> +  > from mercurial import error
> +  > def reposetup(ui, repo):
> +  >     class crashstriprepo(repo.__class__):
> +  >         def transaction(self, desc, *args, **kwargs):
> +  >             tr = super(crashstriprepo, self).transaction(self, desc, *args, **kwargs)
> +  >             if desc == 'strip':
> +  >                 def crash(tra): raise error.Abort('boom')
> +  >                 tr.addpostclose('crash', crash)
> +  >             return tr
> +  >     repo.__class__ = crashstriprepo
> +  > EOF
> +  $ hg strip tip --config extensions.crash=$TESTTMP/crashstrip.py
> +  saved backup bundle to $TESTTMP/issue4736/.hg/strip-backup/5c51d8d6557d-70daef06-backup.hg (glob)
> +  strip failed, full bundle stored in '$TESTTMP/issue4736/.hg/strip-backup/5c51d8d6557d-70daef06-backup.hg'
> +  abort: boom
> +  [255]
> +
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -171,13 +171,12 @@  def strip(ui, repo, nodelist, backup=Tru
                 file, troffset, ignore = tr.entries[i]
                 repo.svfs(file, 'a').truncate(troffset)
                 if troffset == 0:
                     repo.store.markremoved(file)
             tr.close()
-        except: # re-raises
-            tr.abort()
-            raise
+        finally:
+            tr.release()
 
         if saveheads or savebases:
             ui.note(_("adding branch\n"))
             f = vfs.open(chgrpfile, "rb")
             gen = exchange.readbundle(ui, f, chgrpfile, vfs)
diff --git a/tests/test-strip.t b/tests/test-strip.t
--- a/tests/test-strip.t
+++ b/tests/test-strip.t
@@ -824,5 +824,28 @@  strip backup content
   parent:      3:6625a5168474
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     mergeCD
   
+
+Error during post-close callback of the strip transaction
+(They should be gracefully handled and reported)
+
+  $ cat > ../crashstrip.py << EOF
+  > from mercurial import error
+  > def reposetup(ui, repo):
+  >     class crashstriprepo(repo.__class__):
+  >         def transaction(self, desc, *args, **kwargs):
+  >             tr = super(crashstriprepo, self).transaction(self, desc, *args, **kwargs)
+  >             if desc == 'strip':
+  >                 def crash(tra): raise error.Abort('boom')
+  >                 tr.addpostclose('crash', crash)
+  >             return tr
+  >     repo.__class__ = crashstriprepo
+  > EOF
+  $ hg strip tip --config extensions.crash=$TESTTMP/crashstrip.py
+  saved backup bundle to $TESTTMP/issue4736/.hg/strip-backup/5c51d8d6557d-70daef06-backup.hg (glob)
+  strip failed, full bundle stored in '$TESTTMP/issue4736/.hg/strip-backup/5c51d8d6557d-70daef06-backup.hg'
+  abort: boom
+  [255]
+
+