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

# 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.

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!

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