[PATCH V3] transaction: allow running file generators after finalizers

[Augie Fackler]

On Thu, Apr 07, 2016 at 02:11:41PM -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1460063449 25200
> #      Thu Apr 07 14:10:49 2016 -0700
> # Node ID 5e49537579979f11268847526f83bb9dc2baf8d8
> # Parent  ea86cdcd9b50bf38c6b9dd7bbaa04b9c8cc0aefb
> transaction: allow running file generators after finalizers
>

queued, thanks

> Previously, transaction.close would run the file generators before running the
> finalizers (see the list below for what is in each). Since file generators
> contain the bookmarks and the dirstate, this meant we made the dirstate and
> bookmarks visible to external readers before we actually wrote the commits into
> the changelog, which could result in missing bookmarks and missing working copy
> parents (especially on servers with high commit throughput, since pulls might
> fail to see certain bookmarks in this situation).
>
> By moving the changelog writing to be before the bookmark/dirstate writing, we
> ensure the commits are present before they are referenced.
>
> This implementation allows certain file generators to be after the finalizers.
> We didn't want to move all of the generators, since it's important that things
> like phases actually run before the finalizers (otherwise you could expose
> commits as public when they really shouldn't be).
>
> For reference, file generators currently consist of: bookmarks, dirstate, and
> phases. Finalizers currently consist of: changelog, revbranchcache, and fncache.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -23,6 +23,19 @@ from . import (
>
>  version = 2
>
> +# These are the file generators that should only be executed after the
> +# finalizers are done, since they rely on the output of the finalizers (like
> +# the changelog having been written).
> +postfinalizegenerators = set([
> +    'bookmarks',
> +    'dirstate'
> +])
> +
> +class GenerationGroup(object):
> +    ALL='all'
> +    PREFINALIZE='prefinalize'
> +    POSTFINALIZE='postfinalize'
> +
>  def active(func):
>      def _active(self, *args, **kwds):
>          if self.count == 0:
> @@ -276,12 +289,19 @@ class transaction(object):
>          # but for bookmarks that are handled outside this mechanism.
>          self._filegenerators[genid] = (order, filenames, genfunc, location)
>
> -    def _generatefiles(self, suffix=''):
> +    def _generatefiles(self, suffix='', group=GenerationGroup.ALL):
>          # write files registered for generation
>          any = False
> -        for entry in sorted(self._filegenerators.values()):
> +        for id, entry in sorted(self._filegenerators.iteritems()):
>              any = True
>              order, filenames, genfunc, location = entry
> +
> +            # for generation at closing, check if it's before or after finalize
> +            postfinalize = group == GenerationGroup.POSTFINALIZE
> +            if (group != GenerationGroup.ALL and
> +                (id in postfinalizegenerators) != (postfinalize)):
> +                continue
> +
>              vfs = self._vfsmap[location]
>              files = []
>              try:
> @@ -407,10 +427,11 @@ class transaction(object):
>          '''commit the transaction'''
>          if self.count == 1:
>              self.validator(self)  # will raise exception if needed
> -            self._generatefiles()
> +            self._generatefiles(group=GenerationGroup.PREFINALIZE)
>              categories = sorted(self._finalizecallback)
>              for cat in categories:
>                  self._finalizecallback[cat](self)
> +            self._generatefiles(group=GenerationGroup.POSTFINALIZE)
>
>          self.count -= 1
>          if self.count != 0:
> diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
> --- a/tests/test-bookmarks.t
> +++ b/tests/test-bookmarks.t
> @@ -846,3 +846,52 @@ updates the working directory and curren
>    6:81dcce76aa0b
>    $ hg -R ../cloned-bookmarks-update bookmarks | grep ' Y '
>     * Y                         6:81dcce76aa0b
> +
> +  $ cd ..
> +
> +ensure changelog is written before bookmarks
> +  $ hg init orderrepo
> +  $ cd orderrepo
> +  $ touch a
> +  $ hg commit -Aqm one
> +  $ hg book mybook
> +  $ echo a > a
> +
> +  $ cat > $TESTTMP/pausefinalize.py <<EOF
> +  > from mercurial import extensions, localrepo
> +  > import os, time
> +  > def transaction(orig, self, desc, report=None):
> +  >    tr = orig(self, desc, report)
> +  >    def sleep(*args, **kwargs):
> +  >        retry = 20
> +  >        while retry > 0 and not os.path.exists("$TESTTMP/unpause"):
> +  >            retry -= 1
> +  >            time.sleep(0.5)
> +  >        if os.path.exists("$TESTTMP/unpause"):
> +  >            os.remove("$TESTTMP/unpause")
> +  >    # It is important that this finalizer start with 'a', so it runs before
> +  >    # the changelog finalizer appends to the changelog.
> +  >    tr.addfinalize('a-sleep', sleep)
> +  >    return tr
> +  >
> +  > def extsetup(ui):
> +  >    # This extension inserts an artifical pause during the transaction
> +  >    # finalizer, so we can run commands mid-transaction-close.
> +  >    extensions.wrapfunction(localrepo.localrepository, 'transaction',
> +  >                            transaction)
> +  > EOF
> +  $ hg commit -qm two --config extensions.pausefinalize=$TESTTMP/pausefinalize.py &
> +  $ sleep 2
> +  $ hg log -r .
> +  changeset:   0:867bc5792c8c
> +  bookmark:    mybook
> +  tag:         tip
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     one
> +
> +  $ hg bookmarks
> +   * mybook                    0:867bc5792c8c
> +  $ touch $TESTTMP/unpause
> +
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel