[PATCH] transaction: move changelog finalizer to be before bookmark finalizer

[Ryan McElroy]

On 3/16/2016 11:31 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1458196059 25200
> #      Wed Mar 16 23:27:39 2016 -0700
> # Node ID cdf4a94fdc46d27196f5411fc7a4008690834fba
> # Parent  ed75909c4c670a7d9db4a2bef9817a0d5f0b4d9c
> transaction: move changelog finalizer to be before bookmark finalizer
>
> 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.
>
> For reference, file generators currently consist of: bookmarks, dirstate, and
> phases. Finalizers currently consist of: changelog, revbranchcache, and fncache.
> All of the former reference the latter, so therefore the latter should be
> written first.
>
> Technically there's still plenty of race conditions (can the order of finalizers
> affect how external readers see the repo?), but this is a step forward at least.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -407,10 +407,10 @@ class transaction(object):
>           '''commit the transaction'''
>           if self.count == 1:
>               self.validator(self)  # will raise exception if needed
> -            self._generatefiles()
>               categories = sorted(self._finalizecallback)
>               for cat in categories:
>                   self._finalizecallback[cat](self)
> +            self._generatefiles()
>   
>           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 ..
>
This patch looks minimal and safe to me. The particular race condition 
that currently exists is really bad in high-commit-rate repositories -- 
bookmarks often "go missing"altogether, which is a terrible user experience.