Patchwork [3,of,5] transaction: add a file generation mechanism

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 7, 2014, 10:52 p.m.
Message ID <db950150e34324daa298.1407451921@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5327/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 7, 2014, 10:52 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1407447602 25200
#      Thu Aug 07 14:40:02 2014 -0700
# Node ID db950150e34324daa2985b2f51ddc8d0ef6b4705
# Parent  4470369895a36e9603de8850335df283e664394c
transaction: add a file generation mechanism

A new `transaction.addfilegenerator` function is added. It allow external code
to register file to be generated. See inline documentation for details.

It is important to gather all file creations logic on the transaction as at
some point we'll want to mimic the "pre-transaction-commit" logic that we use
for revlog. I'm refering to the logic that lets hooks see the result of the transaction before
it gets actually committed.
Gregory Szorc - Aug. 7, 2014, 11:06 p.m.
Nice. I wonder if we can hook tags or branch caches up to this to make 
the various performance issues with their invalidation go away...

On 8/7/14 3:52 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1407447602 25200
> #      Thu Aug 07 14:40:02 2014 -0700
> # Node ID db950150e34324daa2985b2f51ddc8d0ef6b4705
> # Parent  4470369895a36e9603de8850335df283e664394c
> transaction: add a file generation mechanism
>
> A new `transaction.addfilegenerator` function is added. It allow external code
> to register file to be generated. See inline documentation for details.
>
> It is important to gather all file creations logic on the transaction as at
> some point we'll want to mimic the "pre-transaction-commit" logic that we use
> for revlog. I'm refering to the logic that lets hooks see the result of the transaction before
> it gets actually committed.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -94,10 +94,13 @@ class transaction(object):
>           self.backupsfile = opener.open(self.backupjournal, 'w')
>           if createmode is not None:
>               opener.chmod(self.journal, createmode & 0666)
>               opener.chmod(self.backupjournal, createmode & 0666)
>
> +        # hold file generations to be performed on commit
> +        self._filegenerators = {}
> +
>       def __del__(self):
>           if self.journal:
>               self._abort()
>
>       @active
> @@ -171,10 +174,32 @@ class transaction(object):
>           self.backupmap[file] = len(self.backupentries) - 1
>           self.backupsfile.write("%s\0%s\0" % (file, backupfile))
>           self.backupsfile.flush()
>
>       @active
> +    def addfilegenerator(self, genid, filenames, genfunc, order=0):
> +        """add a function to generates some files at transaction commit
> +
> +        The `genfunc` argument is a function capable of generating proper
> +        content of each entry in the `filename` tuple.
> +
> +        At transaction close time, `genfunc` will be called with one file
> +        object argument per entries in `filenames`.
> +
> +        The transaction itself is responsible for the backup, creation and
> +        final write of such file.
> +
> +        The `genid` argument is used to ensure the same set of file is only
> +        generated once. Call to `addfilegenerator` for a `genid` already
> +        present will overwrite the old entry.
> +
> +        The `order` argument may be used to control the order in which multiple
> +        generator will be executed.
> +        """
> +        self._filegenerators[genid] = (order, filenames, genfunc)
> +
> +    @active
>       def find(self, file):
>           if file in self.map:
>               return self.entries[self.map[file]]
>           if file in self.backupmap:
>               return self.backupentries[self.backupmap[file]]
> @@ -211,10 +236,22 @@ class transaction(object):
>           return self.count > 0
>
>       @active
>       def close(self):
>           '''commit the transaction'''
> +        # write files registered for generation
> +        for order, filenames, genfunc in sorted(self._filegenerators.values()):
> +            files = []
> +            try:
> +                for name in filenames:
> +                    self.addbackup(name)
> +                    files.append(self.opener(name, 'w', atomictemp=True))
> +                genfunc(*files)
> +            finally:
> +                for f in files:
> +                    f.close()
> +
>           if self.count == 1 and self.onclose is not None:
>               self.onclose()
>
>           self.count -= 1
>           if self.count != 0:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Aug. 8, 2014, 10:27 a.m.
On 08/07/2014 04:06 PM, Gregory Szorc wrote:
> Nice. I wonder if we can hook tags or branch caches up to this to make
> the various performance issues with their invalidation go away...

ho, yes that would be an awesome idea. Two minor issues with the current 
implementation

1. the transation has a storeopener those does not have access to the 
cache directory. (and failure to write them should not be fatal)

2. the cache may also be written outside of a transaction (when client 
read the cache and realized it is outdated). so we need a something to 
ask if you are in a transaction or not (kinda trivial to add)

Patch

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -94,10 +94,13 @@  class transaction(object):
         self.backupsfile = opener.open(self.backupjournal, 'w')
         if createmode is not None:
             opener.chmod(self.journal, createmode & 0666)
             opener.chmod(self.backupjournal, createmode & 0666)
 
+        # hold file generations to be performed on commit
+        self._filegenerators = {}
+
     def __del__(self):
         if self.journal:
             self._abort()
 
     @active
@@ -171,10 +174,32 @@  class transaction(object):
         self.backupmap[file] = len(self.backupentries) - 1
         self.backupsfile.write("%s\0%s\0" % (file, backupfile))
         self.backupsfile.flush()
 
     @active
+    def addfilegenerator(self, genid, filenames, genfunc, order=0):
+        """add a function to generates some files at transaction commit
+
+        The `genfunc` argument is a function capable of generating proper
+        content of each entry in the `filename` tuple.
+
+        At transaction close time, `genfunc` will be called with one file
+        object argument per entries in `filenames`.
+
+        The transaction itself is responsible for the backup, creation and
+        final write of such file.
+
+        The `genid` argument is used to ensure the same set of file is only
+        generated once. Call to `addfilegenerator` for a `genid` already
+        present will overwrite the old entry.
+
+        The `order` argument may be used to control the order in which multiple
+        generator will be executed.
+        """
+        self._filegenerators[genid] = (order, filenames, genfunc)
+
+    @active
     def find(self, file):
         if file in self.map:
             return self.entries[self.map[file]]
         if file in self.backupmap:
             return self.backupentries[self.backupmap[file]]
@@ -211,10 +236,22 @@  class transaction(object):
         return self.count > 0
 
     @active
     def close(self):
         '''commit the transaction'''
+        # write files registered for generation
+        for order, filenames, genfunc in sorted(self._filegenerators.values()):
+            files = []
+            try:
+                for name in filenames:
+                    self.addbackup(name)
+                    files.append(self.opener(name, 'w', atomictemp=True))
+                genfunc(*files)
+            finally:
+                for f in files:
+                    f.close()
+
         if self.count == 1 and self.onclose is not None:
             self.onclose()
 
         self.count -= 1
         if self.count != 0: